paritytech / json-rpc-interface-spec

29 stars 3 forks source link

Add `sudo_network_unstable_watch` #91

Closed tomaka closed 8 months ago

tomaka commented 1 year ago

This is a revival of https://github.com/paritytech/smoldot/issues/2245

This new function lets the JSON-RPC client subscribe to the networking state of the server, which allows visualizing what is happening, which in turn should be useful for debugging and teaching purposes.

One opinionated decision I took is that the JSON-RPC server might omit some events if the events are generated too quickly. For example, if a peer-to-peer connection opens then closes repeatedly 1000 times per second, we don't have to send 2000 events per second to the JSON-RPC client and instead we're allowed to not send anything. This means that the JSON-RPC client might not be aware of everything that's happening. There's an event to notify it when that happens so that a warning or something can be shown to the user.

An alternative would be to report everything and generate a stop event if the queue is too large, but that's not much better, because the JSON-RPC client wouldn't be able to know what happens between the moment when the stop happens and the moment when it resubscribes. In other words, this wouldn't solve the problem and is more complicated.

There's no possible way (CAP theorem) to guarantee that the JSON-RPC client will receive events about everything that happens, unless you slow down the networking of the node to the speed of the JSON-RPC client. This isn't something that we want to happen, normally. Maybe we could imagine a privileged mode where that is the case, but I haven't included this in this PR at the moment because it seems overkill.

tomaka commented 10 months ago

As a heads up, I'm going to implement this in smoldot (merged or not). It's one of the milestones that I've put in the smoldot financing treasury proposal.

josepot commented 8 months ago

I have reviewed this PR and I think that it's ready to be merged. Also, I can't wait to leverage this from polkadot-api and it seems that smoldot has already made a lot of progress.

As @tomaka already explained: this API is mostly meant for light-clients and it lives in its own group of functions. Meaning that a simple query to rpc_methods would let the consumer know whether this API is supported. Perhaps, it could make sense to have a separate/complementary API for full-nodes/substrate?

tomaka commented 8 months ago

I've encountered an issue while implementing this in smoldot due to a corner case.

Let's say that you start an instance of smoldot and add the Polkadot and Kusama chains, and add the same IP address and PeerId as bootnode at the same time to both Polkadot and Kusama.

When I wrote this RFC, smoldot would open networking connections for Polkadot and networking connections for Kusama. That's what is reflected in this RFC. In that example scenario, smoldot would open two connections to the same IP address and PeerId, and thus would report through the JSON-RPC function only events that concern the connection of the chain corresponding to that JSON-RPC subscription.

However, smoldot now supports sharing connections. In that example scenario, smoldot would only open one connection and use the same connection for both Polkadot and Kusama. So let's imagine that smoldot connects to that IP address, reports a JSON-RPC event about the connection being open to the active subscriptions, but then realizes that the node is only capable of serving Polkadot and not Kusama. Previously, smoldot would close the Kusama connection and keep the Polkadot one, but now smoldot would keep the connection alive anyway and simply ignore that node when it comes to Kusama.

This leads to a tricky question: should smoldot, in that case, generate an event to the Kusama JSON-RPC subscription that the connection has been closed, even though it hasn't actually been closed? Doing so would be the logical thing to do, but would be misleading. The objective of this JSON-RPC function is to show to the user what smoldot is doing, and reporting events that don't match the reality (even when logically coherent) goes against this objective.

An alternative would be to report every single connection to every subscription, no matter if this connection is going to be used for the chain the subscription corresponds to. This is also misleading and spammy.

I don't really know right now how to solve that problem.

josepot commented 8 months ago

This leads to a tricky question: should smoldot, in that case, generate an event to the Kusama JSON-RPC subscription that the connection has been closed, even though it hasn't actually been closed?

I'm inclined to think that smoldot should generate an event for the Kusama JSON-RPC subscription indicating the connection is closed, focusing on the "logical connection".

From the API consumer's perspective, the connection is effectively "closed" once Smoldot ceases communication with the Kusama node, regardless of the physical connection status. This approach would deem the persistence of the physical connection as an implementation detail.

Given the API is primary intended to be implemented by light-clients, it would be beneficial to update the specification to clearly differentiate between "logical" and "physical" connections, emphasizing that the API's notifications pertain to the "logical connection".