-
-
Notifications
You must be signed in to change notification settings - Fork 84
feat: Add useCache optional param to getLatestBlock()
#340
Conversation
| const latestBlock = await this._fetchLatestBlock(); | ||
| this._newPotentialLatest(latestBlock); | ||
| const latestBlock = await this._updateLatestBlock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic is equivalent. I've only DRY'ed it
This logic is almost equivalent. It fixes a bug where the value from _fetchLatestBlock() was being returned (and resolved) even if this._newPotentialLatest() may not have actually set the block number as the newest in state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure I'm following. _fetchLatestBlock just makes a request, it does not update this._currentBlock. That happens in _newPotentialLatest. But we resolve the #pendingLatestBlock promise after we update _currentBlock anyway. So... shouldn't your code be equivalent?
Maybe you could create a failing test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is that previously, we always used latestBlock from this._fetchLatestBlock. Even if it's not the latest block.
Whereas now we set latestBlock to the return value of _updateLatestBlock which returns this._currentBlock, which is always the latest block.
latestBlock is now always latest, whereas before it was technically possible it was set to an older block (if, somehow, we received a stale response from the RPC endpoint).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whereas before it was technically possible it was set to an older block (if, somehow, we received a stale response from the RPC endpoint).
Okay, I was not aware we had run into that edge case or that we were attempting to solve that in this PR. Based on the original bug report and other reports in the past, I thought that we were only handling the case in which the latest block number stops advancing, not that it goes backward. But I suppose these changes are more technically correct, so okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, we did not run into that edge case, nor was fixing it an intended goal here. I believe it was fixed incidentally while refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, a good example of doing too much in one PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I undo this or move it into a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's okay to include here. You mention in the PR that we want to "correctly fetch and return the most recent block number", and this is part of that, so I think it makes sense.
getLatestBlock()useCache and waitForPending optional params to getLatestBlock()
@jiexi Interesting. Can you give an example of how you expect the middleware to use the new options you've added? |
useCache and waitForPending optional params to getLatestBlock()useCache optional params to getLatestBlock()
useCache optional params to getLatestBlock()useCache optional param to getLatestBlock()
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
…l/fetch-latest-if-stale
Hmm. I'm curious to know how this will work in practice. Requests to |
Wait, this can't be right, or else there'd be infinite recursion between the middleware and the block tracker 🤔 Putting that aside, then... Wouldn't bypassing the block tracker cache mean that we'd make a request for the latest block on each request through the provider, if the block tracker is not running? |
Not exactly. If the tracker is active, it will just wait for the next polling loop. If it's not active, it will only make a request if one hasn't been made within X seconds (the same as the polling interval). We're still capped at making at most one request per polling interval most of the time. There are edge cases where the polling is stopped then started again, where more frequent calls are technically possible. Though this is a pre-existing issue, and not likely to manifest as a major volume issue (for an individual or overall) |
Only if they're made via a network client, or with a provider/middleware pipeline with the network client in it. That's where the caching middleware is. The block tracker itself uses a provider with just a single middleware function, the fetch middleware. No caching middleware. |
| // We want to rate limit calls to this method if we made a direct fetch | ||
| // for the block number because the BlockTracker was not running. We | ||
| // achieve this by delaying the unsetting of the #pendingLatestBlock promise. | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is how we are ensuring we aren't making a new request when calling getLatestBlock({ useCache: false }). Nice.
|
@jiexi @Gudahtt Okay, I re-reviewed this PR and I guess I had missed the addition of I had one thought. Even with these changes, when In other words, in adding My question is, do we need |
|
Great question! That definitely might be a good idea. I feel similarly, the default value (i.e. the behaviour more closely matching how the block tracker acts today) doesn't seem useful even as an option. The reason for making it an optional parameter for now would be to avoid a breaking change, while we're addressing this critical bug at least. It also minimizes risks that this has some unintended impact. |
Gudahtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the changelog as well?
| }); | ||
| await timeoutCallbacks[1](); | ||
| const block2 = await blockPromise2; | ||
| expect(block2).toBe('0x3'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble understanding how this test asserts the description. i.e. if the block tracker didn't wait for the next block, what is stopping this test from passing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the numAutomaticCalls: 2 would only allow 2 setTimeouts to pass. This allows 3 update loops to occur:
- first update happens immediately (not via setTimeout), and queues up setTimeout for second update
- second update happens after setTimeout is executed, and queues up setTimeout for third update
- third update happens after setTimeout is executed
but if the getLatestBlock() call wasn't waiting for the next block, we'd have 2 more setTimeouts we'd need to await for, which would cause the test to fail since it would need to allow 3 setTimeouts to pass.
- first update happens immediately (not via setTimeout), and queues up setTimeout for second update
- first getLatestBlock() is called, waits for second update
- second update happens after setTimeout is executed, and queues up setTimeout for third update
- first getLatestBlock() gets event, queues up setTimeout to release the pendingBlock promise
- second getLatestBlock() is called, waits for third update
- third update happens after setTimeout is executed
- second getLatestBlock() gets event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, I shouldn't need to call timeoutCallbacks though given the automatic calls. Let me play around with this again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned up here 17ae0e0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added explicit _waitingForNextIteration checks here f233852
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but if the getLatestBlock() call wasn't waiting for the next block, we'd have 2 more setTimeouts we'd need to await for
I was thinking of the opposite case. i.e. what if it incorrectly fetches immediately instead of waiting for the next loop. This test would still pass despite that behavior contradicting the description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no, the await pollingLoopPromise1 step should timeout in that case. Nevermind, I think that's covered now as well
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
…l/fetch-latest-if-stale
|
@jiexi Would you mind updating the changelog in this PR? This is a practice that we have slowly been rolling out to all library repos; it is easier to do while you have the context in your head and it saves time later when making a release. (EDIT: Oh, you did, I guess I hadn't refreshed the page. Maybe we can capture fixing |
Mark actually pointed out that this bug didn't actually exist. I was wrong to say it was a bug. |
| const latestBlock = await this._fetchLatestBlock(); | ||
| this._newPotentialLatest(latestBlock); | ||
| try { | ||
| const latestBlock = await this._updateLatestBlock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Stale Block Data in Latest Block Retrieval
The _updateLatestBlock method returns this._currentBlock which can be stale or null if _newPotentialLatest rejects the freshly fetched block. This causes getLatestBlock({ useCache: false }) to return outdated data or throw a runtime error. Also, when the tracker is running, getLatestBlock({ useCache: false }) waits for the next polling event instead of fetching immediately.
Additional Locations (1)
CHANGELOG.md
Outdated
| ### Changed | ||
|
|
||
| - `PollingBlockTracker.getLatestBlock()` now accepts an optional param option `useCache` which ignores the cached block number value in state and instead updates and returns a new block number retrieved within the last `pollingInterval` period when false. Defaults: true ([#340](https://github.com/MetaMask/eth-block-tracker/pull/340)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if "state" could create confusion since this isn't a controller. Maybe we can clean up the wording a bit. We can also document the edge cased we fixed:
| ### Changed | |
| - `PollingBlockTracker.getLatestBlock()` now accepts an optional param option `useCache` which ignores the cached block number value in state and instead updates and returns a new block number retrieved within the last `pollingInterval` period when false. Defaults: true ([#340](https://github.com/MetaMask/eth-block-tracker/pull/340)) | |
| ### Changed | |
| - `PollingBlockTracker.getLatestBlock()` now accepts an optional parameter `useCache` ([#340](https://github.com/MetaMask/eth-block-tracker/pull/340)) | |
| - This option defaults to `true`, but when `false`, it ignores the cached block number and instead updates and returns a new block number, ensuring that the frequency of requests is limited to the `pollingInterval` period | |
| ### Fixed | |
| - Fix `PollingBlockTracker.getLatestBlock()` so it always returns monotonically increasing block numbers ([#340](https://github.com/MetaMask/eth-block-tracker/pull/340)) | |
| - If the RPC endpoint returns a stale block number that is less than a previous request it will effectively be ignored |
(EDIT: If we didn't really fix a bug, then I guess we just have)
| ### Changed | |
| - `PollingBlockTracker.getLatestBlock()` now accepts an optional param option `useCache` which ignores the cached block number value in state and instead updates and returns a new block number retrieved within the last `pollingInterval` period when false. Defaults: true ([#340](https://github.com/MetaMask/eth-block-tracker/pull/340)) | |
| ### Changed | |
| - `PollingBlockTracker.getLatestBlock()` now accepts an optional parameter `useCache` ([#340](https://github.com/MetaMask/eth-block-tracker/pull/340)) | |
| - This option defaults to `true`, but when `false`, it ignores the cached block number and instead updates and returns a new block number, ensuring that the frequency of requests is limited to the `pollingInterval` period |
(Or something like that. Probably could be refined further.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took the first half your suggestion here aa0f910
Gudahtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…)" This reverts commit 1d1406b.
useCacheparam option togetLatestBlock()which ignores the cache block number value in state when false. Defaults: trueThese changes are needed to enable a way for the blockRef middleware
to correctly fetch and return the most recent block number when the BlockTracker isn't polling by avoiding the cache, but also while ensuring that we don't refetch the block number if it is not stale yet.
See: https://consensyssoftware.atlassian.net/browse/NWNT-615
Note
Adds optional
useCachetogetLatestBlock()(defaults true) to bypass cache when needed, refines running/non-running fetch logic with rate limiting, updates tests, and documents in changelog.src/PollingBlockTracker.ts)getLatestBlock({ useCache = true } = {}): supports cache bypass; when running, waits for nextlatestevent; when not running, fetches immediately via_updateLatestBlock()and rate-limits by delaying pending promise cleanup topollingInterval._updateLatestBlock()now returns the latest block string instead ofvoid.checkForLatestBlock(): marked deprecated in JSDoc; now updates and returns latest viagetLatestBlock().src/PollingBlockTracker.test.ts)useCache: falseacross not-running and running states, including concurrency and timing.CHANGELOG.mdunder Unreleased to note the newuseCacheoption and behavior.Written by Cursor Bugbot for commit aa0f910. This will update automatically on new commits. Configure here.