Skip to content

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

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Jun 19, 2025

(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 to 2).

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.

@hstove hstove requested review from kantai, jferrant and fdefelici June 19, 2025 22:29
@aldur aldur moved this to Status: 💻 In Progress in Stacks Core Eng Jun 20, 2025
@aldur aldur added this to the 3.1.0.0.13 milestone Jun 20, 2025
@hstove
Copy link
Contributor Author

hstove commented Jun 20, 2025

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.

Copy link

codecov bot commented Jun 22, 2025

Codecov Report

Attention: Patch coverage is 9.94550% with 661 lines in your changes missing coverage. Please review.

Project coverage is 82.55%. Comparing base (97a96fc) to head (66bfc13).

Files with missing lines Patch % Lines
testnet/stacks-node/src/tests/signer/v0.rs 1.19% 581 Missing ⚠️
testnet/stacks-node/src/tests/signer/mod.rs 5.00% 38 Missing ⚠️
stacks-signer/src/v0/signer_state.rs 68.49% 23 Missing ⚠️
stackslib/src/net/api/postblock_proposal.rs 0.00% 10 Missing ⚠️
...-node/src/burnchains/bitcoin_regtest_controller.rs 0.00% 9 Missing ⚠️
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              
Files with missing lines Coverage Δ
stacks-signer/src/chainstate.rs 94.71% <100.00%> (+0.01%) ⬆️
stacks-signer/src/client/mod.rs 99.23% <100.00%> (+<0.01%) ⬆️
stacks-signer/src/config.rs 91.03% <100.00%> (+0.14%) ⬆️
stacks-signer/src/runloop.rs 89.68% <100.00%> (+0.02%) ⬆️
stacks-signer/src/tests/chainstate.rs 100.00% <100.00%> (ø)
stacks-signer/src/v0/signer.rs 84.30% <100.00%> (-0.08%) ⬇️
...net/stacks-node/src/tests/nakamoto_integrations.rs 87.17% <100.00%> (+0.12%) ⬆️
...-node/src/burnchains/bitcoin_regtest_controller.rs 87.79% <0.00%> (-0.40%) ⬇️
stackslib/src/net/api/postblock_proposal.rs 68.73% <0.00%> (-1.15%) ⬇️
stacks-signer/src/v0/signer_state.rs 76.31% <68.49%> (-1.53%) ⬇️
... and 2 more

... and 41 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97a96fc...66bfc13. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@fdefelici fdefelici left a 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.

@obycode obycode modified the milestones: 3.1.0.0.13, 3.1.0.0.14 Jul 1, 2025
@hstove hstove marked this pull request as ready for review July 2, 2025 14:11
@hstove hstove requested review from a team as code owners July 2, 2025 14:11
@hstove hstove requested review from fdefelici and kantai July 2, 2025 14:24
@kantai kantai moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng Jul 2, 2025
Copy link
Contributor

@fdefelici fdefelici left a 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:

@hstove hstove requested a review from fdefelici July 3, 2025 14:34
@aldur aldur requested a review from rdeioris July 3, 2025 14:52
Copy link
Contributor

@fdefelici fdefelici left a 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

@hstove hstove requested a review from fdefelici July 7, 2025 14:40
@github-project-automation github-project-automation bot moved this from Status: In Review to Status: 💻 In Progress in Stacks Core Eng Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: 💻 In Progress
Development

Successfully merging this pull request may close these issues.

5 participants