Create an async corepc-client by jamillambert · Pull Request #505 · rust-bitcoin/corepc

jamillambert · 2026-02-12T18:26:34Z

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.

tcharding · 2026-02-16T03:03:11Z

@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 · 2026-02-20T02:20:05Z

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

jamillambert · 2026-02-20T15:22:25Z

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.

tcharding · 2026-02-20T20:51:51Z

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 · 2026-02-25T18:53:17Z

@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 · 2026-02-25T19:22:21Z

@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 · 2026-07-09T08:45:26Z

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 · 2026-07-09T22:10:42Z

Sweet. Whats left to undraft this then?

jamillambert · 2026-07-13T18:13:10Z

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.

tcharding · 2026-07-15T01:52:28Z

+ RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError,
+};
+pub use crate::client_async::rpcs::RpcApi;
+pub(crate) use crate::{into_json, log_response};


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

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

tcharding · 2026-07-15T02:02:20Z

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


I think we would be better off without this.

I changed all the test names and removed the allow.

tcharding · 2026-07-15T02:02:45Z

+#![cfg(feature = "v30_and_below")]
+#![cfg(not(feature = "v24_and_below"))]


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

Comment added and bumped to v31

tcharding · 2026-07-15T02:11:03Z

+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);
+}


What if we do this for the first test and for subsequent tests the same but with the comments?
asyncfnget_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?

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

tcharding · 2026-07-15T02:12:24Z

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 · 2026-07-15T04:10:05Z

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

tcharding · 2026-07-17T05:27:07Z

+/// remaining helpers have default implementations derived from it.
+///
+/// [`as_client_error`]: RpcMethodErrorExt::as_client_error
+pub trait RpcMethodErrorExt {


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

Changed to inherent methods.

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 · 2026-07-17T20:02:34Z

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

ACK cd18940

tcharding · 2026-07-19T22:35:25Z

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.

jamillambert · 2026-02-12T18:26:34Z

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.

tcharding · 2026-02-16T03:03:11Z

@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 · 2026-02-20T02:20:05Z

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

jamillambert · 2026-02-20T15:22:25Z

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.

tcharding · 2026-02-20T20:51:51Z

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 · 2026-02-25T18:53:17Z

@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 · 2026-02-25T19:22:21Z

@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 · 2026-07-09T08:45:26Z

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 · 2026-07-09T22:10:42Z

Sweet. Whats left to undraft this then?

jamillambert · 2026-07-13T18:13:10Z

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.

tcharding · 2026-07-15T01:52:28Z

+ RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError,
+};
+pub use crate::client_async::rpcs::RpcApi;
+pub(crate) use crate::{into_json, log_response};


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

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

tcharding · 2026-07-15T02:02:20Z

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


I think we would be better off without this.

I changed all the test names and removed the allow.

tcharding · 2026-07-15T02:02:45Z

+#![cfg(feature = "v30_and_below")]
+#![cfg(not(feature = "v24_and_below"))]


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

Comment added and bumped to v31

tcharding · 2026-07-15T02:11:03Z

+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);
+}


What if we do this for the first test and for subsequent tests the same but with the comments?
asyncfnget_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?

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

tcharding · 2026-07-15T02:12:24Z

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 · 2026-07-15T04:10:05Z

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

tcharding · 2026-07-17T05:27:07Z

+/// remaining helpers have default implementations derived from it.
+///
+/// [`as_client_error`]: RpcMethodErrorExt::as_client_error
+pub trait RpcMethodErrorExt {


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

Changed to inherent methods.

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 · 2026-07-17T20:02:34Z

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

ACK cd18940

tcharding · 2026-07-19T22:35:25Z

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.

jamillambert · 2026-02-12T18:26:34Z

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.

tcharding · 2026-02-16T03:03:11Z

@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 · 2026-02-20T02:20:05Z

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

jamillambert · 2026-02-20T15:22:25Z

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.

tcharding · 2026-02-20T20:51:51Z

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 · 2026-02-25T18:53:17Z

@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 · 2026-02-25T19:22:21Z

@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 · 2026-07-09T08:45:26Z

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 · 2026-07-09T22:10:42Z

Sweet. Whats left to undraft this then?

jamillambert · 2026-07-13T18:13:10Z

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.

tcharding · 2026-07-15T01:52:28Z

+ RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError,
+};
+pub use crate::client_async::rpcs::RpcApi;
+pub(crate) use crate::{into_json, log_response};


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

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

