Skip to content

Conversation

fdefelici
Copy link
Contributor

@fdefelici fdefelici commented Aug 19, 2025

Description

This is the second PR in the roll-out of the new Bitcoin RPC client within BitcoinRegtestController.

In this PR, the following methods (mainly used for testing) have been migrated to use the new client:

  • BitcoinRegtestController::invalidate_block(..)
  • BitcoinRegtestController::get_block_hash(..)
  • BitcoinRegtestController::get_raw_transaction(..)

Notes
This need #6394 to get merged first, to have cargo hack check and integration tests properly working
#6394 has been merged

Applicable issues

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 requested review from a team as code owners August 19, 2025 13:14
@fdefelici fdefelici self-assigned this Aug 19, 2025
@fdefelici fdefelici moved this to Status: In Review in Stacks Core Eng Aug 19, 2025
@fdefelici fdefelici added this to the 3.2.0.0.2 milestone Aug 19, 2025
@fdefelici fdefelici linked an issue Aug 19, 2025 that may be closed by this pull request
@fdefelici fdefelici force-pushed the refactor/btc-rpc-rollout-step1-6387 branch from dbc2e08 to f5ee60f Compare August 20, 2025 09:04
@fdefelici fdefelici force-pushed the refactor/btc-rpc-rollout-step1-6387 branch from f5ee60f to 73b67cc Compare August 20, 2025 10:45
@fdefelici fdefelici changed the title refactor: BitcoinRegtestController RPC roll out (step1) refactor: BitcoinRegtestController RPC roll out (STEP 2) Aug 20, 2025
@fdefelici fdefelici moved this from Status: In Review to Status: 💻 In Progress in Stacks Core Eng Aug 20, 2025
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Bitcoin RPCs should never, ever panic. More generally, networking code should never, ever panic. Not only are network-related errors often transient (e.g. bad connection), but also it makes runtime panics weaponizable to DoS the node.

@fdefelici
Copy link
Contributor Author

fdefelici commented Aug 26, 2025

Bitcoin RPCs should never, ever panic. More generally, networking code should never, ever panic. Not only are network-related errors often transient (e.g. bad connection), but also it makes runtime panics weaponizable to DoS the node.

As explained here #6388 (comment), I preserved the same behaviour as the previous code (scoped with #[cfg(test)]).
However, I'm fine to change this behaviour too, while refactoring RPC calls

@fdefelici
Copy link
Contributor Author

I've merged PR #6394 (Step1), so this PR is effectively ready.

To summarize the open points:

@fdefelici fdefelici moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng Aug 27, 2025
@fdefelici fdefelici requested review from jferrant and jcnelson August 27, 2025 12:36
@fdefelici fdefelici added this pull request to the merge queue Sep 1, 2025
Merged via the queue into stacks-network:develop with commit bc9adb3 Sep 1, 2025
3 of 4 checks passed
@fdefelici fdefelici deleted the refactor/btc-rpc-rollout-step1-6387 branch September 1, 2025 08:48
@github-project-automation github-project-automation bot moved this from Status: In Review to Status: ✅ Done in Stacks Core Eng Sep 1, 2025
Copy link

codecov bot commented Sep 1, 2025

Codecov Report

❌ Patch coverage is 62.50000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.48%. Comparing base (d22bad7) to head (487f280).
⚠️ Report is 54 commits behind head on develop.

Files with missing lines Patch % Lines
...-node/src/burnchains/bitcoin_regtest_controller.rs 62.50% 6 Missing ⚠️

❌ Your project status has failed because the head coverage (74.48%) 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    #6388      +/-   ##
===========================================
+ Coverage    71.24%   74.48%   +3.24%     
===========================================
  Files          556      556              
  Lines       351138   351117      -21     
===========================================
+ Hits        250171   261544   +11373     
+ Misses      100967    89573   -11394     
Files with missing lines Coverage Δ
...-node/src/burnchains/bitcoin_regtest_controller.rs 73.29% <62.50%> (+4.03%) ⬆️

... and 354 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 d22bad7...487f280. 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

github-actions bot commented Sep 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 Sep 9, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 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.

refactor: roll out new RPC client in BitcoinRegtestController
3 participants