From 576d3c90c6475a6a353eb224a03686816418a1c8 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Fri, 10 Nov 2023 08:43:32 -0500 Subject: [PATCH 01/11] Have processor report errors during the DKG to the coordinator --- processor/messages/src/lib.rs | 10 ++- processor/src/key_gen.rs | 159 +++++++++++++++++++++++---------- processor/src/multisigs/mod.rs | 3 +- 3 files changed, 121 insertions(+), 51 deletions(-) diff --git a/processor/messages/src/lib.rs b/processor/messages/src/lib.rs index 317ad6ae3..2013bce1e 100644 --- a/processor/messages/src/lib.rs +++ b/processor/messages/src/lib.rs @@ -50,8 +50,12 @@ pub mod key_gen { pub enum ProcessorMessage { // Created commitments for the specified key generation protocol. Commitments { id: KeyGenId, commitments: Vec> }, + // Participant published invalid commitments. + InvalidCommitments { id: KeyGenId, faulty: Participant }, // Created shares for the specified key generation protocol. Shares { id: KeyGenId, shares: Vec>> }, + // Participant published an invalid share. + InvalidShare { id: KeyGenId, faulty: Participant, blame: Option> }, // Resulting keys from the specified key generation protocol. GeneratedKeyPair { id: KeyGenId, substrate_key: [u8; 32], network_key: Vec }, } @@ -340,8 +344,10 @@ impl ProcessorMessage { let (sub, id) = match msg { // Unique since KeyGenId key_gen::ProcessorMessage::Commitments { id, .. } => (0, id), - key_gen::ProcessorMessage::Shares { id, .. } => (1, id), - key_gen::ProcessorMessage::GeneratedKeyPair { id, .. } => (2, id), + key_gen::ProcessorMessage::InvalidCommitments { id, .. } => (1, id), + key_gen::ProcessorMessage::Shares { id, .. } => (2, id), + key_gen::ProcessorMessage::InvalidShare { id, .. } => (3, id), + key_gen::ProcessorMessage::GeneratedKeyPair { id, .. } => (4, id), }; let mut res = vec![PROCESSSOR_UID, TYPE_KEY_GEN_UID, sub]; diff --git a/processor/src/key_gen.rs b/processor/src/key_gen.rs index 9e20f57e4..e0737660d 100644 --- a/processor/src/key_gen.rs +++ b/processor/src/key_gen.rs @@ -9,7 +9,9 @@ use transcript::{Transcript, RecommendedTranscript}; use ciphersuite::group::GroupEncoding; use frost::{ curve::{Ciphersuite, Ristretto}, - dkg::{Participant, ThresholdParams, ThresholdCore, ThresholdKeys, encryption::*, frost::*}, + dkg::{ + DkgError, Participant, ThresholdParams, ThresholdCore, ThresholdKeys, encryption::*, frost::*, + }, }; use log::info; @@ -199,32 +201,50 @@ impl KeyGen { |id, params: ThresholdParams, (machines, our_commitments): (SecretShareMachines, Vec>), - commitments: HashMap>| { + commitments: HashMap>| + -> Result<_, ProcessorMessage> { let mut rng = secret_shares_rng(id); #[allow(clippy::type_complexity)] fn handle_machine( rng: &mut ChaCha20Rng, + id: KeyGenId, params: ThresholdParams, machine: SecretShareMachine, commitments_ref: &mut HashMap, - ) -> (KeyMachine, HashMap>>) { + ) -> Result< + (KeyMachine, HashMap>>), + ProcessorMessage, + > { // Parse the commitments - let parsed = match commitments_ref - .iter_mut() - .map(|(i, commitments)| { + let mut parsed = HashMap::new(); + for i in 1 ..= params.n() { + let i = Participant::new(i).unwrap(); + let Some(commitments) = commitments_ref.get_mut(&i) else { continue }; + parsed.insert( + i, EncryptionKeyMessage::>::read(commitments, params) - .map(|commitments| (*i, commitments)) - }) - .collect() - { - Ok(commitments) => commitments, - Err(e) => todo!("malicious signer: {:?}", e), - }; + .map_err(|_| ProcessorMessage::InvalidCommitments { id, faulty: i })?, + ); + } match machine.generate_secret_shares(rng, parsed) { - Ok(res) => res, - Err(e) => todo!("malicious signer: {:?}", e), + Ok(res) => Ok(res), + Err(e) => match e { + DkgError::ZeroParameter(_, _) | + DkgError::InvalidThreshold(_, _) | + DkgError::InvalidParticipant(_, _) | + DkgError::InvalidSigningSet | + DkgError::InvalidShare { .. } => unreachable!("{e:?}"), + DkgError::InvalidParticipantQuantity(_, _) | + DkgError::DuplicatedParticipant(_) | + DkgError::MissingParticipant(_) => { + panic!("coordinator sent invalid DKG commitments: {e:?}") + } + DkgError::InvalidProofOfKnowledge(i) => { + Err(ProcessorMessage::InvalidCommitments { id, faulty: i })? + } + }, } } @@ -244,15 +264,24 @@ impl KeyGen { } } - let (substrate_machine, mut substrate_shares) = - handle_machine::(&mut rng, params, substrate_machine, &mut commitments_ref); + let (substrate_machine, mut substrate_shares) = handle_machine::( + &mut rng, + id, + params, + substrate_machine, + &mut commitments_ref, + )?; let (network_machine, network_shares) = - handle_machine(&mut rng, params, network_machine, &mut commitments_ref); + handle_machine(&mut rng, id, params, network_machine, &mut commitments_ref)?; key_machines.push((substrate_machine, network_machine)); - for (_, commitments) in commitments_ref { + for i in 1 ..= params.n() { + let i = Participant::new(i).unwrap(); + let commitments = &commitments_ref[&i]; if !commitments.is_empty() { - todo!("malicious signer: extra bytes"); + // Malicious Participant included extra bytes in their commitments + // (a potential DoS attack) + Err(ProcessorMessage::InvalidCommitments { id, faulty: i })?; } } @@ -263,7 +292,7 @@ impl KeyGen { } shares.push(these_shares); } - (key_machines, shares) + Ok((key_machines, shares)) }; match msg { @@ -307,11 +336,13 @@ impl KeyGen { .unwrap_or_else(|| key_gen_machines(id, params, share_quantity)); CommitmentsDb::set(txn, &id, &commitments); - let (machines, shares) = secret_share_machines(id, params, prior, commitments); - - self.active_share.insert(id.set, (machines, shares.clone())); - - ProcessorMessage::Shares { id, shares } + match secret_share_machines(id, params, prior, commitments) { + Ok((machines, shares)) => { + self.active_share.insert(id.set, (machines, shares.clone())); + ProcessorMessage::Shares { id, shares } + } + Err(e) => e, + } } CoordinatorMessage::Shares { id, shares } => { @@ -323,34 +354,56 @@ impl KeyGen { let (machines, our_shares) = self.active_share.remove(&id.set).unwrap_or_else(|| { let prior = key_gen_machines(id, params, share_quantity); secret_share_machines(id, params, prior, CommitmentsDb::get(txn, &id).unwrap()) + .expect("got Shares for a key gen which faulted") }); let mut rng = share_rng(id); fn handle_machine( rng: &mut ChaCha20Rng, + id: KeyGenId, params: ThresholdParams, machine: KeyMachine, shares_ref: &mut HashMap, - ) -> ThresholdCore { + ) -> Result, ProcessorMessage> { // Parse the shares - let shares = match shares_ref - .iter_mut() - .map(|(i, share)| { - EncryptedMessage::>::read(share, params).map(|share| (*i, share)) + let mut shares = HashMap::new(); + for i in 1 ..= params.n() { + let i = Participant::new(i).unwrap(); + let Some(share) = shares_ref.get_mut(&i) else { continue }; + shares.insert( + i, + EncryptedMessage::>::read(share, params) + .map_err(|_| ProcessorMessage::InvalidShare { id, faulty: i, blame: None })?, + ); + } + + // TODO(now): Handle the blame machine properly + Ok( + (match machine.calculate_share(rng, shares) { + Ok(res) => res, + Err(e) => match e { + DkgError::ZeroParameter(_, _) | + DkgError::InvalidThreshold(_, _) | + DkgError::InvalidParticipant(_, _) | + DkgError::InvalidSigningSet | + DkgError::InvalidProofOfKnowledge(_) => unreachable!("{e:?}"), + DkgError::InvalidParticipantQuantity(_, _) | + DkgError::DuplicatedParticipant(_) | + DkgError::MissingParticipant(_) => { + panic!("coordinator sent invalid DKG shares: {e:?}") + } + DkgError::InvalidShare { participant, blame } => { + Err(ProcessorMessage::InvalidShare { + id, + faulty: participant, + blame: Some(blame.map(|blame| blame.serialize())).flatten(), + })? + } + }, }) - .collect() - { - Ok(shares) => shares, - Err(e) => todo!("malicious signer: {:?}", e), - }; - - // TODO2: Handle the blame machine properly - (match machine.calculate_share(rng, shares) { - Ok(res) => res, - Err(e) => todo!("malicious signer: {:?}", e), - }) - .complete() + .complete(), + ) } let mut substrate_keys = vec![]; @@ -371,12 +424,22 @@ impl KeyGen { } } - let these_substrate_keys = handle_machine(&mut rng, params, machines.0, &mut shares_ref); - let these_network_keys = handle_machine(&mut rng, params, machines.1, &mut shares_ref); - - for (_, shares) in shares_ref { + let these_substrate_keys = + match handle_machine(&mut rng, id, params, machines.0, &mut shares_ref) { + Ok(keys) => keys, + Err(msg) => return msg, + }; + let these_network_keys = + match handle_machine(&mut rng, id, params, machines.1, &mut shares_ref) { + Ok(keys) => keys, + Err(msg) => return msg, + }; + + for i in 1 ..= params.n() { + let i = Participant::new(i).unwrap(); + let shares = &shares_ref[&i]; if !shares.is_empty() { - todo!("malicious signer: extra bytes"); + return ProcessorMessage::InvalidShare { id, faulty: i, blame: None }; } } diff --git a/processor/src/multisigs/mod.rs b/processor/src/multisigs/mod.rs index 55db0f83d..26d8a7822 100644 --- a/processor/src/multisigs/mod.rs +++ b/processor/src/multisigs/mod.rs @@ -938,7 +938,8 @@ impl MultisigManager { } // Save the plans created while scanning - // TODO: Should we combine all of these plans? + // TODO: Should we combine all of these plans to reduce the fees incurred from their + // execution? They're refunds and forwards. Neither should need isolate Plan/Eventualities. MultisigsDb::::set_plans_from_scanning(txn, block_number, plans); // If any outputs were delayed, append them into this block From c7b05b329d0a88610afa4315b3db4a34b30a21f5 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 11 Nov 2023 07:41:34 -0500 Subject: [PATCH 02/11] Add RemoveParticipant, InvalidDkgShare to coordinator --- coordinator/src/main.rs | 19 ++++ coordinator/src/tests/tributary/mod.rs | 21 +++++ coordinator/src/tributary/handle.rs | 6 ++ coordinator/src/tributary/mod.rs | 103 +++++++++++++++++---- coordinator/src/tributary/nonce_decider.rs | 8 ++ docs/DKG Exclusions.md | 23 +++++ 6 files changed, 160 insertions(+), 20 deletions(-) create mode 100644 docs/DKG Exclusions.md diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index 905fc4e51..a8b71df10 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -181,7 +181,9 @@ async fn handle_processor_message( // in-set, making the Tributary relevant ProcessorMessage::KeyGen(inner_msg) => match inner_msg { key_gen::ProcessorMessage::Commitments { id, .. } => Some(id.set.session), + key_gen::ProcessorMessage::InvalidCommitments { id, .. } => Some(id.set.session), key_gen::ProcessorMessage::Shares { id, .. } => Some(id.set.session), + key_gen::ProcessorMessage::InvalidShare { id, .. } => Some(id.set.session), key_gen::ProcessorMessage::GeneratedKeyPair { id, .. } => Some(id.set.session), }, // TODO: Review replacing key with Session in messages? @@ -419,6 +421,15 @@ async fn handle_processor_message( key_gen::ProcessorMessage::Commitments { id, commitments } => { vec![Transaction::DkgCommitments(id.attempt, commitments, Transaction::empty_signed())] } + 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 + // 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)] + } 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, id.attempt); @@ -455,6 +466,14 @@ async fn handle_processor_message( signed: Transaction::empty_signed(), }] } + key_gen::ProcessorMessage::InvalidShare { id, faulty, blame } => { + vec![Transaction::InvalidDkgShare { + attempt: id.attempt, + faulty, + blame, + signed: Transaction::empty_signed(), + }] + } key_gen::ProcessorMessage::GeneratedKeyPair { id, substrate_key, network_key } => { assert_eq!( id.set.network, msg.network, diff --git a/coordinator/src/tests/tributary/mod.rs b/coordinator/src/tests/tributary/mod.rs index b8061d347..c64ef9b67 100644 --- a/coordinator/src/tests/tributary/mod.rs +++ b/coordinator/src/tests/tributary/mod.rs @@ -88,6 +88,11 @@ 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(), + )); + { let mut commitments = vec![random_vec(&mut OsRng, 512)]; for _ in 0 .. (OsRng.next_u64() % 100) { @@ -133,6 +138,22 @@ fn serialize_transaction() { }); } + for i in 0 .. 2 { + test_read_write(Transaction::InvalidDkgShare { + attempt: random_u32(&mut OsRng), + faulty: frost::Participant::new( + u16::try_from(OsRng.next_u64() >> 48).unwrap().saturating_add(1), + ) + .unwrap(), + blame: if i == 0 { + None + } else { + Some(random_vec(&mut OsRng, 500)).filter(|blame| !blame.is_empty()) + }, + signed: random_signed(&mut OsRng), + }); + } + test_read_write(Transaction::DkgConfirmed( random_u32(&mut OsRng), { diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index 253340627..8213f779a 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -178,6 +178,8 @@ pub(crate) async fn handle_application_tx< } match tx { + // TODO: Either remove the validator from Tendermint OR form a second Tributary post-DKG + Transaction::RemoveParticipant(i) => todo!("TODO(now)"), Transaction::DkgCommitments(attempt, commitments, signed) => { let Ok(_) = check_sign_data_len::(txn, spec, signed.signer, commitments.len()) else { return; @@ -327,7 +329,11 @@ pub(crate) async fn handle_application_tx< } } + Transaction::InvalidDkgShare { attempt, faulty, blame, signed } => todo!("TODO(now)"), + Transaction::DkgConfirmed(attempt, shares, signed) => { + // TODO: Error if this participant published InvalidDkgShare + match handle( txn, &DataSpecification { topic: Topic::Dkg, label: DKG_CONFIRMATION_SHARES, attempt }, diff --git a/coordinator/src/tributary/mod.rs b/coordinator/src/tributary/mod.rs index 2284d77b8..25db92d61 100644 --- a/coordinator/src/tributary/mod.rs +++ b/coordinator/src/tributary/mod.rs @@ -233,6 +233,8 @@ impl ReadWrite for SignData { #[derive(Clone, PartialEq, Eq, Debug)] pub enum Transaction { + RemoveParticipant(Participant), + // Once this completes successfully, no more instances should be created. DkgCommitments(u32, Vec>, Signed), DkgShares { @@ -242,6 +244,12 @@ pub enum Transaction { confirmation_nonces: [u8; 64], signed: Signed, }, + InvalidDkgShare { + attempt: u32, + faulty: Participant, + blame: Option>, + signed: Signed, + }, DkgConfirmed(u32, [u8; 32], Signed), // When we have synchrony on a batch, we can allow signing it @@ -279,7 +287,15 @@ impl ReadWrite for Transaction { reader.read_exact(&mut kind)?; match kind[0] { - 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::new(io::ErrorKind::Other, "invalid participant in RemoveParticipant") + })? + })), + + 1 => { let mut attempt = [0; 4]; reader.read_exact(&mut attempt)?; let attempt = u32::from_le_bytes(attempt); @@ -314,7 +330,7 @@ impl ReadWrite for Transaction { Ok(Transaction::DkgCommitments(attempt, commitments, signed)) } - 1 => { + 2 => { let mut attempt = [0; 4]; reader.read_exact(&mut attempt)?; let attempt = u32::from_le_bytes(attempt); @@ -351,7 +367,33 @@ impl ReadWrite for Transaction { Ok(Transaction::DkgShares { attempt, shares, confirmation_nonces, signed }) } - 2 => { + 3 => { + let mut attempt = [0; 4]; + reader.read_exact(&mut attempt)?; + let attempt = u32::from_le_bytes(attempt); + + let mut faulty = [0; 2]; + reader.read_exact(&mut faulty)?; + let faulty = Participant::new(u16::from_le_bytes(faulty)).ok_or_else(|| { + io::Error::new(io::ErrorKind::Other, "invalid participant in InvalidDkgShare") + })?; + + let mut blame_len = [0; 2]; + reader.read_exact(&mut blame_len)?; + let mut blame = vec![0; u16::from_le_bytes(blame_len).into()]; + reader.read_exact(&mut blame)?; + + let signed = Signed::read(reader)?; + + Ok(Transaction::InvalidDkgShare { + attempt, + faulty, + blame: Some(blame).filter(|blame| !blame.is_empty()), + signed, + }) + } + + 4 => { let mut attempt = [0; 4]; reader.read_exact(&mut attempt)?; let attempt = u32::from_le_bytes(attempt); @@ -364,7 +406,7 @@ impl ReadWrite for Transaction { Ok(Transaction::DkgConfirmed(attempt, confirmation_share, signed)) } - 3 => { + 5 => { let mut block = [0; 32]; reader.read_exact(&mut block)?; let mut batch = [0; 5]; @@ -372,19 +414,19 @@ impl ReadWrite for Transaction { Ok(Transaction::Batch(block, batch)) } - 4 => { + 6 => { let mut block = [0; 8]; reader.read_exact(&mut block)?; Ok(Transaction::SubstrateBlock(u64::from_le_bytes(block))) } - 5 => SignData::read(reader).map(Transaction::BatchPreprocess), - 6 => SignData::read(reader).map(Transaction::BatchShare), + 7 => SignData::read(reader).map(Transaction::BatchPreprocess), + 8 => SignData::read(reader).map(Transaction::BatchShare), - 7 => SignData::read(reader).map(Transaction::SignPreprocess), - 8 => SignData::read(reader).map(Transaction::SignShare), + 9 => SignData::read(reader).map(Transaction::SignPreprocess), + 10 => SignData::read(reader).map(Transaction::SignShare), - 9 => { + 11 => { let mut plan = [0; 32]; reader.read_exact(&mut plan)?; @@ -405,8 +447,13 @@ impl ReadWrite for Transaction { fn write(&self, writer: &mut W) -> io::Result<()> { match self { - Transaction::DkgCommitments(attempt, commitments, signed) => { + Transaction::RemoveParticipant(i) => { writer.write_all(&[0])?; + writer.write_all(&u16::from(*i).to_le_bytes()) + } + + Transaction::DkgCommitments(attempt, commitments, signed) => { + writer.write_all(&[1])?; writer.write_all(&attempt.to_le_bytes())?; if commitments.is_empty() { Err(io::Error::new(io::ErrorKind::Other, "zero commitments in DkgCommitments"))? @@ -428,7 +475,7 @@ impl ReadWrite for Transaction { } Transaction::DkgShares { attempt, shares, confirmation_nonces, signed } => { - writer.write_all(&[1])?; + writer.write_all(&[2])?; writer.write_all(&attempt.to_le_bytes())?; // `shares` is a Vec which is supposed to map to a HashMap>. Since we @@ -456,43 +503,53 @@ impl ReadWrite for Transaction { signed.write(writer) } + Transaction::InvalidDkgShare { attempt, faulty, blame, signed } => { + writer.write_all(&[3])?; + writer.write_all(&attempt.to_le_bytes())?; + writer.write_all(&u16::from(*faulty).to_le_bytes())?; + // Flattens Some(vec![]) to None on the expectation no actual blame will be 0-length + assert!(blame.as_ref().map(|blame| blame.len()).unwrap_or(1) != 0); + writer.write_all(blame.as_ref().unwrap_or(&vec![]))?; + signed.write(writer) + } + Transaction::DkgConfirmed(attempt, share, signed) => { - writer.write_all(&[2])?; + writer.write_all(&[4])?; writer.write_all(&attempt.to_le_bytes())?; writer.write_all(share)?; signed.write(writer) } Transaction::Batch(block, batch) => { - writer.write_all(&[3])?; + writer.write_all(&[5])?; writer.write_all(block)?; writer.write_all(batch) } Transaction::SubstrateBlock(block) => { - writer.write_all(&[4])?; + writer.write_all(&[6])?; writer.write_all(&block.to_le_bytes()) } Transaction::BatchPreprocess(data) => { - writer.write_all(&[5])?; + writer.write_all(&[7])?; data.write(writer) } Transaction::BatchShare(data) => { - writer.write_all(&[6])?; + writer.write_all(&[8])?; data.write(writer) } Transaction::SignPreprocess(data) => { - writer.write_all(&[7])?; + writer.write_all(&[9])?; data.write(writer) } Transaction::SignShare(data) => { - writer.write_all(&[8])?; + writer.write_all(&[10])?; data.write(writer) } Transaction::SignCompleted { plan, tx_hash, first_signer, signature } => { - writer.write_all(&[9])?; + writer.write_all(&[11])?; writer.write_all(plan)?; writer .write_all(&[u8::try_from(tx_hash.len()).expect("tx hash length exceed 255 bytes")])?; @@ -507,8 +564,11 @@ impl ReadWrite for Transaction { impl TransactionTrait for Transaction { fn kind(&self) -> TransactionKind<'_> { match self { + Transaction::RemoveParticipant(_) => TransactionKind::Provided("remove"), + Transaction::DkgCommitments(_, _, signed) => TransactionKind::Signed(signed), Transaction::DkgShares { signed, .. } => TransactionKind::Signed(signed), + Transaction::InvalidDkgShare { signed, .. } => TransactionKind::Signed(signed), Transaction::DkgConfirmed(_, _, signed) => TransactionKind::Signed(signed), Transaction::Batch(_, _) => TransactionKind::Provided("batch"), @@ -574,8 +634,11 @@ impl Transaction { ) { fn signed(tx: &mut Transaction) -> &mut Signed { match tx { + Transaction::RemoveParticipant(_) => panic!("signing RemoveParticipant"), + Transaction::DkgCommitments(_, _, ref mut signed) => signed, Transaction::DkgShares { ref mut signed, .. } => signed, + Transaction::InvalidDkgShare { ref mut signed, .. } => signed, Transaction::DkgConfirmed(_, _, ref mut signed) => signed, Transaction::Batch(_, _) => panic!("signing Batch"), diff --git a/coordinator/src/tributary/nonce_decider.rs b/coordinator/src/tributary/nonce_decider.rs index d2f1a9181..d3d583d89 100644 --- a/coordinator/src/tributary/nonce_decider.rs +++ b/coordinator/src/tributary/nonce_decider.rs @@ -85,6 +85,8 @@ impl NonceDecider { pub fn nonce(getter: &G, genesis: [u8; 32], tx: &Transaction) -> Option> { match tx { + Transaction::RemoveParticipant(_) => None, + Transaction::DkgCommitments(attempt, _, _) => { assert_eq!(*attempt, 0); Some(Some(0)) @@ -93,6 +95,12 @@ impl NonceDecider { assert_eq!(*attempt, 0); Some(Some(1)) } + // InvalidDkgShare and DkgConfirmed share a nonce due to the expected existence of only one + // on-chain + Transaction::InvalidDkgShare { attempt, .. } => { + assert_eq!(*attempt, 0); + Some(Some(2)) + } Transaction::DkgConfirmed(attempt, _, _) => { assert_eq!(*attempt, 0); Some(Some(2)) diff --git a/docs/DKG Exclusions.md b/docs/DKG Exclusions.md new file mode 100644 index 000000000..1677da8a0 --- /dev/null +++ b/docs/DKG Exclusions.md @@ -0,0 +1,23 @@ +Upon an issue with the DKG, the honest validators must remove the malicious +validators. Ideally, a threshold signature would be used, yet that would require +a threshold key (which would require authentication by a MuSig signature). A +MuSig signature which specifies the signing set (or rather, the excluded +signers) achieves the most efficiency. + +While that resolves the on-chain behavior, the Tributary also has to perform +exclusion. This has the following forms: + +1) Rejecting further transactions (required) +2) Rejecting further participation in Tendermint + +With regards to rejecting further participation in Tendermint, it's *ideal* to +remove the validator from the list of validators. Each validator removed from +participation, yet not from the list of validators, increases the likelihood of +the network failing to form consensus. + +With regards to the economic security, an honest 67% may remove a faulty +(explicitly or simply offline) 33%, letting 67% of the remaining 67% (4/9ths) +take control of the associated private keys. In such a case, the malicious +parties are defined as the 4/9ths of validators with access to the private key +and the 33% removed (who together form >67% of the originally intended +validator set and have presumably provided enough stake to cover losses). From 99520990803c9fb2b49d4b88963259a8d1ea9ec9 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 11 Nov 2023 10:20:51 -0500 Subject: [PATCH 03/11] Route DKG blame around coordinator --- coordinator/src/main.rs | 20 +++++- coordinator/src/tests/tributary/mod.rs | 4 ++ coordinator/src/tributary/db.rs | 9 ++- coordinator/src/tributary/handle.rs | 85 +++++++++++++++++++++++--- coordinator/src/tributary/mod.rs | 11 +++- processor/messages/src/lib.rs | 8 ++- 6 files changed, 119 insertions(+), 18 deletions(-) diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index a8b71df10..e4c2ff936 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -185,6 +185,7 @@ async fn handle_processor_message( key_gen::ProcessorMessage::Shares { id, .. } => Some(id.set.session), key_gen::ProcessorMessage::InvalidShare { id, .. } => Some(id.set.session), key_gen::ProcessorMessage::GeneratedKeyPair { id, .. } => Some(id.set.session), + key_gen::ProcessorMessage::Blame { id, .. } => Some(id.set.session), }, // TODO: Review replacing key with Session in messages? ProcessorMessage::Sign(inner_msg) => match inner_msg { @@ -466,9 +467,15 @@ async fn handle_processor_message( signed: Transaction::empty_signed(), }] } - key_gen::ProcessorMessage::InvalidShare { id, faulty, blame } => { + key_gen::ProcessorMessage::InvalidShare { id, accuser, faulty, blame } => { + assert_eq!( + id.set.network, msg.network, + "processor claimed to be a different network than it was for in InvalidShare", + ); + vec![Transaction::InvalidDkgShare { attempt: id.attempt, + accuser, faulty, blame, signed: Transaction::empty_signed(), @@ -477,7 +484,7 @@ async fn handle_processor_message( key_gen::ProcessorMessage::GeneratedKeyPair { id, substrate_key, network_key } => { assert_eq!( id.set.network, msg.network, - "processor claimed to be a different network than it was for GeneratedKeyPair", + "processor claimed to be a different network than it was for in GeneratedKeyPair", ); // TODO2: Also check the other KeyGenId fields @@ -495,10 +502,17 @@ async fn handle_processor_message( vec![Transaction::DkgConfirmed(id.attempt, share, Transaction::empty_signed())] } Err(p) => { - todo!("participant {p:?} sent invalid DKG confirmation preprocesses") + vec![Transaction::RemoveParticipant(p)] } } } + key_gen::ProcessorMessage::Blame { id, participant } => { + assert_eq!( + id.set.network, msg.network, + "processor claimed to be a different network than it was for in Blame", + ); + vec![Transaction::RemoveParticipant(participant)] + } }, ProcessorMessage::Sign(msg) => match msg { sign::ProcessorMessage::Preprocess { id, preprocesses } => { diff --git a/coordinator/src/tests/tributary/mod.rs b/coordinator/src/tests/tributary/mod.rs index c64ef9b67..cec3689bf 100644 --- a/coordinator/src/tests/tributary/mod.rs +++ b/coordinator/src/tests/tributary/mod.rs @@ -141,6 +141,10 @@ fn serialize_transaction() { for i in 0 .. 2 { test_read_write(Transaction::InvalidDkgShare { attempt: random_u32(&mut OsRng), + accuser: frost::Participant::new( + u16::try_from(OsRng.next_u64() >> 48).unwrap().saturating_add(1), + ) + .unwrap(), faulty: frost::Participant::new( u16::try_from(OsRng.next_u64() >> 48).unwrap().saturating_add(1), ) diff --git a/coordinator/src/tributary/db.rs b/coordinator/src/tributary/db.rs index 0fb263754..3b1fe3ad9 100644 --- a/coordinator/src/tributary/db.rs +++ b/coordinator/src/tributary/db.rs @@ -87,11 +87,14 @@ impl TributaryDb { fn fatal_slashes_key(genesis: [u8; 32]) -> Vec { Self::tributary_key(b"fatal_slashes", genesis) } - fn fatally_slashed_key(account: [u8; 32]) -> Vec { - Self::tributary_key(b"fatally_slashed", account) + fn fatally_slashed_key(genesis: [u8; 32], account: [u8; 32]) -> Vec { + Self::tributary_key(b"fatally_slashed", (genesis, account).encode()) + } + pub fn is_fatally_slashed(getter: &G, genesis: [u8; 32], account: [u8; 32]) -> bool { + getter.get(Self::fatally_slashed_key(genesis, account)).is_some() } pub fn set_fatally_slashed(txn: &mut D::Transaction<'_>, genesis: [u8; 32], account: [u8; 32]) { - txn.put(Self::fatally_slashed_key(account), []); + txn.put(Self::fatally_slashed_key(genesis, account), []); let key = Self::fatal_slashes_key(genesis); let mut existing = txn.get(&key).unwrap_or(vec![]); diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index 8213f779a..79691ba68 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -14,7 +14,7 @@ use serai_client::{ SeraiValidatorSets, }; -use tributary::Signed; +use tributary::{Signed, TransactionKind, TransactionTrait}; use processor_messages::{ key_gen::{self, KeyGenId}, @@ -69,7 +69,7 @@ pub fn generated_key_pair( DkgConfirmer::share(spec, key, attempt, preprocesses, key_pair) } -pub(crate) fn fatal_slash( +pub(super) fn fatal_slash( txn: &mut D::Transaction<'_>, genesis: [u8; 32], account: [u8; 32], @@ -78,6 +78,33 @@ pub(crate) fn fatal_slash( log::warn!("fatally slashing {}. reason: {}", hex::encode(account), reason); TributaryDb::::set_fatally_slashed(txn, genesis, account); // TODO: disconnect the node from network/ban from further participation in all Tributaries + + // TODO: If during DKG, trigger a re-attempt +} + +// TODO: Once Substrate confirms a key, we need to rotate our validator set OR form a second +// Tributary post-DKG +// https://github.com/serai-dex/serai/issues/426 + +fn fatal_slash_with_participant_index( + spec: &TributarySpec, + txn: &mut ::Transaction<'_>, + i: Participant, + reason: &str, +) { + // Resolve from Participant to ::G + let i = u16::from(i); + let mut validator = None; + for (potential, _) in spec.validators() { + let v_i = spec.i(potential).unwrap(); + if (u16::from(v_i.start) <= i) && (i < u16::from(v_i.end)) { + validator = Some(potential); + break; + } + } + let validator = validator.unwrap(); + + fatal_slash::(txn, spec.genesis(), validator.to_bytes(), reason); } pub(crate) async fn handle_application_tx< @@ -98,6 +125,15 @@ pub(crate) async fn handle_application_tx< ) { let genesis = spec.genesis(); + // Don't handle transactions from fatally slashed participants + // TODO: Because fatally slashed participants can still publish onto the blockchain, they have + // a notable DoS ability + if let TransactionKind::Signed(signed) = tx.kind() { + if TributaryDb::::is_fatally_slashed(txn, genesis, signed.signer.to_bytes()) { + return; + } + } + let handle = |txn: &mut ::Transaction<'_>, data_spec: &DataSpecification, bytes: Vec, @@ -178,8 +214,9 @@ pub(crate) async fn handle_application_tx< } match tx { - // TODO: Either remove the validator from Tendermint OR form a second Tributary post-DKG - Transaction::RemoveParticipant(i) => todo!("TODO(now)"), + Transaction::RemoveParticipant(i) => { + fatal_slash_with_participant_index::(spec, txn, i, "RemoveParticipant Provided TX") + } Transaction::DkgCommitments(attempt, commitments, signed) => { let Ok(_) = check_sign_data_len::(txn, spec, signed.signer, commitments.len()) else { return; @@ -329,7 +366,32 @@ pub(crate) async fn handle_application_tx< } } - Transaction::InvalidDkgShare { attempt, faulty, blame, signed } => todo!("TODO(now)"), + Transaction::InvalidDkgShare { attempt, accuser, faulty, blame, signed } => { + let range = spec.i(signed.signer).unwrap(); + if (u16::from(accuser) < u16::from(range.start)) || + (u16::from(range.end) <= u16::from(accuser)) + { + fatal_slash_with_participant_index::( + spec, + txn, + accuser, + "accused with a Participant index which wasn't theirs", + ); + return; + } + + processors + .send( + spec.set().network, + key_gen::CoordinatorMessage::VerifyBlame { + id: KeyGenId { set: spec.set(), attempt }, + accuser, + accused: faulty, + blame, + }, + ) + .await; + } Transaction::DkgConfirmed(attempt, shares, signed) => { // TODO: Error if this participant published InvalidDkgShare @@ -353,11 +415,14 @@ pub(crate) async fn handle_application_tx< "(including us) fires DkgConfirmed, yet no confirming key pair" ) }); - let Ok(sig) = DkgConfirmer::complete(spec, key, attempt, preprocesses, &key_pair, shares) - else { - // TODO: Full slash - todo!(); - }; + let sig = + match DkgConfirmer::complete(spec, key, attempt, preprocesses, &key_pair, shares) { + Ok(sig) => sig, + Err(p) => { + fatal_slash_with_participant_index::(spec, txn, p, "invalid DkgConfirmer share"); + return; + } + }; publish_serai_tx( spec.set(), diff --git a/coordinator/src/tributary/mod.rs b/coordinator/src/tributary/mod.rs index 25db92d61..6bffe8f08 100644 --- a/coordinator/src/tributary/mod.rs +++ b/coordinator/src/tributary/mod.rs @@ -246,6 +246,7 @@ pub enum Transaction { }, InvalidDkgShare { attempt: u32, + accuser: Participant, faulty: Participant, blame: Option>, signed: Signed, @@ -372,6 +373,12 @@ impl ReadWrite for Transaction { reader.read_exact(&mut attempt)?; let attempt = u32::from_le_bytes(attempt); + let mut accuser = [0; 2]; + reader.read_exact(&mut accuser)?; + let accuser = Participant::new(u16::from_le_bytes(accuser)).ok_or_else(|| { + io::Error::new(io::ErrorKind::Other, "invalid participant in InvalidDkgShare") + })?; + let mut faulty = [0; 2]; reader.read_exact(&mut faulty)?; let faulty = Participant::new(u16::from_le_bytes(faulty)).ok_or_else(|| { @@ -387,6 +394,7 @@ impl ReadWrite for Transaction { Ok(Transaction::InvalidDkgShare { attempt, + accuser, faulty, blame: Some(blame).filter(|blame| !blame.is_empty()), signed, @@ -503,9 +511,10 @@ impl ReadWrite for Transaction { signed.write(writer) } - Transaction::InvalidDkgShare { attempt, faulty, blame, signed } => { + Transaction::InvalidDkgShare { attempt, accuser, faulty, blame, signed } => { writer.write_all(&[3])?; writer.write_all(&attempt.to_le_bytes())?; + writer.write_all(&u16::from(*accuser).to_le_bytes())?; writer.write_all(&u16::from(*faulty).to_le_bytes())?; // Flattens Some(vec![]) to None on the expectation no actual blame will be 0-length assert!(blame.as_ref().map(|blame| blame.len()).unwrap_or(1) != 0); diff --git a/processor/messages/src/lib.rs b/processor/messages/src/lib.rs index 2013bce1e..3303f1709 100644 --- a/processor/messages/src/lib.rs +++ b/processor/messages/src/lib.rs @@ -38,6 +38,8 @@ pub mod key_gen { Commitments { id: KeyGenId, commitments: HashMap> }, // Received shares for the specified key generation protocol. Shares { id: KeyGenId, shares: Vec>> }, + /// Instruction to verify a blame accusation. + VerifyBlame { id: KeyGenId, accuser: Participant, accused: Participant, blame: Option> }, } impl CoordinatorMessage { @@ -55,9 +57,11 @@ pub mod key_gen { // Created shares for the specified key generation protocol. Shares { id: KeyGenId, shares: Vec>> }, // Participant published an invalid share. - InvalidShare { id: KeyGenId, faulty: Participant, blame: Option> }, + InvalidShare { id: KeyGenId, accuser: Participant, faulty: Participant, blame: Option> }, // Resulting keys from the specified key generation protocol. GeneratedKeyPair { id: KeyGenId, substrate_key: [u8; 32], network_key: Vec }, + // Blame this participant. + Blame { id: KeyGenId, participant: Participant }, } } @@ -279,6 +283,7 @@ impl CoordinatorMessage { key_gen::CoordinatorMessage::GenerateKey { id, .. } => (0, id), key_gen::CoordinatorMessage::Commitments { id, .. } => (1, id), key_gen::CoordinatorMessage::Shares { id, .. } => (2, id), + key_gen::CoordinatorMessage::VerifyBlame { id, .. } => (3, id), }; let mut res = vec![COORDINATOR_UID, TYPE_KEY_GEN_UID, sub]; @@ -348,6 +353,7 @@ impl ProcessorMessage { key_gen::ProcessorMessage::Shares { id, .. } => (2, id), key_gen::ProcessorMessage::InvalidShare { id, .. } => (3, id), key_gen::ProcessorMessage::GeneratedKeyPair { id, .. } => (4, id), + key_gen::ProcessorMessage::Blame { id, .. } => (5, id), }; let mut res = vec![PROCESSSOR_UID, TYPE_KEY_GEN_UID, sub]; From 028e773162361bf28796de45f71d3081776f6374 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 11 Nov 2023 13:06:26 -0500 Subject: [PATCH 04/11] Allow public construction of AdditionalBlameMachine Necessary for upcoming work on handling DKG blame in the processor and coordinator. Additionally fixes a publicly reachable panic when commitments parsed with one ThresholdParams are used in a machine using another set of ThresholdParams. Renames InvalidProofOfKnowledge to InvalidCommitments. --- crypto/dkg/src/encryption.rs | 10 +++-- crypto/dkg/src/frost.rs | 81 ++++++++++++++++++++++++----------- crypto/dkg/src/lib.rs | 2 +- crypto/dkg/src/promote.rs | 2 +- crypto/dkg/src/tests/frost.rs | 49 ++++++++++++++------- 5 files changed, 98 insertions(+), 46 deletions(-) diff --git a/crypto/dkg/src/encryption.rs b/crypto/dkg/src/encryption.rs index caf899e87..4d68929ce 100644 --- a/crypto/dkg/src/encryption.rs +++ b/crypto/dkg/src/encryption.rs @@ -341,7 +341,7 @@ pub(crate) enum DecryptionError { #[derive(Clone)] pub(crate) struct Encryption { context: String, - i: Participant, + i: Option, enc_key: Zeroizing, enc_pub_key: C::G, enc_keys: HashMap, @@ -370,7 +370,11 @@ impl Zeroize for Encryption { } impl Encryption { - pub(crate) fn new(context: String, i: Participant, rng: &mut R) -> Self { + pub(crate) fn new( + context: String, + i: Option, + rng: &mut R, + ) -> Self { let enc_key = Zeroizing::new(C::random_nonzero_F(rng)); Self { context, @@ -404,7 +408,7 @@ impl Encryption { participant: Participant, msg: Zeroizing, ) -> EncryptedMessage { - encrypt(rng, &self.context, self.i, self.enc_keys[&participant], msg) + encrypt(rng, &self.context, self.i.unwrap(), self.enc_keys[&participant], msg) } pub(crate) fn decrypt( diff --git a/crypto/dkg/src/frost.rs b/crypto/dkg/src/frost.rs index 454501878..1faeebe56 100644 --- a/crypto/dkg/src/frost.rs +++ b/crypto/dkg/src/frost.rs @@ -133,7 +133,7 @@ impl KeyGenMachine { ); // Additionally create an encryption mechanism to protect the secret shares - let encryption = Encryption::new(self.context.clone(), self.params.i, rng); + let encryption = Encryption::new(self.context.clone(), Some(self.params.i), rng); // Step 4: Broadcast let msg = @@ -249,35 +249,38 @@ impl SecretShareMachine { fn verify_r1( &mut self, rng: &mut R, - mut commitments: HashMap>>, + mut commitment_msgs: HashMap>>, ) -> Result>, FrostError> { validate_map( - &commitments, + &commitment_msgs, &(1 ..= self.params.n()).map(Participant).collect::>(), self.params.i(), )?; - let mut batch = BatchVerifier::::new(commitments.len()); - let mut commitments = commitments - .drain() - .map(|(l, msg)| { - let mut msg = self.encryption.register(l, msg); + let mut batch = BatchVerifier::::new(commitment_msgs.len()); + let mut commitments = HashMap::new(); + for l in (1 ..= self.params.n()).map(Participant) { + let Some(msg) = commitment_msgs.remove(&l) else { continue }; + let mut msg = self.encryption.register(l, msg); + + if msg.commitments.len() != self.params.t().into() { + Err(FrostError::InvalidCommitments(l))?; + } - // Step 5: Validate each proof of knowledge - // This is solely the prep step for the latter batch verification - msg.sig.batch_verify( - rng, - &mut batch, - l, - msg.commitments[0], - challenge::(&self.context, l, msg.sig.R.to_bytes().as_ref(), &msg.cached_msg), - ); + // Step 5: Validate each proof of knowledge + // This is solely the prep step for the latter batch verification + msg.sig.batch_verify( + rng, + &mut batch, + l, + msg.commitments[0], + challenge::(&self.context, l, msg.sig.R.to_bytes().as_ref(), &msg.cached_msg), + ); - (l, msg.commitments.drain(..).collect::>()) - }) - .collect::>(); + commitments.insert(l, msg.commitments.drain(..).collect::>()); + } - batch.verify_vartime_with_vartime_blame().map_err(FrostError::InvalidProofOfKnowledge)?; + batch.verify_vartime_with_vartime_blame().map_err(FrostError::InvalidCommitments)?; commitments.insert(self.params.i, self.our_commitments.drain(..).collect()); Ok(commitments) @@ -470,12 +473,12 @@ impl KeyMachine { Ok(BlameMachine { commitments, encryption, - result: ThresholdCore { + result: Some(ThresholdCore { params, secret_share: secret, group_key: stripes[0], verification_shares, - }, + }), }) } } @@ -484,7 +487,7 @@ impl KeyMachine { pub struct BlameMachine { commitments: HashMap>, encryption: Encryption, - result: ThresholdCore, + result: Option>, } impl fmt::Debug for BlameMachine { @@ -518,7 +521,7 @@ impl BlameMachine { /// tooling to do so. This function is solely intended to force users to acknowledge they're /// completing the protocol, not processing any blame. pub fn complete(self) -> ThresholdCore { - self.result + self.result.unwrap() } fn blame_internal( @@ -585,6 +588,32 @@ impl BlameMachine { #[derive(Debug, Zeroize)] pub struct AdditionalBlameMachine(BlameMachine); impl AdditionalBlameMachine { + /// Create an AdditionalBlameMachine capable of evaluating Blame regardless of if the caller was + /// a member in the DKG protocol. + /// + /// Takes in the parameters for the DKG protocol and all of the participant's commitment + /// messages. + /// + /// This constructor assumes the full validity of the commitment messages. They must be fully + /// authenticated as having come from the supposed party and verified as valid. Usage of invalid + /// commitments is considered undefined behavior, and may cause everything from inaccurate blame + /// to panics. + pub fn new( + rng: &mut R, + context: String, + n: u16, + mut commitment_msgs: HashMap>>, + ) -> Result> { + let mut commitments = HashMap::new(); + let mut encryption = Encryption::new(context, None, rng); + for i in 1 ..= n { + let i = Participant::new(i).unwrap(); + let Some(msg) = commitment_msgs.remove(&i) else { Err(DkgError::MissingParticipant(i))? }; + commitments.insert(i, encryption.register(i, msg).commitments); + } + Ok(AdditionalBlameMachine(BlameMachine { commitments, encryption, result: None })) + } + /// Given an accusation of fault, determine the faulty party (either the sender, who sent an /// invalid secret share, or the receiver, who claimed a valid secret share was invalid). /// @@ -596,7 +625,7 @@ impl AdditionalBlameMachine { /// the caller's job to ensure they're unique in order to prevent multiple instances of blame /// over a single incident. pub fn blame( - self, + &self, sender: Participant, recipient: Participant, msg: EncryptedMessage>, diff --git a/crypto/dkg/src/lib.rs b/crypto/dkg/src/lib.rs index 303bd1207..574cff39d 100644 --- a/crypto/dkg/src/lib.rs +++ b/crypto/dkg/src/lib.rs @@ -94,7 +94,7 @@ pub enum DkgError { /// An invalid proof of knowledge was provided. #[cfg_attr(feature = "std", error("invalid proof of knowledge (participant {0})"))] - InvalidProofOfKnowledge(Participant), + InvalidCommitments(Participant), /// An invalid DKG share was provided. #[cfg_attr(feature = "std", error("invalid share (participant {participant}, blame {blame})"))] InvalidShare { participant: Participant, blame: Option }, diff --git a/crypto/dkg/src/promote.rs b/crypto/dkg/src/promote.rs index 4d9325b53..ac94beb6d 100644 --- a/crypto/dkg/src/promote.rs +++ b/crypto/dkg/src/promote.rs @@ -109,7 +109,7 @@ where &[C1::generator(), C2::generator()], &[original_shares[&i], proof.share], ) - .map_err(|_| DkgError::InvalidProofOfKnowledge(i))?; + .map_err(|_| DkgError::InvalidCommitments(i))?; verification_shares.insert(i, proof.share); } diff --git a/crypto/dkg/src/tests/frost.rs b/crypto/dkg/src/tests/frost.rs index 50a8e90c2..92f687c44 100644 --- a/crypto/dkg/src/tests/frost.rs +++ b/crypto/dkg/src/tests/frost.rs @@ -6,7 +6,7 @@ use ciphersuite::Ciphersuite; use crate::{ Participant, ThresholdParams, ThresholdCore, - frost::{KeyGenMachine, SecretShare, KeyMachine}, + frost::{Commitments, KeyGenMachine, SecretShare, KeyMachine}, encryption::{EncryptionKeyMessage, EncryptedMessage}, tests::{THRESHOLD, PARTICIPANTS, clone_without}, }; @@ -17,12 +17,13 @@ type FrostSecretShares = HashMap>; const CONTEXT: &str = "DKG Test Key Generation"; -// Commit, then return enc key and shares +// Commit, then return commitment messages, enc keys, and shares #[allow(clippy::type_complexity)] fn commit_enc_keys_and_shares( rng: &mut R, ) -> ( HashMap>, + HashMap>>, HashMap, HashMap>, ) { @@ -68,7 +69,7 @@ fn commit_enc_keys_and_shares( }) .collect::>(); - (machines, enc_keys, secret_shares) + (machines, commitments, enc_keys, secret_shares) } fn generate_secret_shares( @@ -89,7 +90,7 @@ fn generate_secret_shares( pub fn frost_gen( rng: &mut R, ) -> HashMap> { - let (mut machines, _, secret_shares) = commit_enc_keys_and_shares::<_, C>(rng); + let (mut machines, _, _, secret_shares) = commit_enc_keys_and_shares::<_, C>(rng); let mut verification_shares = None; let mut group_key = None; @@ -122,7 +123,11 @@ mod literal { use ciphersuite::Ristretto; - use crate::{DkgError, encryption::EncryptionKeyProof, frost::BlameMachine}; + use crate::{ + DkgError, + encryption::EncryptionKeyProof, + frost::{BlameMachine, AdditionalBlameMachine}, + }; use super::*; @@ -130,6 +135,7 @@ mod literal { const TWO: Participant = Participant(2); fn test_blame( + commitment_msgs: HashMap>>, machines: Vec>, msg: FrostEncryptedMessage, blame: Option>, @@ -139,13 +145,26 @@ mod literal { assert_eq!(blamed, ONE); // Verify additional blame also works assert_eq!(additional.blame(ONE, TWO, msg.clone(), blame.clone()), ONE); + + // Verify machines constructed with AdditionalBlameMachine::new work + assert_eq!( + AdditionalBlameMachine::new( + &mut OsRng, + CONTEXT.to_string(), + PARTICIPANTS, + commitment_msgs.clone() + ) + .unwrap() + .blame(ONE, TWO, msg.clone(), blame.clone()), + ONE, + ); } } // TODO: Write a macro which expands to the following #[test] fn invalid_encryption_pop_blame() { - let (mut machines, _, mut secret_shares) = + let (mut machines, commitment_msgs, _, mut secret_shares) = commit_enc_keys_and_shares::<_, Ristretto>(&mut OsRng); // Mutate the PoP of the encrypted message from 1 to 2 @@ -169,12 +188,12 @@ mod literal { }) .collect::>(); - test_blame(machines, secret_shares[&ONE][&TWO].clone(), blame.unwrap()); + test_blame(commitment_msgs, machines, secret_shares[&ONE][&TWO].clone(), blame.unwrap()); } #[test] fn invalid_ecdh_blame() { - let (mut machines, _, mut secret_shares) = + let (mut machines, commitment_msgs, _, mut secret_shares) = commit_enc_keys_and_shares::<_, Ristretto>(&mut OsRng); // Mutate the share to trigger a blame event @@ -209,13 +228,13 @@ mod literal { .collect::>(); blame.as_mut().unwrap().as_mut().unwrap().invalidate_key(); - test_blame(machines, secret_shares[&TWO][&ONE].clone(), blame.unwrap()); + test_blame(commitment_msgs, machines, secret_shares[&TWO][&ONE].clone(), blame.unwrap()); } // This should be largely equivalent to the prior test #[test] fn invalid_dleq_blame() { - let (mut machines, _, mut secret_shares) = + let (mut machines, commitment_msgs, _, mut secret_shares) = commit_enc_keys_and_shares::<_, Ristretto>(&mut OsRng); secret_shares @@ -244,12 +263,12 @@ mod literal { .collect::>(); blame.as_mut().unwrap().as_mut().unwrap().invalidate_dleq(); - test_blame(machines, secret_shares[&TWO][&ONE].clone(), blame.unwrap()); + test_blame(commitment_msgs, machines, secret_shares[&TWO][&ONE].clone(), blame.unwrap()); } #[test] fn invalid_share_serialization_blame() { - let (mut machines, enc_keys, mut secret_shares) = + let (mut machines, commitment_msgs, enc_keys, mut secret_shares) = commit_enc_keys_and_shares::<_, Ristretto>(&mut OsRng); secret_shares.get_mut(&ONE).unwrap().get_mut(&TWO).unwrap().invalidate_share_serialization( @@ -277,12 +296,12 @@ mod literal { }) .collect::>(); - test_blame(machines, secret_shares[&ONE][&TWO].clone(), blame.unwrap()); + test_blame(commitment_msgs, machines, secret_shares[&ONE][&TWO].clone(), blame.unwrap()); } #[test] fn invalid_share_value_blame() { - let (mut machines, enc_keys, mut secret_shares) = + let (mut machines, commitment_msgs, enc_keys, mut secret_shares) = commit_enc_keys_and_shares::<_, Ristretto>(&mut OsRng); secret_shares.get_mut(&ONE).unwrap().get_mut(&TWO).unwrap().invalidate_share_value( @@ -310,6 +329,6 @@ mod literal { }) .collect::>(); - test_blame(machines, secret_shares[&ONE][&TWO].clone(), blame.unwrap()); + test_blame(commitment_msgs, machines, secret_shares[&ONE][&TWO].clone(), blame.unwrap()); } } From a70e72bac131309b0d32ec04841a04c74e1bc699 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 11 Nov 2023 13:14:38 -0500 Subject: [PATCH 05/11] Remove unused error from dleq --- crypto/dleq/src/cross_group/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crypto/dleq/src/cross_group/mod.rs b/crypto/dleq/src/cross_group/mod.rs index 08e92959f..8087a273d 100644 --- a/crypto/dleq/src/cross_group/mod.rs +++ b/crypto/dleq/src/cross_group/mod.rs @@ -95,9 +95,6 @@ impl Generators { /// Error for cross-group DLEq proofs. #[derive(Error, PartialEq, Eq, Debug)] pub enum DLEqError { - /// Invalid proof of knowledge. - #[error("invalid proof of knowledge")] - InvalidProofOfKnowledge, /// Invalid proof length. #[error("invalid proof length")] InvalidProofLength, From 85d9593faad4711c0346f5bbf54c33caa1d970d8 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 11 Nov 2023 14:53:45 -0500 Subject: [PATCH 06/11] Implement support for VerifyBlame in the processor --- processor/messages/src/lib.rs | 58 +++++-- processor/src/key_gen.rs | 303 ++++++++++++++++++++++------------ 2 files changed, 241 insertions(+), 120 deletions(-) diff --git a/processor/messages/src/lib.rs b/processor/messages/src/lib.rs index 3303f1709..a1f221134 100644 --- a/processor/messages/src/lib.rs +++ b/processor/messages/src/lib.rs @@ -33,13 +33,29 @@ pub mod key_gen { pub enum CoordinatorMessage { // Instructs the Processor to begin the key generation process. // TODO: Should this be moved under Substrate? - GenerateKey { id: KeyGenId, params: ThresholdParams, shares: u16 }, + GenerateKey { + id: KeyGenId, + params: ThresholdParams, + shares: u16, + }, // Received commitments for the specified key generation protocol. - Commitments { id: KeyGenId, commitments: HashMap> }, + Commitments { + id: KeyGenId, + commitments: HashMap>, + }, // Received shares for the specified key generation protocol. - Shares { id: KeyGenId, shares: Vec>> }, + Shares { + id: KeyGenId, + shares: Vec>>, + }, /// Instruction to verify a blame accusation. - VerifyBlame { id: KeyGenId, accuser: Participant, accused: Participant, blame: Option> }, + VerifyBlame { + id: KeyGenId, + accuser: Participant, + accused: Participant, + share: Vec, + blame: Option>, + }, } impl CoordinatorMessage { @@ -51,17 +67,39 @@ pub mod key_gen { #[derive(Clone, PartialEq, Eq, Debug, Serialize, Deserialize)] pub enum ProcessorMessage { // Created commitments for the specified key generation protocol. - Commitments { id: KeyGenId, commitments: Vec> }, + Commitments { + id: KeyGenId, + commitments: Vec>, + }, // Participant published invalid commitments. - InvalidCommitments { id: KeyGenId, faulty: Participant }, + InvalidCommitments { + id: KeyGenId, + faulty: Participant, + }, // Created shares for the specified key generation protocol. - Shares { id: KeyGenId, shares: Vec>> }, + Shares { + id: KeyGenId, + shares: Vec>>, + }, // Participant published an invalid share. - InvalidShare { id: KeyGenId, accuser: Participant, faulty: Participant, blame: Option> }, + #[rustfmt::skip] + InvalidShare { + id: KeyGenId, + accuser: Participant, + faulty: Participant, + blame: Option>, + }, // Resulting keys from the specified key generation protocol. - GeneratedKeyPair { id: KeyGenId, substrate_key: [u8; 32], network_key: Vec }, + GeneratedKeyPair { + id: KeyGenId, + substrate_key: [u8; 32], + network_key: Vec, + }, // Blame this participant. - Blame { id: KeyGenId, participant: Participant }, + Blame { + id: KeyGenId, + participant: Participant, + }, } } diff --git a/processor/src/key_gen.rs b/processor/src/key_gen.rs index e0737660d..940edd373 100644 --- a/processor/src/key_gen.rs +++ b/processor/src/key_gen.rs @@ -30,7 +30,7 @@ pub struct KeyConfirmed { create_db!( KeyGenDb { - ParamsDb: (key: &ValidatorSet) -> (ThresholdParams, u16), + ParamsDb: (set: &ValidatorSet) -> (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 @@ -157,18 +157,20 @@ impl KeyGen { txn: &mut D::Transaction<'_>, msg: CoordinatorMessage, ) -> ProcessorMessage { - let context = |id: &KeyGenId| { + const SUBSTRATE_KEY_CONTEXT: &str = "substrate"; + const NETWORK_KEY_CONTEXT: &str = "network"; + let context = |id: &KeyGenId, key| { // TODO2: Also embed the chain ID/genesis block format!( - "Serai Key Gen. Session: {:?}, Network: {:?}, Attempt: {}", - id.set.session, id.set.network, id.attempt + "Serai Key Gen. Session: {:?}, Network: {:?}, Attempt: {}, Key: {}", + id.set.session, id.set.network, id.attempt, key, ) }; let rng = |label, id: KeyGenId| { let mut transcript = RecommendedTranscript::new(label); transcript.append_message(b"entropy", &self.entropy); - transcript.append_message(b"context", context(&id)); + transcript.append_message(b"context", context(&id, "rng")); ChaCha20Rng::from_seed(transcript.rng_seed(b"rng")) }; let coefficients_rng = |id| rng(b"Key Gen Coefficients", id); @@ -186,8 +188,10 @@ impl KeyGen { Participant::new(u16::from(params.i()) + s).unwrap(), ) .unwrap(); - let substrate = KeyGenMachine::new(params, context(&id)).generate_coefficients(&mut rng); - let network = KeyGenMachine::new(params, context(&id)).generate_coefficients(&mut rng); + let substrate = KeyGenMachine::new(params, context(&id, SUBSTRATE_KEY_CONTEXT)) + .generate_coefficients(&mut rng); + let network = KeyGenMachine::new(params, context(&id, NETWORK_KEY_CONTEXT)) + .generate_coefficients(&mut rng); machines.push((substrate.0, network.0)); let mut serialized = vec![]; substrate.1.write(&mut serialized).unwrap(); @@ -197,103 +201,91 @@ impl KeyGen { (machines, commitments) }; - let secret_share_machines = - |id, - params: ThresholdParams, - (machines, our_commitments): (SecretShareMachines, Vec>), - commitments: HashMap>| - -> Result<_, ProcessorMessage> { - let mut rng = secret_shares_rng(id); - - #[allow(clippy::type_complexity)] - fn handle_machine( - rng: &mut ChaCha20Rng, - id: KeyGenId, - params: ThresholdParams, - machine: SecretShareMachine, - commitments_ref: &mut HashMap, - ) -> Result< - (KeyMachine, HashMap>>), - ProcessorMessage, - > { - // Parse the commitments - let mut parsed = HashMap::new(); - for i in 1 ..= params.n() { - let i = Participant::new(i).unwrap(); - let Some(commitments) = commitments_ref.get_mut(&i) else { continue }; - parsed.insert( - i, - EncryptionKeyMessage::>::read(commitments, params) - .map_err(|_| ProcessorMessage::InvalidCommitments { id, faulty: i })?, - ); - } + let secret_share_machines = |id, + params: ThresholdParams, + machines: SecretShareMachines, + commitments: HashMap>| + -> Result<_, ProcessorMessage> { + let mut rng = secret_shares_rng(id); + + #[allow(clippy::type_complexity)] + fn handle_machine( + rng: &mut ChaCha20Rng, + id: KeyGenId, + machine: SecretShareMachine, + commitments: HashMap>>, + ) -> Result< + (KeyMachine, HashMap>>), + ProcessorMessage, + > { + match machine.generate_secret_shares(rng, commitments) { + Ok(res) => Ok(res), + Err(e) => match e { + DkgError::ZeroParameter(_, _) | + DkgError::InvalidThreshold(_, _) | + DkgError::InvalidParticipant(_, _) | + DkgError::InvalidSigningSet | + DkgError::InvalidShare { .. } => unreachable!("{e:?}"), + DkgError::InvalidParticipantQuantity(_, _) | + DkgError::DuplicatedParticipant(_) | + DkgError::MissingParticipant(_) => { + panic!("coordinator sent invalid DKG commitments: {e:?}") + } + DkgError::InvalidCommitments(i) => { + Err(ProcessorMessage::InvalidCommitments { id, faulty: i })? + } + }, + } + } - match machine.generate_secret_shares(rng, parsed) { - Ok(res) => Ok(res), - Err(e) => match e { - DkgError::ZeroParameter(_, _) | - DkgError::InvalidThreshold(_, _) | - DkgError::InvalidParticipant(_, _) | - DkgError::InvalidSigningSet | - DkgError::InvalidShare { .. } => unreachable!("{e:?}"), - DkgError::InvalidParticipantQuantity(_, _) | - DkgError::DuplicatedParticipant(_) | - DkgError::MissingParticipant(_) => { - panic!("coordinator sent invalid DKG commitments: {e:?}") - } - DkgError::InvalidProofOfKnowledge(i) => { - Err(ProcessorMessage::InvalidCommitments { id, faulty: i })? - } - }, - } + let mut substrate_commitments = HashMap::new(); + let mut network_commitments = HashMap::new(); + for i in 1 ..= params.n() { + let i = Participant::new(i).unwrap(); + let mut commitments = commitments[&i].as_slice(); + substrate_commitments.insert( + i, + EncryptionKeyMessage::>::read(&mut commitments, params) + .map_err(|_| ProcessorMessage::InvalidCommitments { id, faulty: i })?, + ); + network_commitments.insert( + i, + EncryptionKeyMessage::>::read(&mut commitments, params) + .map_err(|_| ProcessorMessage::InvalidCommitments { id, faulty: i })?, + ); + if !commitments.is_empty() { + // Malicious Participant included extra bytes in their commitments + // (a potential DoS attack) + Err(ProcessorMessage::InvalidCommitments { id, faulty: i })?; } + } - let mut key_machines = vec![]; - let mut shares = vec![]; - for (m, (substrate_machine, network_machine)) in machines.into_iter().enumerate() { - let mut commitments_ref: HashMap = - commitments.iter().map(|(i, commitments)| (*i, commitments.as_ref())).collect(); - for (i, our_commitments) in our_commitments.iter().enumerate() { - if m != i { - assert!(commitments_ref - .insert( - Participant::new(u16::from(params.i()) + u16::try_from(i).unwrap()).unwrap(), - our_commitments.as_ref(), - ) - .is_none()); - } - } + let mut key_machines = vec![]; + let mut shares = vec![]; + for (m, (substrate_machine, network_machine)) in machines.into_iter().enumerate() { + let actual_i = Participant::new(u16::from(params.i()) + u16::try_from(m).unwrap()).unwrap(); - let (substrate_machine, mut substrate_shares) = handle_machine::( - &mut rng, - id, - params, - substrate_machine, - &mut commitments_ref, - )?; - let (network_machine, network_shares) = - handle_machine(&mut rng, id, params, network_machine, &mut commitments_ref)?; - key_machines.push((substrate_machine, network_machine)); + let mut substrate_commitments = substrate_commitments.clone(); + substrate_commitments.remove(&actual_i); + let (substrate_machine, mut substrate_shares) = + handle_machine::(&mut rng, id, substrate_machine, substrate_commitments)?; - for i in 1 ..= params.n() { - let i = Participant::new(i).unwrap(); - let commitments = &commitments_ref[&i]; - if !commitments.is_empty() { - // Malicious Participant included extra bytes in their commitments - // (a potential DoS attack) - Err(ProcessorMessage::InvalidCommitments { id, faulty: i })?; - } - } + let mut network_commitments = network_commitments.clone(); + network_commitments.remove(&actual_i); + let (network_machine, network_shares) = + handle_machine(&mut rng, id, network_machine, network_commitments.clone())?; - let mut these_shares: HashMap<_, _> = - substrate_shares.drain().map(|(i, share)| (i, share.serialize())).collect(); - for (i, share) in these_shares.iter_mut() { - share.extend(network_shares[i].serialize()); - } - shares.push(these_shares); + key_machines.push((substrate_machine, network_machine)); + + let mut these_shares: HashMap<_, _> = + substrate_shares.drain().map(|(i, share)| (i, share.serialize())).collect(); + for (i, share) in these_shares.iter_mut() { + share.extend(network_shares[i].serialize()); } - Ok((key_machines, shares)) - }; + shares.push(these_shares); + } + Ok((key_machines, shares)) + }; match msg { CoordinatorMessage::GenerateKey { id, params, shares } => { @@ -313,7 +305,7 @@ impl KeyGen { ProcessorMessage::Commitments { id, commitments } } - CoordinatorMessage::Commitments { id, commitments } => { + CoordinatorMessage::Commitments { id, mut commitments } => { info!("Received commitments for {:?}", id); if self.active_share.contains_key(&id.set) { @@ -330,12 +322,22 @@ impl KeyGen { // This *may* be inconsistent if we receive a KeyGen for attempt x, then commitments for // attempt y // The coordinator is trusted to be proper in this regard - let prior = self + let (prior, our_commitments) = self .active_commit .remove(&id.set) .unwrap_or_else(|| key_gen_machines(id, params, share_quantity)); + for (i, our_commitments) in our_commitments.into_iter().enumerate() { + assert!(commitments + .insert( + Participant::new(u16::from(params.i()) + u16::try_from(i).unwrap()).unwrap(), + our_commitments, + ) + .is_none()); + } + CommitmentsDb::set(txn, &id, &commitments); + match secret_share_machines(id, params, prior, commitments) { Ok((machines, shares)) => { self.active_share.insert(id.set, (machines, shares.clone())); @@ -352,9 +354,11 @@ impl KeyGen { // Same commentary on inconsistency as above exists let (machines, our_shares) = self.active_share.remove(&id.set).unwrap_or_else(|| { - let prior = key_gen_machines(id, params, share_quantity); - secret_share_machines(id, params, prior, CommitmentsDb::get(txn, &id).unwrap()) - .expect("got Shares for a key gen which faulted") + let prior = key_gen_machines(id, params, share_quantity).0; + let (machines, shares) = + secret_share_machines(id, params, prior, CommitmentsDb::get(txn, &id).unwrap()) + .expect("got Shares for a key gen which faulted"); + (machines, shares) }); let mut rng = share_rng(id); @@ -362,10 +366,19 @@ impl KeyGen { fn handle_machine( rng: &mut ChaCha20Rng, id: KeyGenId, + // These are the params of our first share, not this machine's shares params: ThresholdParams, + m: usize, machine: KeyMachine, shares_ref: &mut HashMap, ) -> Result, ProcessorMessage> { + let params = ThresholdParams::new( + params.t(), + params.n(), + Participant::new(u16::from(params.i()) + u16::try_from(m).unwrap()).unwrap(), + ) + .unwrap(); + // Parse the shares let mut shares = HashMap::new(); for i in 1 ..= params.n() { @@ -373,12 +386,12 @@ impl KeyGen { let Some(share) = shares_ref.get_mut(&i) else { continue }; shares.insert( i, - EncryptedMessage::>::read(share, params) - .map_err(|_| ProcessorMessage::InvalidShare { id, faulty: i, blame: None })?, + EncryptedMessage::>::read(share, params).map_err(|_| { + ProcessorMessage::InvalidShare { id, accuser: params.i(), faulty: i, blame: None } + })?, ); } - // TODO(now): Handle the blame machine properly Ok( (match machine.calculate_share(rng, shares) { Ok(res) => res, @@ -387,7 +400,7 @@ impl KeyGen { DkgError::InvalidThreshold(_, _) | DkgError::InvalidParticipant(_, _) | DkgError::InvalidSigningSet | - DkgError::InvalidProofOfKnowledge(_) => unreachable!("{e:?}"), + DkgError::InvalidCommitments(_) => unreachable!("{e:?}"), DkgError::InvalidParticipantQuantity(_, _) | DkgError::DuplicatedParticipant(_) | DkgError::MissingParticipant(_) => { @@ -396,6 +409,7 @@ impl KeyGen { DkgError::InvalidShare { participant, blame } => { Err(ProcessorMessage::InvalidShare { id, + accuser: params.i(), faulty: participant, blame: Some(blame.map(|blame| blame.serialize())).flatten(), })? @@ -425,21 +439,26 @@ impl KeyGen { } let these_substrate_keys = - match handle_machine(&mut rng, id, params, machines.0, &mut shares_ref) { + match handle_machine(&mut rng, id, params, m, machines.0, &mut shares_ref) { Ok(keys) => keys, Err(msg) => return msg, }; let these_network_keys = - match handle_machine(&mut rng, id, params, machines.1, &mut shares_ref) { + match handle_machine(&mut rng, id, params, m, machines.1, &mut shares_ref) { Ok(keys) => keys, Err(msg) => return msg, }; for i in 1 ..= params.n() { let i = Participant::new(i).unwrap(); - let shares = &shares_ref[&i]; + let Some(shares) = shares_ref.get(&i) else { continue }; if !shares.is_empty() { - return ProcessorMessage::InvalidShare { id, faulty: i, blame: None }; + return ProcessorMessage::InvalidShare { + id, + accuser: these_substrate_keys.params().i(), + faulty: i, + blame: None, + }; } } @@ -470,6 +489,70 @@ impl KeyGen { network_key: generated_network_key.unwrap().to_bytes().as_ref().to_vec(), } } + + CoordinatorMessage::VerifyBlame { id, accuser, accused, share, blame } => { + let params = ParamsDb::get(txn, &id.set).unwrap().0; + + let mut share_ref = share.as_slice(); + let Ok(substrate_share) = EncryptedMessage::< + Ristretto, + SecretShare<::F>, + >::read(&mut share_ref, params) else { + return ProcessorMessage::Blame { id, participant: accused }; + }; + let Ok(network_share) = EncryptedMessage::< + N::Curve, + SecretShare<::F>, + >::read(&mut share_ref, params) else { + return ProcessorMessage::Blame { id, participant: accused }; + }; + if !share_ref.is_empty() { + return ProcessorMessage::Blame { id, participant: accused }; + } + + let mut substrate_commitment_msgs = HashMap::new(); + let mut network_commitment_msgs = HashMap::new(); + let commitments = CommitmentsDb::get(txn, &id).unwrap(); + for (i, commitments) in commitments { + let mut commitments = commitments.as_slice(); + substrate_commitment_msgs + .insert(i, EncryptionKeyMessage::<_, _>::read(&mut commitments, params).unwrap()); + network_commitment_msgs + .insert(i, EncryptionKeyMessage::<_, _>::read(&mut commitments, params).unwrap()); + } + + // There is a mild DoS here where someone with a valid blame bloats it to the maximum size + // Given the ambiguity, and limited potential to DoS (this being called means *someone* is + // getting fatally slashed) voids the need to ensure blame is minimal + let substrate_blame = + blame.clone().and_then(|blame| EncryptionKeyProof::read(&mut blame.as_slice()).ok()); + let network_blame = + blame.clone().and_then(|blame| EncryptionKeyProof::read(&mut blame.as_slice()).ok()); + + let substrate_blame = AdditionalBlameMachine::new( + &mut rand_core::OsRng, + context(&id, SUBSTRATE_KEY_CONTEXT), + params.n(), + substrate_commitment_msgs, + ) + .unwrap() + .blame(accuser, accused, substrate_share, substrate_blame); + let network_blame = AdditionalBlameMachine::new( + &mut rand_core::OsRng, + context(&id, NETWORK_KEY_CONTEXT), + params.n(), + network_commitment_msgs, + ) + .unwrap() + .blame(accuser, accused, network_share, network_blame); + + // If thw accused was blamed for either, mark them as at fault + if (substrate_blame == accused) || (network_blame == accused) { + return ProcessorMessage::Blame { id, participant: accused }; + } + + ProcessorMessage::Blame { id, participant: accuser } + } } } From 4fa24495ea5760cfca6f697eb87e2f1c43ed641b Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sun, 12 Nov 2023 01:20:13 -0500 Subject: [PATCH 07/11] Have coordinator send the processor share message relevant to Blame --- coordinator/src/main.rs | 4 +++ coordinator/src/tributary/db.rs | 21 +++++++++++++ coordinator/src/tributary/handle.rs | 46 +++++++++++++++++++++++++---- 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index e4c2ff936..5a736752a 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -439,6 +439,9 @@ async fn handle_processor_message( .i(pub_key) .expect("processor message to DKG for a session we aren't a validator in"); + // TODO: This is [receiver_share][sender_share] and is later transposed to + // [sender_share][receiver_share]. Make this [sender_share][receiver_share] from the + // start? // `tx_shares` needs to be done here as while it can be serialized from the HashMap // without further context, it can't be deserialized without context let mut tx_shares = Vec::with_capacity(shares.len()); @@ -497,6 +500,7 @@ async fn handle_processor_message( id.attempt, ); + // TODO: If a processor fails to generate a key pair, it'll desync here match share { Ok(share) => { vec![Transaction::DkgConfirmed(id.attempt, share, Transaction::empty_signed())] diff --git a/coordinator/src/tributary/db.rs b/coordinator/src/tributary/db.rs index 3b1fe3ad9..65ebbf78d 100644 --- a/coordinator/src/tributary/db.rs +++ b/coordinator/src/tributary/db.rs @@ -108,6 +108,27 @@ impl TributaryDb { txn.put(key, existing); } + fn share_for_blame_key(genesis: &[u8], from: Participant, to: Participant) -> Vec { + Self::tributary_key(b"share_for_blame", (genesis, u16::from(from), u16::from(to)).encode()) + } + pub fn save_share_for_blame( + txn: &mut D::Transaction<'_>, + genesis: &[u8], + from: Participant, + to: Participant, + share: &[u8], + ) { + txn.put(Self::share_for_blame_key(genesis, from, to), share); + } + pub fn share_for_blame( + getter: &G, + genesis: &[u8], + from: Participant, + to: Participant, + ) -> Option> { + getter.get(Self::share_for_blame_key(genesis, from, to)) + } + // The plan IDs associated with a Substrate block fn plan_ids_key(genesis: &[u8], block: u64) -> Vec { Self::tributary_key(b"plan_ids", [genesis, block.to_le_bytes().as_ref()].concat()) diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index 79691ba68..6dfd20e32 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -269,7 +269,28 @@ pub(crate) async fn handle_application_tx< } } - // Only save our share's bytes + // Save each share as needed for blame + { + let from = spec.i(signed.signer).unwrap(); + for (to, shares) in shares.iter().enumerate() { + // 0-indexed (the enumeration) to 1-indexed (Participant) + let mut to = u16::try_from(to).unwrap() + 1; + // Adjust for the omission of the sender's own shares + if to >= u16::from(from.start) { + to += u16::from(from.end) - u16::from(from.start); + } + let to = Participant::new(to).unwrap(); + + for (sender_share, share) in shares.iter().enumerate() { + let from = + Participant::new(u16::from(from.start) + u16::try_from(sender_share).unwrap()) + .unwrap(); + TributaryDb::::save_share_for_blame(txn, &genesis, from, to, share); + } + } + } + + // Filter down to only our share's bytes for handle let our_i = spec .i(Ristretto::generator() * key.deref()) .expect("in a tributary we're not a validator for"); @@ -366,20 +387,34 @@ pub(crate) async fn handle_application_tx< } } + // TODO: Only accept one of either InvalidDkgShare/DkgConfirmed per signer + // TODO: Ban self-accusals Transaction::InvalidDkgShare { attempt, accuser, faulty, blame, signed } => { let range = spec.i(signed.signer).unwrap(); if (u16::from(accuser) < u16::from(range.start)) || (u16::from(range.end) <= u16::from(accuser)) { - fatal_slash_with_participant_index::( - spec, + fatal_slash::( txn, - accuser, + genesis, + signed.signer.to_bytes(), "accused with a Participant index which wasn't theirs", ); return; } + if !((u16::from(range.start) <= u16::from(faulty)) && + (u16::from(faulty) < u16::from(range.end))) + { + fatal_slash::( + txn, + genesis, + signed.signer.to_bytes(), + "accused self of having an InvalidDkgShare", + ); + return; + } + processors .send( spec.set().network, @@ -387,6 +422,7 @@ pub(crate) async fn handle_application_tx< id: KeyGenId { set: spec.set(), attempt }, accuser, accused: faulty, + share: TributaryDb::::share_for_blame(txn, &genesis, accuser, faulty).unwrap(), blame, }, ) @@ -394,8 +430,6 @@ pub(crate) async fn handle_application_tx< } Transaction::DkgConfirmed(attempt, shares, signed) => { - // TODO: Error if this participant published InvalidDkgShare - match handle( txn, &DataSpecification { topic: Topic::Dkg, label: DKG_CONFIRMATION_SHARES, attempt }, From 3e962b9719137139aa284a4ef8bc49e5d74c9063 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sun, 12 Nov 2023 01:48:39 -0500 Subject: [PATCH 08/11] Remove desync between processors reporting InvalidShare and ones reporting GeneratedKeyPair --- coordinator/src/main.rs | 19 +++++++++++++--- coordinator/src/tributary/handle.rs | 34 +++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index 5a736752a..62d30d6d1 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -476,13 +476,27 @@ async fn handle_processor_message( "processor claimed to be a different network than it was for in InvalidShare", ); - vec![Transaction::InvalidDkgShare { + // Check if the MuSig signature had any errors as if so, we need to provide + // RemoveParticipant + // 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::(&txn, key, spec, id.attempt) + { + vec![Transaction::RemoveParticipant(faulty)] + } else { + vec![] + }; + + txs.push(Transaction::InvalidDkgShare { attempt: id.attempt, accuser, faulty, blame, signed: Transaction::empty_signed(), - }] + }); + + txs } key_gen::ProcessorMessage::GeneratedKeyPair { id, substrate_key, network_key } => { assert_eq!( @@ -500,7 +514,6 @@ async fn handle_processor_message( id.attempt, ); - // TODO: If a processor fails to generate a key pair, it'll desync here match share { Ok(share) => { vec![Transaction::DkgConfirmed(id.attempt, share, Transaction::empty_signed())] diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index 6dfd20e32..aaecfd25d 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -1,14 +1,14 @@ use core::{ops::Deref, future::Future}; use std::collections::HashMap; -use zeroize::Zeroizing; +use zeroize::{Zeroize, Zeroizing}; use ciphersuite::{group::GroupEncoding, Ciphersuite, Ristretto}; use frost::dkg::Participant; use scale::{Encode, Decode}; use serai_client::{ - Signature, + Public, Signature, validator_sets::primitives::{ValidatorSet, KeyPair}, subxt::utils::Encoded, SeraiValidatorSets, @@ -22,7 +22,7 @@ use processor_messages::{ sign::{self, SignId}, }; -use serai_db::Db; +use serai_db::{Get, Db}; use crate::{ processors::Processors, @@ -56,7 +56,33 @@ pub fn dkg_confirmation_nonces( DkgConfirmer::preprocess(spec, key, attempt) } -#[allow(clippy::needless_pass_by_ref_mut)] +// If there's an error generating a key pair, return any errors which would've occured when +// executing the DkgConfirmer in order to stay in sync with those who did. +// +// The caller must ensure only error_generating_key_pair or generated_key_pair is called for a +// given attempt. +pub fn error_generating_key_pair( + getter: &G, + key: &Zeroizing<::F>, + spec: &TributarySpec, + attempt: u32, +) -> Option { + let preprocesses = + TributaryDb::::confirmation_nonces(getter, spec.genesis(), attempt).unwrap(); + + // 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 = (Public([0xff; 32]), vec![0xffu8; 0].try_into().unwrap()); + match DkgConfirmer::share(spec, key, attempt, preprocesses, &key_pair) { + Ok(mut share) => { + // Zeroize the share to ensure it's not accessed + share.zeroize(); + None + } + Err(p) => Some(p), + } +} + pub fn generated_key_pair( txn: &mut D::Transaction<'_>, key: &Zeroizing<::F>, From c247cbee1b3e533e272c9e3e87abbe1f22d9fe0c Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sun, 12 Nov 2023 02:37:19 -0500 Subject: [PATCH 09/11] Route blame on sign between processor and coordinator Doesn't yet act on it in coordinator. --- coordinator/src/main.rs | 18 +++++- processor/messages/src/lib.rs | 19 +++--- processor/src/signer.rs | 96 +++++++++++++++++++------------ processor/src/substrate_signer.rs | 96 +++++++++++++++++++------------ 4 files changed, 147 insertions(+), 82 deletions(-) diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index 62d30d6d1..8298040b3 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -189,7 +189,10 @@ async fn handle_processor_message( }, // TODO: Review replacing key with Session in messages? ProcessorMessage::Sign(inner_msg) => match inner_msg { - // We'll only receive Preprocess and Share if we're actively signing + // We'll only receive InvalidParticipant/Preprocess/Share if we're actively signing + sign::ProcessorMessage::InvalidParticipant { id, .. } => { + Some(SubstrateDb::::session_for_key(&txn, &id.key).unwrap()) + } sign::ProcessorMessage::Preprocess { id, .. } => { Some(SubstrateDb::::session_for_key(&txn, &id.key).unwrap()) } @@ -264,6 +267,9 @@ async fn handle_processor_message( None } // We'll only fire these if we are the Substrate signer, making the Tributary relevant + coordinator::ProcessorMessage::InvalidParticipant { id, .. } => { + Some(SubstrateDb::::session_for_key(&txn, &id.key).unwrap()) + } coordinator::ProcessorMessage::BatchPreprocess { id, .. } => { Some(SubstrateDb::::session_for_key(&txn, &id.key).unwrap()) } @@ -532,6 +538,11 @@ async fn handle_processor_message( } }, ProcessorMessage::Sign(msg) => match msg { + sign::ProcessorMessage::InvalidParticipant { .. } => { + // TODO: Locally increase slash points to maximum (distinct from an explicitly fatal + // slash) and censor transactions (yet don't explicitly ban) + vec![] + } sign::ProcessorMessage::Preprocess { id, preprocesses } => { if id.attempt == 0 { MainDb::::save_first_preprocess( @@ -582,6 +593,11 @@ async fn handle_processor_message( }, ProcessorMessage::Coordinator(inner_msg) => match inner_msg { coordinator::ProcessorMessage::SubstrateBlockAck { .. } => unreachable!(), + coordinator::ProcessorMessage::InvalidParticipant { .. } => { + // TODO: Locally increase slash points to maximum (distinct from an explicitly fatal + // slash) and censor transactions (yet don't explicitly ban) + vec![] + } coordinator::ProcessorMessage::BatchPreprocess { id, block, preprocesses } => { log::info!( "informed of batch (sign ID {}, attempt {}) for block {}", diff --git a/processor/messages/src/lib.rs b/processor/messages/src/lib.rs index a1f221134..d08afffb0 100644 --- a/processor/messages/src/lib.rs +++ b/processor/messages/src/lib.rs @@ -140,8 +140,10 @@ pub mod sign { } } - #[derive(Clone, PartialEq, Eq, Debug, Zeroize, Encode, Decode, Serialize, Deserialize)] + #[derive(Clone, PartialEq, Eq, Debug, Zeroize, Serialize, Deserialize)] pub enum ProcessorMessage { + // Participant sent an invalid message during the sign protocol. + InvalidParticipant { id: SignId, participant: Participant }, // Created preprocess for the specified signing protocol. Preprocess { id: SignId, preprocesses: Vec> }, // Signed share for the specified signing protocol. @@ -198,9 +200,10 @@ pub mod coordinator { pub id: [u8; 32], } - #[derive(Clone, PartialEq, Eq, Debug, Zeroize, Encode, Decode, Serialize, Deserialize)] + #[derive(Clone, PartialEq, Eq, Debug, Zeroize, Serialize, Deserialize)] pub enum ProcessorMessage { SubstrateBlockAck { network: NetworkId, block: u64, plans: Vec }, + InvalidParticipant { id: BatchSignId, participant: Participant }, BatchPreprocess { id: BatchSignId, block: BlockHash, preprocesses: Vec> }, BatchShare { id: BatchSignId, shares: Vec<[u8; 32]> }, } @@ -401,10 +404,11 @@ impl ProcessorMessage { ProcessorMessage::Sign(msg) => { let (sub, id) = match msg { // Unique since SignId - sign::ProcessorMessage::Preprocess { id, .. } => (0, id.encode()), - sign::ProcessorMessage::Share { id, .. } => (1, id.encode()), + sign::ProcessorMessage::InvalidParticipant { id, .. } => (0, id.encode()), + sign::ProcessorMessage::Preprocess { id, .. } => (1, id.encode()), + sign::ProcessorMessage::Share { id, .. } => (2, id.encode()), // Unique since a processor will only sign a TX once - sign::ProcessorMessage::Completed { id, .. } => (2, id.to_vec()), + sign::ProcessorMessage::Completed { id, .. } => (3, id.to_vec()), }; let mut res = vec![PROCESSSOR_UID, TYPE_SIGN_UID, sub]; @@ -417,8 +421,9 @@ impl ProcessorMessage { (0, (network, block).encode()) } // Unique since BatchSignId - coordinator::ProcessorMessage::BatchPreprocess { id, .. } => (1, id.encode()), - coordinator::ProcessorMessage::BatchShare { id, .. } => (2, id.encode()), + coordinator::ProcessorMessage::InvalidParticipant { id, .. } => (1, id.encode()), + coordinator::ProcessorMessage::BatchPreprocess { id, .. } => (2, id.encode()), + coordinator::ProcessorMessage::BatchShare { id, .. } => (3, id.encode()), }; let mut res = vec![PROCESSSOR_UID, TYPE_COORDINATOR_UID, sub]; diff --git a/processor/src/signer.rs b/processor/src/signer.rs index b86c3118b..3f07c0708 100644 --- a/processor/src/signer.rs +++ b/processor/src/signer.rs @@ -5,7 +5,7 @@ use rand_core::OsRng; use ciphersuite::group::GroupEncoding; use frost::{ - ThresholdKeys, + ThresholdKeys, FrostError, sign::{Writable, PreprocessMachine, SignMachine, SignatureMachine}, }; @@ -470,7 +470,7 @@ impl Signer { msg: CoordinatorMessage, ) -> Option { match msg { - CoordinatorMessage::Preprocesses { id, mut preprocesses } => { + CoordinatorMessage::Preprocesses { id, preprocesses } => { if self.verify_id(&id).is_err() { return None; } @@ -487,23 +487,22 @@ impl Signer { Some(machine) => machine, }; - let preprocesses = match preprocesses - .drain() - .map(|(l, preprocess)| { - let mut preprocess_ref = preprocess.as_ref(); - let res = machines[0] - .read_preprocess::<&[u8]>(&mut preprocess_ref) - .map(|preprocess| (l, preprocess)); - if !preprocess_ref.is_empty() { - todo!("malicious signer: extra bytes"); - } - res - }) - .collect::, _>>() - { - Ok(preprocesses) => preprocesses, - Err(e) => todo!("malicious signer: {:?}", e), - }; + let mut parsed = HashMap::new(); + for l in { + let mut keys = preprocesses.keys().cloned().collect::>(); + keys.sort(); + keys + } { + let mut preprocess_ref = preprocesses.get(&l).unwrap().as_slice(); + let Ok(res) = machines[0].read_preprocess(&mut preprocess_ref) else { + return Some(ProcessorMessage::InvalidParticipant { id, participant: l }); + }; + if !preprocess_ref.is_empty() { + return Some(ProcessorMessage::InvalidParticipant { id, participant: l }); + } + parsed.insert(l, res); + } + let preprocesses = parsed; // Only keep a single machine as we only need one to get the signature let mut signature_machine = None; @@ -520,7 +519,18 @@ impl Signer { // Use an empty message, as expected of TransactionMachines let (machine, share) = match machine.sign(preprocesses, &[]) { Ok(res) => res, - Err(e) => todo!("malicious signer: {:?}", e), + Err(e) => match e { + FrostError::InternalError(_) | + FrostError::InvalidParticipant(_, _) | + FrostError::InvalidSigningSet(_) | + FrostError::InvalidParticipantQuantity(_, _) | + FrostError::DuplicatedParticipant(_) | + FrostError::MissingParticipant(_) => unreachable!(), + + FrostError::InvalidPreprocess(l) | FrostError::InvalidShare(l) => { + return Some(ProcessorMessage::InvalidParticipant { id, participant: l }) + } + }, }; if m == 0 { signature_machine = Some(machine); @@ -534,7 +544,7 @@ impl Signer { Some(ProcessorMessage::Share { id, shares: serialized_shares }) } - CoordinatorMessage::Shares { id, mut shares } => { + CoordinatorMessage::Shares { id, shares } => { if self.verify_id(&id).is_err() { return None; } @@ -557,21 +567,22 @@ impl Signer { Some(machine) => machine, }; - let mut shares = match shares - .drain() - .map(|(l, share)| { - let mut share_ref = share.as_ref(); - let res = machine.read_share::<&[u8]>(&mut share_ref).map(|share| (l, share)); - if !share_ref.is_empty() { - todo!("malicious signer: extra bytes"); - } - res - }) - .collect::, _>>() - { - Ok(shares) => shares, - Err(e) => todo!("malicious signer: {:?}", e), - }; + let mut parsed = HashMap::new(); + for l in { + let mut keys = shares.keys().cloned().collect::>(); + keys.sort(); + keys + } { + let mut share_ref = shares.get(&l).unwrap().as_slice(); + let Ok(res) = machine.read_share(&mut share_ref) else { + return Some(ProcessorMessage::InvalidParticipant { id, participant: l }); + }; + if !share_ref.is_empty() { + return Some(ProcessorMessage::InvalidParticipant { id, participant: l }); + } + parsed.insert(l, res); + } + let mut shares = parsed; for (i, our_share) in our_shares.into_iter().enumerate().skip(1) { assert!(shares.insert(self.keys[i].params().i(), our_share).is_none()); @@ -579,7 +590,18 @@ impl Signer { let tx = match machine.complete(shares) { Ok(res) => res, - Err(e) => todo!("malicious signer: {:?}", e), + Err(e) => match e { + FrostError::InternalError(_) | + FrostError::InvalidParticipant(_, _) | + FrostError::InvalidSigningSet(_) | + FrostError::InvalidParticipantQuantity(_, _) | + FrostError::DuplicatedParticipant(_) | + FrostError::MissingParticipant(_) => unreachable!(), + + FrostError::InvalidPreprocess(l) | FrostError::InvalidShare(l) => { + return Some(ProcessorMessage::InvalidParticipant { id, participant: l }) + } + }, }; // Save the transaction in case it's needed for recovery diff --git a/processor/src/substrate_signer.rs b/processor/src/substrate_signer.rs index 49ee51bb0..0a6e0b6df 100644 --- a/processor/src/substrate_signer.rs +++ b/processor/src/substrate_signer.rs @@ -6,7 +6,7 @@ use rand_core::OsRng; use ciphersuite::group::GroupEncoding; use frost::{ curve::Ristretto, - ThresholdKeys, + ThresholdKeys, FrostError, algorithm::Algorithm, sign::{ Writable, PreprocessMachine, SignMachine, SignatureMachine, AlgorithmMachine, @@ -246,7 +246,7 @@ impl SubstrateSigner { msg: CoordinatorMessage, ) -> Option { match msg { - CoordinatorMessage::BatchPreprocesses { id, mut preprocesses } => { + CoordinatorMessage::BatchPreprocesses { id, preprocesses } => { if self.verify_id(&id).is_err() { return None; } @@ -263,23 +263,22 @@ impl SubstrateSigner { Some(preprocess) => preprocess, }; - let preprocesses = match preprocesses - .drain() - .map(|(l, preprocess)| { - let mut preprocess_ref = preprocess.as_ref(); - let res = machines[0] - .read_preprocess::<&[u8]>(&mut preprocess_ref) - .map(|preprocess| (l, preprocess)); - if !preprocess_ref.is_empty() { - todo!("malicious signer: extra bytes"); - } - res - }) - .collect::, _>>() - { - Ok(preprocesses) => preprocesses, - Err(e) => todo!("malicious signer: {:?}", e), - }; + let mut parsed = HashMap::new(); + for l in { + let mut keys = preprocesses.keys().cloned().collect::>(); + keys.sort(); + keys + } { + let mut preprocess_ref = preprocesses.get(&l).unwrap().as_slice(); + let Ok(res) = machines[0].read_preprocess(&mut preprocess_ref) else { + return Some((ProcessorMessage::InvalidParticipant { id, participant: l }).into()); + }; + if !preprocess_ref.is_empty() { + return Some((ProcessorMessage::InvalidParticipant { id, participant: l }).into()); + } + parsed.insert(l, res); + } + let preprocesses = parsed; // Only keep a single machine as we only need one to get the signature let mut signature_machine = None; @@ -296,7 +295,18 @@ impl SubstrateSigner { let (machine, share) = match machine.sign(preprocesses, &batch_message(&self.signable[&id.id])) { Ok(res) => res, - Err(e) => todo!("malicious signer: {:?}", e), + Err(e) => match e { + FrostError::InternalError(_) | + FrostError::InvalidParticipant(_, _) | + FrostError::InvalidSigningSet(_) | + FrostError::InvalidParticipantQuantity(_, _) | + FrostError::DuplicatedParticipant(_) | + FrostError::MissingParticipant(_) => unreachable!(), + + FrostError::InvalidPreprocess(l) | FrostError::InvalidShare(l) => { + return Some((ProcessorMessage::InvalidParticipant { id, participant: l }).into()) + } + }, }; if m == 0 { signature_machine = Some(machine); @@ -314,7 +324,7 @@ impl SubstrateSigner { Some((ProcessorMessage::BatchShare { id, shares: serialized_shares }).into()) } - CoordinatorMessage::BatchShares { id, mut shares } => { + CoordinatorMessage::BatchShares { id, shares } => { if self.verify_id(&id).is_err() { return None; } @@ -337,21 +347,22 @@ impl SubstrateSigner { Some(signing) => signing, }; - let mut shares = match shares - .drain() - .map(|(l, share)| { - let mut share_ref = share.as_ref(); - let res = machine.read_share::<&[u8]>(&mut share_ref).map(|share| (l, share)); - if !share_ref.is_empty() { - todo!("malicious signer: extra bytes"); - } - res - }) - .collect::, _>>() - { - Ok(shares) => shares, - Err(e) => todo!("malicious signer: {:?}", e), - }; + let mut parsed = HashMap::new(); + for l in { + let mut keys = shares.keys().cloned().collect::>(); + keys.sort(); + keys + } { + let mut share_ref = shares.get(&l).unwrap().as_slice(); + let Ok(res) = machine.read_share(&mut share_ref) else { + return Some((ProcessorMessage::InvalidParticipant { id, participant: l }).into()); + }; + if !share_ref.is_empty() { + return Some((ProcessorMessage::InvalidParticipant { id, participant: l }).into()); + } + parsed.insert(l, res); + } + let mut shares = parsed; for (i, our_share) in our_shares.into_iter().enumerate().skip(1) { assert!(shares.insert(self.keys[i].params().i(), our_share).is_none()); @@ -359,7 +370,18 @@ impl SubstrateSigner { let sig = match machine.complete(shares) { Ok(res) => res, - Err(e) => todo!("malicious signer: {:?}", e), + Err(e) => match e { + FrostError::InternalError(_) | + FrostError::InvalidParticipant(_, _) | + FrostError::InvalidSigningSet(_) | + FrostError::InvalidParticipantQuantity(_, _) | + FrostError::DuplicatedParticipant(_) | + FrostError::MissingParticipant(_) => unreachable!(), + + FrostError::InvalidPreprocess(l) | FrostError::InvalidShare(l) => { + return Some((ProcessorMessage::InvalidParticipant { id, participant: l }).into()) + } + }, }; info!("signed batch {} with attempt #{}", hex::encode(id.id), id.attempt); From 80ff3dacb699aff9d31f1d2c4642b6d9879973b4 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sun, 12 Nov 2023 03:57:48 -0500 Subject: [PATCH 10/11] Move txn usage as needed for stable Rust to build --- coordinator/src/tributary/handle.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index aaecfd25d..c234b7e8d 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -441,6 +441,7 @@ pub(crate) async fn handle_application_tx< return; } + let share = TributaryDb::::share_for_blame(txn, &genesis, accuser, faulty).unwrap(); processors .send( spec.set().network, @@ -448,7 +449,7 @@ pub(crate) async fn handle_application_tx< id: KeyGenId { set: spec.set(), attempt }, accuser, accused: faulty, - share: TributaryDb::::share_for_blame(txn, &genesis, accuser, faulty).unwrap(), + share, blame, }, ) From 7634b80464d4b56e2a2c5e01463e565a1f95ff48 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sun, 12 Nov 2023 04:40:06 -0500 Subject: [PATCH 11/11] Correct InvalidDkgShare serialization --- coordinator/src/tributary/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/coordinator/src/tributary/mod.rs b/coordinator/src/tributary/mod.rs index 6bffe8f08..ab4631dec 100644 --- a/coordinator/src/tributary/mod.rs +++ b/coordinator/src/tributary/mod.rs @@ -516,9 +516,14 @@ impl ReadWrite for Transaction { writer.write_all(&attempt.to_le_bytes())?; writer.write_all(&u16::from(*accuser).to_le_bytes())?; writer.write_all(&u16::from(*faulty).to_le_bytes())?; + // Flattens Some(vec![]) to None on the expectation no actual blame will be 0-length assert!(blame.as_ref().map(|blame| blame.len()).unwrap_or(1) != 0); + let blame_len = + u16::try_from(blame.as_ref().unwrap_or(&vec![]).len()).expect("blame exceeded 64 KB"); + writer.write_all(&blame_len.to_le_bytes())?; writer.write_all(blame.as_ref().unwrap_or(&vec![]))?; + signed.write(writer) }