Skip to content

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

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

Open
TheBlueMatt opened this issue Mar 2, 2021 · 5 comments
Open

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

TheBlueMatt opened this issue Mar 2, 2021 · 5 comments

Comments

@TheBlueMatt
Copy link
Member

Because we also support Bitcoin Core REST and potentially nginx/Cloudflare-proxied REST, we ended up with our own one-dependency HTTP client in rust-lightning. We currently use it with our own JSON parsing, though switching to this crate instead may be nice. In order to do that, we'd need to move the HTTP bits to here (they support async, but without the tokio feature, the "async" keyword on the functions is unused, just need a macro to figure out how to insert-or-not an async keyword), maybe in its own subcrate.

The current HTTP/RPC/REST client stuff is at https://github.com/rust-bitcoin/rust-lightning/tree/main/lightning-block-sync/src

@jkczyz
Copy link

jkczyz commented Mar 4, 2021

Note that there are still uses of await inside the async functions where they need to call other async functions.

@TheBlueMatt
Copy link
Member Author

Hmm, right, would need a macro to cover those, too, though if you took the std option and stripped them it should all work, just need to macro-ize it.

@stevenroose
Copy link
Collaborator

I wonder if it shouldn't be a common problem for API client packages that want to support both async and sync calls..

I might still have a branch where I tried to implement batching with potential async support in mind by having every API method return some kind of Request object that you could they call .sync(), .async() or .batch(&mut batch) on. So basically your call would be client.blockchain_info().sync()?;. I think I got stuck because you need to store closures inside the objects for some post-HTTP processing (like hex etc) and my experience with closures in Rust was very limited at the time.

Unless there is another common way to solve this problem in the ecosystem, I might give that a try.

@jkczyz
Copy link

jkczyz commented Mar 13, 2021

Seems like a reasonable approach, though it may take a little thought on how to refactor read_response. It uses a different type of TcpStream depending on whether its sync or async and wraps it with different types as well. There's also a place where we needed custom code since a dependency does not support async. See the conditional compilation for tokio here:

https://github.com/rust-bitcoin/rust-lightning/blob/2cb5b1af992263e219f47c1f09763ffd17fcda15/lightning-block-sync/src/http.rs#L193

sr-gi added a commit to talaia-labs/rust-teos that referenced this issue Sep 16, 2021
Adapts other actors to fit the Carrier in. Temporarily uses both rust-lightnings rpc and bitcoincore-rpc rpc clients. Waiting for both to be merged.

Related to:
rust-bitcoin/rust-bitcoincore-rpc#166
@stevenroose
Copy link
Collaborator

Please give some feedback on this idea I have, it might be relevant to this discussion: #212

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

No branches or pull requests

3 participants