tcharding · 2026-07-15T02:02:20Z

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


I think we would be better off without this.

I changed all the test names and removed the allow.

tcharding · 2026-07-15T02:02:45Z

+#![cfg(feature = "v30_and_below")]
+#![cfg(not(feature = "v24_and_below"))]


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

Comment added and bumped to v31

tcharding · 2026-07-15T02:11:03Z

+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);
+}


What if we do this for the first test and for subsequent tests the same but with the comments?
asyncfnget_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?

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

tcharding · 2026-07-15T02:12:24Z

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 · 2026-07-15T04:10:05Z

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

tcharding · 2026-07-17T05:27:07Z

+/// remaining helpers have default implementations derived from it.
+///
+/// [`as_client_error`]: RpcMethodErrorExt::as_client_error
+pub trait RpcMethodErrorExt {


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

Changed to inherent methods.

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 · 2026-07-17T20:02:34Z

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

ACK cd18940

tcharding · 2026-07-19T22:35:25Z

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.

jamillambert · 2026-02-12T18:26:34Z

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.

tcharding · 2026-02-16T03:03:11Z

@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 · 2026-02-20T02:20:05Z

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

jamillambert · 2026-02-20T15:22:25Z

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.

tcharding · 2026-02-20T20:51:51Z

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 · 2026-02-25T18:53:17Z

@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 · 2026-02-25T19:22:21Z

@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 · 2026-07-09T08:45:26Z

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 · 2026-07-09T22:10:42Z

Sweet. Whats left to undraft this then?

jamillambert · 2026-07-13T18:13:10Z

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.

tcharding · 2026-07-15T01:52:28Z

+ RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError,
+};
+pub use crate::client_async::rpcs::RpcApi;
+pub(crate) use crate::{into_json, log_response};


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

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

tcharding · 2026-07-15T02:02:20Z

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


I think we would be better off without this.

I changed all the test names and removed the allow.

tcharding · 2026-07-15T02:02:45Z

+#![cfg(feature = "v30_and_below")]
+#![cfg(not(feature = "v24_and_below"))]


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

Comment added and bumped to v31

tcharding · 2026-07-15T02:11:03Z

+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);
+}


What if we do this for the first test and for subsequent tests the same but with the comments?
asyncfnget_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?

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

tcharding · 2026-07-15T02:12:24Z

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 · 2026-07-15T04:10:05Z

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

tcharding · 2026-07-17T05:27:07Z

+/// remaining helpers have default implementations derived from it.
+///
+/// [`as_client_error`]: RpcMethodErrorExt::as_client_error
+pub trait RpcMethodErrorExt {


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

Changed to inherent methods.

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 · 2026-07-17T20:02:34Z

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

ACK cd18940

tcharding · 2026-07-19T22:35:25Z

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.

jamillambert · 2026-02-12T18:26:34Z

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.

tcharding · 2026-02-16T03:03:11Z

@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 · 2026-02-20T02:20:05Z

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

jamillambert · 2026-02-20T15:22:25Z

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.

tcharding · 2026-02-20T20:51:51Z

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 · 2026-02-25T18:53:17Z

@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 · 2026-02-25T19:22:21Z

@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 · 2026-07-09T08:45:26Z

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 · 2026-07-09T22:10:42Z

Sweet. Whats left to undraft this then?

jamillambert · 2026-07-13T18:13:10Z

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.

tcharding · 2026-07-15T01:52:28Z

+ RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError,
+};
+pub use crate::client_async::rpcs::RpcApi;
+pub(crate) use crate::{into_json, log_response};


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

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

tcharding · 2026-07-15T02:02:20Z

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


I think we would be better off without this.

I changed all the test names and removed the allow.

tcharding · 2026-07-15T02:02:45Z

+#![cfg(feature = "v30_and_below")]
+#![cfg(not(feature = "v24_and_below"))]


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

Comment added and bumped to v31

tcharding · 2026-07-15T02:11:03Z

+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);
+}


What if we do this for the first test and for subsequent tests the same but with the comments?
asyncfnget_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?

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

tcharding · 2026-07-15T02:12:24Z

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 · 2026-07-15T04:10:05Z

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

tcharding · 2026-07-17T05:27:07Z

