-
Notifications
You must be signed in to change notification settings - Fork 697
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
base: develop
Are you sure you want to change the base?
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6212 +/- ##
===========================================
+ Coverage 82.45% 82.55% +0.10%
===========================================
Files 541 541
Lines 344244 344309 +65
Branches 323 323
===========================================
+ Hits 283833 284251 +418
+ Misses 60403 60050 -353
Partials 8 8
... and 41 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.
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 looks fine!
Added some minor remarks.
I also noticed that:
- there is a small clippy issue to be addressed: https://github.com/stacks-network/stacks-core/actions/runs/16027643852/job/45219629003?pr=6212
- and a bunch of flaky tests where one is tx replay related: https://github.com/stacks-network/stacks-core/actions/runs/16027643859/job/45221079154?pr=6212 (which I tested succesfully locally)
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've resolved some of the remarks and updated the others.
@hstove, let me know your thoughts on also the remaining older comments
(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.