Skip to content

Conversation

@tvpeter
Copy link
Collaborator

@tvpeter tvpeter commented Nov 18, 2025

Description

This PR adds the following methods for versions 28, 29, and 30 of bitcoind:

  • getblock
  • getbestblockhash
  • getblock (verbosity = 1)
  • getblockcount
  • getblockhash
  • getblockfilter
  • getblockheader
  • getblockheader (verbose = true)
  • getrawmempool
  • getrawtransaction

Also in this PR

  • Update corepc-types to v0.11.0
  • Uses corepc-node for integration tests
  • Re-export corepc-types, jsonrpc crates
  • Update jsonrpc to v0.19.0 and switched from minreq_http to bitreq_http

Fixes #4
Fixes #19
Fixes #20

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • [] I've updated CHANGELOG.md

@oleonardolima oleonardolima self-requested a review November 19, 2025 17:38
@oleonardolima oleonardolima added the enhancement New feature or request label Nov 19, 2025
@tvpeter tvpeter force-pushed the feat/blockchain-rpc-methods branch 2 times, most recently from 6543635 to 3a16fa4 Compare November 25, 2025 05:22
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

So far I agree with the way you've defined the client methods. I mainly focused on reviewing the implementation of the client code, and haven't spent as much time looking at the test code yet.

By convention, the commits should appear as type(module): Summary, for example

3a16fa4 test: Add integration tests
6f95a7f feat(client): Add blockchain methods

Other notes:

  • Going forward try to have more descriptive API docs
  • We should look into testing the client using corepc_node to minimize the amount of manual testing needed and so we can run the integration tests in CI.

@tvpeter tvpeter force-pushed the feat/blockchain-rpc-methods branch 2 times, most recently from 3632601 to 685e258 Compare December 10, 2025 17:16
@tvpeter tvpeter force-pushed the feat/blockchain-rpc-methods branch from c5f1106 to 61599ad Compare December 11, 2025 14:06
@tvpeter tvpeter requested a review from ValuedMammal December 11, 2025 14:10
@ValuedMammal
Copy link
Collaborator

ValuedMammal commented Dec 12, 2025

I can work on testing Core v28.

Edit: tvpeter#1.

@tvpeter
Copy link
Collaborator Author

tvpeter commented Dec 12, 2025

I can work on testing Core v28.

Alright, that's fine👍. Thank you

@tvpeter tvpeter force-pushed the feat/blockchain-rpc-methods branch 2 times, most recently from bb3f1b1 to b0731c1 Compare December 13, 2025 09:59
@ValuedMammal
Copy link
Collaborator

To consider this ready to merge I would want to check Client compatibility with the 3 latest versions of Bitcoin Core, I think we should tackle #19 as well, and it would be great to have a draft PR on bdk_bitcoind_rpc using the new Client struct.

@tvpeter tvpeter force-pushed the feat/blockchain-rpc-methods branch from cbbedbe to 0778704 Compare December 18, 2025 07:34
@oleonardolima oleonardolima moved this to Needs Review in BDK Chain Dec 19, 2025
Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

cACK 6a38a71

I did a first round of review in this one, I reviewed commit by commit, so some comments might not apply as they were addressed in other commits. However, I didn't tested it yet.

Overall, I'd like to mention my comment about the verbose options, I don't those are necessary w.r.t to bdk_bitcoind_rpc usage as we don't use those AFAICT.

I think most of the methods could just be a one-line fn call, it's just decoding/deserializing it either by accessing a specific field or calling .into_model(), but that's my personal taste.

About the method's documentation, you could update it to link it to the types (e.g [Header]).

About the tests, do you plan to integrate the usage of corepc_node for it in this PR, or in a follow-up one ?

