From 13cbc991495c7343bdaa688f855c6cecc8f0ad2e Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Tue, 10 Oct 2023 23:55:59 -0400 Subject: [PATCH] Properly define the on-chain handover protocol The new key publishing `Batch`s is more than sufficient. Also uses the correct key to verify the published `Batch`s authenticity. --- coordinator/src/main.rs | 2 + docs/processor/Multisig Rotation.md | 10 ++-- substrate/in-instructions/pallet/Cargo.toml | 1 + substrate/in-instructions/pallet/src/lib.rs | 52 ++++++++++++--------- substrate/validator-sets/pallet/src/lib.rs | 26 +++++++---- 5 files changed, 55 insertions(+), 36 deletions(-) diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index e0891cf6..2132c56c 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -898,6 +898,8 @@ async fn handle_processor_messages( if id.attempt == 0 { MainDb::::save_first_preprocess(&mut txn, spec.set().network, id.id, preprocess); + // TODO: If this is the new key's first Batch, only create this TX once we verify + // all prior published `Batch`s Some(Transaction::Batch(block.0, id.id)) } else { Some(Transaction::BatchPreprocess(SignData { diff --git a/docs/processor/Multisig Rotation.md b/docs/processor/Multisig Rotation.md index 822eecca..ff5c3d28 100644 --- a/docs/processor/Multisig Rotation.md +++ b/docs/processor/Multisig Rotation.md @@ -122,9 +122,9 @@ The following timeline is established: Once all the 6 hour period has expired, no `Eventuality`s remain, and all outputs are forwarded, the multisig publishes a final `Batch` of the first block, plus `CONFIRMATIONS`, which met these conditions, regardless of if it - would've otherwise had a `Batch`. Then, it reports to Substrate has closed. - No further actions by it, nor its validators, are expected (unless those - validators remain present in the new multisig). + would've otherwise had a `Batch`. No further actions by it, nor its + validators, are expected (unless, of course, those validators remain present + in the new multisig). 7) The new multisig confirms all transactions from all prior multisigs were made as expected, including the reported `Batch`s. @@ -151,8 +151,8 @@ The following timeline is established: actually be achieved (at least, not without a ZK proof the published `Batch`s were correct). -8) The new multisig reports a successful close of the prior multisig, and - becomes the sole multisig with full responsibilities. +8) The new multisig publishes the next `Batch`, signifying the accepting of full + responsibilities and a successful close of the prior multisig. ### Latency and Fees diff --git a/substrate/in-instructions/pallet/Cargo.toml b/substrate/in-instructions/pallet/Cargo.toml index d8a312d7..fbaf8001 100644 --- a/substrate/in-instructions/pallet/Cargo.toml +++ b/substrate/in-instructions/pallet/Cargo.toml @@ -50,5 +50,6 @@ std = [ "in-instructions-primitives/std", "tokens-pallet/std", + "validator-sets-pallet/std", ] default = ["std"] diff --git a/substrate/in-instructions/pallet/src/lib.rs b/substrate/in-instructions/pallet/src/lib.rs index 83b882d2..22e95467 100644 --- a/substrate/in-instructions/pallet/src/lib.rs +++ b/substrate/in-instructions/pallet/src/lib.rs @@ -80,29 +80,21 @@ pub mod pallet { } } - fn key_for_network(network: NetworkId) -> Result { - // TODO: Get the latest session - let session = Session(0); - + fn key_for_network(network: NetworkId) -> Result<(Session, Option, Option), InvalidTransaction> { + let session = ValidatorSets::::session(network); let mut set = ValidatorSet { session, network }; - // TODO: If this session just set their keys, it'll invalidate any batches in the mempool - // Should there be a transitory period/future-set cut off? - if let Some(keys) = ValidatorSets::::keys(set) { - Ok(keys.0) - } else { - // If this set hasn't set their keys yet, use the previous set's - if set.session.0 == 0 { - // Since there haven't been any keys set, no signature can legitimately exist - Err(InvalidTransaction::BadProof)?; - } + let latest = ValidatorSets::::keys(set).map(|keys| keys.0); + let prior = if set.session.0 != 0 { set.session.0 -= 1; - - if let Some(keys) = ValidatorSets::::keys(set) { - Ok(keys.0) - } else { - Err(InvalidTransaction::BadProof)? - } + ValidatorSets::::keys(set).map(|keys| keys.0) + } else { + None + }; + // If there's no keys set, then this must be an invalid signature + if prior.is_none() && latest.is_none() { + Err(InvalidTransaction::BadProof)?; } + Ok((session, prior, latest)) } #[pallet::call] @@ -155,7 +147,7 @@ pub mod pallet { }; let network = batch.batch.network; - let key = key_for_network::(network)?; + let (current_session, prior, current) = key_for_network::(network)?; // verify the batch size // TODO: Merge this encode with the one done by batch_message @@ -164,10 +156,26 @@ pub mod pallet { } // verify the signature - if !key.verify(&batch_message(&batch.batch), &batch.signature) { + let batch_message = batch_message(&batch.batch); + // Check the prior key first since only a single `Batch` (the last one) will be when prior is + // Some yet prior wasn't the signing key + let valid_by_prior = if let Some(key) = prior { + key.verify(&batch_message, &batch.signature) + } else { false }; + let valid = valid_by_prior || (if let Some(key) = current { + key.verify(&batch_message, &batch.signature) + } else { false }); + if !valid { Err(InvalidTransaction::BadProof)?; } + // If it wasn't valid by the prior key, meaning it was valid by the current key, the current + // key is publishing `Batch`s. This should only happen once the current key has verified all + // `Batch`s published by the prior key, meaning they are accepting the hand-over. + if prior.is_some() && (!valid_by_prior) { + ValidatorSets::::retire_session(network, Session(current_session.0 - 1)); + } + // check that this validator set isn't publishing a batch more than once per block let current_block = >::block_number(); let last_block = LastBatchBlock::::get(network).unwrap_or(Zero::zero()); diff --git a/substrate/validator-sets/pallet/src/lib.rs b/substrate/validator-sets/pallet/src/lib.rs index 6db3bc24..4427d653 100644 --- a/substrate/validator-sets/pallet/src/lib.rs +++ b/substrate/validator-sets/pallet/src/lib.rs @@ -58,7 +58,7 @@ pub mod pallet { #[pallet::storage] pub type CurrentSession = StorageMap<_, Identity, NetworkId, Session, OptionQuery>; impl Pallet { - fn session(network: NetworkId) -> Session { + pub fn session(network: NetworkId) -> Session { if network == NetworkId::Serai { Session(pallet_session::Pallet::::current_index()) } else { @@ -206,12 +206,6 @@ pub mod pallet { let set = ValidatorSet { network, session }; Pallet::::deposit_event(Event::NewSet { set }); if network != NetworkId::Serai { - // Remove the keys for the set prior to the one now rotating out - if session.0 >= 2 { - let prior_to_now_rotating = ValidatorSet { network, session: Session(session.0 - 2) }; - MuSigKeys::::remove(prior_to_now_rotating); - Keys::::remove(prior_to_now_rotating); - } MuSigKeys::::set(set, Some(musig_key(set, &participants))); } Participants::::set(network, participants.try_into().unwrap()); @@ -398,8 +392,16 @@ pub mod pallet { // Handover is automatically complete for Serai as it doesn't have a handover protocol // TODO: Update how handover completed is determined. It's not on set keys. It's on new // set accepting responsibility - let handover_completed = (network == NetworkId::Serai) || - Keys::::contains_key(ValidatorSet { network, session: Self::session(network) }); + let handover_completed = (network == NetworkId::Serai) || { + let current_session = Self::session(network); + // This function shouldn't be used on genesis + debug_assert!(current_session != Session(0)); + // Check the prior session had its keys cleared, which happens once its retired + !Keys::::contains_key(ValidatorSet { + network, + session: Session(current_session.0 - 1), + }) + }; // Only spawn a NewSet if the current set was actually established with a completed // handover protocol if handover_completed { @@ -411,6 +413,12 @@ pub mod pallet { pub fn validators(network: NetworkId) -> Vec { Self::participants(network).into() } + + pub fn retire_session(network: NetworkId, session: Session) { + let set = ValidatorSet { network, session }; + MuSigKeys::::remove(set); + Keys::::remove(set); + } } }