Change the chainHead functions to have proper limits

[tomaka]

Close #58
Close #23
Close #22
Close #63

See discussion in #58

The list of changes are:

• chainHead_storage, chainHead_body, and chainHead_call are no longer subscriptions but simple request-responses.
• chainHead_storage, chainHead_body, and chainHead_call now return a JSON object rather than just a subscriptionId. This JSON object indicates whether the operation could be successfully started.
• The notifications that were previously generated by chainHead_storage, chainHead_body and chainHead_call are now generated by the chainHead_follow instead, and now have an operation- prefix.
• chainHead_storageContinue has been renamed to chainHead_continue and must now be passed a followSubscriptionId.
• chainHead_stopBody, chainHead_stopStorage and chainHead_stopCall have been merged into chainHead_stopOperation and must now be passed a followSubscriptionId.
• In the case of chainHead_storage, when a subscription successfully starts, is also indicated the number of items that were ignored. Note that it is possible for all items to be discarded, in which case the subscription will just end with "event": "done" right after.
• Removed networkConfig from everywhere. This turned out to be too difficult to implement for the server when it needs to manage its own resources.
• Removed the disjoint events from chainHead_storage, chainHead_body, and chainHead_call, and replace them with a limitReached return value. Even though this situation is more appropriate as a return value, it was previously an event in order to make the API easier to use. Since we're switching to returning an object instead of just a subscriptionId, might as well handle disjoint the proper way as well.
• chainHead_follow might now return a JSON-RPC error if there are already more than 2 active subscriptions.
• In the case of chainHead_storage, it is no longer forbidden to generate an error event after some items notifications have been generated.

Note that error is still events rather than a return value. In the case of chainHead_unstable_call, a light client must first download a bunch of stuff, and then only later an error might happen, and thus it is more appropriate as an event.
In the case of chainHead_unstable_storage, the error could be a return value, but I kept it as an event for consistency.

Remarks for JSON-RPC client developers

When it comes to chainHead_follow, there is a minimum of two active chainHead_follow subscriptions per client. This means that you normally don't need to handle a situation where a JSON-RPC error is returned.

When it comes to storage requests, body requests, and call requests, the requests to start should be treated as if they were a queue. If limitReached is returned or discardedItems is non-zero, just pause the queue. If the queue is too large, you must either discard unnecessary entries or show error messages to the user. Maybe do read about queue theory on the Internet to understand what this is about.

[josepot]

The refactor on my end is going to be quite significant 😅 . However, I'm convinced that this is a much better approach. Thanks @tomaka !

[lexnv]

Nice! With this, we'd simplify code from substrate's point of view! 👍

[tomaka]

I've resolved the big conversation and pushed a bunch of commits for the second attempt.

[josepot]

LGTM! Thanks @tomaka !

[lexnv]

nit: Should the event here be operation-body-done to keep in sync with the explination from below and with "event": "operation-storage-items"?

[lexnv]

Amazing! Thanks @tomaka! 👍

[jsdw]

Generally looks good to me, and I appreciate the compromise on subscription bits which will give us time to re-think the JSON-RPC bits without blocking this stuff being implemented :)

Just a couple of things I spotted:

• Should archive_unstable_storageContinue now be archive_unstable_continue?
• Related to a different PR but since I noticed it now: it looks like archive_unstable_storage is now out of sync with the chainHead version re how it works; should it be updated to be in sync?
• Now that methods send notifications via the chainHead_follow subscription, I guess a downside is that if the follow subscription ends with "stop" (eg becasue it can't keep up), I'll also no longer get back the block body or whatever that I had asked for I think?

[tomaka]

Should archive_unstable_storageContinue now be archive_unstable_continue?
Related to a different PR but since I noticed it now: it looks like archive_unstable_storage is now out of sync with the chainHead version re how it works; should it be updated to be in sync?

The plan right now is work on and stabilize chainHead ASAP.

The chainHead functions fix the legacy JSON-RPC API in the sense that you can't properly do with the legacy API what you can do with chainHead.

archive however is just general clean ups, and nothing critical. If you're interested in doing archive calls you can use the legacy JSON-RPC API and it works.

For this reason I'm leaving it lagging behind a bit right now.

Now that methods send notifications via the chainHead_follow subscription, I guess a downside is that if the follow subscription ends with "stop" (eg becasue it can't keep up), I'll also no longer get back the block body or whatever that I had asked for I think?

This is something I've realized and I don't think it changes anything.

From the point of view of the JSON-RPC client, at any point in time a body/call/storage request can return inaccessible or disjoint. While the spec (before this PR) says that an operation continues even if stop is generated, a client actually has no guarantee that the operation will actually finish.

From the point of view of a light client server, the way it's implemented right now is: when body/call/storage is called, all the information necessary for the request is copied from the block and put in some local variables. Thanks to this, the request can continue even if the chainHead_follow dies.
Because right now a body/call/storage request can continue even when the follow dies, that's pretty much the only possible implementation strategy. You can't really do it differently than copying the information needed.
After this change, however, the implementation can be different and slightly more light. I don't think that in smoldot I will bother improving the implementation, but at least it could be possible.

[jsdw]

Thanks, that all makes sense to me; happy to see this merge!

[josepot]

From the point of view of the JSON-RPC client, at any point in time a body/call/storage request can return inaccessible or disjoint. While the spec (before this PR) says that an operation continues even if stop is generated, a client actually has no guarantee that the operation will actually finish.

From the point of view of a light client server, the way it's implemented right now is: when body/call/storage is called, all the information necessary for the request is copied from the block and put in some local variables. Thanks to this, the request can continue even if the chainHead_follow dies. Because right now a body/call/storage request can continue even when the follow dies, that's pretty much the only possible implementation strategy. You can't really do it differently than copying the information needed. After this change, however, the implementation can be different and slightly more light. I don't think that in smoldot I will bother improving the implementation, but at least it could be possible.

I just want to double-check my understanding here. In the event that the follow subscription emits a stop event while multiple operations are still in process, how does the spec handle this? Is there a guarantee that all ongoing operations will eventually reach a state of being fulfilled or errored? Or is it possible, as per the spec, that some operations might remain unresolved indefinitely?

For the sake of clarity, it would be beneficial if the spec explicitly states this. It should either confirm that all operations will ultimately be either fulfilled or errored, or conversely, state that some operations may potentially be left "in limbo". If I were to express my preference, I'd ideally like the spec to rule out the possibility of operations being left "in limbo" 😅.

EDIT:

Upon further reflection, I'm considering whether it might be more logical in the event of a stop event to assume that all ongoing operations have been "disjointed". If the client, by chance, happens to have data in memory that could fulfill some operations, the light-client should dispatch those events immediately prior to the stop event. This would make things a lot simpler IMO.

EDIT 2:

I just noticed that in fact the spec is very clear about this:

No more event will be generated with this subscription.

This effectively means that all ongoing operations are being disjoined, right?

[tomaka]

This effectively means that all operations are being disjoined, right?

Yes