Skip to content

Create an async corepc-client#505

Merged
tcharding merged 5 commits into
rust-bitcoin:masterfrom
jamillambert:0212-WIP-client-async
Jul 19, 2026
Merged

Create an async corepc-client#505
tcharding merged 5 commits into
rust-bitcoin:masterfrom
jamillambert:0212-WIP-client-async

Conversation

@jamillambert

@jamillambert jamillambert commented Feb 12, 2026

Copy link
Copy Markdown
Collaborator

This PR adds an async corepc-client with the RPCs used by BDK.

The patches are structured to make it easier to see what was changed from sync to async. First the sync version is copied and renamed, then in a separate patch it is changed to async.

@jamillambert jamillambert changed the title WIP: Create an async corepc-client by adding async support to jsonrpc WIP: Create an async corepc-client (adding async support to jsonrpc) Feb 12, 2026
@jamillambert
jamillambert force-pushed the 0212-WIP-client-async branch 2 times, most recently from 6981d8f to 6cd8bb2 Compare February 12, 2026 18:53
Comment thread jsonrpc/src/client_async.rs Outdated
Comment thread jsonrpc/Cargo.toml
Comment thread jsonrpc/Cargo.toml
@tcharding

Copy link
Copy Markdown
Member

@apoelstra whats your take man? Would you be willing to have jsonrpc take changes to be able to use bitreq in an async manner? It obviously smashes the dep tree if --all-features are used.

@tcharding

Copy link
Copy Markdown
Member

Holla at me if/when you would like some review mate.

@jamillambert
jamillambert force-pushed the 0212-WIP-client-async branch 4 times, most recently from 1c174a6 to 7a79b87 Compare February 20, 2026 15:09
@jamillambert

Copy link
Copy Markdown
Collaborator Author

Rewrote the async client:

  • Removed all the version folders where the RPC macros were defined.
  • Removed macroization from client_async module.
  • Created bdk_client module that implements the the required RPC methods in it using the shared client methods in mod.rs.
    • Supports v25 to v30 by checking the server version and then using the required vtype version before converting into the model type.
    • The bdk_client RPC methods all return the modelled types. This made the error handling difficult since all of the error types for converting into the modelled type are version specific types. I am not sure how to handle this ergonomically. Currently they all return the same Error type defined in the async client.
    • There are a few RPCs that return a single field, e.g. getblock returns a Block and getblockcount a u32. Would it be better to return e.g. Result<u32> instead of Result<GetBlockCount> ?
  • Rewrote the tests to use the new bdk_client rpc calls.

@tcharding thoughts? in particular the error handling of converting to the modelled type. Returning a modelled type vs the inner field e.g. Block or u32. And having a bdk_client module that only implements the RPCs, the idea was to be able to add another one for e.g. LDK with their required RPCs and arguments but sharing the new, call etc. methods.

Comment thread client/src/client_async/bdk_client.rs Outdated
Comment thread client/src/client_async/bdk_client.rs Outdated
Comment thread client/src/client_async/bdk_client.rs Outdated
@tcharding

Copy link
Copy Markdown
Member

Rewrote the async client:

  • Removed all the version folders where the RPC macros were defined.
  • Removed macroization from client_async module.
  • Created bdk_client module that implements the the required RPC methods in it using the shared client methods in mod.rs.

After writing a massive post I realised this solution is not going to work (see bottom for explanation). I think we should inline the bdk module and just document that its designed explicitly with that project in mind.

How about throwing this at the top:

//! Async JSON-RPC client designed explicitly to support BDK.
//!
//! ## Project decisions
//!
//! * Support Core versions 25 to 30.
  • Supports v25 to v30 by checking the server version and then using the required vtype version before converting into the model type.

Troublesome, commented already on the code.

  • The bdk_client RPC methods all return the modelled types. This made the error handling difficult since all of the error types for converting into the modelled type are version specific types. I am not sure how to handle this ergonomically. Currently they all return the same Error type defined in the async client.

Another solution is to have an error type for each function. Then it can have a variant (assuming its an enum) for the exact error returned by into_model.

  • There are a few RPCs that return a single field, e.g. getblock returns a Block and getblockcount a u32. Would it be better to return e.g. Result<u32> instead of Result<GetBlockCount> ?

Agreed that a wrapper type is not useful to returen (eg GetBlockCount). At first blush I think you are correct and returning the inner type is better (its a u64 in this case). After upgrade to bitcoin v0.33 we may have better concrete types to return if needed but your argument stands.

  • Rewrote the tests to use the new bdk_client rpc calls.

Nice.

@tcharding thoughts? in particular the error handling of converting to the modelled type. Returning a modelled type vs the inner field e.g. Block or u32. And having a bdk_client module that only implements the RPCs, the idea was to be able to add another one for e.g. LDK with their required RPCs and arguments but sharing the new, call etc. methods.

I don't think separation by project module is not going to work because the modules will conflict with each other. And if we feature gate the features will not be additive. We could try some macro stuff but I think it defeats the purpose which is to write a simple clean client that others can fork if they need to.

@apoelstra

Copy link
Copy Markdown
Member

@apoelstra whats your take man? Would you be willing to have jsonrpc take changes to be able to use bitreq in an async manner? It obviously smashes the dep tree if --all-features are used.

Would this then tie all users of the crate to tokio? Why can't it be executor-agnostic?

@jamillambert
jamillambert force-pushed the 0212-WIP-client-async branch 3 times, most recently from fd21983 to 8448aab Compare February 25, 2026 19:10
@jamillambert

Copy link
Copy Markdown
Collaborator Author

