Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we remove sudo_sessionKeys_unstable_generate? #105

Closed
bkchr opened this issue Sep 26, 2023 · 8 comments
Closed

Should we remove sudo_sessionKeys_unstable_generate? #105

bkchr opened this issue Sep 26, 2023 · 8 comments

Comments

@bkchr
Copy link
Member

bkchr commented Sep 26, 2023

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
Copy link
Contributor

tomaka commented Sep 27, 2023

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
Copy link
Member Author

bkchr commented Sep 27, 2023

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
Copy link
Contributor

lexnv commented Sep 27, 2023

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
Copy link
Contributor

tomaka commented Sep 27, 2023

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
Copy link
Member Author

bkchr commented Sep 27, 2023

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
Copy link
Contributor

lexnv commented Sep 28, 2023

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

@tomaka
Copy link
Contributor

tomaka commented Sep 28, 2023

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
Copy link
Member Author

bkchr commented Sep 29, 2023

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: paritytech/polkadot-sdk#1739

@bkchr bkchr closed this as completed Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants