From 77edd007255faf256db9026850b1a31201ede22f Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Wed, 13 Dec 2023 14:03:07 -0500 Subject: [PATCH] Handle the combination of DKG removals with re-attempts With a DKG removal comes a reduction in the amount of participants which was ignored by re-attempts. Now, we determine n/i based on the parties removed, and deterministically obtain the context of who was removd. --- coordinator/src/main.rs | 34 +-- coordinator/src/substrate/mod.rs | 3 +- coordinator/src/tests/tributary/mod.rs | 11 +- coordinator/src/tests/tributary/sync.rs | 4 +- coordinator/src/tributary/db.rs | 9 +- coordinator/src/tributary/handle.rs | 204 ++++++++++++++---- coordinator/src/tributary/mod.rs | 56 +++++ coordinator/src/tributary/scanner.rs | 57 +++-- coordinator/src/tributary/signing_protocol.rs | 50 +++-- coordinator/src/tributary/spec.rs | 51 +++-- coordinator/src/tributary/transaction.rs | 42 ++-- coordinator/tributary/src/block.rs | 2 - coordinator/tributary/src/blockchain.rs | 1 + coordinator/tributary/src/transaction.rs | 3 +- processor/src/key_gen.rs | 15 +- 15 files changed, 410 insertions(+), 132 deletions(-) diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index 3a09b5102..efec10104 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -108,14 +108,14 @@ async fn add_tributary( // This is safe due to the message-queue deduplicating based off the intent system let set = spec.set(); let our_i = spec - .i(Ristretto::generator() * key.deref()) + .i(&[], Ristretto::generator() * key.deref()) .expect("adding a tributary for a set we aren't in set for"); processors .send( set.network, processor_messages::key_gen::CoordinatorMessage::GenerateKey { id: processor_messages::key_gen::KeyGenId { session: set.session, attempt: 0 }, - params: frost::ThresholdParams::new(spec.t(), spec.n(), our_i.start).unwrap(), + params: frost::ThresholdParams::new(spec.t(), spec.n(&[]), our_i.start).unwrap(), shares: u16::from(our_i.end) - u16::from(our_i.start), }, ) @@ -370,21 +370,24 @@ async fn handle_processor_message( signed: Transaction::empty_signed(), }] } - key_gen::ProcessorMessage::InvalidCommitments { id: _, faulty } => { + key_gen::ProcessorMessage::InvalidCommitments { id, faulty } => { // This doesn't need the ID since it's a Provided transaction which everyone will provide - // With this provision comes explicit ordering (with regards to other RemoveParticipant - // transactions) and group consensus + // With this provision comes explicit ordering (with regards to other + // RemoveParticipantDueToDkg transactions) and group consensus // Accordingly, this can't be replayed // It could be included on-chain early/late with regards to the chain's active attempt, - // which attempt scheduling is written to avoid - vec![Transaction::RemoveParticipant(faulty)] + // which attempt scheduling is written to make a non-issue by auto re-attempting once a + // fatal slash occurs, regardless of timing + vec![Transaction::RemoveParticipantDueToDkg { attempt: id.attempt, participant: faulty }] } key_gen::ProcessorMessage::Shares { id, mut shares } => { // Create a MuSig-based machine to inform Substrate of this key generation let nonces = crate::tributary::dkg_confirmation_nonces(key, spec, &mut txn, id.attempt); + let removed = crate::tributary::removed_as_of_dkg_attempt(&txn, genesis, id.attempt) + .expect("participating in a DKG attempt yet we didn't track who was removed yet?"); let our_i = spec - .i(pub_key) + .i(&removed, pub_key) .expect("processor message to DKG for a session we aren't a validator in"); // `tx_shares` needs to be done here as while it can be serialized from the HashMap @@ -392,7 +395,7 @@ async fn handle_processor_message( let mut tx_shares = Vec::with_capacity(shares.len()); for shares in &mut shares { tx_shares.push(vec![]); - for i in 1 ..= spec.n() { + for i in 1 ..= spec.n(&removed) { let i = Participant::new(i).unwrap(); if our_i.contains(&i) { if shares.contains_key(&i) { @@ -415,13 +418,16 @@ async fn handle_processor_message( } key_gen::ProcessorMessage::InvalidShare { id, accuser, faulty, blame } => { // Check if the MuSig signature had any errors as if so, we need to provide - // RemoveParticipant + // RemoveParticipantDueToDkg // As for the safety of calling error_generating_key_pair, the processor is presumed // to only send InvalidShare or GeneratedKeyPair for a given attempt let mut txs = if let Some(faulty) = crate::tributary::error_generating_key_pair(&mut txn, key, spec, id.attempt) { - vec![Transaction::RemoveParticipant(faulty)] + vec![Transaction::RemoveParticipantDueToDkg { + attempt: id.attempt, + participant: faulty, + }] } else { vec![] }; @@ -457,14 +463,14 @@ async fn handle_processor_message( }] } Err(p) => { - vec![Transaction::RemoveParticipant(p)] + vec![Transaction::RemoveParticipantDueToDkg { attempt: id.attempt, participant: p }] } } } // This is a response to the ordered VerifyBlame, which is why this satisfies the provided // transaction's needs to be perfectly ordered - key_gen::ProcessorMessage::Blame { id: _, participant } => { - vec![Transaction::RemoveParticipant(participant)] + key_gen::ProcessorMessage::Blame { id, participant } => { + vec![Transaction::RemoveParticipantDueToDkg { attempt: id.attempt, participant }] } }, ProcessorMessage::Sign(msg) => match msg { diff --git a/coordinator/src/substrate/mod.rs b/coordinator/src/substrate/mod.rs index 999a1e3e1..c03f0ca24 100644 --- a/coordinator/src/substrate/mod.rs +++ b/coordinator/src/substrate/mod.rs @@ -268,6 +268,7 @@ async fn handle_block( let ValidatorSetsEvent::KeyGen { set, key_pair } = key_gen else { panic!("KeyGen event wasn't KeyGen: {key_gen:?}"); }; + let substrate_key = key_pair.0 .0; processors .send( set.network, @@ -289,7 +290,7 @@ async fn handle_block( ) .await; let mut txn = db.txn(); - SeraiDkgCompleted::set(&mut txn, set, &()); + SeraiDkgCompleted::set(&mut txn, set, &substrate_key); HandledEvent::handle_event(&mut txn, hash, event_id); txn.commit(); } diff --git a/coordinator/src/tests/tributary/mod.rs b/coordinator/src/tests/tributary/mod.rs index 0b1eae1c1..84c223438 100644 --- a/coordinator/src/tests/tributary/mod.rs +++ b/coordinator/src/tests/tributary/mod.rs @@ -123,10 +123,13 @@ fn serialize_sign_data() { #[test] fn serialize_transaction() { - test_read_write(Transaction::RemoveParticipant( - frost::Participant::new(u16::try_from(OsRng.next_u64() >> 48).unwrap().saturating_add(1)) - .unwrap(), - )); + test_read_write(Transaction::RemoveParticipantDueToDkg { + attempt: u32::try_from(OsRng.next_u64() >> 32).unwrap(), + participant: frost::Participant::new( + u16::try_from(OsRng.next_u64() >> 48).unwrap().saturating_add(1), + ) + .unwrap(), + }); { let mut commitments = vec![random_vec(&mut OsRng, 512)]; diff --git a/coordinator/src/tests/tributary/sync.rs b/coordinator/src/tests/tributary/sync.rs index 8723c04cb..0a468c63e 100644 --- a/coordinator/src/tests/tributary/sync.rs +++ b/coordinator/src/tests/tributary/sync.rs @@ -29,7 +29,7 @@ async fn sync_test() { let mut keys = new_keys(&mut OsRng); let spec = new_spec(&mut OsRng, &keys); // Ensure this can have a node fail - assert!(spec.n() > spec.t()); + assert!(spec.n(&[]) > spec.t()); let mut tributaries = new_tributaries(&keys, &spec) .await @@ -142,7 +142,7 @@ async fn sync_test() { // Because only `t` validators are used in a commit, take n - t nodes offline // leaving only `t` nodes. Which should force it to participate in the consensus // of next blocks. - let spares = usize::from(spec.n() - spec.t()); + let spares = usize::from(spec.n(&[]) - spec.t()); for thread in p2p_threads.iter().take(spares) { thread.abort(); } diff --git a/coordinator/src/tributary/db.rs b/coordinator/src/tributary/db.rs index f87bf2322..cf8358227 100644 --- a/coordinator/src/tributary/db.rs +++ b/coordinator/src/tributary/db.rs @@ -42,15 +42,18 @@ pub enum Accumulation { NotReady, } +// TODO: Move from genesis to set for indexing create_db!( Tributary { SeraiBlockNumber: (hash: [u8; 32]) -> u64, SeraiDkgRemoval: (spec: ValidatorSet, removing: [u8; 32]) -> (), - SeraiDkgCompleted: (spec: ValidatorSet) -> (), + 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]>, @@ -58,6 +61,7 @@ create_db!( RemovalNonces: (genesis: [u8; 32], removing: [u8; 32], attempt: u32) -> HashMap>, DkgKeyPair: (genesis: [u8; 32], attempt: u32) -> KeyPair, + KeyToDkgAttempt: (key: [u8; 32]) -> u32, DkgCompleted: (genesis: [u8; 32]) -> (), LocallyDkgRemoved: (genesis: [u8; 32], validator: [u8; 32]) -> (), AttemptDb: (genesis: [u8; 32], topic: &Topic) -> u32, @@ -74,13 +78,14 @@ impl FatallySlashed { Self::set(txn, genesis, account, &()); let mut existing = FatalSlashes::get(txn, genesis).unwrap_or_default(); - // Don't append if we already have it + // Don't append if we already have it, which can occur upon multiple faults if existing.iter().any(|existing| existing == &account) { return; } 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 18f8ad32f..4ab630665 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -39,7 +39,9 @@ pub fn dkg_confirmation_nonces( txn: &mut impl DbTxn, attempt: u32, ) -> [u8; 64] { - (DkgConfirmer { key, spec, txn, attempt }).preprocess() + DkgConfirmer::new(key, spec, txn, attempt) + .expect("getting DKG confirmation nonces for unknown attempt") + .preprocess() } // If there's an error generating a key pair, return any errors which would've occured when @@ -58,7 +60,10 @@ pub fn error_generating_key_pair( // Sign a key pair which can't be valid // (0xff used as 0 would be the Ristretto identity point, 0-length for the network key) let key_pair = KeyPair(Public([0xff; 32]), vec![0xffu8; 0].try_into().unwrap()); - match (DkgConfirmer { key, spec, txn, attempt }).share(preprocesses, &key_pair) { + match DkgConfirmer::new(key, spec, txn, attempt) + .expect("reporting an error during DKG for an unrecognized attempt") + .share(preprocesses, &key_pair) + { Ok(mut share) => { // Zeroize the share to ensure it's not accessed share.zeroize(); @@ -76,13 +81,20 @@ pub fn generated_key_pair( attempt: u32, ) -> Result<[u8; 32], Participant> { DkgKeyPair::set(txn, spec.genesis(), attempt, key_pair); + KeyToDkgAttempt::set(txn, key_pair.0 .0, &attempt); let preprocesses = ConfirmationNonces::get(txn, spec.genesis(), attempt).unwrap(); - (DkgConfirmer { key, spec, txn, attempt }).share(preprocesses, key_pair) + DkgConfirmer::new(key, spec, txn, attempt) + .expect("claiming to have generated a key pair for an unrecognized attempt") + .share(preprocesses, key_pair) } -fn unflatten(spec: &TributarySpec, data: &mut HashMap>) { +fn unflatten( + spec: &TributarySpec, + removed: &[::G], + data: &mut HashMap>, +) { for (validator, _) in spec.validators() { - let range = spec.i(validator).unwrap(); + let range = spec.i(removed, validator).unwrap(); let Some(all_segments) = data.remove(&range.start) else { continue; }; @@ -99,6 +111,7 @@ impl::G], data_spec: &DataSpecification, signer: ::G, data: &Vec, @@ -109,8 +122,10 @@ impl::G], data_spec: &DataSpecification, bytes: Vec, signed: &Signed, @@ -234,15 +250,16 @@ impl::G], signer: ::G, len: usize, ) -> Result<(), ()> { - let signer_i = self.spec.i(signer).unwrap(); + let signer_i = self.spec.i(removed, signer).unwrap(); if len != usize::from(u16::from(signer_i.end) - u16::from(signer_i.start)) { self .fatal_slash( @@ -255,16 +272,23 @@ impl) -> DkgRemoval<'_, T> { + fn dkg_removal<'a>( + &'a mut self, + removed: &'a [::G], + data: &'a SignData<[u8; 32]>, + ) -> DkgRemoval<'a, T> { DkgRemoval { - spec: self.spec, 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) { let genesis = self.spec.genesis(); @@ -279,19 +303,37 @@ impl { - self.fatal_slash_with_participant_index(i, "RemoveParticipant Provided TX").await + Transaction::RemoveParticipantDueToDkg { attempt, participant } => { + self + .fatal_slash_with_participant_index( + &removed_as_of_dkg_attempt(self.txn, genesis, attempt).unwrap_or_else(|| { + panic!( + "removed a participant due to a provided transaction with an attempt not {}", + "locally handled?" + ) + }), + participant, + "RemoveParticipantDueToDkg Provided TX", + ) + .await } Transaction::DkgCommitments { attempt, commitments, signed } => { - let Ok(_) = self.check_sign_data_len(signed.signer, commitments.len()).await else { + let Some(removed) = removed_as_of_dkg_attempt(self.txn, genesis, attempt) else { + self + .fatal_slash(signed.signer.to_bytes(), "DkgCommitments with an unrecognized attempt") + .await; + return; + }; + let Ok(_) = self.check_sign_data_len(&removed, signed.signer, commitments.len()).await + else { return; }; let data_spec = DataSpecification { topic: Topic::Dkg, label: Label::Preprocess, attempt }; - match self.handle_data(&data_spec, commitments.encode(), &signed).await { + match self.handle_data(&removed, &data_spec, commitments.encode(), &signed).await { Accumulation::Ready(DataSet::Participating(mut commitments)) => { log::info!("got all DkgCommitments for {}", hex::encode(genesis)); - unflatten(self.spec, &mut commitments); + unflatten(self.spec, &removed, &mut commitments); self .processors .send( @@ -311,17 +353,23 @@ impl { - let Ok(_) = self.check_sign_data_len(signed.signer, shares.len()).await else { + let Some(removed) = removed_as_of_dkg_attempt(self.txn, genesis, attempt) else { + self + .fatal_slash(signed.signer.to_bytes(), "DkgShares with an unrecognized attempt") + .await; + return; + }; + let Ok(_) = self.check_sign_data_len(&removed, signed.signer, shares.len()).await else { return; }; let sender_i = self .spec - .i(signed.signer) + .i(&removed, signed.signer) .expect("transaction added to tributary by signer who isn't a participant"); let sender_is_len = u16::from(sender_i.end) - u16::from(sender_i.start); for shares in &shares { - if shares.len() != (usize::from(self.spec.n() - sender_is_len)) { + if shares.len() != (usize::from(self.spec.n(&removed) - sender_is_len)) { self.fatal_slash(signed.signer.to_bytes(), "invalid amount of DKG shares").await; return; } @@ -329,7 +377,7 @@ impl { log::info!("got all DkgShares for {}", hex::encode(genesis)); @@ -440,7 +488,13 @@ impl { - let range = self.spec.i(signed.signer).unwrap(); + let Some(removed) = removed_as_of_dkg_attempt(self.txn, genesis, attempt) else { + self + .fatal_slash(signed.signer.to_bytes(), "InvalidDkgShare with an unrecognized attempt") + .await; + return; + }; + let range = self.spec.i(&removed, signed.signer).unwrap(); if !range.contains(&accuser) { self .fatal_slash( @@ -457,7 +511,15 @@ impl { + let Some(removed) = removed_as_of_dkg_attempt(self.txn, genesis, attempt) else { + self + .fatal_slash(signed.signer.to_bytes(), "DkgConfirmed with an unrecognized attempt") + .await; + return; + }; + let data_spec = DataSpecification { topic: Topic::DkgConfirmation, label: Label::Share, attempt }; - match self.handle_data(&data_spec, confirmation_share.to_vec(), &signed).await { + match self.handle_data(&removed, &data_spec, confirmation_share.to_vec(), &signed).await { Accumulation::Ready(DataSet::Participating(shares)) => { log::info!("got all DkgConfirmed for {}", hex::encode(genesis)); + let Some(removed) = removed_as_of_dkg_attempt(self.txn, genesis, attempt) else { + panic!( + "DkgConfirmed for everyone yet didn't have the removed parties for this attempt", + ); + }; + let preprocesses = ConfirmationNonces::get(self.txn, genesis, attempt).unwrap(); // TODO: This can technically happen under very very very specific timing as the txn put // happens before DkgConfirmed, yet the txn commit isn't guaranteed to @@ -487,16 +562,17 @@ impl sig, - Err(p) => { - self.fatal_slash_with_participant_index(p, "invalid DkgConfirmer share").await; - return; - } - }; + let mut confirmer = DkgConfirmer::new(self.our_key, self.spec, self.txn, attempt) + .expect("confirming DKG for unrecognized attempt"); + let sig = match confirmer.complete(preprocesses, &key_pair, shares) { + Ok(sig) => sig, + Err(p) => { + self + .fatal_slash_with_participant_index(&removed, p, "invalid DkgConfirmer share") + .await; + return; + } + }; DkgCompleted::set(self.txn, genesis, &()); @@ -527,13 +603,20 @@ impl { RemovalNonces::set(self.txn, genesis, data.plan, data.attempt, &results); - let Ok(share) = self.dkg_removal(&data).share(results) else { + 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; @@ -562,7 +645,8 @@ impl { + // Provided transactions ensure synchrony on any signing protocol, and we won't start + // signing with threshold keys before we've confirmed them on-chain + let Some(removed) = + crate::tributary::removed_as_of_set_keys(self.txn, self.spec.set(), genesis) + else { + self + .fatal_slash( + data.signed.signer.to_bytes(), + "signing despite not having set keys on substrate", + ) + .await; + return; + }; let signer = data.signed.signer; - let Ok(_) = self.check_sign_data_len(signer, data.data.len()).await else { + let Ok(_) = self.check_sign_data_len(&removed, signer, data.data.len()).await else { return; }; let expected_len = match data.label { @@ -672,6 +769,7 @@ impl { - let Ok(_) = self.check_sign_data_len(data.signed.signer, data.data.len()).await else { + let Some(removed) = + crate::tributary::removed_as_of_set_keys(self.txn, self.spec.set(), genesis) + else { + self + .fatal_slash( + data.signed.signer.to_bytes(), + "signing despite not having set keys on substrate", + ) + .await; + return; + }; + let Ok(_) = self.check_sign_data_len(&removed, data.signed.signer, data.data.len()).await + else { return; }; @@ -716,9 +826,9 @@ impl Option::G>> { + if attempt == 0 { + Some(vec![]) + } else { + FatalSlashesAsOfDkgAttempt::get(getter, genesis, attempt).map(|keys| { + keys.iter().map(|key| ::G::from_bytes(key).unwrap()).collect() + }) + } +} + +pub fn latest_removed(getter: &impl Get, genesis: [u8; 32]) -> Vec<::G> { + #[allow(clippy::unwrap_or_default)] + FatalSlashes::get(getter, genesis) + .unwrap_or(vec![]) + .iter() + .map(|key| ::G::from_bytes(key).unwrap()) + .collect() +} + +pub fn removed_as_of_set_keys( + getter: &impl Get, + set: ValidatorSet, + genesis: [u8; 32], +) -> Option::G>> { + // SeraiDkgCompleted has the key placed on-chain. + // This key can be uniquely mapped to an attempt so long as one participant was honest, which we + // assume as a presumably honest participant. + // Resolve from generated key to attempt to fatally slashed as of attempt. + + // This expect will trigger if this is prematurely called and Substrate has tracked the keys yet + // we haven't locally synced and handled the Tributary + // All callers of this, at the time of writing, ensure the Tributary has sufficiently synced + // making the panic with context more desirable than the None + let attempt = KeyToDkgAttempt::get(getter, SeraiDkgCompleted::get(getter, set)?) + .expect("key completed on-chain didn't have an attempt related"); + 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 02365ee7c..0097e0b80 100644 --- a/coordinator/src/tributary/scanner.rs +++ b/coordinator/src/tributary/scanner.rs @@ -133,10 +133,19 @@ pub struct TributaryBlockHandler< impl TributaryBlockHandler<'_, T, Pro, PST, PTT, RID, P> { - async fn dkg_removal_attempt(&mut self, removing: [u8; 32], attempt: u32) { - let preprocess = - (DkgRemoval { spec: self.spec, key: self.our_key, txn: self.txn, removing, attempt }) - .preprocess(); + 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, @@ -144,7 +153,7 @@ impl::G], + i: Participant, + reason: &str, + ) { // Resolve from Participant to ::G let i = u16::from(i); let mut validator = None; for (potential, _) in self.spec.validators() { - let v_i = self.spec.i(potential).unwrap(); + let v_i = self.spec.i(removed, potential).unwrap(); if (u16::from(v_i.start) <= i) && (i < u16::from(v_i.end)) { validator = Some(potential); break; @@ -250,19 +264,34 @@ impl { + #[allow(clippy::unwrap_or_default)] + FatalSlashesAsOfDkgAttempt::set( + self.txn, + genesis, + attempt, + &FatalSlashes::get(self.txn, genesis).unwrap_or(vec![]), + ); + if DkgCompleted::get(self.txn, genesis).is_none() { // Since it wasn't completed, instruct the processor to start the next attempt let id = processor_messages::key_gen::KeyGenId { session: self.spec.set().session, attempt }; - let our_i = self.spec.i(Ristretto::generator() * self.our_key.deref()).unwrap(); - // TODO: Handle removed parties (modify n/i to accept list of removed) + let removed = crate::tributary::latest_removed(self.txn, genesis); + let our_i = + self.spec.i(&removed, Ristretto::generator() * self.our_key.deref()).unwrap(); + // TODO: Don't fatal slash, yet don't include, parties who have been offline so long as - // we still meet the needed threshold. We'd need a complete DKG protocol we then remove - // the offline participants from. publishing the DKG protocol completed without them. + // we still meet the needed threshold. This will have to have any parties removed for + // being offline, who aren't participating in the confirmed key, drop the Tributary and + // notify their processor. + + // TODO: Instead of DKG confirmations taking a n-of-n MuSig, have it take a t-of-n with + // a specification of those explicitly removed and those removed due to being offline. let params = - frost::ThresholdParams::new(self.spec.t(), self.spec.n(), our_i.start).unwrap(); + frost::ThresholdParams::new(self.spec.t(), self.spec.n(&removed), our_i.start) + .unwrap(); let shares = u16::from(our_i.end) - u16::from(our_i.start); self @@ -284,7 +313,7 @@ impl { diff --git a/coordinator/src/tributary/signing_protocol.rs b/coordinator/src/tributary/signing_protocol.rs index 2db2d9644..9cf34499c 100644 --- a/coordinator/src/tributary/signing_protocol.rs +++ b/coordinator/src/tributary/signing_protocol.rs @@ -217,19 +217,20 @@ impl SigningProtocol<'_, T, C> { // 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 = - spec.i(::generator() * our_key.deref()).unwrap().start; + spec.i(removed, ::generator() * our_key.deref()).unwrap().start; assert!(map.insert(our_threshold_i, vec![]).is_none()); let spec_validators = spec.validators(); let key_from_threshold_i = |threshold_i| { for (key, _) in &spec_validators { - if threshold_i == spec.i(*key).unwrap().start { + if threshold_i == spec.i(removed, *key).unwrap().start { return *key; } } @@ -262,13 +263,25 @@ fn threshold_i_map_to_keys_and_musig_i_map( } pub(crate) struct DkgConfirmer<'a, T: DbTxn> { - pub(crate) key: &'a Zeroizing<::F>, - pub(crate) spec: &'a TributarySpec, - pub(crate) txn: &'a mut T, - pub(crate) attempt: u32, + key: &'a Zeroizing<::F>, + spec: &'a TributarySpec, + removed: Vec<::G>, + txn: &'a mut T, + attempt: u32, } impl DkgConfirmer<'_, T> { + pub(crate) fn new<'a>( + key: &'a Zeroizing<::F>, + spec: &'a TributarySpec, + txn: &'a mut T, + attempt: u32, + ) -> Option> { + // This relies on how confirmations are inlined into the DKG protocol and they accordingly + // share attempts + let removed = crate::tributary::removed_as_of_dkg_attempt(txn, spec.genesis(), attempt)?; + Some(DkgConfirmer { key, spec, removed, txn, attempt }) + } fn signing_protocol(&mut self) -> SigningProtocol<'_, T, (&'static [u8; 12], u32)> { let context = (b"DkgConfirmer", self.attempt); SigningProtocol { key: self.key, spec: self.spec, txn: self.txn, context } @@ -289,8 +302,14 @@ 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.key, preprocesses, false).1; + 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); self.signing_protocol().share_internal(&participants, preprocesses, &msg) } @@ -309,7 +328,8 @@ impl DkgConfirmer<'_, T> { key_pair: &KeyPair, shares: HashMap>, ) -> Result<[u8; 64], Participant> { - let shares = threshold_i_map_to_keys_and_musig_i_map(self.spec, self.key, shares, false).1; + let shares = + threshold_i_map_to_keys_and_musig_i_map(self.spec, &self.removed, self.key, shares, false).1; let machine = self .share_internal(preprocesses, key_pair) @@ -323,6 +343,7 @@ impl DkgConfirmer<'_, T> { 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, @@ -362,8 +383,13 @@ impl DkgRemoval<'_, T> { &mut self, preprocesses: HashMap>, ) -> Result<(AlgorithmSignatureMachine, [u8; 32]), Participant> { - let (participants, preprocesses) = - threshold_i_map_to_keys_and_musig_i_map(self.spec, self.key, preprocesses, true); + 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) } @@ -381,7 +407,7 @@ impl DkgRemoval<'_, T> { shares: HashMap>, ) -> Result<(Vec, [u8; 64]), Participant> { let (participants, shares) = - threshold_i_map_to_keys_and_musig_i_map(self.spec, self.key, shares, true); + 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 diff --git a/coordinator/src/tributary/spec.rs b/coordinator/src/tributary/spec.rs index 95f9e595b..e8858ca2a 100644 --- a/coordinator/src/tributary/spec.rs +++ b/coordinator/src/tributary/spec.rs @@ -1,5 +1,5 @@ use core::{ops::Range, fmt::Debug}; -use std::io; +use std::{io, collections::HashMap}; use transcript::{Transcript, RecommendedTranscript}; @@ -88,26 +88,53 @@ impl TributarySpec { self.start_time } - pub fn n(&self) -> u16 { - self.validators.iter().map(|(_, weight)| weight).sum() + pub fn n(&self, removed_validators: &[::G]) -> u16 { + self + .validators + .iter() + .map(|(validator, weight)| if removed_validators.contains(validator) { 0 } else { *weight }) + .sum() } pub fn t(&self) -> u16 { - ((2 * self.n()) / 3) + 1 + // t doesn't change with regards to the amount of removed validators + ((2 * self.n(&[])) / 3) + 1 } - pub fn i(&self, key: ::G) -> Option> { + pub fn i( + &self, + removed_validators: &[::G], + key: ::G, + ) -> Option> { + let mut all_is = HashMap::new(); let mut i = 1; for (validator, weight) in &self.validators { - if validator == &key { - return Some(Range { - start: Participant::new(i).unwrap(), - end: Participant::new(i + weight).unwrap(), - }); - } + all_is.insert( + *validator, + Range { start: Participant::new(i).unwrap(), end: Participant::new(i + weight).unwrap() }, + ); i += weight; } - None + + let original_i = all_is.get(&key)?.clone(); + let mut result_i = original_i.clone(); + for removed_validator in removed_validators { + let removed_i = all_is + .get(removed_validator) + .expect("removed validator wasn't present in set to begin with"); + // If the queried key was removed, return None + if &original_i == removed_i { + return None; + } + + // If the removed was before the queried, shift the queried down accordingly + if removed_i.start < original_i.start { + let removed_shares = u16::from(removed_i.end) - u16::from(removed_i.start); + result_i.start = Participant::new(u16::from(original_i.start) - removed_shares).unwrap(); + result_i.end = Participant::new(u16::from(original_i.end) - removed_shares).unwrap(); + } + } + Some(result_i) } pub fn validators(&self) -> Vec<(::G, u64)> { diff --git a/coordinator/src/tributary/transaction.rs b/coordinator/src/tributary/transaction.rs index 65a8eae85..ebbdcc17f 100644 --- a/coordinator/src/tributary/transaction.rs +++ b/coordinator/src/tributary/transaction.rs @@ -130,7 +130,10 @@ impl SignData { #[derive(Clone, PartialEq, Eq)] pub enum Transaction { - RemoveParticipant(Participant), + RemoveParticipantDueToDkg { + attempt: u32, + participant: Participant, + }, DkgCommitments { attempt: u32, @@ -194,9 +197,10 @@ pub enum Transaction { impl Debug for Transaction { fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> Result<(), core::fmt::Error> { match self { - Transaction::RemoveParticipant(participant) => fmt - .debug_struct("Transaction::RemoveParticipant") + Transaction::RemoveParticipantDueToDkg { attempt, participant } => fmt + .debug_struct("Transaction::RemoveParticipantDueToDkg") .field("participant", participant) + .field("attempt", attempt) .finish(), Transaction::DkgCommitments { attempt, commitments: _, signed } => fmt .debug_struct("Transaction::DkgCommitments") @@ -255,12 +259,19 @@ impl ReadWrite for Transaction { reader.read_exact(&mut kind)?; match kind[0] { - 0 => Ok(Transaction::RemoveParticipant({ - let mut participant = [0; 2]; - reader.read_exact(&mut participant)?; - Participant::new(u16::from_le_bytes(participant)) - .ok_or_else(|| io::Error::other("invalid participant in RemoveParticipant"))? - })), + 0 => Ok(Transaction::RemoveParticipantDueToDkg { + attempt: { + let mut attempt = [0; 4]; + reader.read_exact(&mut attempt)?; + u32::from_le_bytes(attempt) + }, + participant: { + let mut participant = [0; 2]; + reader.read_exact(&mut participant)?; + Participant::new(u16::from_le_bytes(participant)) + .ok_or_else(|| io::Error::other("invalid participant in RemoveParticipantDueToDkg"))? + }, + }), 1 => { let mut attempt = [0; 4]; @@ -424,9 +435,10 @@ impl ReadWrite for Transaction { fn write(&self, writer: &mut W) -> io::Result<()> { match self { - Transaction::RemoveParticipant(i) => { + Transaction::RemoveParticipantDueToDkg { attempt, participant } => { writer.write_all(&[0])?; - writer.write_all(&u16::from(*i).to_le_bytes()) + writer.write_all(&attempt.to_le_bytes())?; + writer.write_all(&u16::from(*participant).to_le_bytes()) } Transaction::DkgCommitments { attempt, commitments, signed } => { @@ -545,7 +557,7 @@ impl ReadWrite for Transaction { impl TransactionTrait for Transaction { fn kind(&self) -> TransactionKind<'_> { match self { - Transaction::RemoveParticipant(_) => TransactionKind::Provided("remove"), + Transaction::RemoveParticipantDueToDkg { .. } => TransactionKind::Provided("remove"), Transaction::DkgCommitments { attempt, commitments: _, signed } => { TransactionKind::Signed((b"dkg", attempt).encode(), signed) @@ -623,7 +635,9 @@ impl Transaction { ) { fn signed(tx: &mut Transaction) -> (u32, &mut Signed) { let nonce = match tx { - Transaction::RemoveParticipant(_) => panic!("signing RemoveParticipant"), + Transaction::RemoveParticipantDueToDkg { .. } => { + panic!("signing RemoveParticipantDueToDkg") + } Transaction::DkgCommitments { .. } => 0, Transaction::DkgShares { .. } => 1, @@ -645,7 +659,7 @@ impl Transaction { ( nonce, match tx { - Transaction::RemoveParticipant(_) => panic!("signing RemoveParticipant"), + Transaction::RemoveParticipantDueToDkg { .. } => panic!("signing RemoveParticipant"), Transaction::DkgCommitments { ref mut signed, .. } => signed, Transaction::DkgShares { ref mut signed, .. } => signed, diff --git a/coordinator/tributary/src/block.rs b/coordinator/tributary/src/block.rs index f931b5b76..d832e8997 100644 --- a/coordinator/tributary/src/block.rs +++ b/coordinator/tributary/src/block.rs @@ -249,8 +249,6 @@ impl Block { } last_tx_order = current_tx_order; - // TODO: should we modify the verify_transaction to take `Transaction` or - // use this pattern of verifying tendermint Txs and app txs differently? match tx { Transaction::Tendermint(tx) => { match verify_tendermint_tx::(tx, schema.clone(), &commit) { diff --git a/coordinator/tributary/src/blockchain.rs b/coordinator/tributary/src/blockchain.rs index 71767f307..7668de78d 100644 --- a/coordinator/tributary/src/blockchain.rs +++ b/coordinator/tributary/src/blockchain.rs @@ -272,6 +272,7 @@ impl Blockchain { provided_in_chain, allow_non_local_provided, ); + // Drop this TXN's changes as we're solely verifying the block drop(txn); res } diff --git a/coordinator/tributary/src/transaction.rs b/coordinator/tributary/src/transaction.rs index 6f238ef54..1aa37c59e 100644 --- a/coordinator/tributary/src/transaction.rs +++ b/coordinator/tributary/src/transaction.rs @@ -179,7 +179,6 @@ pub trait Transaction: 'static + Send + Sync + Clone + Eq + Debug + ReadWrite { pub trait GAIN: FnMut(&::G, &[u8]) -> Option {} impl::G, &[u8]) -> Option> GAIN for F {} -// This will only cause mutations when the transaction is valid pub(crate) fn verify_transaction( tx: &T, genesis: [u8; 32], @@ -204,7 +203,7 @@ pub(crate) fn verify_transaction( Err(TransactionError::InvalidSigner)?; } - // TODO: Use Schnorr half-aggregation and a batch verification here + // TODO: Use a batch verification here if !signature.verify(*signer, tx.sig_hash(genesis)) { Err(TransactionError::InvalidSignature)?; } diff --git a/processor/src/key_gen.rs b/processor/src/key_gen.rs index 9f987794f..1a35f4d27 100644 --- a/processor/src/key_gen.rs +++ b/processor/src/key_gen.rs @@ -29,7 +29,7 @@ pub struct KeyConfirmed { create_db!( KeyGenDb { - ParamsDb: (session: &Session) -> (ThresholdParams, u16), + ParamsDb: (session: &Session, attempt: u32) -> (ThresholdParams, u16), // Not scoped to the set since that'd have latter attempts overwrite former // A former attempt may become the finalized attempt, even if it doesn't in a timely manner // Overwriting its commitments would be accordingly poor @@ -152,7 +152,10 @@ impl KeyGen { pub fn in_set(&self, session: &Session) -> bool { // We determine if we're in set using if we have the parameters for a session's key generation - ParamsDb::get(&self.db, session).is_some() + // The usage of 0 for the attempt is valid so long as we aren't malicious and accordingly + // aren't fatally slashed + // TODO: Revisit once we do DKG removals for being offline + ParamsDb::get(&self.db, session, 0).is_some() } #[allow(clippy::type_complexity)] @@ -319,7 +322,7 @@ impl KeyGen { self.active_share.remove(&id.session).is_none() { // If we haven't handled this session before, save the params - ParamsDb::set(txn, &id.session, &(params, shares)); + ParamsDb::set(txn, &id.session, id.attempt, &(params, shares)); } let (machines, commitments) = key_gen_machines(id, params, shares); @@ -338,7 +341,7 @@ impl KeyGen { panic!("commitments when already handled commitments"); } - let (params, share_quantity) = ParamsDb::get(txn, &id.session).unwrap(); + let (params, share_quantity) = ParamsDb::get(txn, &id.session, id.attempt).unwrap(); // Unwrap the machines, rebuilding them if we didn't have them in our cache // We won't if the processor rebooted @@ -373,7 +376,7 @@ impl KeyGen { CoordinatorMessage::Shares { id, shares } => { info!("Received shares for {:?}", id); - let (params, share_quantity) = ParamsDb::get(txn, &id.session).unwrap(); + let (params, share_quantity) = ParamsDb::get(txn, &id.session, id.attempt).unwrap(); // Same commentary on inconsistency as above exists let (machines, our_shares) = self.active_share.remove(&id.session).unwrap_or_else(|| { @@ -514,7 +517,7 @@ impl KeyGen { } CoordinatorMessage::VerifyBlame { id, accuser, accused, share, blame } => { - let params = ParamsDb::get(txn, &id.session).unwrap().0; + let params = ParamsDb::get(txn, &id.session, id.attempt).unwrap().0; let mut share_ref = share.as_slice(); let Ok(substrate_share) = EncryptedMessage::<