[POC] generalized requests to support sync and async #212

stevenroose · 2022-03-01T15:00:16Z

So this is just a proof-of-concept of an idea I've had to finally have a way to support both sync and async in this crate.

It could go hand in hand with supporting batching and deprecating old-hyper in favor of the custom HTTP transport from LDK.

Basically, it introduces a Request struct that basically holds everything you need to perform an RPC and then it has some methods sync (perform the request sync), async (perform the request async) and batch (add the request to a batch).

In principle, this construction could even be pulled upstream the jsonrpc crate. Based on this construction we can decide how to change the API to support sync and async. Options are (1) use features to have the same method name have different signatures for sync/async, (2) support both and have _sync and _async method or (3) some kind of hybrid like I did in this POC.

As you see, inside the RpcApi impl, there is one actual impl method _request and the others are basically templated and could easily be made into a macro. However I think this crate previously moved away from having macros, so I'm not against just writing it all out in full.

Everything is open for discussion, the new construction compiles if you comment out all the other method not yet converted in the RpcApi trait..

RCasatta · 2022-03-01T16:21:32Z

client/src/client.rs

+
+fn converter_hex<T: bitcoin::consensus::encode::Decodable>(raw: &RawValue) -> Result<T> {
+    let hex: &str = serde_json::from_str(raw.get())?;
+    let bytes = Vec::<u8>::from_hex(&hex)?;


once bitcoin_hashes is released we can skip the Vec allocation with rust-bitcoin/bitcoin_hashes#135

JeremyRubin · 2022-03-02T00:47:25Z

once concept would be to have only async API, but provide a blocking client executor that uses non-async internally.

This would keep the core API pure (and always async), but allow using the async code from non async functions with the sync client.

RCasatta · 2022-03-02T07:54:08Z

once concept would be to have only async API, but provide a blocking client executor that uses non-async internally.

This would keep the core API pure (and always async), but allow using the async code from non async functions with the sync client.

One advantage of sync is that it requires a lot less dependency, using async+blocking client would prevent that

JeremyRubin · 2022-03-02T10:38:33Z

Not quite true; for example you can build a simple executor https://rust-lang.github.io/async-book/02_execution/04_executor.html and not pull in e..g tokio

jkczyz · 2022-03-02T22:44:23Z

client/src/client.rs

+pub struct Request<'c, T: 'static> {
+    method: &'static str,
+    args: Vec<serde_json::Value>, //TODO(stevenroose) something more efficient?
+    converter: &'static dyn Fn(&RawValue) -> Result<T>,


I believe you can build this into the method signatures instead of explicitly calling a converter (e.g., https://github.com/lightningdevkit/rust-lightning/blob/5e86bbf97030c69b0118d1bef39554febfb1cbef/lightning-block-sync/src/rpc.rs#L40-L41).

👍

the only exception being if we need >1 "TryFrom" for a given type?

Yeah, the RPC example is assuming JSON responses, which could be examined and converted as needed (e.g. https://github.com/lightningdevkit/rust-lightning/blob/b89f8815c3d40d8c5bf9aa60af5b3fbb2ee78f25/lightning-block-sync/src/convert.rs#L42-L72).

But in the following REST example, the response could be either JSON or binary depending on the parameters, which I think is what you are alluding to.

https://github.com/lightningdevkit/rust-lightning/blob/b89f8815c3d40d8c5bf9aa60af5b3fbb2ee78f25/lightning-block-sync/src/rest.rs#L29-L35

There the response format needs to be explicit at the call site. For example:

https://github.com/lightningdevkit/rust-lightning/blob/b89f8815c3d40d8c5bf9aa60af5b3fbb2ee78f25/lightning-block-sync/src/rest.rs#L38-L58

So basically, you define conversion from bytes to some response format:

https://github.com/lightningdevkit/rust-lightning/blob/b89f8815c3d40d8c5bf9aa60af5b3fbb2ee78f25/lightning-block-sync/src/http.rs#L456-L478

And then any conversions from those formats to the desired types.

jkczyz · 2022-03-02T22:46:10Z

client/src/client.rs

+        conv(&raw)
+    }
+
+    fn async(self, client: &dyn RpcClient) -> Result<T> {


Should this also be an async method?

TheBlueMatt · 2022-03-04T22:43:16Z

client/src/client.rs

-        let res: Response = self.call("getnetworkinfo", &[])?;
-        Ok(res.version)
+    }
+    #[cfg(not(feature = "async"))]


Ideally the API should support both, no? That way two dependencies setting different feature flags wouldn't cause compile failure.

spoke with @stevenroose about this, we think there is a very clean approach for this by impl future for the request type directly. he's prototyping it :)

sander2 · 2022-03-22T09:28:13Z

@stevenroose any update on this? We're quite interested in having async support - in the past @gregdhill made #144 but unfortunately that never went anywhere

JeremyRubin · 2022-03-23T03:48:36Z

in theory if you need something right away, you can use https://crates.io/crates/bitcoincore-rpc-json-async

note that this uses the sapio flavoured bitcoin crates, though, not the standard ones. I would accept a patch that let's you choose via a flag which dep you take.

stevenroose · 2022-04-25T18:36:45Z

Big update on this. I did a WIP/POC refactor to rust-jsonrpc and to this branch. It's not finished, I will have to add two traits that will have all the actual client's methods SyncClient and AsyncClient with the appropriate return types, but internally they will use all the methods inside the requests module.

I went kind of in a rabbit hole to reduce allocations, but the result is quite an ugly construction that requires double references &'r &'r str etc which is ugly but might also not be optimal (I have no idea about performance hits because of this, but I guess it can't be worse than allocating lots of things again?)

There's still some things I didn't figure out, like wrappers around argument inputs, but I think we might be able to create the wrappers on the trait level, so that would in the end be possible. Right now some of those arguments that need internal conversion are just re-allocated, but those could be optimized using such wrapper types lateron.

For now, I'll be working on the traits that should just be grinding through all the calls twice (I might be tempted to write a Rust macro for this, but maybe I'll just do a vim macro to manually convert them) to build the traits.

The end result should be two traits (SyncClient and AsyncClient) with all calls and then also a Batch object with all the calls but all 3 places internally should use the requests module.

stevenroose · 2023-04-09T17:04:44Z

This is almost finished, but blocked on rust-bitcoin/rust-bitcoin#1785 since this create bumped the rust-bitcoin dep to 0.30.

Kixunil · 2023-04-09T17:48:41Z

Is there any chance of having generic HTTP implementation so that hyper can be optionally used?

Kixunil

I only reviewed this lightly.

Kixunil · 2023-04-09T17:51:05Z

client/src/lib.rs

+        let bytes = bitcoin::consensus::encode::serialize(self);
+        HexParam::write_hex(&bytes[..], f)
+    }
+}


bitcoin 0.30 has direct support for this which is also significantly more performant. See bitcoin::consensus::serde.

Ah yeah I focussed on optimizing all the parsing stuff but didn't really look too much at the input stuff. This is indeed quite a silly impl.

Hmm, the consensus::serde stuff is interesting, I'll take a look.

Kixunil · 2023-04-09T17:58:03Z

client/src/requests.rs

+into_json_from!(u64);
+into_json_from!(i32);
+into_json_from!(i64);
+into_json_from!(f64);


I'm not sure what these do but it looks like maybe you're trying to do what erased-serde should be used for?

Hmm, I don't think it's relased to the erased crate. But arguably it's just a way to limit the Into<serde_json::Value> to anything not-string.

Kixunil · 2023-04-09T18:02:05Z

json/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "bitcoincore-rpc-json"
-version = "0.16.0"
+version = "1.0.0-rc0"


This looks quite bold considering crazy-new change and the amount of breaking changes at Core.

Yeah version is a draft. Can be anything, but I'd like to be able to release rc's while also still being able to patch the current branch. So either it would be 0.20 or something or 1.0. Doesn't have to be too bold, we can go until 2^64-1 probably with versions ;)

1.0+ means there should be minimal if any breaking changes.

Kixunil · 2023-04-09T18:03:14Z

json/src/lib.rs

-    pub fr_75th: Amount,
-    #[serde(with = "bitcoin::util::amount::serde::as_sat")]
-    pub fr_90th: Amount,
+    #[serde(with = "serde_feerate::sat_per_vb")]


Oh, we should add this to bitcoin.

Kixunil · 2023-04-09T18:04:18Z

json/src/lib.rs

+
+/// Type to represent feerates.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct FeeRate {


We already have this in bitcoin.

stevenroose · 2023-04-09T18:05:36Z

Is there any chance of having generic HTTP implementation so that hyper can be optionally used?

I'm planning a compile feature to support hyper, yeah. IIRC there is already code for that in the jsonrpc crate, in this crate it would be simply having a feature-gated constructor, I guess that uses that one.

stevenroose · 2023-04-11T17:23:02Z

I'm going to close this and open a new one.

This was referenced Mar 1, 2022

Move lightning-block-sync's HTTP logic into here #166

Open

Async version of bitcoincore-rpc #78

Open

RCasatta reviewed Mar 1, 2022

View reviewed changes

stevenroose force-pushed the syncasync branch from e4c4577 to e5f1a69 Compare March 2, 2022 14:03

jkczyz reviewed Mar 2, 2022

View reviewed changes

TheBlueMatt reviewed Mar 4, 2022

View reviewed changes

sr-gi mentioned this pull request Mar 11, 2022

Merge bitcoin_cli::BitcoindClient and bitcoincore_rpc::Client talaia-labs/rust-teos#42

Open

stevenroose force-pushed the syncasync branch from e5f1a69 to 46a9ef0 Compare April 8, 2022 20:38

sander2 mentioned this pull request Nov 2, 2022

Switch to async bitcoin rpc interlay/interbtc-clients#406

Open

stevenroose force-pushed the syncasync branch from 1c315a2 to dcc6c7d Compare November 29, 2022 20:08

stevenroose force-pushed the syncasync branch from dcc6c7d to a2de6ec Compare March 29, 2023 00:32

stevenroose mentioned this pull request Apr 9, 2023

Use marker type to enforce validation of Address's network rust-bitcoin/rust-bitcoin#1489

Merged

Kixunil reviewed Apr 9, 2023

View reviewed changes

stevenroose force-pushed the syncasync branch 3 times, most recently from c1e7e7b to 029e778 Compare April 10, 2023 23:36

stevenroose added 2 commits April 11, 2023 04:50

ci: Remove rustfmt requirement

981a7fe

sync/async refactor

7df1966

stevenroose force-pushed the syncasync branch from 029e778 to 7df1966 Compare April 11, 2023 17:16

stevenroose closed this Apr 11, 2023

stevenroose mentioned this pull request Apr 11, 2023

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] generalized requests to support sync and async #212

