Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

possibly restrictive bound on sp_session::SessionKeys in spawn_tasks #14198

Closed
kianenigma opened this issue May 23, 2023 · 4 comments
Closed
Labels
Z7-question Issue is a question. Closer should answer.

Comments

@kianenigma
Copy link
Contributor

kianenigma commented May 23, 2023

Is it beyond imagination for a node to not have session keys at all? What is the reasoning behind this?

For example, a runtime+node that is configured to use manual seal (eg. the smart-contract-node) must provide this useless implementation.

https://github.com/paritytech/substrate-contracts-node/blob/3419c499ed32a754df3298f9d744e0858ee451be/runtime/src/lib.rs#L484-L494

related paritytech/polkadot-sdk#186. I am trying to minimize the noise+boilerplate needed to setup a runtime+node that has the least number of pallets, and works with manual seal.

Similar example with the Metadata api: #11743

@kianenigma kianenigma added the Z7-question Issue is a question. Closer should answer. label May 23, 2023
@bkchr
Copy link
Member

bkchr commented May 23, 2023

The node doesn't know the format of the session keys nor what kind of session keys exists. So, it needs to call in the runtime to get this info.

The generate method is for example used for the CLI --alice etc. The decode is required when you want to check if particular session keys are registered at the node.

paritytech/polkadot-sdk#27 this will remove the compile time bound and make it a runtime error. Then we can make it optional and complain to users if they want to use something, but they don't have the runtime api not implemented.

@kianenigma
Copy link
Contributor Author

The node doesn't know the format of the session keys nor what kind of session keys exists. So, it needs to call in the runtime to get this info.

If a node is using manual seal, or potentially other consensus primitives that don't require a session key, why should it need that?

paritytech/polkadot-sdk#27 this will remove the compile time bound and make it a runtime error. Then we can make it optional and complain to users if they want to use something, but they don't have the runtime api not implemented.

Okay, this sounds better.

All in all, it is unclear to me what are our assumptions around session keys. Is that a mandatory, substrate-wide concept? Or is it optional, based on some consensus primitives? the code is the former, but the latter seems more correct to me.

@bkchr
Copy link
Member

bkchr commented May 24, 2023

Or is it optional, based on some consensus primitives?

Partly optional, but not dependent on consensus primitives. Stuff like im-online or authority discovery have their own session keys as well. So, it is a substrate-wide concept. Nevertheless, it should be optional.

@kianenigma
Copy link
Contributor Author

I will close this in favor of paritytech/polkadot-sdk#27

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z7-question Issue is a question. Closer should answer.
Projects
Status: done
Development

No branches or pull requests

2 participants