refactor(tree): block buffer #5879
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
rkrasiuk
added
C-bug
mattsse
requested changes
Dec 28, 2023
| num::NonZeroUsize, | ||
| }; | ||
|
|
||
| use crate::metrics::BlockBufferMetrics; | ||
| /// Type that contains blocks by number and hash. | ||
| pub type BufferedBlocks = BTreeMap<BlockNumber, HashMap<BlockHash, SealedBlockWithSenders>>; |
There was a problem hiding this comment.
god I hate type aliases for maps like this
| pub(crate) hash_to_num: HashMap<BlockHash, BlockNumber>, | ||
| /// All blocks in the buffer stored by their block hash. | ||
| pub(crate) blocks: HashMap<BlockHash, SealedBlockWithSenders>, | ||
| /// Map of any parent block hash (even the ones not currently in the buffer) |
There was a problem hiding this comment.
why would we track block hashes of blocks that are not in the buffer?
There was a problem hiding this comment.
because we want the ability to look up buffered blocks by their parent. e.g. block comes in, we can insert it into tree and we want to check buffer for any children blocks that can now be added to the tree as well
mattsse
approved these changes
Dec 28, 2023
|
pending @rakita |
rakita
reviewed
Dec 29, 2023
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.
Description
This PR refactors the internals of
BlockBuffer, improves documentation, and fixes the bug that can potentially cause unbounded memory growth.BlockBufferinternals refactor:BTreeMap<BlockNumber, HashMap<BlockHash, SealedBlockWithSenders>>toHashMap<BlockHash, SealedBlockWithSenders>. this reduces the complexity of accessing blocks within a buffer as well as the need forhash_to_nummappingearliest_blockscollectionBug
Unbounded memory growth
Upon removing blocks from the buffer, it did not clean up the parent hash relationship properly.
The bug can be reproduced by running
cargo test -p reth-blockchain-tree --lib -- block_buffer::tests --nocaptureon test/block-buffer-lens branch.Testing
This PR also improves test coverage of the buffer by having more explicit assertions for its inner collections.