-
Notifications
You must be signed in to change notification settings - Fork 699
feat: add failsafe to transaction replay #6212
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
feat: add failsafe to transaction replay #6212
Conversation
After some discussion, we're going to update this to use the rule of "once there are 2 burn blocks past the previous tip, clear the replay set if we're still in it". We'll use a config value for the "2 burn blocks" value. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6212 +/- ##
===========================================
+ Coverage 75.33% 80.57% +5.24%
===========================================
Files 555 555
Lines 350915 350983 +68
===========================================
+ Hits 264358 282821 +18463
+ Misses 86557 68162 -18395
... and 237 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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.
Overall the implementation looks fine!
As expected, all tx_replay_*
tests are currently failing due to the two-tenure limit, for the reasons reported in the PR description. Once the current approach is finalized, these tests will likely need to be revisited and properly adjusted to align with the new logic.
I've included a few remarks throughout the review suggesting possible improvements for readability and maintainability.
This reverts commit c6ca6b9.
I've re-requested reviews here - after the latest round of test flakiness fixes from develop, CI is looking good here. |
LGTM aside from all the failing tests. I saw similar tests failing in my 2 phase commit PR. maybe take a look there and see if my fixes also fix the issues you are seeing (they are some subtle timing fixes that are probably causing issues on develop but don't always manifest) |
The return type was `Result<bool>`, but it only needed to be `bool`
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.
LGMT!
Just a small remark about an unused method.
PS: Don't know if it is by chance, but all tests are green....I'll take a screenshot :)
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.
LGMT!
Failing tests don't seem related to this change.
a2ff29c
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
(Opening as a draft - the test could be improved, and I'm not very confident in the "rules" implemented here)
This PR implements a 'failsafe' for transaction replay - if we've had 2 burn blocks since the new fork tip, clear the replay set. While this is very imperfect, it prioritizes liveness of the chain over guarantees about replay getting executed as expected. Most of the time, this shouldn't make a difference anyways. A new config field,
reset_replay_set_after_fork_blocks
, is provided to allow changing this value (which defaults to2
).I've also refactored many of the transaction replay tests to do shallower forks, which aligns much more with reality. This actually caught a bug in the fork detection logic, which we were getting away with due to the tests using deeper forks. We now use a descendency check to determine whether a new burn block is a fork, where we previously did a simple check against the height of a new burn block.