Improve block field functions by accessing sideband information internally #4465
Conversation
This file contains 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
pwojcikdev
reviewed
Mar 6, 2024
| @@ -166,7 +166,7 @@ void nano::bootstrap_ascending::service::inspect (store::transaction const & tx, | |||
| break; | |||
| 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 (); | |||
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.
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
clemahieu
force-pushed
the
block_fields
branch
2 times, most recently
from
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.
clemahieu
force-pushed
the
block_fields
branch
2 times, most recently
from
7fdf312
to
ec4706a
Compare
pwojcikdev
reviewed
Mar 8, 2024
clemahieu
commented
Mar 8, 2024
pwojcikdev
reviewed
Mar 8, 2024
pwojcikdev
reviewed
Mar 9, 2024
| @@ -3679,7 +3679,7 @@ void nano::json_handler::republish () | |||
| while (block_d != nullptr && hash != source) | |||
| { | |||
| 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 ()); | |||
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
| @@ -3641,7 +3641,7 @@ void nano::json_handler::republish () | |||
| block = node.ledger.block (transaction, hash); | |||
| 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 ()); | |||
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.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
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.