Skip to content

fix: do not break UTXO chain #6157

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

Closed
wants to merge 2 commits into from

Conversation

obycode
Copy link
Contributor

@obycode obycode commented May 27, 2025

Bad timing between the Stacks node's processing and the Bitcoin network including a block commit can cause a miner to submit a block commit which breaks the UTXO chain, severly hurting its chances of winning blocks. It is better to just miss this commit than to break the UTXO chain.

Bad timing between the Stacks node's processing and the Bitcoin network
including a block commit can cause a miner to submit a block commit
which breaks the UTXO chain, severly hurting its chances of winning
blocks. It is better to just miss this commit than to break the UTXO
chain.
@obycode obycode requested review from a team as code owners May 27, 2025 19:47
@obycode
Copy link
Contributor Author

obycode commented May 27, 2025

TODO: this still needs an integration test

@obycode obycode moved this to Status: 💻 In Progress in Stacks Core Eng May 27, 2025
@obycode obycode added this to the 3.1.0.0.12 milestone May 27, 2025
@obycode obycode self-assigned this May 27, 2025
Copy link

codecov bot commented May 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.90%. Comparing base (d23c861) to head (52fcf78).
Report is 288 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6157      +/-   ##
===========================================
+ Coverage    82.91%   83.90%   +0.98%     
===========================================
  Files          543      538       -5     
  Lines       390327   389503     -824     
  Branches       323        0     -323     
===========================================
+ Hits        323654   326814    +3160     
+ Misses       66665    62689    -3976     
+ Partials         8        0       -8     
Files with missing lines Coverage Δ
...-node/src/burnchains/bitcoin_regtest_controller.rs 89.72% <100.00%> (+0.41%) ⬆️

... and 70 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 d23c861...52fcf78. 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.

@obycode
Copy link
Contributor Author

obycode commented Jun 2, 2025

I am failing to come up with a way to test this without intrusive changes in the code that are more likely to cause a new bug then test this one. If someone else is able to help come up with a test for this, please feel free to contribute, otherwise, I am leaning towards just having this reviewed as-is.

@fdefelici
Copy link
Contributor

I am failing to come up with a way to test this without intrusive changes in the code that are more likely to cause a new bug then test this one. If someone else is able to help come up with a test for this, please feel free to contribute, otherwise, I am leaning towards just having this reviewed as-is.

As discussed during the Nakasync:

  • Writing a true unit test doesn't seem feasible at the moment due to the current architecture of BitcoinRegtestController, which is tightly coupled with bitcoind APIs via BitcoinRPCRequest (used for fetching unspent UTXOs and other blockchain data).
  • Unless we're open to refactoring the design to decouple these dependencies, an alternative would be to write an integration test. This would involve setting up bitcoind using BitcoinCoreController and generating blocks to produce UTXOs, possibly allowing us to simulate the behavior of BitcoinRegtestController::build_leader_block_commit_tx.

@kantai kantai moved this from Status: 💻 In Progress to Status: 📋 Backlog in Stacks Core Eng Jun 23, 2025
fdefelici added a commit to fdefelici/stacks-core that referenced this pull request Jun 24, 2025
@fdefelici
Copy link
Contributor

Trying creating tests in this PR: #6219

@obycode
Copy link
Contributor Author

obycode commented Jul 1, 2025

Closing -- included in #6219

@obycode obycode closed this Jul 1, 2025
@github-project-automation github-project-automation bot moved this from Status: 📋 Backlog to Status: ✅ Done in Stacks Core Eng Jul 1, 2025
fdefelici added a commit to fdefelici/stacks-core that referenced this pull request Jul 4, 2025
Copy link

github-actions bot commented Jul 9, 2025

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.

@github-actions github-actions bot added the locked label Jul 9, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants