limit by-root requests to non-finalized blocks #3293
Merged
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
arnetheduck
force-pushed
the
no-more-blocks
branch
from
5b9177b
to
a3b67ed
Compare
Presently, we keep a mapping from block root to `BlockRef` in memory - this has simplified reasoning about the dag, but is not sustainable with the chain growing. We can distinguish between two cases where by-root access is useful: * unfinalized blocks - this is where the beacon chain is operating generally, by validating incoming data as interesting for future fork choice decisions - bounded by the length of the unfinalized period * finalized blocks - historical access in the REST API etc - no bounds, really In this PR, we limit the by-root block index to the first use case: finalized chain data can more efficiently be addressed by slot number. Future work includes: * limiting the `BlockRef` horizon in general - each instance is 40 bytes+overhead which adds up - this needs further refactoring to deal with the tail vs state problem * persisting the finalized slot-to-hash index - this one also keeps growing unbounded (albeit slowly) Anyway, this PR easily shaves ~128mb of memory usage at the time of writing. * No longer honor `BeaconBlocksByRoot` requests outside of the non-finalized period - previously, Nimbus would generously return any block through this libp2p request - per the spec, finalized blocks should be fetched via `BeaconBlocksByRange` instead. * return `Opt[BlockRef]` instead of `nil` when blocks can't be found - this becomes a lot more common now and thus deserves more attention * `dag.blocks` -> `dag.forkBlocks` - this index only carries unfinalized blocks from now - `finalizedBlocks` covers the other `BlockRef` instances * in backfill, verify that the last backfilled block leads back to genesis, or panic * add backfill timings to log * fix missing check that `BlockRef` block can be fetched with `getForkedBlock` reliably * shortcut doppelganger check when feature is not enabled * in REST/JSON-RPC, fetch blocks without involving `BlockRef`
arnetheduck
force-pushed
the
no-more-blocks
branch
from
a3b67ed
to
1622233
Compare
zah
approved these changes
Jan 21, 2022
| # is on is no longer a viable fork candidate - we can't tell which is which | ||
| # at this stage, but we can check if we've seen the parent block previously | ||
| # and thus prevent requests for it to be downloaded again. | ||
| if dag.db.containsBlock(blck.parent_root): |
There was a problem hiding this comment.
We should probably have a metric for the number of database queries we perform, so we can see when changes in the DAG logic have an effect on them.
There was a problem hiding this comment.
yeah, I'm not too happy about this check in general - for gossip, it's fine because it happens after signature verification, but I need to think a bit more about what happens during sync - sync is in fact the main issue: if we return MissingParent here, it will go look for it which would be bad.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in 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.
Presently, we keep a mapping from block root to
BlockRefin memory -this has simplified reasoning about the dag, but is not sustainable with
the chain growing.
We can distinguish between two cases where by-root access is useful:
generally, by validating incoming data as interesting for future fork
choice decisions - bounded by the length of the unfinalized period
really
In this PR, we limit the by-root block index to the first use case:
finalized chain data can more efficiently be addressed by slot number.
Future work includes:
BlockRefhorizon in general - each instance is 40bytes+overhead which adds up - this needs further refactoring to deal
with the tail vs state problem
growing unbounded (albeit slowly)
Anyway, this PR easily shaves ~128mb of memory usage at the time of
writing.
BeaconBlocksByRootrequests outside of thenon-finalized period - previously, Nimbus would generously return any
block through this libp2p request - per the spec, finalized blocks
should be fetched via
BeaconBlocksByRangeinstead.Opt[BlockRef]instead ofnilwhen blocks can't be found -this becomes a lot more common now and thus deserves more attention
dag.blocks->dag.forkBlocks- this index only carries unfinalizedblocks from now -
finalizedBlockscovers the otherBlockRefinstances
genesis, or panic
BlockRefblock can be fetched withgetForkedBlockreliablyBlockRef