-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make LogPoller more robust against local finality violations #12605
Merged
Commits on Apr 24, 2024
-
Reduce unnecessary code duplication
getCurrentBlockMaybeHandleReorg is called just before the for loop over unfinalized blocks begins, and at the end of each iteration. Simplifying by moving them both to the beginning of the for loop
Configuration menu - View commit details
-
Copy full SHA for 6531a16 - Browse repository at this point
Copy the full SHA 6531a16View commit details -
Fix bugs in TestLogPoller_BackupPollAndSaveLogsSkippingLogsThatAreTooOld
This fixes 2 bugs on develop branch in this test, and removes some unused commented code. First Bug ========= The first bug was causing a false positive PASS on develop branch, which was obscuring a (very minor) bug in BackupPoller that's been fixed in this PR. The comment here about what the test was intended to test is still correct: // Only the 2nd batch + 1 log from a previous batch should be backfilled, because we perform backfill starting // from one block behind the latest finalized block Contrary to the comment, the code was returning 2 logs from the 1st batch (Data=9 & Data=10), plus 9 of 10 logs from the 2nd batch. This was incorrect behavior, but the test was also checking for the same incorrect behavior (looking for 11 logs with first one being Data=9) instead of what's described in the comment. The bug in the production code was that it starts the Backup Poller at Finalized - 1 instead of Finalized. This is a harmless "bug", just unnecessarily starting a block too early, since there's no reason for backup logpoller to re-request the same finalized logs that's already been processed. Now, the code returns the last log from the 1st batch + all but one logs from the 2nd batch, which is correct. (It can't return the last log because that goes beyond the last safe block.) So the test checks that there are 10 logs with first one being Data=10 (last log from the first batch.) Second Bug ========== The second bug was passing firstBatchBlock and secondBatchBlock directly to markBlockAsFinalized() instead of passing firstBatchBlock - 1 and secondBatchBlock - 1. This was only working because of a bug in the version of geth we're currently using: when you request the pending block from simulated geth, it gives you back the current block (1 block prior) instead of the current block. (For example, in the first case, even though we wanted block 11, the latest current block, we request block 12 and get back block 11.) This has been fixed in the latest version of geth... so presumably if we don't fix this here the test would have started failing as soon as we upgrade to the latest version of geth. It doesn't change any behavior of the test for the present version of geth, just makes it more clear that we want block 11 not 12.
Configuration menu - View commit details
-
Copy full SHA for a1ac808 - Browse repository at this point
Copy the full SHA a1ac808View commit details -
Check that all results from batchFetchBlocks() are finalized aside fr…
…om "latest" batchFetchBlocks() will now fetch the "finalized" block along with the rest of each batch, and validate that all of the block numbers (aside from the special when "lateest" is requested) are <= the finalized block number returned. Also, change backfill() to always save the last block of each batch of logs requested, rather than the last block of the logs returned. This only makes a difference if the last block requested has no logs matching the filter, but this change is essential for being able to safely change lastSafeBlockNumber from latestFinalizedBlock - 1 to latestFinalizedBlock
Configuration menu - View commit details
-
Copy full SHA for 4fe3295 - Browse repository at this point
Copy the full SHA 4fe3295View commit details -
Configuration menu - View commit details
-
Copy full SHA for f39e434 - Browse repository at this point
Copy the full SHA f39e434View commit details -
Configuration menu - View commit details
-
Copy full SHA for cbfc6c3 - Browse repository at this point
Copy the full SHA cbfc6c3View commit details -
Configuration menu - View commit details
-
Copy full SHA for 711cfa7 - Browse repository at this point
Copy the full SHA 711cfa7View commit details -
Configuration menu - View commit details
-
Copy full SHA for 4059181 - Browse repository at this point
Copy the full SHA 4059181View commit details -
Configuration menu - View commit details
-
Copy full SHA for f199234 - Browse repository at this point
Copy the full SHA f199234View commit details -
Configuration menu - View commit details
-
Copy full SHA for 738716d - Browse repository at this point
Copy the full SHA 738716dView commit details
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.