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

Generate ownership proof for SessionKeys #48

Merged
merged 3 commits into from
May 7, 2024

Conversation

bkchr
Copy link
Contributor

@bkchr bkchr commented Nov 13, 2023

This RFC changes the SessionKeys runtime api to support generating a proof of ownership of the generated session keys.

```

The `proof` being a SCALE encoded tuple over all signatures of each private session
key signing the `owner`. The actual type of each signature depends on the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the message being signed is the SCALE-encoded account_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

pub proof: Vec<u8>,
}

fn SessionKeys_generate_session_keys(owner: Vec<u8>, seed: Option<Vec<u8>>) -> OpaqueGeneratedSessionKeys;
Copy link
Contributor

@tomaka tomaka Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An AccountId is a u64 I believe. How does that translate to a Vec<u8>?
I suppose through SCALE encoding, but since the function signature here is the "high level one", shouldn't it be u64 here? Otherwise, the AccountId gets SCALE-encoded twice, no?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd presumably sign the actual controller key here, not some reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An AccountId is a u64 I believe. How does that translate to a Vec<u8>?

AccountId for Polkadot is an enum with public keys of sr/ed25519 or ecdsa. Other chain can also have different account ids. Thus, this should be a Vec<u8> that represents the SCALE encoded account id.

Comment on lines 20 to 22
When a user sets new `SessionKeys` on chain the chain can currently not ensure that the user
actually has control over the private keys of the `SessionKeys`. With the RFC applied the chain is able
to ensure that the user actually is in possession of the private keys.
Copy link
Contributor

@tomaka tomaka Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worded differently, you want to give the possibility to the implementation of SessionKeys_generate_session_keys to know which AccountId the host controls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I mean generate_session_keys doesn't care as it just signs whatever you pass it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least for me, the motivation does not go deep enough. Why should the chain care?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should the chain care?

Care that you are in possession of the keys? The chain doesn't really need to care. However, someone could may include a transaction faster than you with your public keys registered. So, someone could theoretically prevent that a validator is rotating its keys.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL. Ok, makes sense! Thanks!

@burdges
Copy link

burdges commented Nov 14, 2023

In this RFC, all session keys first acknowledge their controller key by signing it, after which their controller key certifies them. Yes, we do obtain the desired back certificates this way, but I'd envisioned doing this in part the other way round, plus adding a master session key.

It'd look like this:

  1. All other subordinate session keys back certify this master session key, likely the grandpa key.
  2. The master session key certifies all other subordinate session keys, along with their certificates of the master session key, probably as pairs (subordinate_session_key,its_signature_on_grandpa_key).
  3. Operator certifies the master session key with their controller key.
  4. At node start, the master session key certifies the controller key along with both its certificate of the master session key and a random nonce. We call this the active certificate.
  5. Pre-sync, the starting node announces its certificate collection, including this new one created upon startup.
  6. An already active synced node posts the active certificate on-chain via some intrinsic, replacing the previous one.
  7. The starting node waits until its active certificate get finalized before running protocols with equivocation slashing, like grandpa or babe.
  8. Any node shuts down if its active certificate get replaced on-chain.

In emergencies, a command line option could overrides 7, and maybe this could happen automatically, but usually 7 prevents nodes being slashed for operator error.

In this, 1-2 have same orientation as proposed here, but 3-4 have the reversed orientation. We must check a two step certificate chain, including two back certificates, before believing a session key, but this should be a rare enough it does not matter. Our nonce in 4 could be some ephemeral key like maybe the transport layer key.

Anyways..

If there is a pressing need to do something simpler first then fine, and validator operators rarely get slashed for equivocations these days, maybe they get 0% slashed or we refund them, but it's maybe worth defending against the equivocations footgun like this. A master session key permits changing session keys without impacting user experience for validator operators, and consolidates the nonce lock under one session key, but this could be done without the master session key.

@bkchr bkchr changed the title RFC: Generate ownership proof for SessionKeys Generate ownership proof for SessionKeys Nov 22, 2023
The RPC that exists around this runtime api needs to be updated to support passing the account id
and for returning the ownership proof alongside the public session keys.

UIs would need to be updated to support the new RPC and the changed on chain logic.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will they break with this update? Or is there a deprecation/upgrade path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a new RPC for this. So, they will not break. However when the runtime is upgraded it will require the proofs which only the new UI generates. There is also only polkadot-js which supports this AFAIK.


## Summary

This RFC proposes to changes the `SessionKeys::generate_session_keys` runtime api interface. This runtime api is used by validator operators to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, why do we need a runtime API to generate keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the node doesn't know what session keys the runtime is using.

Copy link

@davxy davxy Apr 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of generating the keys in the runtime, which in fact ends up generating the keys via some host call, would be nice to have instead the runtime returning a list of (CryptoTypeId, KeyTypeId).
Then generate the keys directly using the keystore.

The advantage of this approach would be to deprecate the host calls to generate the keys from the runtime which (IMO) is a bit awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These host calls are also required for benchmarking to generate private keys. CryptoTypeId didn't exist at the time this runtime api was introduced. This is also really the topic of this RFC.

In the end you also still need to call into the runtime to do this. In hindsight I would also design some of the host calls differently to make nodes for example "more generic". This can be done, but yeah isn't part of this RFC and this RFC will also not make any of that more complicated.

@bkchr
Copy link
Contributor Author

bkchr commented Apr 24, 2024

/rfc propose

@paritytech-rfc-bot
Copy link
Contributor

Hey @bkchr, here is a link you can use to create the referendum aiming to approve this RFC number 0048.

Instructions
  1. Open the link.

  2. Switch to the Submission tab.

  1. Adjust the transaction if needed (for example, the proposal Origin).

  2. Submit the Transaction


It is based on commit hash adb59579ee424853a3174740b65e991a2a8d31aa.

The proposed remark text is: RFC_APPROVE(0048,2c5d27eb220a205e0c6634379a4a9cf6b6208362de7eb22660305f5dcfef7ab3).

@burdges
Copy link

burdges commented Apr 25, 2024

We'd reduce slashing risks considerably if we handle this like I propsed above: #48 (comment)

Copy link

Voting for this referenda is ongoing.

Vote for it here

Copy link

github-actions bot commented May 7, 2024

PR can be merged.

Write the following command to trigger the bot

/rfc process 0x34f2bcbf3ef8b5ef7d7ab74cef6d391c1f6aee227daa5a67108f9e9c8ffd00aa

@bkchr
Copy link
Contributor Author

bkchr commented May 7, 2024

/rfc process 0x34f2bcbf3ef8b5ef7d7ab74cef6d391c1f6aee227daa5a67108f9e9c8ffd00aa

@paritytech-rfc-bot paritytech-rfc-bot bot merged commit 4d0a344 into main May 7, 2024
@paritytech-rfc-bot
Copy link
Contributor

The on-chain referendum has approved the RFC.

@bkchr bkchr deleted the bkchr-session-keys-runime-api branch May 7, 2024 20:43
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

Successfully merging this pull request may close these issues.

None yet

5 participants