Prevent Overflow LRU Cache from Exploding #4801
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
ethDreamer
changed the title
michaelsproul
requested changes
Oct 4, 2023
beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
Outdated
Show resolved
Hide resolved
beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs
Outdated
Show resolved
Hide resolved
beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs
Outdated
Show resolved
Hide resolved
beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs
Outdated
Show resolved
Hide resolved
beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs
Outdated
Show resolved
Hide resolved
michaelsproul
added
waiting-on-author
bors bot
pushed a commit
that referenced
this pull request
Oct 5, 2023
## Issue Addressed While reviewing #4801 I noticed that our use of `take_while` in the block replayer means that if a state root iterator _with gaps_ is provided, some additonal state roots will be dropped unnecessarily. In practice the impact is small, because once there's _one_ state root miss, the whole tree hash cache needs to be built anyway, and subsequent misses are less costly. However this was still a little inefficient, so I figured it's better to fix it. ## Proposed Changes Use [`peeking_take_while`](https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.peeking_take_while) to avoid consuming the next element when checking whether it satisfies the slot predicate. ## Additional Info There's a gist here that shows the basic dynamics in isolation: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=40b623cc0febf9ed51705d476ab140c5. Changing the `peeking_take_while` to a `take_while` causes the assert to fail. Similarly I've added a new test `block_replayer_peeking_state_roots` which fails if the same change is applied inside `get_state_root`.
beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs
Outdated
Show resolved
Hide resolved
ethDreamer
added
ready-for-review
realbigsean
reviewed
Oct 6, 2023
| .apply_blocks(vec![diet_executed_block.block.clone_as_blinded()], None) | ||
| .map(|block_replayer| block_replayer.into_state()) | ||
| .and_then(|mut state| { | ||
| state |
There was a problem hiding this comment.
if we're explicit about building all caches here, is it possible we're doing too much work? for example if we're not verifying signatures, do we need the pubkey cache? and if we're not crossing an epoch boundary do we need the next epoch's committee cache? I think all the caches should be built on demand, so maybe just removing the explicit building would be better. What do you think @michaelsproul
There was a problem hiding this comment.
Yeah, I guess in most cases we will need these caches when we process the next block, but in the case of a reorg or a block on a side chain, we may not. So it might be a bit more resilient if we don't build any caches here (less DoS risk).
In the past we've had some issues with caches not getting auto-built when they're required, but I think we're past that now, and hopefully Hydra helps flush out any cases we've missed.
There was a problem hiding this comment.
So I did some messing around and it looks like we only need to build the exit cache & the tree hash cache in order to have equality with the original state. Should I build those or build nothing?
There was a problem hiding this comment.
I guess in most cases we will need these caches when we process the next block
Oh true. Well I guess the scenario this mechanism is designed for is when we have a bunch of heads. So if we don't get a next block on a head it'd still be wasted work, right? Also building caches on-demand as we get each head's "next block" might be better to spread out this work over a longer period of time.
So I did some messing around and it looks like we only need to build the exit cache & the tree hash cache in order to have equality with the original state. Should I build those or build nothing?
Yea sure, building the ones we know we need makes sense to me
michaelsproul
approved these changes
Oct 11, 2023
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
## Issue Addressed While reviewing sigp#4801 I noticed that our use of `take_while` in the block replayer means that if a state root iterator _with gaps_ is provided, some additonal state roots will be dropped unnecessarily. In practice the impact is small, because once there's _one_ state root miss, the whole tree hash cache needs to be built anyway, and subsequent misses are less costly. However this was still a little inefficient, so I figured it's better to fix it. ## Proposed Changes Use [`peeking_take_while`](https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.peeking_take_while) to avoid consuming the next element when checking whether it satisfies the slot predicate. ## Additional Info There's a gist here that shows the basic dynamics in isolation: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=40b623cc0febf9ed51705d476ab140c5. Changing the `peeking_take_while` to a `take_while` causes the assert to fail. Similarly I've added a new test `block_replayer_peeking_state_roots` which fails if the same change is applied inside `get_state_root`.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
## Issue Addressed While reviewing sigp#4801 I noticed that our use of `take_while` in the block replayer means that if a state root iterator _with gaps_ is provided, some additonal state roots will be dropped unnecessarily. In practice the impact is small, because once there's _one_ state root miss, the whole tree hash cache needs to be built anyway, and subsequent misses are less costly. However this was still a little inefficient, so I figured it's better to fix it. ## Proposed Changes Use [`peeking_take_while`](https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.peeking_take_while) to avoid consuming the next element when checking whether it satisfies the slot predicate. ## Additional Info There's a gist here that shows the basic dynamics in isolation: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=40b623cc0febf9ed51705d476ab140c5. Changing the `peeking_take_while` to a `take_while` causes the assert to fail. Similarly I've added a new test `block_replayer_peeking_state_roots` which fails if the same change is applied inside `get_state_root`.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
## Issue Addressed While reviewing sigp#4801 I noticed that our use of `take_while` in the block replayer means that if a state root iterator _with gaps_ is provided, some additonal state roots will be dropped unnecessarily. In practice the impact is small, because once there's _one_ state root miss, the whole tree hash cache needs to be built anyway, and subsequent misses are less costly. However this was still a little inefficient, so I figured it's better to fix it. ## Proposed Changes Use [`peeking_take_while`](https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.peeking_take_while) to avoid consuming the next element when checking whether it satisfies the slot predicate. ## Additional Info There's a gist here that shows the basic dynamics in isolation: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=40b623cc0febf9ed51705d476ab140c5. Changing the `peeking_take_while` to a `take_while` causes the assert to fail. Similarly I've added a new test `block_replayer_peeking_state_roots` which fails if the same change is applied inside `get_state_root`.
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.
Issue Addressed
In a nutshell, the problem with the
OverflowLRUCachebefore this PR is that it caches anAvailabilityPendingExecutedBlockfor each entry, which contains the entireBeaconState. If we end up with multiple forks and many unavailable blocks, this cache can have many entries, causing it to consume large amounts of memory.I've addressed this by creating a small
StateLRUCachewhich will accept theAvailabilityPendingExecutedBlock, move theBeaconStateto a very limited LRU cache and return aDietAvailablityPendingExecutedBlockwhich contains all the same data except theBeaconStatehas been replaced with a root.If many unavailable blocks are stored at the same time in the
DataAvailabiltyCache, the excess states will be dropped. If those states are needed later they are recovered by loading the parentBeaconStatefrom disk and replaying the block.