fix(provider): Do not overflow LRU cache capacity in ChainStreamPoller #1052
Conversation
popzxc
requested review from
DaniPopes,
gakonst,
mattsse,
onbjerg,
prestwich and
Evalir
as code owners
| Self { | ||
| client: client.clone(), | ||
| poll_task: PollerBuilder::new(client, "eth_blockNumber", ()), | ||
| next_yield: NO_BLOCK_NUMBER, | ||
| next_yield, | ||
| known_blocks: LruCache::new(BLOCK_CACHE_SIZE), |
There was a problem hiding this comment.
Also, I'm not quite sure why an LRU cache is used here. Wouldn't a simple VecDeque suffice?
There was a problem hiding this comment.
yeah, imo vecdeque would be more appropriate here actually
| @@ -106,8 +112,77 @@ impl<T: Transport + Clone, N: Network> ChainStreamPoller<T, N> { | |||
| } | |||
| }; | |||
| self.known_blocks.put(number, block); | |||
| if self.known_blocks.len() == BLOCK_CACHE_SIZE.get() { | |||
There was a problem hiding this comment.
The implication here is that we will have to wait for the next value from poller_task while we technically know already that there are more blocks we can fetch. However, implementing it may be somewhat complicated and feels like a premature optimization.
There was a problem hiding this comment.
hmm, imo like this loop's logic is flawed because this allows iterating next_yield..tip before we start yielding blocks again
I consider this a hotfix, but ideally we can solve this my improving this stream impl.
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(all(test, feature = "anvil-api"))] // Tests rely heavily on ability to mine blocks on demand. |
There was a problem hiding this comment.
Writing tests without anvil_mine would be more complex, so I hope it's fine given that tests are run with --all-features in CI.
| @@ -106,8 +112,77 @@ impl<T: Transport + Clone, N: Network> ChainStreamPoller<T, N> { | |||
| } | |||
| }; | |||
| self.known_blocks.put(number, block); | |||
| if self.known_blocks.len() == BLOCK_CACHE_SIZE.get() { | |||
There was a problem hiding this comment.
hmm, imo like this loop's logic is flawed because this allows iterating next_yield..tip before we start yielding blocks again
I consider this a hotfix, but ideally we can solve this my improving this stream impl.
| Self { | ||
| client: client.clone(), | ||
| poll_task: PollerBuilder::new(client, "eth_blockNumber", ()), | ||
| next_yield: NO_BLOCK_NUMBER, | ||
| next_yield, | ||
| known_blocks: LruCache::new(BLOCK_CACHE_SIZE), |
There was a problem hiding this comment.
yeah, imo vecdeque would be more appropriate here actually
Motivation
I started to work on handling reorgs in
ChainStreamPoller, but noticed a few adjacent problems that probably should be fixed first.Here, I noticed that we don't check whether we've reached the LRU cache capacity. As a result, if a lot of blocks are created in a short time (e.g. via
anvil_mine), some blocks may be skipped.This PR fixes that, and also adds a few basic tests for
ChainStreamPoller.Solution
Only add new items to the cache if we don't reach capacity.
PR Checklist