Skip to content

refactor: BitcoinCoreController rcp rollout + housekeeping #6385

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

Merged
merged 10 commits into from
Aug 22, 2025

Conversation

fdefelici
Copy link
Contributor

@fdefelici fdefelici commented Aug 18, 2025

Description

This patch is a follow-up to #6329 and introduces the following changes :

  • New method roll out: adoption of new BitcoinCoreController methods introduced in refactor: bitcoin rpc porting #6329.
  • RPC migration: migration from BitcoinRPCRequest to BitcoinRpcClient, replacing stop api.
  • Housekeeping: test updates, documentation, logging (replacing eprintln!), and standardized error handling.
  • Module reorganization: with helium now definitively deprecated, its misplaced tests near BitcoinCoreController have been removed, and BitcoinCoreController module reorganized.

Here a commit breakdown for easier review:

  • Commit 1-7 (up to 4f2e749): are about: New method roll-out, RPC Migration and Housekeeping
  • Commit 8 (da1bf1d): removes deprecated helium tests
  • Commit 9 (3429977): move a BitcoinCoreController related test from neon_integration.rs
  • Commit 10 (4a9dfd9): introduces a dedicated bitcoin module structure, moving BitcoinCoreController related code in src/burnchain/bitcoin/core_controller.rs (for logic) and src/tests/bitcoin/core_controller_integrations.rs (for integration tests)

Basically up-to Commit 9, the changes are related to the existing src/tests/bitcoin_regtest.rs module, while in Commit 10 the module bitcoin_regtest.rs is split into src/burnchain/bitcoin/core_controller.rs and src/tests/bitcoin/core_controller_integrations.rs.

