Have ability to validate with multiple keys on the same node

[folsen]

In refactoring in #3296 we came across the idea that we'd like to have the ability to have multiple "validator keys" in the keystore to be able to run multiple validators from the same node.

So for instance in GRANDPA we would replace something like this:

fn is_voter(
	voters: &Arc<VoterSet<AuthorityId>>,
	keystore: &Option<KeyStorePtr>,
) -> Option<AuthorityPair> {
	match keystore {
		Some(keystore) => voters.voters().iter()
			.find_map(|(p, _)| keystore.read().key_pair::<AuthorityPair>(&p).ok()),
		None => None,
	}
} 

To something like this:

fn is_voter(
	voters: &Arc<VoterSet<AuthorityId>>,
	keystore: &Option<KeyStorePtr>,
) -> Vec<AuthorityPair> {
	match keystore {
		Some(keystore) => voters.voters().iter()
			.filter_map(|(p, _)| keystore.read().key_pair::<AuthorityPair>(&p).ok()).collect(),
		None => vec![],
	}
}

However, the change is not this simple. Currently (as I understand it) a future for GRANDPA is started in run_grandpa_voter which is essentially a state machine keeping track of voting behavior for one validator, picking the key for that validator with the is_voter function above. Basically just picking the first key it finds that matches an active validator for the Era.

If we wanted to vote on behalf of multiple validators, we would need a way to have one of these state machines per key that we have, instead of just looking for the first active key we can find.

This is a bit of an inversion of responsibility, where now the GRANDPA voter is responsible for looking for a key that is active and using that (if there are multiple it would be like an arbitrary choice of any active key). We would need to change this around to have something push the keys to use to the future, which means we need to re-instantiate the future somehow when the keys rotate and we get new session keys in place in the keystore.

This would have some effect on networking as well, as we can't associate an address with a single validator anymore, but behind one address might be multiple validators.

Have talked this through with @andresilva who considers this non-trivial. Would like to hear thoughts from @rphmeier and @tomaka as well who might have some insight into how to best achieve this goal.

Hopefully I'm clear enough in this issue about what needs to be accomplished and the problems I see right now.

[tomaka]

This exact change is something I've had in mind for a long time, and requires as a first step #3097, which is blocked by refactoring the service creation code, which itself is blocked by #3296.

In the long term, I don't even see the need for the keystore. The key should just be read from the RPC endpoint and we'd directly modify the GrandPa state machine, as you described, and to me the reason why the keystore exists at all is to bypass the current issues with the code design.

In the short term, I don't really know what to do. We can pile up another hack by creating a channel between GrandPa and the keystore, or something like that.

This would have some effect on networking as well, as we can't associate an address with a single validator anymore, but behind one address might be multiple validators.

I don't think this has an effect on the networking, unless the GrandPa code somehow relies on the PeerId.

[folsen]

In the short term, I don't really know what to do. We can pile up another hack by creating a channel between GrandPa and the keystore, or something like that.

IMO (though @gavofyork might disagree) this is not needed for short-term and would accept getting this in at the end of those refactorings assuming we highly prioritize getting those done. Personally I’d rather work on that refactoring rather than a hack to get this in.

[burdges]

Are these these use cases in mind?

• implement in one node a bridge between substrate chains using grandpa
• run babe as block production on parachains

[tomaka]

Are these these use cases in mind?

The point is that the code should be designed in such a way that the use case doesn't matter.
All that the GRANDPA code needs is access to a gossiping interface and to a database of blocks.
All that the BABE code needs is access to some database of blocks.
Code that can only be run once simultaneously is something we did in the 1990s and hopefully we won't do that in Substrate.

[bkchr]

In the long term, I don't even see the need for the keystore. The key should just be read from the RPC endpoint and we'd directly modify the GrandPa state machine, as you described, and to me the reason why the keystore exists at all is to bypass the current issues with the code design.

The RPC endpoint does not know anything about the key. We have 2 endpoints, one for setting one key to the keystore and another one that rotates all session keys at once. So in general, all this data is just opaque for the endpoint and it does not even know the number of sessions keys the runtime is using.
I don't see the keystore as any hack because of the current design. You need something that persists the keys and makes them available for all. You don't want to re-implement password protected key management in every part of Substrate.

We could provide some sort of channel that informs about new keys for a given key type, but that wouldn't solve the Grandpa problem. Babe couldn't also not do anything with the information at the point we insert a new key. After inserting a new key, it is not used by the consensus. You will first need to register the key in the runtime and wait for a new era for your new keys to be activated.

For Babe having multiple active validator keys at one node, shouldn't be any problem. Even if all validator keys for one slot are on one validator, producing multiple blocks would not make any sense at all. You maybe loose the uncle reward, but you also have configured multiple valid keys at one machine.
For Grandpa we have this problem as described by @folsen. It more looks like we should spawn multiple futures per authority rotation. If the authorities rotate, we look into the keystore and start a future for each authority we have on our node.

I don't see any reason at all why something like Grandpa should expose to the service the kind of keys it require. For Aura, that supports different types of keys, we need to set the type accordingly but not more. These "sub-services" or however you want to name them, should just be polled and anything else should be done internally.

[andresilva]

For BABE we can have the proposer try all the available keys that are active in the current set. This is easier than spinning one proposer per-key because we don't need to track authority set changes (we check at proposal time), but it's less efficient and less concurrent.

For GRANDPA the internal state-machine can be divided into two parts: the finality-grandpa state machine (that runs the GRANDPA protocol), and keeping track of authority set changes to update state and restart the voter. For GRANDPA we need to spin up multiple voters, but since we already track authority set changes in the state machine, when one happens we check all keys available in the key store and start a voter task for each key that is in the authority set (we do the same thing on startup).

This feature also makes things like #3353 more complicated. What should we do if one GRANDPA voter fails? It doesn't seem reasonable to shutdown the node if we have other voters validating, but right now we also have no way of effectively notifying the user about it.

[burdges]

In the long term, I don't even see the need for the keystore. The key should just be read from the RPC endpoint and we'd directly modify the GrandPa state machine, as you described, and to me the reason why the keystore exists at all is to bypass the current issues with the code design.

I'm maybe miss-reading this, but.. I'd favor this flow:

1. an operator first sends an RPC request to generate a session key, or maybe a one part of a session key,
2. substrate creates the new key, stores the session key pair into the keystore, and return only the public keys to the operator, and
3. the operator signs the full session public key with their controller key and returns this session certificate via the RPC.

A node might validate with multiple session keys merely by possessing multiple valid session certificates by different controller keys, but a later session certificate issued by the same controller key becoming valid should invalidate its older session certificate.

We do not want session secret keys to be handled manually or even exist outside the keystore and substrate process. If they do then validators will be slashed needlessly, ala cosmos.

In future, if running in SGX, then in step 2 the public key would also be signed by the SGX enclave, which becomes a promise to the operator and their nominators that the session secret key will never leave that SGX enclave.

[gterzian]

This exact change is something I've had in mind for a long time, and requires as a first step #3097, which is blocked by refactoring the service creation code, which itself is blocked by #3296.

The second link is to a merged PR, so I assume that is not a blocker anymore.

Re the first one(putting everything into Service), it might be a good idea to do (part-of-)it while solving a specific problem, such as the one highlighted here, as opposed to as a stand-alone refactoring.

Is anyone working on this already?

If not, I would be interested. If this requires moving grandpa into the service, it could be done at the same time, addressing part of #3097