Create an async corepc-client

[jamillambert]

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]

@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]

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

[jamillambert]

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]

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]

@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]

@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]

Reviving this PR.

I rebased on master now that jsonrpc has async support. The commit sequence is recreated to be simpler. Still a work in progress.

[jamillambert]

@tcharding I think this is ready for preliminary review.

The last patch is a new idea to make the async client universal by not having any inherent RPC methods. Users instead add an extension trait to implement the RPCs that they need in their own codebase. The RPCs for BDK have already been done in this crate and can be used as a template for others. Users don't need to fork the client and can instead depend on the published version which is better for any fixes or security updates. It can also stay in this repo.

The docs will need to be improved, I'm leaving it for now due to this possible change in structure.

The CI failure for v31 needs the related PR and then I'll see if any of the async client tests need to be updated.

[tcharding]

For a production client all this error stuff could be better I rekon.

[jamillambert]

I made the errors simpler and better.

[tcharding]

We can just put the subset of RPC methods we want directly on the Client type. Downstream can still define extension traits and name collisions can still be avoided by calling their trait methods using trait syntax or whatever its called.

Does BDK use the verbose methods? At first blush I don't like them, bit ugly for a 'bare bones client'.

[tcharding]

I rekon we add an example that adds an extension trait and a single method, then that can be used as documentation and to link LLMs too.

[jamillambert]

The reason for this change was to separate Client from the BDK RPCs. Otherwise we have a BDK client which can be modified for other projects, rather than the way it is here where it is a universal client with a trait premade for BDK.

So I still prefer to have no RPCs at all in the Client, and keep the full set of BDK RPCs in the prewritten trait.

I also think that having extra RPCs in this trait is a good idea rather than just one example and force everyone to write their own. If we put in all that BDK need it works out of the box for them, and if a few can be added to fill the needs of LDK and possibly there is only a few changes needed to this set to support multiple users. I understand the concern that supporting multiple projects will cause conflicts in the RPC methods if they are all together. But if we can support multiple projects without conflicts why not? i.e. The policy is we don't support adding downstream RPC requirements to the Client you need to add the trait to your own codebase, but on an individual basis we can consider if only a small change is required to support whole project then why not.

EDIT: AI says LDK only need getblockchaininfo and gettxout added to the trait so that is supports them too.

[tcharding]

I agree with all that about supporting bdk and ldk. I rekon we should do so as methods on the client still though not a trait. We can chat about it on the phone tomorrow though.