src/client.rs Outdated
Comment on lines 125 to 129
/// Get block verboseone
pub fn get_block_verbose(&self, block_hash: &BlockHash) -> Result<GetBlockVerboseOne, Error> {
let res: GetBlockVerboseOne = self.call("getblock", &[json!(block_hash), json!(1)])?;
Ok(res)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to have support for getblock with the verbose flag? I don't see it being used by bdk_bitcoind_rpc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the same as get_block_info that is used by the Emitter.

src/client.rs Outdated
Comment on lines 131 to 135
/// Get best block hash
pub fn get_best_block_hash(&self) -> Result<BlockHash, Error> {
let res: String = self.call("getbestblockhash", &[])?;
Ok(res.parse()?)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, I don't think we need to support it.

src/client.rs Outdated
let res: String = self.call("getbestblockhash", &[])?;
Ok(res.parse()?)
let best_block_hash: String = self.call("getbestblockhash", &[])?;
Ok(best_block_hash.parse()?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the .parse() uses the rust-bitcoin consensus encode/decode in the background ? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it doesn't. Rust-bitcoin implements the FromStr trait for BlockHash making .parse() method available (as it is available on any type that implements the trait). It is preferred for handling endianness representations

@tvpeter
Copy link
Collaborator Author

tvpeter commented Dec 19, 2025

About the tests, do you plan to integrate the usage of corepc_node for it in this PR, or in a follow-up one ?

yes, the corepc_node will be part of this PR.

Those comments that are not already addressed will be addressed in the next push.
Thank you.

@oleonardolima
Copy link
Collaborator

@tvpeter Let me know when you need another round of review here.

@tvpeter
Copy link
Collaborator Author

tvpeter commented Jan 21, 2026

@tvpeter Let me know when you need another round of review here.

Alright

Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

Made some comments regarding the error module

  • Suggestions for simpler names
  • Suggest to remove unused HexToBytes
  • Don't implement Error::source ?
  • Use explicit names when doing map_err

src/error.rs Outdated
Error::DecodeHex(e) => Some(e),
Error::GetBlockVerboseOneError(e) => Some(e),
Error::Overflow(e) => Some(e),
Error::GetBlockFilterError(e) => Some(e),
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ Is it necessary to implement source here (If not, I'd say remove it). From the docs for std::error::Error:

In error types that wrap an underlying error, the underlying error should be either returned by the outer error’s Error::source(), or rendered by the outer error’s Display implementation, but not both.

@ValuedMammal
Copy link
Collaborator

While reviewing the code in test_rpc_client.rs I thought it'd be worthwhile to develop a simple test harness, similar to what's used in bdk_testenv but tailored to what we need here compare.

@tvpeter
Copy link
Collaborator Author

tvpeter commented Jan 28, 2026

While reviewing the code in test_rpc_client.rs I thought it'd be worthwhile to develop a simple test harness, similar to what's used in bdk_testenv but tailored to what we need here compare.

Yes, I noticed how clean the TestEnv made the tests across the crates.

@tvpeter tvpeter force-pushed the feat/blockchain-rpc-methods branch 3 times, most recently from ee2ebf6 to f076f6f Compare January 30, 2026 13:53
@tvpeter
Copy link
Collaborator Author

tvpeter commented Jan 30, 2026

While reviewing the code in test_rpc_client.rs I thought it'd be worthwhile to develop a simple test harness, similar to what's used in bdk_testenv but tailored to what we need here compare.

Can this come in a different PR?

Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK f076f6f; tested on Bitcoin Core versions 28.1 and 29.0.

I have some remaining comments that can probably be done in a followup PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

📌 Overlooked it, but seems we'll still need an implementation of std::error::Error for Error.

impl std::error::Error for Error {}

@ValuedMammal ValuedMammal mentioned this pull request Jan 30, 2026
3 tasks
@tvpeter tvpeter force-pushed the feat/blockchain-rpc-methods branch from f076f6f to fe96e78 Compare January 30, 2026 20:49
tvpeter and others added 5 commits January 30, 2026 22:48
- getblockcount
- getblockhash
- getblockfilter
- getblockheader
- getrawmempool
- getrawtransaction
- update `corepc-types` to v0.11.0 to support
Bitcoind v30
- refactor tests
- update the error enum
@tvpeter tvpeter force-pushed the feat/blockchain-rpc-methods branch from fe96e78 to 6ff6641 Compare January 30, 2026 21:51
@tvpeter tvpeter requested a review from oleonardolima January 30, 2026 21:54
- update json-rpc to v0.19.0
- switch from minreq_http to bitreq_http
@tvpeter tvpeter mentioned this pull request Feb 3, 2026
3 tasks
Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

Looks like it's almost there, I'm only looking forward to bitcoindevkit/bdk#2109 being rebased on top of master so I could test it through that PR.

Also, we should use the same standard of other BDK projects and not commit the Cargo.lock file.

[dependencies]
corepc-types = { version = "0.11.0", features = ["default"]}
jsonrpc = { version = "0.18.0", features = ["simple_http", "simple_tcp", "minreq_http", "simple_uds", "proxy"] }
jsonrpc = { version = "0.19.0", features = ["simple_http", "simple_tcp", "bitreq_http", "simple_uds", "proxy"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: do we need all these features ? I guess probably only bitreq and proxy ones are sufficient.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initially, I wanted all features because the client should support all JSON-RPC transport layers. It appears that #27 will eventually remove them.

@ValuedMammal
Copy link
Collaborator

reACK 06526db

@ValuedMammal ValuedMammal merged commit ae754b3 into bitcoindevkit:master Feb 11, 2026
9 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Chain Feb 11, 2026
ValuedMammal added a commit that referenced this pull request Feb 11, 2026
f1db39a ci: Run `test_rpc_client` integration tests (valued mammal)
ee52af4 test: Improve unit, integration tests (valued mammal)
d77416c error: Unimplement `Error::source` (valued mammal)

Pull request description:

  ### Description

  This is a follow up to #5 that introduces a `TestEnv` struct used for integration tests and fixes a number of nitpicks that were previously left out.

  Summary:

  - Add simple `TestEnv` struct with convenient interface for improved ergonomics
  - Avoid manually implementing RPC calls (e.g. `generate_to_address`)
  - Move auth tests to `client::test_auth` module
  - Ignore (or remove) test using potentially insecure `std::env::tempdir`
  - Don't call `node.stop()`, as this is handled by the `Drop` implementation of `Node`
  - Add missing tests for existing RPC methods `get_block_verbose`, `get_block_header_verbose`
  - Don't print to stdout
  - Have more effective test assertions. For example it's sufficient to check that the RPC method returns the expected type. Keep some basic "sanity check" assertions, but try to avoid testing functionality that is defined outside of this library (e.g. the length of a `BlockHash`, and Bitcoin Core itself).

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

ACKs for top commit:
  ValuedMammal:
    self-ACK f1db39a

Tree-SHA512: 3634e0e00ed910b979eb998ad13b6a786f827a679495a12237c543a9a8e291eb04602cf0105753fa56dc542cdb57471d87bf630b8afee3bd1564d558cb1b016b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

3 participants