+/// remaining helpers have default implementations derived from it.
+///
+/// [`as_client_error`]: RpcMethodErrorExt::as_client_error
+pub trait RpcMethodErrorExt {


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

Changed to inherent methods.

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 · 2026-07-17T20:02:34Z

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

ACK cd18940

tcharding · 2026-07-19T22:35:25Z

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.

jamillambert · 2026-02-12T18:26:34Z

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.

tcharding · 2026-02-16T03:03:11Z

@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 · 2026-02-20T02:20:05Z

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

jamillambert · 2026-02-20T15:22:25Z

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.

tcharding · 2026-02-20T20:51:51Z

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 · 2026-02-25T18:53:17Z

@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 · 2026-02-25T19:22:21Z

@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 · 2026-07-09T08:45:26Z

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 · 2026-07-09T22:10:42Z

Sweet. Whats left to undraft this then?

jamillambert · 2026-07-13T18:13:10Z

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.

tcharding · 2026-07-15T01:52:28Z

+ RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError,
+};
+pub use crate::client_async::rpcs::RpcApi;
+pub(crate) use crate::{into_json, log_response};


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

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

tcharding · 2026-07-15T02:02:20Z

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


I think we would be better off without this.

I changed all the test names and removed the allow.

tcharding · 2026-07-15T02:02:45Z

+#![cfg(feature = "v30_and_below")]
+#![cfg(not(feature = "v24_and_below"))]


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

Comment added and bumped to v31

tcharding · 2026-07-15T02:11:03Z

+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);
+}


What if we do this for the first test and for subsequent tests the same but with the comments?
asyncfnget_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?

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

tcharding · 2026-07-15T02:12:24Z

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 · 2026-07-15T04:10:05Z

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

tcharding · 2026-07-17T05:27:07Z

+/// remaining helpers have default implementations derived from it.
+///
+/// [`as_client_error`]: RpcMethodErrorExt::as_client_error
+pub trait RpcMethodErrorExt {


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

Changed to inherent methods.

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 · 2026-07-17T20:02:34Z

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

ACK cd18940

tcharding · 2026-07-19T22:35:25Z

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.

jamillambert · 2026-02-12T18:26:34Z

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.

tcharding · 2026-02-16T03:03:11Z

@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 · 2026-02-20T02:20:05Z

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

jamillambert · 2026-02-20T15:22:25Z

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.

tcharding · 2026-02-20T20:51:51Z

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 · 2026-02-25T18:53:17Z

@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 · 2026-02-25T19:22:21Z

@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 · 2026-07-09T08:45:26Z

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 · 2026-07-09T22:10:42Z

Sweet. Whats left to undraft this then?

jamillambert · 2026-07-13T18:13:10Z

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.

tcharding · 2026-07-15T01:52:28Z

+ RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError,
+};
+pub use crate::client_async::rpcs::RpcApi;
+pub(crate) use crate::{into_json, log_response};


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

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

tcharding · 2026-07-15T02:02:20Z

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


I think we would be better off without this.

I changed all the test names and removed the allow.

tcharding · 2026-07-15T02:02:45Z

+#![cfg(feature = "v30_and_below")]
+#![cfg(not(feature = "v24_and_below"))]


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

Comment added and bumped to v31

tcharding · 2026-07-15T02:11:03Z

+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);
+}


What if we do this for the first test and for subsequent tests the same but with the comments?
asyncfnget_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?

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

tcharding · 2026-07-15T02:12:24Z

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 · 2026-07-15T04:10:05Z

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

tcharding · 2026-07-17T05:27:07Z

+/// remaining helpers have default implementations derived from it.
+///
+/// [`as_client_error`]: RpcMethodErrorExt::as_client_error
+pub trait RpcMethodErrorExt {


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

Changed to inherent methods.

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 · 2026-07-17T20:02:34Z

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

ACK cd18940

tcharding · 2026-07-19T22:35:25Z

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.

jamillambert · 2026-02-12T18:26:34Z

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.

tcharding · 2026-02-16T03:03:11Z

@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 · 2026-02-20T02:20:05Z

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

jamillambert · 2026-02-20T15:22:25Z

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.

tcharding · 2026-02-20T20:51:51Z

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 · 2026-02-25T18:53:17Z

@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 · 2026-02-25T19:22:21Z

@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 · 2026-07-09T08:45:26Z

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 · 2026-07-09T22:10:42Z

Sweet. Whats left to undraft this then?

jamillambert · 2026-07-13T18:13:10Z

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.

tcharding · 2026-07-15T01:52:28Z

+ RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError,
+};
+pub use crate::client_async::rpcs::RpcApi;
+pub(crate) use crate::{into_json, log_response};


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

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

