Make wildcard storage change subscriptions RPC-unsafe #11259
Conversation
koute
added
A0-please_review
koute
added
the
D3-trivial 🧸
| fn from(_: UnsafeRpcError) -> rpc::Error { | ||
| rpc::Error::method_not_found() | ||
| fn from(error: UnsafeRpcError) -> rpc::Error { | ||
| rpc::Error { code: rpc::ErrorCode::MethodNotFound, message: error.to_string(), data: None } |
There was a problem hiding this comment.
We should really emit another error code for this.
The method is still found but denied because it's unsafe.
Perhaps out of scope for this PR...
There was a problem hiding this comment.
Personally I agree, although the specs says this:
-32601 Method not found The method does not exist / is not available.
So technically this does fall under the "is not available". Unfortunately the specs don't define a "forbidden" error code, so we'd have to use one of the server error codes for that, which... is probably fine? I don't know how those error codes are used by the consumers of our API though; will changing this error code break anything for anyone? (Which I imagine it could, if someone's already using the current error code to detect this situation.)
But yeah, that's out-of-scope for this PR; I just wanted to make the situation at least a little better while I was at it.
…orage_wildcard_subscription
|
bot merge |
|
|
Sorry, I don't quite get what you're asking here. Could you rephrase the question? |
* When an RPC is rejected because it's RPC-unsafe say why in the error message * Make wildcard storage change subscriptions RPC-unsafe * Apply suggestions from code review Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Fix typo * Fix tests Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
* When an RPC is rejected because it's RPC-unsafe say why in the error message * Make wildcard storage change subscriptions RPC-unsafe * Apply suggestions from code review Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Fix typo * Fix tests Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
* When an RPC is rejected because it's RPC-unsafe say why in the error message * Make wildcard storage change subscriptions RPC-unsafe * Apply suggestions from code review Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Fix typo * Fix tests Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
This PR changes the wildcard storage change subscriptions (that is: subscriptions to all of the changes) to be RPC-unsafe. (So they can only be made if the node is running with
--rpc-methods=unsafeenabled.)Subscriptions to concrete keys are unchanged and are still RPC-safe.
This is a breaking change. If this breaks your use case or you know someone whose use case this will break now is the time to speak up.
Why do this?
See this issue for more details: paritytech/polkadot-sdk#61 (in particular the second part of this comment)
Basically, the use cases for wildcard storage change subscriptions are somewhat niche, and since this API will send out all of the storage changes for every block that is imported and will not send any fixup notifications when those changes end up being not finalized (or if previously unseen changes from before the subscription was made will get finalized) it requires extra care to use correctly. If you use a wildcard subscription you either need to 1) not care about this (because you just want a list of all of the changes in every block, and you don't care what the finalized state will be), or 2) track all of the changes between the last finalized block and the new one and traverse though it on every block finalization and gather the changes yourself.
This is not obvious when casually using the API, hence the RPC-unsafe designation to discourage casual use.
Wildcard subscriptions are also something that the light client doesn't support, and AFAIK we want light clients to be the new default way to interface with the network in the future anyway, so this is also a reason to discourage casual use of wildcard subscriptions.
cc @jacogr
One more thing
I've also changed the error message when you call an RPC-unsafe API when the node doesn't allow RPC-unsafe calls; before the changes you'd get an error that the method was not found, and now you get told that the call was rejected because it's RPC-unsafe. This should hopefully help with any "why did they remove this method?!" confusion that anyone who was using this API may face.