Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Total refactor to support sync & async clients #294

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

stevenroose
Copy link
Collaborator

@stevenroose stevenroose commented Apr 11, 2023

This is a reboot of #212.

So this turned into an insane amount of changes that I've been working on on and off for the last several years.

The current state of things is

  • primarily: in need for feedback and review, but more specifically
  • near ready, SyncClient and AsyncClient API seems good to go
  • supports all methods in current master
  • locally passed tests for all bitcoind version we support (and 22.0 as well)
  • example how to use with hyper (latest)
  • currently based on an MR branch of jsonrpc and pending on 1 or 2 MRs for rust-bitcoin

Some highlights of the changes:

  • Instead of having one RpcApi trait, we have a requests module that is meant for internal use that builds Request objects that is fully ready to be sent to the server, with serializable type wrappers for all arguments. The intent here is to reduce re-allocation for argument serialization and focus is made to reduce allocations as much as possible. This has the side effect that sometimes we have things like &'r &'r str in internal APIs, but the external API is clean.
  • Two new traits: SyncClient and AsyncClient that provide the actual RPC methods, but internally call the methods inside the request module to reduce code duplication.
  • Many argument types have been generalized, so you can easily pass &Transaction, &Vec<u8> or &str as arguments for txs f.e.. Same for blocks, addresses, PSBTs and sighash types.
  • Quite some refactor of intergration test to make it more ergonomic when many tests fail :)

As for review, I realize the changes are massive and I believe it makes more sense to first review the new code as is, looking at requests.rs, sync_client.rs, async_client.rs and maybe client.rs. And only then review changes to maybe integration_test/src/main.rs to check if nothing accidentally got removed.

By way of not forgetting, I keep a todo.md file inside the branch so that I don't lose them, current items are:

  • Generic optional arguments are quite inergonomic because the [None] variant requires a generic argument:
    F.e. Option::<&bitcoin::EcdsaSighashType>::None.
    This counts for all Option<impl SomeTrait> in arguments, I think for now that's only SighashParam and AddressParam.
    (Sighash type is more often used optionally, maybe we can make our own type for that and not need generics?)

  • Fix logging

@stevenroose stevenroose force-pushed the syncasync branch 4 times, most recently from ecaf84f to 493e164 Compare April 11, 2023 17:53
@Kixunil
Copy link

Kixunil commented Apr 11, 2023

Perhaps it'd make sense to upgrade to bitcoin 0.30 first and hen do this PR? Also I just remembered setting version to 1.0.0 is literally wrong because this crate has an unstable dependency in public API: bitcoin.

@Kixunil
Copy link

Kixunil commented Apr 11, 2023

  • Many argument types have been generalized, so you can easily pass &Transaction, &Vec<u8> or &str as arguments for txs f.e.. Same for blocks, addresses, PSBTs and sighash types.

Also this looks like a footgun unless it's explicit. (didn't check)

@stevenroose
Copy link
Collaborator Author

Perhaps it'd make sense to upgrade to bitcoin 0.30 first and hen do this PR?

That already happened in master.

Also I just remembered setting version to 1.0.0 is literally wrong because this crate has an unstable dependency in public API: bitcoin.

What do you mean with an unstable dependency? The 0.x in just a convention.. rust-miniscript has major versions and relies on _hashes and secp that have not.. But anyway the version is just a placeholder now, but I wouldn't mind bumping to 1.0.0 either way, this crate will have breaking changes quite often anyway so whether we could 0.x.0 or x.0.0 doesn't make much difference.

@stevenroose
Copy link
Collaborator Author

  • Many argument types have been generalized, so you can easily pass &Transaction, &Vec<u8> or &str as arguments for txs f.e.. Same for blocks, addresses, PSBTs and sighash types.

Also this looks like a footgun unless it's explicit. (didn't check)

It's so that you can pass in hex txs as well. Same for string addresses, etc. You can argue that people should be encouraged to use strong types, and that's what the default return values are, but it's just more efficient to pass in strings, especially if you have methods that allow you to get strings too.

@Kixunil
Copy link

Kixunil commented Apr 13, 2023

Yes, it's a convention that is so common that it's probably best to consider it a rule. 0.x means unstable API - frequent changes, 1+.x means stable API - infrequent changes. It's a shame miniscript got this wrong. Anyway we're trying to push for stabilizing bitcoin or its parts and there's a good chance we could stabilize parts needed here somewhat sooner. But don't expect anything less than a year.

I understand why Vec<u8> (should be &[u8], I guess) or &str are accepted. I just think it should be explicit. Something like RawBytes(b) and HexStr(s).

@tcharding
Copy link
Member

Have you considered just having a single Client and using maybe_async to add the async/await stuff?

@tcharding
Copy link
Member

I had a read and it looks good to me.

@Kixunil
Copy link

Kixunil commented May 4, 2023

@tcharding that one uses feature flags to switch between sync and async which is a horrible approach. NACK from me.