@apoelstra whats your take man? Would you be willing to have jsonrpc take changes to be able to use bitreq in an async manner? It obviously smashes the dep tree if --all-features are used.

Would this then tie all users of the crate to tokio? Why can't it be executor-agnostic?

The changes to jsonrpc to make it support async is executor-agnostic. It is only the use of bitreq and async that requires tokio in the current state of this PR.

@jamillambert

Copy link
Copy Markdown
Collaborator Author

Were you planning on doing the corepc-regtest-client rename as part of this or afterwards? At a cursory look everything looks good.

I was planning on doing the rename afterwards since it also requires changing the current testing client.

@tcharding

Copy link
Copy Markdown
Member

Sweet. Whats left to undraft this then?

@jamillambert
jamillambert force-pushed the 0212-WIP-client-async branch 2 times, most recently from 20a03b0 to 9fb1295 Compare July 13, 2026 18:02
@jamillambert
jamillambert marked this pull request as ready for review July 13, 2026 18:08
@jamillambert

Copy link
Copy Markdown
Collaborator Author

Sweet. Whats left to undraft this then?

I got it to work locally on BDK. It did require breaking changes due to being async, but I think this is the right choice. This testing resulted in the last patch to make the use in BDK easier, and probably other downstream too.

Undrafted.

Comment thread client/src/client_async/mod.rs Outdated
RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError,
};
pub use crate::client_async::rpcs::RpcApi;
pub(crate) use crate::{into_json, log_response};

@tcharding tcharding Jul 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This import is unusual, why the pub(crate)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done as part of taking all your suggested changes from #664

Comment thread integration_test/tests/bdk_client.rs Outdated

#![cfg(feature = "v30_and_below")]
#![cfg(not(feature = "v24_and_below"))]
#![allow(non_snake_case)] // Test names intentionally use double underscore.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we would be better off without this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I changed all the test names and removed the allow.

Comment thread integration_test/tests/bdk_client.rs Outdated
Comment on lines +5 to +6
#![cfg(feature = "v30_and_below")]
#![cfg(not(feature = "v24_and_below"))]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps add a comment stating why we only test versions 25-30

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Comment added and bumped to v31

Comment thread integration_test/tests/bdk_client.rs Outdated
Comment on lines +25 to +34
async fn async__get_best_block_hash__modelled() {
let node = BitcoinD::with_wallet(Wallet::None, &[]);
let client = async_client_for(&node);

let model: Result<bitcoin::BlockHash, GetBestBlockHashError> =
client.get_best_block_hash().await;
let model = model.unwrap();
let expected = node.client.best_block_hash().expect("best_block_hash");
assert_eq!(model, expected);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What if we do this for the first test and for subsequent tests the same but with the comments?

async fn get_best_block_hash() {
    let node = BitcoinD::with_wallet(Wallet::None, &[]);
    let client = async_client_for(&node);

    // Tests that the async-client has this function.
    let got  = client.get_best_block_hash().await.unwrap();
    // Grabs the block hash using the sync client which we know works.
    let want = node.client.best_block_hash().expect("best_block_hash");

    // Tests tat the async client returned the same block hash as the async client.
    assert_eq!(got, want);
}

The reason is that we are expecting BDK devs to look at this test file. We are just testing the functionality and how usage of the client might look. I'm undecided about the explicit error stuff, leaning towards not testing it here and just making sure we get it right. There are not that many errors anyways. Open to your thoughts though?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I changed the tests to your example and assumed it was a typo and you meant without the comments for later tests?

@tcharding

Copy link
Copy Markdown
Member

Everything else looks good. The error stuff is pretty funky but I don't have a better solution right now. We can change it later if we want to.

@tcharding

Copy link
Copy Markdown
Member

Hey mate, I had a bit of a play because I wanted to get rid of the Raw<Box> stuff, in the end I did not get very far but a bunch of other little cleanups fell out: #664

@jamillambert
jamillambert force-pushed the 0212-WIP-client-async branch from 9a64ce6 to a51f47b Compare July 16, 2026 18:52
Comment thread client/src/client_async/error.rs Outdated
/// remaining helpers have default implementations derived from it.
///
/// [`as_client_error`]: RpcMethodErrorExt::as_client_error
pub trait RpcMethodErrorExt {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Bro the extension patter is specifically for bypassing the orphan rule. It makes no sense to use it within a single crate.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changed to inherent methods.

@jamillambert
jamillambert force-pushed the 0212-WIP-client-async branch 4 times, most recently from 1fa0971 to 2add770 Compare July 17, 2026 19:38
Edit the copy of the sync client to make it async.

Add a set of RPCs that try the latest version of the returned type,
and fall back to the previous version if that fails. This works for the
RPCs here because of the change in return shape between the versions.

Add integration tests for the async client.

Update the README.
@jamillambert
jamillambert force-pushed the 0212-WIP-client-async branch from 2add770 to cd18940 Compare July 17, 2026 20:01
@jamillambert

Copy link
Copy Markdown
Collaborator Author

Hey mate, I had a bit of a play because I wanted to get rid of the Raw<Box> stuff, in the end I did not get very far but a bunch of other little cleanups fell out: #664

Thanks, I included all of your cleanups and redid the error handling.

@tcharding tcharding left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK cd18940

@tcharding
tcharding merged commit 626c0a5 into rust-bitcoin:master Jul 19, 2026
67 of 68 checks passed
@tcharding

tcharding commented Jul 19, 2026

Copy link
Copy Markdown
Member

Mad! Thanks for sticking with this mate. Sorry if my review the other day was a bit prickly. Nice to see this go in. Lets do the split into two crates and release this fella.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants