search

paritytech / json-rpc-interface-spec

29 stars 3 forks source link

Should we remove `sudo_sessionKeys_unstable_generate`? #105

Closed bkchr closed 1 year ago

bkchr commented 1 year ago

As described in spec for sudo_sessionKeys_unstable_generate, this only maps to the SessionKeys runtime api. Given that, clients can just use state_call for calling the SessionKeys api.

tomaka commented 1 year ago

The problem is that you need to insert the keys in the keystore as well.

We could in principle instead add a JSON-RPC function to add a private key to the keystore, and let the JSON-RPC client do the runtime call, but sending private keys over the Internet or even in memory isn't the greatest idea.

bkchr commented 1 year ago

In Substrate the host functions for generating the key are storing the private keys. The RPC implementation isn't involved in this at all. The RPC implementation doesn't know what the runtime is doing.

lexnv commented 1 year ago

Just for reference, this is how the sessionsKeys implementation in substrate looks like, will put on hold that PR until we decide what's best

tomaka commented 1 year ago

Ah I see.

There's still a problem, though: since state_call (and chainHead_call/archive_call) are safe, we don't want everyone to be able to add keys to the keystore.

bkchr commented 1 year ago

Nodes that are validators should not expose any RPC functionality at all and for full nodes we could let them run with a ephemeral keystore.

lexnv commented 1 year ago

If I got this right, we could remove the sessionsKeys group. @tomaka what do you think would be best for end-users? 🤔

tomaka commented 1 year ago

I'm not sure.

It's clear to me that the runtime calls done through state_call and equivalents should be "pure" (i.e. the output only depends on the inputs). They shouldn't have access to the offchain worker functions (like time and randomness), because we don't want the JSON-RPC client to ask the node to start HTTP requests and to add transactions to the pool and things like that.

Another point is that it doesn't really make sense to be able to call sudo_sessionKeys_unstable_generate against a block other than the current best or finalized block (I'm actually not sure which one is appropriate).

I also disagree with "validator nodes should simply not allow public JSON-RPC requests". If we go this direction, it would be a subtle thing that validator operators should know, and if they don't know this they can suffer massive fuck ups. We should prevent potential fuck ups as much as possible instead of going the direction of "well people should just know better".

bkchr commented 1 year ago

They shouldn't have access to the offchain worker functions (like time and randomness), because we don't want the JSON-RPC client to ask the node to start HTTP requests and to add transactions to the pool and things like that.

Good point! I also realized that we actually do this already, aka register the keystore as extension for these runtime calls. So, they would fail in state_call.

Yeah, then this issue is actually not required! Ty for the input. However, I will open a pr to change the spec in the light of: https://github.com/paritytech/polkadot-sdk/pull/1739