@tcharding
Copy link
Member

Not disagreeing just wanting to understand; why is a feature flag approach bad? I can't come up with an example usecase that one would want both an async client and a sync client.

@stevenroose
Copy link
Collaborator Author

@tcharding that one uses feature flags to switch between sync and async which is a horrible approach. NACK from me.

Yeah same opinion.

@stevenroose
Copy link
Collaborator Author

Not disagreeing just wanting to understand; why is a feature flag approach bad? I can't come up with an example usecase that one would want both an async client and a sync client.

Just off the top of my head, not 100% sure this scenario makes sense. But basically it's trivial to turn an async client into a sync one, so a library that somehow doesn't want to support async code (tokio..) could take a sync client as an input and a larger project using async can still pass a sync client (wrapped async one) into that lib. In the feature approach, the sync-only lib wouldn't even compile, or would not be able to compile in the same project as any other lib that uses async-only.

Copy link

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

I like this improvement. It is based on a refactor of the rust-jsonrpc crate ( apoelstra/rust-jsonrpc#56) which isn't 100% complete but this PR is very dependant on. My comments are really about the rust-jsonrpc refactor as I think the changes in this PR look good.

I like the rework of the rust-jsonrpc transport traits to use the new Param vs making the client apps manage serde::RawValues themselves which can get tricky and I think you have achieved an approach with minimal allocations that should "just work"™ most the time.

My only major comment is that I believe the helpers for constructing Params and serializing them should live in the rust-jsonrpc crate. The params macro and and the v, ov, b, r, or and raw methods in particular. Other clients of the rust-jsonrpc crate will need those.

It is a style nit but I also don't like naming any methods using single letter contractions like v, ov, b, r, or. It might feel obvious to you as the author but new readers cannot parse usages of these helpers without spending time jumping around the code.


/// Convert a basic type into a value parameter.
#[inline]
fn v<T: IntoJsonValue>(v: T) -> Param<'static> {

Choose a reason for hiding this comment

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

Nit: so this is a personal style opinion but shortened function names like this are really hard to follow when they are used later in the code and I really don't see the benefit of their shortness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah the main benefit was because I was typing them tens or hundreds of times. I can use rust-analyzer to change their names now though I guess. I agree they would be useful to have in rust-jsonrpc as constructors on the Param type. Like, API design is still more or less open for these two refactors, once I get some review/agreement on the design, we can try clean that up more. I do agree some more things can move into rust-jsonrpc.

@@ -10,14 +10,8 @@ jobs:
matrix:
include:
- rust: stable
env:
RUSTFMTCHK: true

Choose a reason for hiding this comment

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

Very Nit: I don't really have any history in this repo but just wanted to register a vote in favor of keeping rustfmt. We have had the discussion before elsewhere so won't repeat but IMHO formatting consistency is more important than disliking some of rustfmt behaviour

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🙃 Let's say I did this to see if CI would work without having to already deal with how rustfmt is going to decide how I should have formatted my code.

@stevenroose
Copy link
Collaborator Author

Thanks for the review! I'm really wanting to get this out of my head, so I'd like to either get this out as a 1.0.0-rc0 and invite others to help improve from there, or maybe fork into another lib. Or just drop it alltogether.

@tcharding
Copy link
Member

Don't you have to push apoelstra/rust-jsonrpc#56 through first? I'm happy to review that if/when in comes off WIP or is it ready to go (excluding rebase on master). To rebase you'll need to add SyncTransport impl for MnreqHttpTransport.

@stevenroose
Copy link
Collaborator Author

I'm seeing them two as a change together though, the jsonrpc change is only useful in function of this MR, if no one wants this or if this will never land there is no point in making the jsonrpc changes.

@tcharding
Copy link
Member

The LDK folk want to see async in rust-json so I do not think you'll get any pushback against the changes there being merged. The reason for the quietness is, in my opinion, not negative just neither I nor Andrew care much and no one else seems to review/watch/comment on rust-jsonrpc. If you get the two PRs ready, I'll review them. The LDK folk have been hounding us so this is actually quite useful work. JSONRPC is just not that exciting to me.

@apoelstra
Copy link
Member

apoelstra commented Jun 5, 2023

Agreed, I'd love to have async support in rust-jsonprc and here, but I don't ever use async and I have enough borrowck issues without introducing implicit state machines and Pins everywhere. So I'm not going to write (or review) any code since I never use async.

@tcharding
Copy link
Member

Perhaps some of the LDK folk will review it? CC @TheBlueMatt to see if anyone in his crew is psyched enough for this to do reviews.

ErikDeSmedt added a commit to ErikDeSmedt/ark that referenced this pull request Jul 31, 2024
Request a client from the BitcoinD-struct and use it in a test.

I now, the client is `sync` and it blocks the run-time.
However, we currently don't have an async client to work with.

@steven: It would be great if we would get

rust-bitcoin/rust-bitcoincore-rpc#294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants