Rework light client by jsdw · Pull Request #1475 · paritytech/subxt

jsdw · 2024-03-11T13:04:07Z

The main changes:

Move almost all light client code into subxt_lightclient; only a little glue in Subxt to integrate it.
LightClient struct to create light client instance given relay chain spec, and to create parachain connections on top of this.
LightClientRpc returned from each of the above which is the means to interact with the chain.
Subxt implements RpcClientT for this, so that it can be used to construct an OnlineClient.
Function provided for low level control but not exported yet (maybe best to hide Smoldot away unless somebody has a good reason to need to configure it directly.
Each light client RPC connection has an associated BackgroundTask. This isn't exposed yet, but can eventually be in order that users can drive it and exert backpressure if need be (I'm unsure what effect that would have on Smoldot though!).

Other changes along the way:

light client background task is now per-chain-connection so no need to combine and then separate messages from different chains, and in the future we can opt to drive each one manually if we want.
Handle error messages on subscriptions, too.
Generally split errors up so that we get quite specific erros back when constructing, calling or getting responses back from the light client stuff.

Things not done:

Exposing something like OnlineClient::from_chain_spec(); need a relay + parachain version etc, prob need a builder or something to make it nice. Instead I made it easier to instantiate an OnlineClient from an RPC client (impl Into) so it's actually pretty easy anyway now.
I didn't add a nice from_url to the ChainSpecConfig. I staretd doing this but it involved pulling in extra deps and feature flags etc, and the code was just easier to keep in subxt. So I exposed a utility function to fetch chain spec from url instead. I'm ok with this because it's actually only really a dev use case anyway; we prob shouldn't encourage it too much!

…chainspec from a URL

jsdw · 2024-03-12T15:04:49Z

lightclient/src/background.rs

+>;
+
+/// Type of subscription IDs we can get back.
+pub type SubscriptionId = String;


It was previously an unsigned int, but even if Smoldot currently does that, I didn't think we should rely on it since it probably also expects us to treat the valeus as opaque.

jsdw · 2024-03-12T15:05:41Z

lightclient/src/background.rs

+    to_backend: mpsc::UnboundedSender<Message>,
+}
+
+impl BackgroundTaskHandle {


I added this to give a nice "API" to interact with the background task, so that nothing outside of the backend task actually needs to care about messages and such

jsdw · 2024-03-12T15:09:03Z

lightclient/src/background.rs

+    /// Run the background task, which:
+    /// - Forwards messages/subscription requests to Smoldot from the front end.
+    /// - Forwards responses back from Smoldot to the front end.
+    pub async fn run(self) {


A simplified version of what was called start_task before. No longer takes in any args, so the API for BackgroundTask is super simple; just .run().await it to start it running (and in the future we can return something users can poll which will just wrap this).

jsdw · 2024-03-12T15:10:10Z

lightclient/src/background.rs

+                let from_back_fut = channels.from_back.next().fuse();
+            }
+
+            futures::select! {


I moved to this I think to avoid allocating somewhere or annoying lifetime things; can't remember exactly now. It just takes two channels as inputs though and so I think should have no cancellation concerns (this is the usual footgun with select!

Looks good, btw you asked me exactly this question "What is the usual usual footgun with select!" when interviewing me as a job applicant about a year ago.

I need to make a note somewhere because if I move to doing a code review style task then future::select! is def a candidate to put somewhere :D

you are good, streams are cancel-safe.

I like tokio::select better because it doesn't require pinning the futures

I normally prefer the function call version to avoid some footguns but iirc I had an issue that made it hard to use it nicely; I'll have to try again at some point!

jsdw · 2024-03-12T15:11:50Z

lightclient/src/background.rs

 struct ActiveSubscription {
    /// Channel to send the subscription notifications back to frontend.
-    sender: mpsc::UnboundedSender<Box<RawValue>>,
+    notification_sender: mpsc::UnboundedSender<Result<Box<RawValue>, JsonRpcError>>,


I tweaked subscriptions a bit to:

a) return any subscription notification errors that come back (previously these were being ignored I think) and
b) send back the ID and repsonse channel rather than one channel for ID and one for responses, just to simplify usage a little.

jsdw · 2024-03-12T15:13:19Z

lightclient/src/chain_config.rs

+}
+
+/// Configuration to connect to a chain.
+pub struct ChainConfig<'a> {


This can grow to have more options like about max json RPC subs etc if we want to expose them. A user can then either build this config and configure it, or pass a chain spec &str directly, which will convert into this easily.

jsdw · 2024-03-12T15:15:10Z

lightclient/src/rpc.rs

+    NotificationError {
+        /// RPC method that generated the notification.
+        method: String,
+        /// Subscription ID.
+        subscription_id: String,
+        /// Result.
+        error: Box<RawValue>,
+    },


This file is copied from the previous impl in background.rs, but I added this NotificationError to handle the case when we get errors back in subscriptions. Worth double checking that this is right :)

jsdw · 2024-03-12T15:17:35Z

subxt/src/utils/fetch_chain_spec.rs

+
+cfg_jsonrpsee! {
+    /// Fetch a chain spec from an RPC node at the given URL.
+    pub async fn fetch_chainspec_from_rpc_node(url: impl AsRef<str>) -> Result<Box<RawValue>, FetchChainspecError> {


I tried putting a fn like this (from_url) on ChainConfig but it required pulling lots of deps in etc, and since it's a dev use case mostly I was sortof ok with just exposing this util function instead in Subxt, since it's easier to do it here. Not the cleanest though!

lexnv · 2024-03-14T10:22:27Z

subxt/examples/light_client_parachains.rs

-    // Connecting to a parachain is a multi step process.
+    // Instantiate a light client with the Polkadot relay chain,
+    // and connect it to Asset Hub, too.
+    let (lightclient, polkadot_rpc) = LightClient::relay_chain(POLKADOT_SPEC)?;


Looks amazing!

lexnv

LGTM! I like the idea of having a background task per chain ID, this should isolate things quite nicely 👍

jsdw · 2024-03-14T11:09:38Z

I added back a compile error and docsrs thing that went missing, and updated the book, swapping the examples around so now connecting to parachains is the basic example, and connecting to a local node is more advanced :)

tadeohepperle

I think the interface is well designed, good job!

tadeohepperle · 2024-03-15T13:31:12Z

lightclient/src/background.rs

+                let from_back_fut = channels.from_back.next().fuse();
+            }
+
+            futures::select! {


Looks good, btw you asked me exactly this question "What is the usual usual footgun with select!" when interviewing me as a job applicant about a year ago.

lightclient/src/background.rs

tadeohepperle · 2024-03-15T13:36:06Z

lightclient/src/background.rs

+                    // Send the method response and a channel to receive notifications back.
+                    if pending_subscription
+                        .response_sender
+                        .send(Ok((sub_id.clone(), sub_rx)))


Could we avoid allocation here by using Arc<str> for subscription ids? Probably irrelevant though, feel free to ignore.

I'd guess it's not worth while because we (technically, but not really in reality) need to allocate a String anyway for the deserializing bit, and then the Arc is another alloc, and we onlyclone it once anyways :)

tadeohepperle · 2024-03-15T13:42:03Z

lightclient/src/lib.rs

+    /// https://docs.rs/wasm-bindgen-futures/latest/wasm_bindgen_futures/fn.future_to_promise.html.
+    pub fn parachain<'a>(
+        &self,
+        chain_config: impl Into<ChainConfig<'a>>,


This interface with the relay_chain and parachain functions looks really clean.

Polkadot-Forum · 2024-04-04T08:02:55Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/parity-tech-update-for-march/7226/1

jsdw added 9 commits March 8, 2024 17:19

WIP second pass over light client code for simpler API

2607d22

First pass new light client

952a805

pub(crate) LightClientRpc::new_raw(), and fmt

0300a44

Update examples and add back a way to configure boot nodes and fetch …

6af49f4

…chainspec from a URL

Fix light client examples

440d7d2

remove unused deps and tidy lightclient feature flags

66147a4

fix wasm error

f7f0219

LightClientRpc can be cloned

fef750c

update light client tests

23383bf

jsdw commented Mar 12, 2024

View reviewed changes

jsdw added 3 commits March 12, 2024 15:21

Other small fixes

b2d4128

exclude mod unless jsonrpsee

0974044

Fix wasm-lightclient-tests

8d67d1a

jsdw marked this pull request as ready for review March 12, 2024 16:50

jsdw requested a review from a team as a code owner March 12, 2024 16:50

Merge branch 'master' into jsdw-lightclient-2

4072072

jsdw mentioned this pull request Mar 13, 2024

Allow either backend to be used in light client code #1455

Closed

lexnv reviewed Mar 14, 2024

View reviewed changes

lexnv approved these changes Mar 14, 2024

View reviewed changes

jsdw added 3 commits March 14, 2024 10:52

add back docsrs bit and web+native feature flag compile error

b2a5a22

update book and light client example names

0577fb6

Merge branch 'master' into jsdw-lightclient-2

a4eadfc

fix docs

0601fb5

tadeohepperle approved these changes Mar 15, 2024

View reviewed changes

jsdw merged commit b069c44 into master Mar 15, 2024

jsdw deleted the jsdw-lightclient-2 branch March 15, 2024 15:21

This was referenced Mar 21, 2024

LightClient: Higher level API for multi-chain usecase #1253

Closed

Prep to release 0.35 #1489

Merged

Conversation

jsdw commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lexnv left a comment

Choose a reason for hiding this comment

Uh oh!

jsdw commented Mar 14, 2024

Uh oh!

tadeohepperle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Polkadot-Forum commented Apr 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jsdw commented Mar 11, 2024 •

edited

Loading