[POC] generalized requests to support sync and async #212

stevenroose commented Mar 1, 2022 •

edited

Loading

RCasatta Mar 1, 2022

JeremyRubin commented Mar 2, 2022

RCasatta commented Mar 2, 2022

JeremyRubin commented Mar 2, 2022

jkczyz Mar 2, 2022

JeremyRubin Mar 3, 2022

jkczyz Mar 4, 2022

jkczyz Mar 2, 2022

TheBlueMatt Mar 4, 2022

JeremyRubin Mar 5, 2022

sander2 commented Mar 22, 2022

JeremyRubin commented Mar 23, 2022

stevenroose commented Apr 25, 2022

stevenroose commented Apr 9, 2023

Kixunil commented Apr 9, 2023

Kixunil left a comment

Kixunil Apr 9, 2023

stevenroose Apr 9, 2023

stevenroose Apr 9, 2023

Kixunil Apr 9, 2023

stevenroose Apr 9, 2023

Kixunil Apr 9, 2023

stevenroose Apr 9, 2023

Kixunil Apr 9, 2023

Kixunil Apr 9, 2023

Kixunil Apr 9, 2023

stevenroose commented Apr 9, 2023

stevenroose commented Apr 11, 2023

[POC] generalized requests to support sync and async #212

[POC] generalized requests to support sync and async #212

Conversation

stevenroose commented Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

JeremyRubin commented Mar 2, 2022

RCasatta commented Mar 2, 2022

JeremyRubin commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sander2 commented Mar 22, 2022

JeremyRubin commented Mar 23, 2022

stevenroose commented Apr 25, 2022

stevenroose commented Apr 9, 2023

Kixunil commented Apr 9, 2023

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Apr 9, 2023

stevenroose commented Apr 11, 2023

stevenroose commented Mar 1, 2022 •

edited

Loading