Skip to content
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

Fix: RBF-mechanism should check bitcoin RPC #4206

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Conversation

kantai
Copy link
Member

@kantai kantai commented Dec 21, 2023

Description

The stacks miner's bitcoin operation handling tracks submitted transactions via the ongoing_op mechanism. When deciding whether or not to attempt an RBF, it checks if the ongoing_op has been confirmed. Currently, it does this via the burnchain_db, however, this will not work if the transaction is confirmed, but it was unparseable. In this case, the miner will continuously attempt to RBF an already confirmed transaction, leading to bitcoin mempool rejections.

Relevant Scenario

This scenario surfaced during the nakamoto "controlled testnet" testing through a pretty convoluted series of events:

  1. The neon_node (required for booting through epochs 2.0-2.5) is set to mine very quickly (i.e., very low wait_on_blocks time)
  2. The regtest bitcoind network is set to mine a new block whenever it sees a bitcoin tx in the mempool.
  3. Not infrequently, the stacks miner will assemble and attempt to submit a block during bitcoin block 1 (usually this is a "resubmission" of an identical block commit), but when actually submitted, bitcoin block 2 has already arrived. This requires some pretty specific interlacing of events to occur. The bitcoin_regtest_controller normally performs a check to see if the ongoing_op matches the about-to-submit op, but since bitcoin block 2 has now been mined, ongoing_op is confirmed, so the 'resubmission' is submitted.
  4. When the above scenario happens at a reward cycle boundary, the block commit fails to parse: instead of having PoX outputs, its a prepare phase single-burn-output commit.
  5. The ongoing_op never appears in the burnchaindb even though it has been confirmed, so the miner tries to RBF it, but it gets rejected.

Actually fixing the timing issue in step 3 above isn't possible: no matter how late the stacks-node checks for a new bitcoin block before submitting the block commit, it's always possible that the bitcoin block arrives right "before" the commit is submitted (or equivalently, the bitcoin block just doesn't include the commit). But fixing the tracking of ongoing_op is possible, and that's what this PR does.

Additional info (benefits, drawbacks, caveats)

In theory, this could be opened against develop rather than next. I don't get a sense that this is a huge issue in practice, otherwise I'd expect there'd be a bug report already. In which case, this seems mostly relevant for the testing infrastructures for next, so I just want to simplify the workstream by opening this against next directly.

@kantai kantai requested review from jcnelson and obycode December 21, 2023 20:08
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (c4ec042) 82.86% compared to head (ed2ae3a) 82.83%.

Files Patch % Lines
...-node/src/burnchains/bitcoin_regtest_controller.rs 87.17% 5 Missing ⚠️
testnet/stacks-node/src/tests/neon_integrations.rs 97.72% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4206      +/-   ##
==========================================
- Coverage   82.86%   82.83%   -0.04%     
==========================================
  Files         429      429              
  Lines      302128   302249     +121     
==========================================
- Hits       250368   250356      -12     
- Misses      51760    51893     +133     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a beautiful thing - finding this issue, fixing it, and coming up with a good way to test for it, all of it 🙌

@kantai kantai merged commit 70f7b7d into next Dec 21, 2023
2 checks passed
@kantai kantai deleted the fix/rbf-check-confirmed branch December 21, 2023 21:42
@blockstack-devops
Copy link
Contributor

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.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants