Skip to content

Conversation

@fdefelici
Copy link
Contributor

@fdefelici fdefelici commented Sep 24, 2025

Description

This patch addresses an issue in the BitcoinRegtestController where instantiating a BitcoinRpcClient required Bitcoin RPC credentials unconditionally.

While RPC credentials are indeed mandatory for nodes configured as miners, they should not be required for other node types (e.g., follower nodes), becase they don't use a RPC client at all.

Previously (before the RPC rollout in #6387), it was possible to instantiate an RPC client without credentials (even though this behavior is not supported by bitcoind).
In this patch, instead of permitting instantiation without authentication, I’ve made the BitcoinRegtestController’s behavior explicit and dependent on the node type:

  • Miner node: requires both the Bitcoin Indexer and a valid Bitcoin RPC client.
  • Other node (Follower/Signer): requires the Bitcoin Indexer, but does not need a Bitcoin RPC client.

The idea is to make the code behavior clearer around this scenario.

Is this reasoning correct?

Context:
This issue was surfaced by @Jiloc, on develop branch, during live testing with a follower node using:

cargo run --bin stacks-node -- start --config ./sample/conf/testnet-follower-conf.toml

Which resulted in the following error:

Process abort due to thread panic: panicked at stacks-node\src\burnchains\bitcoin_regtest_controller.rs:375:14:
unable to instantiate the RPC client!: MissingCredentials

Applicable issues

  • fixes #

Additional info (benefits, drawbacks, caveats)

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 added this to the 3.2.0.0.2 milestone Sep 24, 2025
@fdefelici fdefelici self-assigned this Sep 24, 2025
@fdefelici fdefelici added the bug Unwanted or unintended property causing functional harm label Sep 24, 2025
@fdefelici fdefelici linked an issue Sep 24, 2025 that may be closed by this pull request
@fdefelici fdefelici force-pushed the bugfix/btc-rpc-only-for-miner branch from c3167b1 to 9fd6728 Compare September 24, 2025 14:47
@fdefelici fdefelici force-pushed the bugfix/btc-rpc-only-for-miner branch from 7882156 to bafbfde Compare September 25, 2025 07:29
@fdefelici
Copy link
Contributor Author

Just an heads-up:

  • I also added some unit tests to check for BitcoinRegetestController creation behaviour depending on the node type, and
  • fixed the tests::neon_integrations::antientropy_integration_test that was failing due to it was trying to generate bitcoin blocks (for testing purpose) using a follower node configuration.

@fdefelici fdefelici marked this pull request as ready for review September 25, 2025 09:51
@fdefelici fdefelici requested review from a team as code owners September 25, 2025 09:51
@fdefelici fdefelici added this pull request to the merge queue Sep 30, 2025
Merged via the queue into stacks-network:develop with commit 4da4c6a Sep 30, 2025
3 of 5 checks passed
@fdefelici fdefelici deleted the bugfix/btc-rpc-only-for-miner branch September 30, 2025 09:21
@codecov
Copy link

codecov bot commented Sep 30, 2025

Codecov Report

❌ Patch coverage is 82.48175% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.21%. Comparing base (895537e) to head (be46dbb).
⚠️ Report is 15 commits behind head on develop.

Files with missing lines Patch % Lines
...-node/src/burnchains/bitcoin_regtest_controller.rs 84.96% 20 Missing ⚠️
stacks-node/src/tests/neon_integrations.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6534      +/-   ##
===========================================
- Coverage    79.51%   79.21%   -0.31%     
===========================================
  Files          566      566              
  Lines       345340   345421      +81     
===========================================
- Hits        274596   273610     -986     
- Misses       70744    71811    +1067     
Files with missing lines Coverage Δ
...-node/src/burnchains/rpc/bitcoin_rpc_client/mod.rs 98.05% <ø> (ø)
stacks-node/src/tests/neon_integrations.rs 38.52% <0.00%> (-11.68%) ⬇️
...-node/src/burnchains/bitcoin_regtest_controller.rs 68.97% <84.96%> (-6.60%) ⬇️

... and 96 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 895537e...be46dbb. 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.

@github-actions
Copy link

github-actions bot commented Oct 8, 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 Oct 8, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Unwanted or unintended property causing functional harm locked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: roll out new RPC client in BitcoinRegtestController

4 participants