tcharding · 2026-07-15T02:02:20Z

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


I think we would be better off without this.

I changed all the test names and removed the allow.

tcharding · 2026-07-15T02:02:45Z

+#![cfg(feature = "v30_and_below")]
+#![cfg(not(feature = "v24_and_below"))]


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

Comment added and bumped to v31

tcharding · 2026-07-15T02:11:03Z

+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);
+}


What if we do this for the first test and for subsequent tests the same but with the comments?
asyncfnget_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?

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

tcharding · 2026-07-15T02:12:24Z

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 · 2026-07-15T04:10:05Z

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

tcharding · 2026-07-17T05:27:07Z

+/// remaining helpers have default implementations derived from it.
+///
+/// [`as_client_error`]: RpcMethodErrorExt::as_client_error
+pub trait RpcMethodErrorExt {


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

Changed to inherent methods.

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 · 2026-07-17T20:02:34Z

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

ACK cd18940

tcharding · 2026-07-19T22:35:25Z

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.

github-actionsBot added C-client C-integration-test C-jsonrpc labels Feb 12, 2026

jamillambert force-pushed the 0212-WIP-client-async branch from d5524e1 to 9a77aa2Compare February 12, 2026 18:37

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 mentioned this pull request Feb 12, 2026
WIP: Create an async corepc-client (adding jsonrpc support into the corepc-client crate) #506
Closed

jamillambert force-pushed the 0212-WIP-client-async branch 2 times, most recently from 6981d8f to 6cd8bb2Compare February 12, 2026 18:53

tnull mentioned this pull request Feb 13, 2026
Clarify stance on corepc-client to be integration-test-only #507
Closed

tcharding reviewed Feb 16, 2026
View reviewed changes

Comment threadjsonrpc/src/client_async.rs Outdated

tcharding reviewed Feb 16, 2026
View reviewed changes

Comment threadjsonrpc/Cargo.toml

tcharding reviewed Feb 16, 2026
View reviewed changes

Comment threadjsonrpc/Cargo.toml

jamillambert force-pushed the 0212-WIP-client-async branch from 6cd8bb2 to 87dfe15Compare February 18, 2026 16:58

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

tcharding reviewed Feb 20, 2026
View reviewed changes

Comment threadclient/src/client_async/bdk_client.rs Outdated

tcharding reviewed Feb 20, 2026
View reviewed changes

Comment threadclient/src/client_async/bdk_client.rs Outdated

tcharding reviewed Feb 20, 2026
View reviewed changes

Comment threadclient/src/client_async/bdk_client.rs Outdated

jamillambert force-pushed the 0212-WIP-client-async branch from 7a79b87 to 2f2ca44Compare February 25, 2026 16:38

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

jamillambert force-pushed the 0212-WIP-client-async branch 2 times, most recently from 20a03b0 to 9fb1295Compare July 13, 2026 18:02

jamillambert marked this pull request as ready for review July 13, 2026 18:08

jamillambert requested review from TheBlueMatt, oleonardolima and tnull as code owners July 13, 2026 18:08

jamillambert force-pushed the 0212-WIP-client-async branch from 9fb1295 to 9a64ce6Compare July 14, 2026 12:03

tcharding mentioned this pull request Jul 14, 2026
Fork or create new crate to replace rust-bitcoincore-rpc bitcoindevkit/bdk_wallet#72
Open

tcharding reviewed Jul 15, 2026
View reviewed changes

tcharding mentioned this pull request Jul 15, 2026
NO MERGE: Review suggestions for 505 #664
Closed

jamillambert force-pushed the 0212-WIP-client-async branch from 9a64ce6 to a51f47bCompare July 16, 2026 18:52

tcharding reviewed Jul 17, 2026
View reviewed changes

jamillambert force-pushed the 0212-WIP-client-async branch 4 times, most recently from 1fa0971 to 2add770Compare July 17, 2026 19:38

jamillambert force-pushed the 0212-WIP-client-async branch from 2add770 to cd18940Compare July 17, 2026 20:01

tcharding approved these changes Jul 19, 2026
View reviewed changes

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