The new structure should make it easier to locate and maintain Bitcoin-related code and tests. Once the BitcoinRegtestsController refactor is complete, we can fully consolidate the structure (tracked in #6251).

If Commit 10 feels too large for this PR, I can move it into a follow-up PR.

Applicable issues

Additional info (benefits, drawbacks, caveats)

A couple of additional refactoring opportunities:

  • The existing tests in stacks-node/src/tests/bitcoin_regtest.rs (bitcoind_integration_test and bitcoind_integration_test_segwit) appear more closely related to RunLoop in stacks-node/src/run_loop/helium.rs. Even though they exercise BitcoinCoreController behavior as a side effect (as most integration tests do), it may be more consistent to move them into stacks-node/src/tests/integrations.rs, alongside other similar integration tests.

#[test]
#[ignore]
fn bitcoind_integration_test() {
if env::var("BITCOIND_TEST") != Ok("1".into()) {
return;
}
bitcoind_integration(false);
}
#[test]
#[ignore]
fn bitcoind_integration_test_segwit() {
if env::var("BITCOIND_TEST") != Ok("1".into()) {
return;
}
bitcoind_integration(true);
}

  • Even if primarily a test utility, it could make sense to move BitcoinCoreController alongside BitcoinRegtestController under stacks-node/src/burnchains. This could help to consolidate Bitcoin-related code in one place.

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@fdefelici fdefelici self-assigned this Aug 18, 2025
@fdefelici fdefelici added this to the 3.2.0.0.2 milestone Aug 18, 2025
@fdefelici fdefelici moved this to Status: 💻 In Progress in Stacks Core Eng Aug 18, 2025
@aldur aldur linked an issue Aug 18, 2025 that may be closed by this pull request
@jcnelson
Copy link
Member

The existing tests in stacks-node/src/tests/bitcoin_regtest.rs (bitcoind_integration_test and bitcoind_integration_test_segwit) appear more closely related to RunLoop in stacks-node/src/run_loop/helium.rs. Even though they exercise BitcoinCoreController behavior as a side effect (as most integration tests do), it may be more consistent to move them into stacks-node/src/tests/integrations.rs, alongside other similar integration tests.

helium.rs should be considered deprecated, as well as the tests for it. The Helium network was an early devnet implementation before Clarinet existed. It and its runloop can and should be removed, and its integration tests repurposed / refactored to work on the mainnet node implementation. We had kept it around because for a time it was not clear whether or not it was still being used, but I don't think it's needed anymore (cc @kantai @obycode @zone117x @hugoclrd)

@fdefelici
Copy link
Contributor Author

The existing tests in stacks-node/src/tests/bitcoin_regtest.rs (bitcoind_integration_test and bitcoind_integration_test_segwit) appear more closely related to RunLoop in stacks-node/src/run_loop/helium.rs. Even though they exercise BitcoinCoreController behavior as a side effect (as most integration tests do), it may be more consistent to move them into stacks-node/src/tests/integrations.rs, alongside other similar integration tests.

helium.rs should be considered deprecated, as well as the tests for it. The Helium network was an early devnet implementation before Clarinet existed. It and its runloop can and should be removed, and its integration tests repurposed / refactored to work on the mainnet node implementation. We had kept it around because for a time it was not clear whether or not it was still being used, but I don't think it's needed anymore (cc @kantai @obycode @zone117x @hugoclrd)

Good to know, thanks! I had also noticed some helium related logic in the BitcoinRegtestController implementation, and I was planning to ask in another PR whether that behavior should still be preserved.

If it’s confirmed that helium is no longer needed, I could then:

  • Drop the tests in stacks-node/src/tests/bitcoin_regtest.rs as part of this PR
  • Open a dedicated issue to track the removal of helium related code across the codebase

@fdefelici
Copy link
Contributor Author

For the record: @obycode confirmed that Hiro is still using helium for some API integration testing. However, those tests depend on an outdated Stacks image, so Hiro will need to rewrite them regardless.

With that in mind, we can safely proceed with removing helium from the codebase.

I’ve opened a dedicated issue (#6408) to track this work.

@fdefelici fdefelici marked this pull request as ready for review August 21, 2025 09:20
@fdefelici fdefelici requested review from a team as code owners August 21, 2025 09:20
@fdefelici fdefelici moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng Aug 21, 2025
@fdefelici fdefelici force-pushed the refactor/btc-core-ctrl branch from ecd10af to 4a9dfd9 Compare August 21, 2025 09:57
@fdefelici fdefelici requested a review from a team as a code owner August 21, 2025 09:57
@aldur aldur requested review from obycode, Jiloc and jferrant August 21, 2025 14:57
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 looks great!

@jferrant
Copy link
Contributor

This was so easy to follow. Looks great. :)

@github-project-automation github-project-automation bot moved this from Status: In Review to Status: 💻 In Progress in Stacks Core Eng Aug 21, 2025
@fdefelici fdefelici added this pull request to the merge queue Aug 22, 2025
Merged via the queue into stacks-network:develop with commit e63ecca Aug 22, 2025
282 of 285 checks passed
@fdefelici fdefelici deleted the refactor/btc-core-ctrl branch August 22, 2025 11:20
@github-project-automation github-project-automation bot moved this from Status: 💻 In Progress to Status: ✅ Done in Stacks Core Eng Aug 22, 2025
Copy link

codecov bot commented Aug 22, 2025

Codecov Report

❌ Patch coverage is 75.07331% with 85 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.17%. Comparing base (26112c7) to head (4a9dfd9).
⚠️ Report is 60 commits behind head on develop.

Files with missing lines Patch % Lines
stacks-node/src/tests/neon_integrations.rs 60.78% 20 Missing ⚠️
stacks-node/src/tests/nakamoto_integrations.rs 66.66% 15 Missing ⚠️
...-node/src/burnchains/bitcoin_regtest_controller.rs 39.13% 14 Missing ⚠️
stacks-node/src/tests/epoch_21.rs 0.00% 13 Missing ⚠️
...cks-node/src/burnchains/bitcoin/core_controller.rs 94.84% 5 Missing ⚠️
stacks-node/src/tests/epoch_205.rs 0.00% 4 Missing ⚠️
.../src/tests/bitcoin/core_controller_integrations.rs 95.58% 3 Missing ⚠️
stacks-node/src/tests/bitcoin_rpc_integrations.rs 90.32% 3 Missing ⚠️
stacks-node/src/tests/epoch_22.rs 0.00% 3 Missing ⚠️
stacks-node/src/tests/epoch_24.rs 0.00% 2 Missing ⚠️
... and 2 more

❌ Your project status has failed because the head coverage (75.17%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6385      +/-   ##
===========================================
- Coverage    75.62%   75.17%   -0.45%     
===========================================
  Files          555      556       +1     
  Lines       350915   350622     -293     
===========================================
- Hits        265365   263588    -1777     
- Misses       85550    87034    +1484     
Files with missing lines Coverage Δ
stacks-node/src/burnchains/mod.rs 62.50% <ø> (ø)
...-node/src/burnchains/rpc/bitcoin_rpc_client/mod.rs 95.20% <ø> (ø)
...tacks-node/src/burnchains/rpc/rpc_transport/mod.rs 92.75% <ø> (+1.44%) ⬆️
stacks-node/src/tests/mod.rs 73.20% <ø> (+4.90%) ⬆️
stacks-node/src/tests/signer/mod.rs 53.96% <100.00%> (+4.77%) ⬆️
stacks-node/src/tests/epoch_23.rs 0.00% <0.00%> (ø)
stacks-node/src/tests/epoch_24.rs 0.00% <0.00%> (ø)
stacks-node/src/tests/stackerdb.rs 0.00% <0.00%> (ø)
.../src/tests/bitcoin/core_controller_integrations.rs 95.58% <95.58%> (ø)
stacks-node/src/tests/bitcoin_rpc_integrations.rs 90.70% <90.32%> (+0.57%) ⬆️
... and 7 more

... and 304 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 26112c7...4a9dfd9. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Refactor: BitcoinRPCRequest Porting
4 participants