Skip to content

refactor: bitcoin rpc porting #6329

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

Open
wants to merge 69 commits into
base: develop
Choose a base branch
from

Conversation

fdefelici
Copy link
Contributor

@fdefelici fdefelici commented Jul 24, 2025

Description

This PR is the first step in refactoring the BitcoinRegtestController to enhance logic correctness, reduce code duplication, increase test coverage. and improve documentation.

It primarily introduces a port of the BitcoinRPCRequest implementation, with the following key changes:

  • Introduces a RpcTransport layer using the reqwest crate (as used in StacksClient from the stacks-signer package).
  • Adds a BitcoinRpcClient layer that exposes the relevant RPC APIs.
  • Aligns the reqwest version used across stacks-node and stacks-signer with the version already resolved in Cargo.lock
  • Includes a small enhancement to BitcoinCoreController to make it suitable for the new tests added in this PR.

To simplify review, the code introduced here is not yet integrated with the rest of the codebase. Follow-up PRs will handle that integration:

  • A PR to apply the BitcoinCoreController improvements (currently marked with TODOs).
  • One PR (or more if get to big) replacing the existing BitcoinRPCRequest with the new BitcoinRpcClient in BitcoinRegtestController.

Applicable issues

Additional info (benefits, drawbacks, caveats)

A few design decisions were made with the goal of preserving the current usage of the Bitcoin RPC API. Feedback on the following points would be appreciated:

  • Limited Rpc API Scope: This implementation includes only the subset of the Bitcoin RPC API currently used in the codebase, both in terms of available methods and supported input/output parameters. The idea is to keep the implementation minimal for now, with the flexibility to expand it as needed in the future.

  • Nonce Handling in RpcTransport: The original BitcoinRegtestController code uses a fixed nonce value ("stacks") as the id parameter in RPC requests (see code example below). To stay consistent, this PR allows customizable nonces in RpcTransport. However, if this behavior is not strictly required, we could simplify the implementation by generating nonces automatically and internally, avoiding the need to expose this as a configurable option. Would removing nonce configurability be acceptable, or is there a use case for keeping it?

pub fn get_raw_transaction(config: &Config, txid: &Txid) -> RPCResult<String> {
debug!("Get raw transaction {txid}");
let payload = BitcoinRPCRequest {
method: "getrawtransaction".to_string(),
params: vec![format!("{txid}").into()],
id: "stacks".to_string(),
jsonrpc: "2.0".to_string(),
};

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 added 27 commits July 8, 2025 10:01
@fdefelici fdefelici self-assigned this Jul 24, 2025
@fdefelici fdefelici marked this pull request as ready for review July 24, 2025 14:28
@aldur aldur moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng Aug 11, 2025
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! I just had a few minor comments.

@fdefelici fdefelici requested a review from obycode August 12, 2025 07:14
obycode
obycode previously approved these changes Aug 12, 2025
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.

LGTM!

jcnelson
jcnelson previously approved these changes Aug 12, 2025
@fdefelici fdefelici dismissed stale reviews from jcnelson and obycode via ec2db90 August 14, 2025 10:21
@fdefelici
Copy link
Contributor Author

PR updated with the merge of #6366 (related to BitcoinAddress regtest HRP management).

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.

LGTM

Copy link
Contributor

@Jiloc Jiloc left a comment

Choose a reason for hiding this comment

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

LGTM!

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

Successfully merging this pull request may close these issues.

Refactor: BitcoinRPCRequest
4 participants