diff --git a/Cargo.lock b/Cargo.lock index 49cbd7fb..39e6bcdd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7735,6 +7735,7 @@ version = "0.1.0" dependencies = [ "frame-support", "frame-system", + "hashbrown 0.14.3", "pallet-babe", "pallet-grandpa", "parity-scale-codec", diff --git a/coordinator/src/substrate/mod.rs b/coordinator/src/substrate/mod.rs index c03f0ca2..da48423d 100644 --- a/coordinator/src/substrate/mod.rs +++ b/coordinator/src/substrate/mod.rs @@ -28,7 +28,7 @@ use tokio::{sync::mpsc, time::sleep}; use crate::{ Db, processors::Processors, - tributary::{TributarySpec, SeraiDkgRemoval, SeraiDkgCompleted}, + tributary::{TributarySpec, SeraiDkgCompleted}, }; mod db; @@ -222,19 +222,6 @@ async fn handle_block( // Define an indexed event ID. let mut event_id = 0; - if HandledEvent::is_unhandled(db, hash, event_id) { - let mut txn = db.txn(); - for removal in serai.as_of(hash).validator_sets().participant_removed_events().await? { - let ValidatorSetsEvent::ParticipantRemoved { set, removed } = removal else { - panic!("ParticipantRemoved event wasn't ParticipantRemoved: {removal:?}"); - }; - SeraiDkgRemoval::set(&mut txn, set, removed.0, &()); - } - HandledEvent::handle_event(&mut txn, hash, event_id); - txn.commit(); - } - event_id += 1; - // If a new validator set was activated, create tributary/inform processor to do a DKG for new_set in serai.as_of(hash).validator_sets().new_set_events().await? { // Individually mark each event as handled so on reboot, we minimize duplicates diff --git a/coordinator/src/tests/tributary/dkg.rs b/coordinator/src/tests/tributary/dkg.rs index 21792533..c0d6e984 100644 --- a/coordinator/src/tests/tributary/dkg.rs +++ b/coordinator/src/tests/tributary/dkg.rs @@ -347,11 +347,18 @@ async fn dkg_test() { } #[async_trait::async_trait] impl PublishSeraiTransaction for CheckPublishSetKeys { - async fn publish_set_keys(&self, set: ValidatorSet, key_pair: KeyPair, signature: Signature) { + async fn publish_set_keys( + &self, + set: ValidatorSet, + removed: Vec, + key_pair: KeyPair, + signature: Signature, + ) { assert_eq!(set, self.spec.set()); + assert!(removed.is_empty()); assert_eq!(self.key_pair, key_pair); assert!(signature.verify( - &*serai_client::validator_sets::primitives::set_keys_message(&set, &key_pair), + &*serai_client::validator_sets::primitives::set_keys_message(&set, &[], &key_pair), &serai_client::Public( frost::dkg::musig::musig_key::( &serai_client::validator_sets::primitives::musig_context(set), @@ -362,16 +369,6 @@ async fn dkg_test() { ), )); } - - async fn publish_remove_participant( - &self, - set: ValidatorSet, - removing: [u8; 32], - signers: Vec, - signature: Signature, - ) { - ().publish_remove_participant(set, removing, signers, signature).await - } } // The scanner should successfully try to publish a transaction with a validly signed signature diff --git a/coordinator/src/tests/tributary/mod.rs b/coordinator/src/tests/tributary/mod.rs index 3f84378f..513418bd 100644 --- a/coordinator/src/tests/tributary/mod.rs +++ b/coordinator/src/tests/tributary/mod.rs @@ -26,17 +26,14 @@ mod sync; #[async_trait::async_trait] impl PublishSeraiTransaction for () { - async fn publish_set_keys(&self, _set: ValidatorSet, _key_pair: KeyPair, _signature: Signature) { - panic!("publish_set_keys was called in test") - } - async fn publish_remove_participant( + async fn publish_set_keys( &self, _set: ValidatorSet, - _removing: [u8; 32], - _signers: Vec, + _removed: Vec, + _key_pair: KeyPair, _signature: Signature, ) { - panic!("publish_remove_participant was called in test") + panic!("publish_set_keys was called in test") } } @@ -226,17 +223,6 @@ fn serialize_transaction() { signed: random_signed_with_nonce(&mut OsRng, 2), }); - { - let mut key = [0; 32]; - OsRng.fill_bytes(&mut key); - test_read_write(Transaction::DkgRemoval(random_sign_data(&mut OsRng, key, Label::Preprocess))); - } - { - let mut key = [0; 32]; - OsRng.fill_bytes(&mut key); - test_read_write(Transaction::DkgRemoval(random_sign_data(&mut OsRng, key, Label::Share))); - } - { let mut block = [0; 32]; OsRng.fill_bytes(&mut block); diff --git a/coordinator/src/tributary/db.rs b/coordinator/src/tributary/db.rs index cf835822..0aca6507 100644 --- a/coordinator/src/tributary/db.rs +++ b/coordinator/src/tributary/db.rs @@ -19,7 +19,6 @@ use crate::tributary::{Label, Transaction}; pub enum Topic { Dkg, DkgConfirmation, - DkgRemoval([u8; 32]), SubstrateSign(SubstrateSignableId), Sign([u8; 32]), } @@ -46,14 +45,12 @@ pub enum Accumulation { create_db!( Tributary { SeraiBlockNumber: (hash: [u8; 32]) -> u64, - SeraiDkgRemoval: (spec: ValidatorSet, removing: [u8; 32]) -> (), SeraiDkgCompleted: (spec: ValidatorSet) -> [u8; 32], TributaryBlockNumber: (block: [u8; 32]) -> u32, LastHandledBlock: (genesis: [u8; 32]) -> [u8; 32], FatalSlashes: (genesis: [u8; 32]) -> Vec<[u8; 32]>, FatalSlashesAsOfDkgAttempt: (genesis: [u8; 32], attempt: u32) -> Vec<[u8; 32]>, - FatalSlashesAsOfFatalSlash: (genesis: [u8; 32], fatally_slashed: [u8; 32]) -> Vec<[u8; 32]>, FatallySlashed: (genesis: [u8; 32], account: [u8; 32]) -> (), DkgShare: (genesis: [u8; 32], from: u16, to: u16) -> Vec, PlanIds: (genesis: &[u8], block: u64) -> Vec<[u8; 32]>, @@ -85,7 +82,6 @@ impl FatallySlashed { existing.push(account); FatalSlashes::set(txn, genesis, &existing); - FatalSlashesAsOfFatalSlash::set(txn, genesis, account, &existing); } } diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index e02cda19..30bb661a 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -1,8 +1,6 @@ use core::ops::Deref; use std::collections::HashMap; -use rand_core::OsRng; - use zeroize::{Zeroize, Zeroizing}; use ciphersuite::{group::GroupEncoding, Ciphersuite, Ristretto}; @@ -25,7 +23,7 @@ use crate::{ processors::Processors, tributary::{ *, - signing_protocol::{DkgConfirmer, DkgRemoval}, + signing_protocol::DkgConfirmer, scanner::{ RecognizedIdType, RIDTrait, PublishSeraiTransaction, PTTTrait, TributaryBlockHandler, }, @@ -278,21 +276,6 @@ impl< Ok(()) } - fn dkg_removal<'a>( - &'a mut self, - removed: &'a [::G], - data: &'a SignData<[u8; 32]>, - ) -> DkgRemoval<'a, T> { - DkgRemoval { - key: self.our_key, - spec: self.spec, - txn: self.txn, - removed, - removing: data.plan, - attempt: data.attempt, - } - } - // TODO: Don't call fatal_slash in here, return the party to fatal_slash to ensure no further // execution occurs pub(crate) async fn handle_application_tx(&mut self, tx: Transaction) { @@ -582,7 +565,15 @@ impl< DkgCompleted::set(self.txn, genesis, &()); - self.publish_serai_tx.publish_set_keys(self.spec.set(), key_pair, Signature(sig)).await; + self + .publish_serai_tx + .publish_set_keys( + self.spec.set(), + removed.into_iter().map(|key| key.to_bytes().into()).collect(), + key_pair, + Signature(sig), + ) + .await; } Accumulation::Ready(DataSet::NotParticipating) => { panic!("wasn't a participant in DKG confirmination shares") @@ -591,98 +582,6 @@ impl< } } - Transaction::DkgRemoval(data) => { - let signer = data.signed.signer; - let expected_len = match data.label { - Label::Preprocess => 64, - Label::Share => 32, - }; - if (data.data.len() != 1) || (data.data[0].len() != expected_len) { - self.fatal_slash(signer.to_bytes(), "unexpected length data for dkg removal").await; - return; - } - - let Some(removed) = - crate::tributary::removed_as_of_fatal_slash(self.txn, genesis, data.plan) - else { - self.fatal_slash(signer.to_bytes(), "removing someone who wasn't fatally slashed").await; - return; - }; - - let data_spec = DataSpecification { - topic: Topic::DkgRemoval(data.plan), - label: data.label, - attempt: data.attempt, - }; - let Accumulation::Ready(DataSet::Participating(results)) = - self.handle_data(&removed, &data_spec, data.data.encode(), &data.signed).await - else { - return; - }; - - match data.label { - Label::Preprocess => { - RemovalNonces::set(self.txn, genesis, data.plan, data.attempt, &results); - - let Ok(share) = self.dkg_removal(&removed, &data).share(results) else { - // TODO: Locally increase slash points to maximum (distinct from an explicitly fatal - // slash) and censor transactions (yet don't explicitly ban) - return; - }; - - let mut tx = Transaction::DkgRemoval(SignData { - plan: data.plan, - attempt: data.attempt, - label: Label::Preprocess, - data: vec![share.to_vec()], - signed: Transaction::empty_signed(), - }); - tx.sign(&mut OsRng, genesis, self.our_key); - self.publish_tributary_tx.publish_tributary_tx(tx).await; - } - Label::Share => { - let preprocesses = - RemovalNonces::get(self.txn, genesis, data.plan, data.attempt).unwrap(); - - let Ok((signers, signature)) = - self.dkg_removal(&removed, &data).complete(preprocesses, results) - else { - // TODO: Locally increase slash points to maximum (distinct from an explicitly fatal - // slash) and censor transactions (yet don't explicitly ban) - return; - }; - - // We need to only handle this if we're not actively removing any of the signers - // At the start of this function, we only handle messages from non-fatally slashed - // participants, so this is held - // - // The created Substrate call will fail if a removed validator was one of the signers - // Since: - // 1) publish_serai_tx will block this task until the TX is published - // 2) We won't scan any more TXs/blocks until we handle this TX - // The TX *must* be successfully published *before* we start removing any more - // signers - // - // Accordingly, if the signers aren't currently being removed, they won't be removed - // by the time this transaction is successfully published *unless* a malicious 34% - // participates with the non-participating 33% to continue operation and produce a - // distinct removal (since the non-participating won't block in this block) - // - // This breaks BFT and is accordingly within bounds - - // TODO: The above isn't true. It blocks until the TX is published, not included the - // finalized chain. We just need to inline remove_participant into set_keys to avoid - // all of this. - - LocallyDkgRemoved::set(self.txn, genesis, data.plan, &()); - self - .publish_serai_tx - .publish_remove_participant(self.spec.set(), data.plan, signers, Signature(signature)) - .await; - } - } - } - Transaction::CosignSubstrateBlock(hash) => { AttemptDb::recognize_topic( self.txn, diff --git a/coordinator/src/tributary/mod.rs b/coordinator/src/tributary/mod.rs index f320cd83..5c78df88 100644 --- a/coordinator/src/tributary/mod.rs +++ b/coordinator/src/tributary/mod.rs @@ -66,16 +66,6 @@ pub fn removed_as_of_set_keys( removed_as_of_dkg_attempt(getter, genesis, attempt) } -pub fn removed_as_of_fatal_slash( - getter: &impl Get, - genesis: [u8; 32], - fatally_slashed: [u8; 32], -) -> Option::G>> { - FatalSlashesAsOfFatalSlash::get(getter, genesis, fatally_slashed).map(|keys| { - keys.iter().map(|key| ::G::from_bytes(key).unwrap()).collect() - }) -} - pub async fn publish_signed_transaction( txn: &mut D::Transaction<'_>, tributary: &Tributary, diff --git a/coordinator/src/tributary/scanner.rs b/coordinator/src/tributary/scanner.rs index e4fcad01..c688f601 100644 --- a/coordinator/src/tributary/scanner.rs +++ b/coordinator/src/tributary/scanner.rs @@ -1,8 +1,6 @@ use core::{marker::PhantomData, ops::Deref, future::Future, time::Duration}; use std::sync::Arc; -use rand_core::OsRng; - use zeroize::Zeroizing; use ciphersuite::{group::GroupEncoding, Ciphersuite, Ristretto}; @@ -29,13 +27,7 @@ use tributary::{ }, }; -use crate::{ - Db, - processors::Processors, - substrate::BatchInstructionsHashDb, - tributary::{*, signing_protocol::*}, - P2p, -}; +use crate::{Db, processors::Processors, substrate::BatchInstructionsHashDb, tributary::*, P2p}; #[derive(Clone, Copy, PartialEq, Eq, Debug, Encode, Decode)] pub enum RecognizedIdType { @@ -72,12 +64,11 @@ impl< #[async_trait::async_trait] pub trait PublishSeraiTransaction { - async fn publish_set_keys(&self, set: ValidatorSet, key_pair: KeyPair, signature: Signature); - async fn publish_remove_participant( + async fn publish_set_keys( &self, set: ValidatorSet, - removing: [u8; 32], - signers: Vec, + removed: Vec, + key_pair: KeyPair, signature: Signature, ); } @@ -137,8 +128,14 @@ mod impl_pst_for_serai { #[async_trait::async_trait] impl PublishSeraiTransaction for Serai { - async fn publish_set_keys(&self, set: ValidatorSet, key_pair: KeyPair, signature: Signature) { - let tx = SeraiValidatorSets::set_keys(set.network, key_pair, signature); + async fn publish_set_keys( + &self, + set: ValidatorSet, + removed: Vec, + key_pair: KeyPair, + signature: Signature, + ) { + let tx = SeraiValidatorSets::set_keys(set.network, removed, key_pair, signature); async fn check(serai: SeraiValidatorSets<'_>, set: ValidatorSet, _: ()) -> bool { if matches!(serai.keys(set).await, Ok(Some(_))) { log::info!("another coordinator set key pair for {:?}", set); @@ -151,42 +148,6 @@ mod impl_pst_for_serai { log::info!("published set keys for {set:?}"); } } - - async fn publish_remove_participant( - &self, - set: ValidatorSet, - removing: [u8; 32], - signers: Vec, - signature: Signature, - ) { - let tx = SeraiValidatorSets::remove_participant( - set.network, - SeraiAddress(removing), - signers, - signature, - ); - async fn check(serai: SeraiValidatorSets<'_>, set: ValidatorSet, removing: [u8; 32]) -> bool { - if let Ok(Some(_)) = serai.keys(set).await { - log::info!( - "keys were set before we personally could publish the removal for {}", - hex::encode(removing) - ); - return true; - } - - if let Ok(Some(participants)) = serai.participants(set.network).await { - if !participants.iter().any(|(participant, _)| participant.0 == removing) { - log::info!("another coordinator published removal for {:?}", hex::encode(removing),); - return true; - } - } - false - } - common_pst!([u8; 32], check); - if publish(self, set, tx, removing).await { - log::info!("published remove participant for {}", hex::encode(removing)); - } - } } } @@ -231,30 +192,6 @@ impl< P: P2p, > TributaryBlockHandler<'_, T, Pro, PST, PTT, RID, P> { - async fn attempt_dkg_removal(&mut self, removing: [u8; 32], attempt: u32) { - let genesis = self.spec.genesis(); - let removed = crate::tributary::removed_as_of_fatal_slash(self.txn, genesis, removing) - .expect("attempting DKG removal to remove someone who wasn't removed"); - let preprocess = (DkgRemoval { - key: self.our_key, - spec: self.spec, - txn: self.txn, - removed: &removed, - removing, - attempt, - }) - .preprocess(); - let mut tx = Transaction::DkgRemoval(SignData { - plan: removing, - attempt, - label: Label::Preprocess, - data: vec![preprocess.to_vec()], - signed: Transaction::empty_signed(), - }); - tx.sign(&mut OsRng, genesis, self.our_key); - self.publish_tributary_tx.publish_tributary_tx(tx).await; - } - pub async fn fatal_slash(&mut self, slashing: [u8; 32], reason: &str) { // TODO: If this fatal slash puts the remaining set below the threshold, spin @@ -266,12 +203,6 @@ impl< // TODO: If during DKG, trigger a re-attempt // Despite triggering a re-attempt, this DKG may still complete and may become in-use - - // If during a DKG, remove the participant - if DkgCompleted::get(self.txn, genesis).is_none() { - AttemptDb::recognize_topic(self.txn, genesis, Topic::DkgRemoval(slashing)); - self.attempt_dkg_removal(slashing, 0).await; - } } // TODO: Once Substrate confirms a key, we need to rotate our validator set OR form a second @@ -404,16 +335,6 @@ impl< Topic::DkgConfirmation => { panic!("re-attempting DkgConfirmation when we should be re-attempting the Dkg") } - Topic::DkgRemoval(removing) => { - if DkgCompleted::get(self.txn, genesis).is_none() && - LocallyDkgRemoved::get(self.txn, genesis, removing).is_none() && - SeraiDkgCompleted::get(self.txn, self.spec.set()).is_none() && - SeraiDkgRemoval::get(self.txn, self.spec.set(), removing).is_none() - { - // Since it wasn't completed, attempt a new DkgRemoval - self.attempt_dkg_removal(removing, attempt).await; - } - } Topic::SubstrateSign(inner_id) => { let id = processor_messages::coordinator::SubstrateSignId { session: self.spec.set().session, diff --git a/coordinator/src/tributary/signing_protocol.rs b/coordinator/src/tributary/signing_protocol.rs index 9cf34499..a9df5b3c 100644 --- a/coordinator/src/tributary/signing_protocol.rs +++ b/coordinator/src/tributary/signing_protocol.rs @@ -1,9 +1,8 @@ /* A MuSig-based signing protocol executed with the validators' keys. - This is used for confirming the results of a DKG on-chain, an operation requiring all validators, - and for removing another validator before the DKG completes, an operation requiring a - supermajority of validators. + This is used for confirming the results of a DKG on-chain, an operation requiring all validators + which aren't specified as removed while still satisfying a supermajority. Since we're using the validator's keys, as needed for their being the root of trust, the coordinator must perform the signing. This is distinct from all other group-signing operations, @@ -65,7 +64,7 @@ use rand_core::OsRng; use blake2::{Digest, Blake2s256}; use ciphersuite::{ - group::{ff::PrimeField, Group, GroupEncoding}, + group::{ff::PrimeField, GroupEncoding}, Ciphersuite, Ristretto, }; use frost::{ @@ -79,10 +78,8 @@ use frost_schnorrkel::Schnorrkel; use scale::Encode; use serai_client::{ - Public, SeraiAddress, - validator_sets::primitives::{ - KeyPair, musig_context, set_keys_message, remove_participant_message, - }, + Public, + validator_sets::primitives::{KeyPair, musig_context, set_keys_message}, }; use serai_db::*; @@ -212,15 +209,11 @@ impl SigningProtocol<'_, T, C> { // Get the keys of the participants, noted by their threshold is, and return a new map indexed by // the MuSig is. -// -// If sort_by_keys = true, the MuSig is will index the keys once sorted. Else, the MuSig is will -// index the validators in the order they've been defined. fn threshold_i_map_to_keys_and_musig_i_map( spec: &TributarySpec, removed: &[::G], our_key: &Zeroizing<::F>, mut map: HashMap>, - sort_by_keys: bool, ) -> (Vec<::G>, HashMap>) { // Insert our own index so calculations aren't offset let our_threshold_i = @@ -243,10 +236,6 @@ fn threshold_i_map_to_keys_and_musig_i_map( for threshold_i in threshold_is { sorted.push((key_from_threshold_i(threshold_i), map.remove(&threshold_i).unwrap())); } - if sort_by_keys { - // Substrate expects these signers to be sorted by key - sorted.sort_by(|(key1, _), (key2, _)| key1.to_bytes().cmp(&key2.to_bytes())); - } // Now that signers are sorted, with their shares, create a map with the is needed for MuSig let mut participants = vec![]; @@ -302,15 +291,13 @@ impl DkgConfirmer<'_, T> { key_pair: &KeyPair, ) -> Result<(AlgorithmSignatureMachine, [u8; 32]), Participant> { let participants = self.spec.validators().iter().map(|val| val.0).collect::>(); - let preprocesses = threshold_i_map_to_keys_and_musig_i_map( - self.spec, - &self.removed, - self.key, - preprocesses, - false, - ) - .1; - let msg = set_keys_message(&self.spec.set(), key_pair); + let preprocesses = + threshold_i_map_to_keys_and_musig_i_map(self.spec, &self.removed, self.key, preprocesses).1; + let msg = set_keys_message( + &self.spec.set(), + &self.removed.iter().map(|key| Public(key.to_bytes())).collect::>(), + key_pair, + ); self.signing_protocol().share_internal(&participants, preprocesses, &msg) } // Get the share for this confirmation, if the preprocesses are valid. @@ -329,7 +316,7 @@ impl DkgConfirmer<'_, T> { shares: HashMap>, ) -> Result<[u8; 64], Participant> { let shares = - threshold_i_map_to_keys_and_musig_i_map(self.spec, &self.removed, self.key, shares, false).1; + threshold_i_map_to_keys_and_musig_i_map(self.spec, &self.removed, self.key, shares).1; let machine = self .share_internal(preprocesses, key_pair) @@ -339,83 +326,3 @@ impl DkgConfirmer<'_, T> { self.signing_protocol().complete_internal(machine, shares) } } - -pub(crate) struct DkgRemoval<'a, T: DbTxn> { - pub(crate) key: &'a Zeroizing<::F>, - pub(crate) spec: &'a TributarySpec, - pub(crate) removed: &'a [::G], - pub(crate) txn: &'a mut T, - pub(crate) removing: [u8; 32], - pub(crate) attempt: u32, -} - -impl DkgRemoval<'_, T> { - fn signing_protocol(&mut self) -> SigningProtocol<'_, T, (&'static [u8; 10], [u8; 32], u32)> { - let context = (b"DkgRemoval", self.removing, self.attempt); - SigningProtocol { key: self.key, spec: self.spec, txn: self.txn, context } - } - - fn preprocess_internal( - &mut self, - participants: Option<&[::G]>, - ) -> (AlgorithmSignMachine, [u8; 64]) { - // We won't know the participants when we first preprocess - // If we don't, we use our key alone as the participant - let just_us = [::G::generator() * self.key.deref()]; - let to_musig = if let Some(participants) = participants { participants } else { &just_us }; - - let (machine, preprocess) = self.signing_protocol().preprocess_internal(to_musig); - - // If we're now specifying participants, confirm the commitments were the same - if participants.is_some() { - let (_, theoretical_preprocess) = self.signing_protocol().preprocess_internal(&just_us); - assert_eq!(theoretical_preprocess, preprocess); - } - - (machine, preprocess) - } - // Get the preprocess for this confirmation. - pub(crate) fn preprocess(&mut self) -> [u8; 64] { - self.preprocess_internal(None).1 - } - - fn share_internal( - &mut self, - preprocesses: HashMap>, - ) -> Result<(AlgorithmSignatureMachine, [u8; 32]), Participant> { - let (participants, preprocesses) = threshold_i_map_to_keys_and_musig_i_map( - self.spec, - self.removed, - self.key, - preprocesses, - true, - ); - let msg = remove_participant_message(&self.spec.set(), Public(self.removing)); - self.signing_protocol().share_internal(&participants, preprocesses, &msg) - } - // Get the share for this confirmation, if the preprocesses are valid. - pub(crate) fn share( - &mut self, - preprocesses: HashMap>, - ) -> Result<[u8; 32], Participant> { - self.share_internal(preprocesses).map(|(_, share)| share) - } - - pub(crate) fn complete( - &mut self, - preprocesses: HashMap>, - shares: HashMap>, - ) -> Result<(Vec, [u8; 64]), Participant> { - let (participants, shares) = - threshold_i_map_to_keys_and_musig_i_map(self.spec, self.removed, self.key, shares, true); - let signers = participants.iter().map(|key| SeraiAddress(key.to_bytes())).collect::>(); - - let machine = self - .share_internal(preprocesses) - .expect("trying to complete a machine which failed to preprocess") - .0; - - let signature = self.signing_protocol().complete_internal(machine, shares)?; - Ok((signers, signature)) - } -} diff --git a/coordinator/src/tributary/transaction.rs b/coordinator/src/tributary/transaction.rs index ebbdcc17..185bb754 100644 --- a/coordinator/src/tributary/transaction.rs +++ b/coordinator/src/tributary/transaction.rs @@ -160,8 +160,6 @@ pub enum Transaction { signed: Signed, }, - DkgRemoval(SignData<[u8; 32]>), - // Co-sign a Substrate block. CosignSubstrateBlock([u8; 32]), @@ -223,9 +221,6 @@ impl Debug for Transaction { .field("attempt", attempt) .field("signer", &hex::encode(signed.signer.to_bytes())) .finish_non_exhaustive(), - Transaction::DkgRemoval(sign_data) => { - fmt.debug_struct("Transaction::DkgRemoval").field("sign_data", sign_data).finish() - } Transaction::CosignSubstrateBlock(block) => fmt .debug_struct("Transaction::CosignSubstrateBlock") .field("block", &hex::encode(block)) @@ -389,15 +384,13 @@ impl ReadWrite for Transaction { Ok(Transaction::DkgConfirmed { attempt, confirmation_share, signed }) } - 5 => SignData::read(reader).map(Transaction::DkgRemoval), - - 6 => { + 5 => { let mut block = [0; 32]; reader.read_exact(&mut block)?; Ok(Transaction::CosignSubstrateBlock(block)) } - 7 => { + 6 => { let mut block = [0; 32]; reader.read_exact(&mut block)?; let mut batch = [0; 4]; @@ -405,16 +398,16 @@ impl ReadWrite for Transaction { Ok(Transaction::Batch { block, batch: u32::from_le_bytes(batch) }) } - 8 => { + 7 => { let mut block = [0; 8]; reader.read_exact(&mut block)?; Ok(Transaction::SubstrateBlock(u64::from_le_bytes(block))) } - 9 => SignData::read(reader).map(Transaction::SubstrateSign), - 10 => SignData::read(reader).map(Transaction::Sign), + 8 => SignData::read(reader).map(Transaction::SubstrateSign), + 9 => SignData::read(reader).map(Transaction::Sign), - 11 => { + 10 => { let mut plan = [0; 32]; reader.read_exact(&mut plan)?; @@ -512,37 +505,32 @@ impl ReadWrite for Transaction { signed.write_without_nonce(writer) } - Transaction::DkgRemoval(data) => { - writer.write_all(&[5])?; - data.write(writer) - } - Transaction::CosignSubstrateBlock(block) => { - writer.write_all(&[6])?; + writer.write_all(&[5])?; writer.write_all(block) } Transaction::Batch { block, batch } => { - writer.write_all(&[7])?; + writer.write_all(&[6])?; writer.write_all(block)?; writer.write_all(&batch.to_le_bytes()) } Transaction::SubstrateBlock(block) => { - writer.write_all(&[8])?; + writer.write_all(&[7])?; writer.write_all(&block.to_le_bytes()) } Transaction::SubstrateSign(data) => { - writer.write_all(&[9])?; + writer.write_all(&[8])?; data.write(writer) } Transaction::Sign(data) => { - writer.write_all(&[10])?; + writer.write_all(&[9])?; data.write(writer) } Transaction::SignCompleted { plan, tx_hash, first_signer, signature } => { - writer.write_all(&[11])?; + writer.write_all(&[10])?; writer.write_all(plan)?; writer .write_all(&[u8::try_from(tx_hash.len()).expect("tx hash length exceed 255 bytes")])?; @@ -572,10 +560,6 @@ impl TransactionTrait for Transaction { TransactionKind::Signed((b"dkg", attempt).encode(), signed) } - Transaction::DkgRemoval(data) => { - TransactionKind::Signed((b"dkg_removal", data.plan, data.attempt).encode(), &data.signed) - } - Transaction::CosignSubstrateBlock(_) => TransactionKind::Provided("cosign"), Transaction::Batch { .. } => TransactionKind::Provided("batch"), @@ -601,7 +585,7 @@ impl TransactionTrait for Transaction { } fn verify(&self) -> Result<(), TransactionError> { - // TODO: Check DkgRemoval and SubstrateSign's lengths here + // TODO: Check SubstrateSign's lengths here if let Transaction::SignCompleted { first_signer, signature, .. } = self { if !signature.verify(*first_signer, self.sign_completed_challenge()) { @@ -644,8 +628,6 @@ impl Transaction { Transaction::InvalidDkgShare { .. } => 2, Transaction::DkgConfirmed { .. } => 2, - Transaction::DkgRemoval(data) => data.label.nonce(), - Transaction::CosignSubstrateBlock(_) => panic!("signing CosignSubstrateBlock"), Transaction::Batch { .. } => panic!("signing Batch"), @@ -666,8 +648,6 @@ impl Transaction { Transaction::InvalidDkgShare { ref mut signed, .. } => signed, Transaction::DkgConfirmed { ref mut signed, .. } => signed, - Transaction::DkgRemoval(ref mut data) => &mut data.signed, - Transaction::CosignSubstrateBlock(_) => panic!("signing CosignSubstrateBlock"), Transaction::Batch { .. } => panic!("signing Batch"), diff --git a/substrate/abi/src/validator_sets.rs b/substrate/abi/src/validator_sets.rs index f6244b1d..64eb8b4a 100644 --- a/substrate/abi/src/validator_sets.rs +++ b/substrate/abi/src/validator_sets.rs @@ -8,15 +8,10 @@ use serai_validator_sets_primitives::*; pub enum Call { set_keys { network: NetworkId, + removed_participants: Vec, key_pair: KeyPair, signature: Signature, }, - remove_participant { - network: NetworkId, - to_remove: SeraiAddress, - signers: Vec, - signature: Signature, - }, allocate { network: NetworkId, amount: Amount, diff --git a/substrate/client/src/serai/validator_sets.rs b/substrate/client/src/serai/validator_sets.rs index bf04f5e8..9b283635 100644 --- a/substrate/client/src/serai/validator_sets.rs +++ b/substrate/client/src/serai/validator_sets.rs @@ -126,36 +126,22 @@ impl<'a> SeraiValidatorSets<'a> { .await } - pub async fn musig_key(&self, set: ValidatorSet) -> Result, SeraiError> { - self.0.storage(PALLET, "MuSigKeys", (sp_core::hashing::twox_64(&set.encode()), set)).await - } - // TODO: Store these separately since we almost never need both at once? pub async fn keys(&self, set: ValidatorSet) -> Result, SeraiError> { self.0.storage(PALLET, "Keys", (sp_core::hashing::twox_64(&set.encode()), set)).await } - pub fn set_keys(network: NetworkId, key_pair: KeyPair, signature: Signature) -> Transaction { + pub fn set_keys( + network: NetworkId, + removed_participants: Vec, + key_pair: KeyPair, + signature: Signature, + ) -> Transaction { Serai::unsigned(serai_abi::Call::ValidatorSets(serai_abi::validator_sets::Call::set_keys { network, + removed_participants, key_pair, signature, })) } - - pub fn remove_participant( - network: NetworkId, - to_remove: SeraiAddress, - signers: Vec, - signature: Signature, - ) -> Transaction { - Serai::unsigned(serai_abi::Call::ValidatorSets( - serai_abi::validator_sets::Call::remove_participant { - network, - to_remove, - signers, - signature, - }, - )) - } } diff --git a/substrate/client/tests/common/validator_sets.rs b/substrate/client/tests/common/validator_sets.rs index 057742db..9c29d776 100644 --- a/substrate/client/tests/common/validator_sets.rs +++ b/substrate/client/tests/common/validator_sets.rs @@ -5,14 +5,14 @@ use rand_core::OsRng; use sp_core::{Pair, sr25519::Signature}; -use ciphersuite::{group::GroupEncoding, Ciphersuite, Ristretto}; +use ciphersuite::{Ciphersuite, Ristretto}; use frost::dkg::musig::musig; use schnorrkel::Schnorrkel; use serai_client::{ primitives::insecure_pair_from_name, validator_sets::{ - primitives::{ValidatorSet, KeyPair, musig_context, musig_key, set_keys_message}, + primitives::{ValidatorSet, KeyPair, musig_context, set_keys_message}, ValidatorSetsEvent, }, SeraiValidatorSets, Serai, @@ -26,19 +26,6 @@ pub async fn set_keys(serai: &Serai, set: ValidatorSet, key_pair: KeyPair) -> [u let public = pair.public(); let public_key = ::read_G::<&[u8]>(&mut public.0.as_ref()).unwrap(); - assert_eq!( - serai - .as_of_latest_finalized_block() - .await - .unwrap() - .validator_sets() - .musig_key(set) - .await - .unwrap() - .unwrap(), - musig_key(set, &[public]).0 - ); - let secret_key = ::read_F::<&[u8]>( &mut pair.as_ref().secret.to_bytes()[.. 32].as_ref(), ) @@ -46,18 +33,6 @@ pub async fn set_keys(serai: &Serai, set: ValidatorSet, key_pair: KeyPair) -> [u assert_eq!(Ristretto::generator() * secret_key, public_key); let threshold_keys = musig::(&musig_context(set), &Zeroizing::new(secret_key), &[public_key]).unwrap(); - assert_eq!( - serai - .as_of_latest_finalized_block() - .await - .unwrap() - .validator_sets() - .musig_key(set) - .await - .unwrap() - .unwrap(), - threshold_keys.group_key().to_bytes() - ); let sig = frost::tests::sign_without_caching( &mut OsRng, @@ -66,13 +41,13 @@ pub async fn set_keys(serai: &Serai, set: ValidatorSet, key_pair: KeyPair) -> [u Schnorrkel::new(b"substrate"), &HashMap::from([(threshold_keys.params().i(), threshold_keys.into())]), ), - &set_keys_message(&set, &key_pair), + &set_keys_message(&set, &[], &key_pair), ); // Set the key pair let block = publish_tx( serai, - &SeraiValidatorSets::set_keys(set.network, key_pair.clone(), Signature(sig.to_bytes())), + &SeraiValidatorSets::set_keys(set.network, vec![], key_pair.clone(), Signature(sig.to_bytes())), ) .await; diff --git a/substrate/client/tests/validator_sets.rs b/substrate/client/tests/validator_sets.rs index d66b68fb..a487b51c 100644 --- a/substrate/client/tests/validator_sets.rs +++ b/substrate/client/tests/validator_sets.rs @@ -5,7 +5,7 @@ use sp_core::{sr25519::Public, Pair}; use serai_client::{ primitives::{NETWORKS, NetworkId, insecure_pair_from_name}, validator_sets::{ - primitives::{Session, ValidatorSet, KeyPair, musig_key}, + primitives::{Session, ValidatorSet, KeyPair}, ValidatorSetsEvent, }, Serai, @@ -58,7 +58,6 @@ serai_test!( .collect::>(); let participants_ref: &[_] = participants.as_ref(); assert_eq!(participants_ref, [public].as_ref()); - assert_eq!(vs_serai.musig_key(set).await.unwrap().unwrap(), musig_key(set, &[public]).0); } let block = set_keys(&serai, set, key_pair.clone()).await; diff --git a/substrate/validator-sets/pallet/Cargo.toml b/substrate/validator-sets/pallet/Cargo.toml index 50d47ab7..c678edec 100644 --- a/substrate/validator-sets/pallet/Cargo.toml +++ b/substrate/validator-sets/pallet/Cargo.toml @@ -16,6 +16,8 @@ rustdoc-args = ["--cfg", "docsrs"] ignored = ["scale", "scale-info"] [dependencies] +hashbrown = { version = "0.14", default-features = false, features = ["ahash", "inline-more"] } + scale = { package = "parity-scale-codec", version = "3", default-features = false, features = ["derive"] } scale-info = { version = "2", default-features = false, features = ["derive"] } diff --git a/substrate/validator-sets/pallet/src/lib.rs b/substrate/validator-sets/pallet/src/lib.rs index 3b26f6ad..be125ebb 100644 --- a/substrate/validator-sets/pallet/src/lib.rs +++ b/substrate/validator-sets/pallet/src/lib.rs @@ -257,10 +257,6 @@ pub mod pallet { type PendingDeallocations = StorageMap<_, Blake2_128Concat, (NetworkId, Session, Public), Amount, OptionQuery>; - /// The MuSig key for a validator set. - #[pallet::storage] - pub type MuSigKeys = StorageMap<_, Twox64Concat, ValidatorSet, Public, OptionQuery>; - /// The generated key pair for a given validator set instance. #[pallet::storage] #[pallet::getter(fn keys)] @@ -343,13 +339,6 @@ pub mod pallet { let set = ValidatorSet { network, session }; Pallet::::deposit_event(Event::NewSet { set }); - // Only set the MuSig key for non-Serai sets, as only non-Serai sets should publish keys - if network != NetworkId::Serai { - MuSigKeys::::set( - set, - Some(musig_key(set, &participants.iter().map(|(id, _)| *id).collect::>())), - ); - } Participants::::set(network, Some(participants.try_into().unwrap())); } } @@ -411,26 +400,6 @@ pub mod pallet { } } - impl Pallet { - fn verify_signature( - set: ValidatorSet, - key_pair: &KeyPair, - signature: &Signature, - ) -> Result<(), Error> { - // Confirm a key hasn't been set for this set instance - if Keys::::get(set).is_some() { - Err(Error::AlreadyGeneratedKeys)? - } - - let Some(musig_key) = MuSigKeys::::get(set) else { Err(Error::NonExistentValidatorSet)? }; - if !musig_key.verify(&set_keys_message(&set, key_pair), signature) { - Err(Error::BadSignature)?; - } - - Ok(()) - } - } - impl Pallet { fn account() -> T::AccountId { system_address(b"ValidatorSets").into() @@ -658,7 +627,6 @@ pub mod pallet { } pub fn retire_set(set: ValidatorSet) { - MuSigKeys::::remove(set); Keys::::remove(set); Pallet::::deposit_event(Event::SetRetired { set }); } @@ -754,6 +722,7 @@ pub mod pallet { pub fn set_keys( origin: OriginFor, network: NetworkId, + removed_participants: Vec, key_pair: KeyPair, signature: Signature, ) -> DispatchResult { @@ -763,36 +732,25 @@ pub mod pallet { // (called by pre_dispatch) checks it let _ = signature; - let session = Self::session(NetworkId::Serai).unwrap(); - - let set = ValidatorSet { session, network }; + let session = Self::session(network).unwrap(); + let set = ValidatorSet { network, session }; Keys::::set(set, Some(key_pair.clone())); + + // This does not remove from TotalAllocatedStake or InSet in order to: + // 1) Not decrease the stake present in this set. This means removed participants are + // still liable for the economic security of the external network. This prevents + // a decided set, which is economically secure, from falling below the threshold. + // 2) Not allow parties removed to immediately deallocate, per commentary on deallocation + // scheduling (https://github.com/serai-dex/serai/issues/394). + for removed in removed_participants { + Self::deposit_event(Event::ParticipantRemoved { set, removed }); + } Self::deposit_event(Event::KeyGen { set, key_pair }); Ok(()) } - #[pallet::call_index(1)] - #[pallet::weight(0)] // TODO - pub fn remove_participant( - origin: OriginFor, - network: NetworkId, - to_remove: Public, - signers: Vec, - signature: Signature, - ) -> DispatchResult { - ensure_none(origin)?; - - // Nothing occurs here as validate_unsigned does everything - let _ = network; - let _ = to_remove; - let _ = signers; - let _ = signature; - - Ok(()) - } - #[pallet::call_index(2)] #[pallet::weight(0)] // TODO pub fn allocate(origin: OriginFor, network: NetworkId, amount: Amount) -> DispatchResult { @@ -850,128 +808,79 @@ pub mod pallet { fn validate_unsigned(_: TransactionSource, call: &Self::Call) -> TransactionValidity { // Match to be exhaustive match call { - Call::set_keys { network, ref key_pair, ref signature } => { + Call::set_keys { network, ref removed_participants, ref key_pair, ref signature } => { + let network = *network; + // Don't allow the Serai set to set_keys, as they have no reason to do so - // This should already be covered by the lack of key in MuSigKeys, yet it doesn't hurt to - // be explicit - if network == &NetworkId::Serai { - Err(InvalidTransaction::Custom(0))?; - } - - let session = Self::session(NetworkId::Serai).unwrap(); - - let set = ValidatorSet { session, network: *network }; - match Self::verify_signature(set, key_pair, signature) { - Err(Error::AlreadyGeneratedKeys) => Err(InvalidTransaction::Stale)?, - Err(Error::NonExistentValidatorSet) | - Err(Error::InsufficientAllocation) | - Err(Error::NotEnoughAllocated) | - Err(Error::AllocationWouldRemoveFaultTolerance) | - Err(Error::AllocationWouldPreventFaultTolerance) | - Err(Error::DeallocationWouldRemoveParticipant) | - Err(Error::DeallocationWouldRemoveFaultTolerance) | - Err(Error::NonExistentDeallocation) | - Err(Error::NonExistentValidator) | - Err(Error::DeallocationWouldRemoveEconomicSecurity) | - Err(Error::BadSignature) => Err(InvalidTransaction::BadProof)?, - Err(Error::__Ignore(_, _)) => unreachable!(), - Ok(()) => (), - } - - ValidTransaction::with_tag_prefix("ValidatorSets") - .and_provides((0, set)) - .longevity(u64::MAX) - .propagate(true) - .build() - } - Call::remove_participant { network, to_remove, signers, signature } => { - if network == &NetworkId::Serai { + if network == NetworkId::Serai { Err(InvalidTransaction::Custom(0))?; } // Confirm this set has a session - let Some(current_session) = Self::session(*network) else { + let Some(current_session) = Self::session(network) else { Err(InvalidTransaction::Custom(1))? }; - // This is needed as modify storage variables of the latest decided session - assert_eq!(Pallet::::latest_decided_session(*network), Some(current_session)); - let set = ValidatorSet { network: *network, session: current_session }; + + let set = ValidatorSet { network, session: current_session }; + // Confirm it has yet to set keys if Keys::::get(set).is_some() { - Err(InvalidTransaction::Custom(2))?; + Err(InvalidTransaction::Stale)?; } - let mut participants = + // This is a needed precondition as this uses storage variables for the latest decided + // session on this assumption + assert_eq!(Pallet::::latest_decided_session(network), Some(current_session)); + + // This does not slash the removed participants as that'll be done at the end of the + // set's lifetime + let mut removed = hashbrown::HashSet::new(); + for participant in removed_participants { + // Confirm this wasn't duplicated + if removed.contains(&participant.0) { + Err(InvalidTransaction::Custom(2))?; + } + removed.insert(participant.0); + } + + let participants = Participants::::get(network).expect("session existed without participants"); - // Require signers be sorted to ensure no duplicates are present - let mut last_signer = None; + let mut all_key_shares = 0; + let mut signers = vec![]; let mut signing_key_shares = 0; - for signer in signers { - if let Some(last_signer) = last_signer { - if last_signer >= signer { - Err(InvalidTransaction::Custom(3))?; - } - } - last_signer = Some(signer); + for participant in participants { + let participant = participant.0; + let shares = InSet::::get(network, participant) + .expect("participant from Participants wasn't InSet"); + all_key_shares += shares; - // Doesn't use InSet as InSet *includes* removed validators - // Only non-removed validators should be considered as contributing - let Some(shares) = participants - .iter() - .find(|(participant, _)| participant == to_remove) - .map(|(_, shares)| shares) - else { - Err(InvalidTransaction::Custom(4))? - }; + if removed.contains(&participant.0) { + continue; + } + + signers.push(participant); signing_key_shares += shares; } - // Check 67% are participating in this removal - // This is done by iterating over InSet, which isn't mutated on removal, and reading the - // shares from that - let mut all_key_shares = 0; - for shares in InSet::::iter_prefix_values(network) { - all_key_shares += shares; + { + let f = all_key_shares - signing_key_shares; + if signing_key_shares < ((2 * f) + 1) { + Err(InvalidTransaction::Custom(3))?; + } } - // 2f + 1 - if signing_key_shares < ((2 * (all_key_shares - signing_key_shares)) + 1) { - Err(InvalidTransaction::Custom(5))?; - } - - // Perform the removal - let Some(removal_index) = - participants.iter().position(|participant| &participant.0 == to_remove) - else { - Err(InvalidTransaction::Custom(6))? - }; - participants.remove(removal_index); // Verify the signature with the MuSig key of the signers - if !musig_key(set, signers) - .verify(&remove_participant_message(&set, *to_remove), signature) + // We theoretically don't need set_keys_message to bind to removed_participants, as the + // key we're signing with effectively already does so, yet there's no reason not to + if !musig_key(set, &signers) + .verify(&set_keys_message(&set, removed_participants, key_pair), signature) { Err(InvalidTransaction::BadProof)?; } - // Set the new MuSig key - MuSigKeys::::set( - set, - Some(musig_key(set, &participants.iter().map(|(id, _)| *id).collect::>())), - ); - Participants::::set(network, Some(participants)); - - // This does not remove from TotalAllocatedStake or InSet in order to: - // 1) Not decrease the stake present in this set. This means removed participants are - // still liable for the economic security of the external network. This prevents - // a decided set, which is economically secure, from falling below the threshold. - // 2) Not allow parties removed to immediately deallocate, per commentary on deallocation - // scheduling (https://github.com/serai-dex/serai/issues/394). - - Pallet::::deposit_event(Event::ParticipantRemoved { set, removed: *to_remove }); - ValidTransaction::with_tag_prefix("ValidatorSets") - .and_provides((1, set, to_remove)) + .and_provides(set) .longevity(u64::MAX) .propagate(true) .build() diff --git a/substrate/validator-sets/primitives/src/lib.rs b/substrate/validator-sets/primitives/src/lib.rs index 358199b8..df7cf18e 100644 --- a/substrate/validator-sets/primitives/src/lib.rs +++ b/substrate/validator-sets/primitives/src/lib.rs @@ -98,14 +98,13 @@ pub fn musig_key(set: ValidatorSet, set_keys: &[Public]) -> Public { Public(dkg::musig::musig_key::(&musig_context(set), &keys).unwrap().to_bytes()) } -/// The message for the remove_participant signature. -pub fn remove_participant_message(set: &ValidatorSet, removed: Public) -> Vec { - (b"ValidatorSets-remove_participant", set, removed).encode() -} - /// The message for the set_keys signature. -pub fn set_keys_message(set: &ValidatorSet, key_pair: &KeyPair) -> Vec { - (b"ValidatorSets-set_keys", set, key_pair).encode() +pub fn set_keys_message( + set: &ValidatorSet, + removed_participants: &[Public], + key_pair: &KeyPair, +) -> Vec { + (b"ValidatorSets-set_keys", set, removed_participants, key_pair).encode() } /// For a set of validators whose key shares may exceed the maximum, reduce until they equal the