Improve block field functions by accessing sideband information internally#4465
Merged
clemahieu merged 17 commits intonanocurrency:developfrom Mar 9, 2024
Merged
Improve block field functions by accessing sideband information internally#4465clemahieu merged 17 commits intonanocurrency:developfrom
clemahieu merged 17 commits intonanocurrency:developfrom
Conversation
pwojcikdev
reviewed
Mar 6, 2024
| case nano::block_status::gap_source: | ||
| { | ||
| const auto account = block.previous ().is_zero () ? block.account ().value () : ledger.account (tx, block.previous ()).value (); | ||
| const auto account = block.previous ().is_zero () ? block.account_field ().value () : ledger.account (tx, block.previous ()).value (); |
Contributor
There was a problem hiding this comment.
This line looks a bit suspicious, it seems to rely on the fact that if both source and previous block is missing, then ledger processing code will return block_status::gap_previous. Otherwise it would crash. No need to do anything, just something that might bite later down the road, with subsequent refactorings.
Contributor
Author
There was a problem hiding this comment.
Yea it relies on the fact that if previous.is_zero (), then it must be an open or state block and therefore the account field is present. Since the block failed to be inserted it won't have sideband at this point.
pwojcikdev
previously approved these changes
Mar 7, 2024
64e753a to
3ad6992
Compare
…r than querying the genesis block.
…y contains the field. Fixes up call sites that checked against the old sentinel value 0. Convert many call sites to using ledger::account(block&) when possible.
7fdf312 to
ec4706a
Compare
pwojcikdev
reviewed
Mar 8, 2024
clemahieu
commented
Mar 8, 2024
pwojcikdev
reviewed
Mar 8, 2024
pwojcikdev
reviewed
Mar 9, 2024
| { | ||
| hashes.push_back (previous); | ||
| source = node.ledger.block_source (transaction, *block_d); | ||
| source = block_d->source_field ().value_or (block_d->is_send () ? 0 : block_d->link_field ().value_or (0).as_block_hash ()); |
Contributor
There was a problem hiding this comment.
A short node.ledger.block_source ( .. ) call changed into this relatively long conditional statement, is this intentional?
pwojcikdev
previously approved these changes
Mar 9, 2024
pwojcikdev
reviewed
Mar 9, 2024
| if (sources != 0) // Republish source chain | ||
| { | ||
| nano::block_hash source (node.ledger.block_source (transaction, *block)); | ||
| nano::block_hash source = block->source_field ().value_or (block->link_field ().value_or (0).as_block_hash ()); |
Contributor
There was a problem hiding this comment.
Another line that got more complicated after the refactor than before.
…ccesses the account field is renamed to block::account_field.
…y contains the field.
…ccesses the account field is renamed to block::balance_field.
…ually contains the field.
…estination is created which accesses the the destination field for send blocks.
… contains the field.
…ce is moved to block::source which accesses the source field for open/receive/state blocks that are receives.
…ontains the field.
A specialised variant of is_send for state blocks that do not have sideband was added to dependent_block_visitor so the value can be calculated even if the block does not yet have sideband.
…ive/open/state block that's a receive
…nge block or a state block with a zero link field.
… number to an account number are replaced with calls to block::destination (). Usages that converted this number to a block_hash are replaced with calls to block::source, and usages that check this number for being an epoch directly access the link field.
…actually contains the field.
…nal depending if the block has the field. Add block::previous with backwards compatibility and a todo to fix up usages to not check for sentinel values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change moves the responsibility of determining if a field is in the block or in the block's sideband on to the block class.
Previously call sites would need to either inspect the block type or check for a sentinel value to determine where the block field information was stored. This complicates call sites, duplicates code, and could be error prone.
Direct block field access e.g. account/link/destination/balance are suffixed with _field and return an std::optional depending if the block type contains the requested field.
The replacement field functions check block type and determine if the value should be retrieved from the field or the block sideband.
The conversion for each field is done with two commits in order to ensure all call sites are inspected and updated to the new semantics.