From 3f3f6b2d0c35af515fb1f1dd9db9c7394cb220ca Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Fri, 1 Sep 2023 00:16:43 -0400 Subject: [PATCH] Properly route attempt around DkgConfirmer --- coordinator/src/main.rs | 17 ++++++--------- coordinator/src/tests/tributary/dkg.rs | 4 ++-- coordinator/src/tributary/handle.rs | 30 +++++++++++++++++--------- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index e1dae98e..fe8f8952 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -480,11 +480,10 @@ pub async fn handle_processors( loop { // TODO: Dispatch this message to a task dedicated to handling this processor, preventing one - // processor from holding up all the others - // In order to do so, we need to locally track if a message was handled, even if its ID is - // greater than what the message-queue tracks as ack'd - // TODO: Shouldn't the processor also perform such checks? That its ack doesn't exceed the - // queue's? + // processor from holding up all the others. This would require a peek method be added to the + // message-queue (to view multiple future messages at once) + // TODO: Do we handle having handled a message, by DB, yet having rebooted before `ack`ing it? + // Does the processor? let msg = processors.recv().await; // TODO2: This is slow, and only works as long as a network only has a single Tributary @@ -511,12 +510,7 @@ pub async fn handle_processors( } key_gen::ProcessorMessage::Shares { id, mut shares } => { // Create a MuSig-based machine to inform Substrate of this key generation - // DkgConfirmer has a TODO noting it's only secure for a single usage, yet this ensures - // the TODO is resolved before unsafe usage - if id.attempt != 0 { - panic!("attempt wasn't 0"); - } - let nonces = crate::tributary::dkg_confirmation_nonces(&key, &spec); + let nonces = crate::tributary::dkg_confirmation_nonces(&key, &spec, id.attempt); let mut tx_shares = Vec::with_capacity(shares.len()); for i in 1 ..= spec.n() { @@ -549,6 +543,7 @@ pub async fn handle_processors( &key, &spec, &(Public(substrate_key), network_key.try_into().unwrap()), + id.attempt, ); txn.commit(); diff --git a/coordinator/src/tests/tributary/dkg.rs b/coordinator/src/tests/tributary/dkg.rs index 8ade2e09..1da472fa 100644 --- a/coordinator/src/tests/tributary/dkg.rs +++ b/coordinator/src/tests/tributary/dkg.rs @@ -172,7 +172,7 @@ async fn dkg_test() { let mut tx = Transaction::DkgShares { attempt, shares, - confirmation_nonces: crate::tributary::dkg_confirmation_nonces(key, &spec), + confirmation_nonces: crate::tributary::dkg_confirmation_nonces(key, &spec, 0), signed: Transaction::empty_signed(), }; tx.sign(&mut OsRng, spec.genesis(), key, 1); @@ -287,7 +287,7 @@ async fn dkg_test() { // albeit poor let mut txn = scanner_db.0.txn(); let share = - crate::tributary::generated_key_pair::(&mut txn, key, &spec, &key_pair).unwrap(); + crate::tributary::generated_key_pair::(&mut txn, key, &spec, &key_pair, 0).unwrap(); txn.commit(); let mut tx = Transaction::DkgConfirmed(attempt, share, Transaction::empty_signed()); diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index 98aff266..868500c0 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -48,6 +48,7 @@ impl DkgConfirmer { fn preprocess_internal( spec: &TributarySpec, key: &Zeroizing<::F>, + attempt: u32, ) -> (AlgorithmSignMachine, [u8; 64]) { // TODO: Does Substrate already have a validator-uniqueness check? let validators = spec.validators().iter().map(|val| val.0).collect::>(); @@ -57,7 +58,7 @@ impl DkgConfirmer { let mut entropy_transcript = RecommendedTranscript::new(b"DkgConfirmer Entropy"); entropy_transcript.append_message(b"spec", spec.serialize()); entropy_transcript.append_message(b"key", Zeroizing::new(key.to_bytes())); - // TODO: This is incredibly insecure unless message-bound (or bound via the attempt) + entropy_transcript.append_message(b"attempt", attempt.to_le_bytes()); Zeroizing::new(entropy_transcript).rng_seed(b"preprocess") }); let (machine, preprocess) = AlgorithmMachine::new( @@ -71,17 +72,22 @@ impl DkgConfirmer { (machine, preprocess.serialize().try_into().unwrap()) } // Get the preprocess for this confirmation. - fn preprocess(spec: &TributarySpec, key: &Zeroizing<::F>) -> [u8; 64] { - Self::preprocess_internal(spec, key).1 + fn preprocess( + spec: &TributarySpec, + key: &Zeroizing<::F>, + attempt: u32, + ) -> [u8; 64] { + Self::preprocess_internal(spec, key, attempt).1 } fn share_internal( spec: &TributarySpec, key: &Zeroizing<::F>, + attempt: u32, preprocesses: HashMap>, key_pair: &KeyPair, ) -> Result<(AlgorithmSignatureMachine, [u8; 32]), Participant> { - let machine = Self::preprocess_internal(spec, key).0; + let machine = Self::preprocess_internal(spec, key, attempt).0; let preprocesses = preprocesses .into_iter() .map(|(p, preprocess)| { @@ -109,20 +115,22 @@ impl DkgConfirmer { fn share( spec: &TributarySpec, key: &Zeroizing<::F>, + attempt: u32, preprocesses: HashMap>, key_pair: &KeyPair, ) -> Result<[u8; 32], Participant> { - Self::share_internal(spec, key, preprocesses, key_pair).map(|(_, share)| share) + Self::share_internal(spec, key, attempt, preprocesses, key_pair).map(|(_, share)| share) } fn complete( spec: &TributarySpec, key: &Zeroizing<::F>, + attempt: u32, preprocesses: HashMap>, key_pair: &KeyPair, shares: HashMap>, ) -> Result<[u8; 64], Participant> { - let machine = Self::share_internal(spec, key, preprocesses, key_pair) + let machine = Self::share_internal(spec, key, attempt, preprocesses, key_pair) .expect("trying to complete a machine which failed to preprocess") .0; @@ -193,8 +201,9 @@ fn read_known_to_exist_data( pub fn dkg_confirmation_nonces( key: &Zeroizing<::F>, spec: &TributarySpec, + attempt: u32, ) -> [u8; 64] { - DkgConfirmer::preprocess(spec, key) + DkgConfirmer::preprocess(spec, key, attempt) } #[allow(clippy::needless_pass_by_ref_mut)] @@ -203,10 +212,10 @@ pub fn generated_key_pair( key: &Zeroizing<::F>, spec: &TributarySpec, key_pair: &KeyPair, + attempt: u32, ) -> Result<[u8; 32], Participant> { TributaryDb::::save_currently_completing_key_pair(txn, spec.genesis(), key_pair); - let attempt = 0; // TODO let Some(preprocesses) = read_known_to_exist_data::( txn, spec, @@ -220,7 +229,7 @@ pub fn generated_key_pair( ) else { panic!("wasn't a participant in confirming a key pair"); }; - DkgConfirmer::share(spec, key, preprocesses, key_pair) + DkgConfirmer::share(spec, key, attempt, preprocesses, key_pair) } #[allow(clippy::too_many_arguments)] // TODO @@ -430,7 +439,8 @@ pub async fn handle_application_tx< "(including us) fires DkgConfirmed, yet no confirming key pair" ) }); - let Ok(sig) = DkgConfirmer::complete(spec, key, preprocesses, &key_pair, shares) else { + let Ok(sig) = DkgConfirmer::complete(spec, key, attempt, preprocesses, &key_pair, shares) + else { // TODO: Full slash todo!(); };