diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index 203d29e7..7ba1a825 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -37,7 +37,7 @@ use ::tributary::{ mod tributary; use crate::tributary::{ - TributarySpec, SignData, Transaction, NonceDecider, scanner::RecognizedIdType, PlanIds, + TributarySpec, SignData, Transaction, scanner::RecognizedIdType, PlanIds, }; mod db; @@ -698,24 +698,7 @@ async fn handle_processor_message( } } TransactionKind::Signed(_) => { - log::trace!("getting next nonce for Tributary TX in response to processor message"); - - let nonce = loop { - let Some(nonce) = - NonceDecider::nonce(&txn, genesis, &tx).expect("signed TX didn't have nonce") - else { - // This can be None if the following events occur, in order: - // 1) We scanned the relevant transaction(s) in a Tributary block - // 2) The processor was sent a message and responded - // 3) The Tributary TXN has yet to be committed - log::warn!("nonce has yet to be saved for processor-instigated transaction"); - sleep(Duration::from_millis(100)).await; - continue; - }; - break nonce; - }; - tx.sign(&mut OsRng, genesis, key, nonce); - + tx.sign(&mut OsRng, genesis, key); publish_signed_transaction(&mut txn, tributary, tx).await; } } @@ -1066,7 +1049,7 @@ pub async fn run( } }); - move |set: ValidatorSet, genesis, id_type, id: Vec, nonce| { + move |set: ValidatorSet, genesis, id_type, id: Vec| { log::debug!("recognized ID {:?} {}", id_type, hex::encode(&id)); let mut raw_db = raw_db.clone(); let key = key.clone(); @@ -1104,7 +1087,7 @@ pub async fn run( }), }; - tx.sign(&mut OsRng, genesis, &key, nonce); + tx.sign(&mut OsRng, genesis, &key); let mut first = true; loop { diff --git a/coordinator/src/tests/tributary/dkg.rs b/coordinator/src/tests/tributary/dkg.rs index 104a8e1d..b8bf5129 100644 --- a/coordinator/src/tests/tributary/dkg.rs +++ b/coordinator/src/tests/tributary/dkg.rs @@ -88,7 +88,7 @@ async fn dkg_test() { handle_new_blocks::<_, _, _, _, _, _, LocalP2p>( &mut scanner_db, key, - |_, _, _, _, _| async { + |_, _, _, _| async { panic!("provided TX caused recognized_id to be called in new_processors") }, &processors, @@ -114,7 +114,7 @@ async fn dkg_test() { handle_new_blocks::<_, _, _, _, _, _, LocalP2p>( &mut scanner_db, &keys[0], - |_, _, _, _, _| async { + |_, _, _, _| async { panic!("provided TX caused recognized_id to be called after Commitments") }, &processors, @@ -193,7 +193,7 @@ async fn dkg_test() { handle_new_blocks::<_, _, _, _, _, _, LocalP2p>( &mut scanner_db, &keys[0], - |_, _, _, _, _| async { + |_, _, _, _| async { panic!("provided TX caused recognized_id to be called after some shares") }, &processors, @@ -241,7 +241,7 @@ async fn dkg_test() { handle_new_blocks::<_, _, _, _, _, _, LocalP2p>( &mut scanner_db, &keys[0], - |_, _, _, _, _| async { panic!("provided TX caused recognized_id to be called after shares") }, + |_, _, _, _| async { panic!("provided TX caused recognized_id to be called after shares") }, &processors, |_, _| async { panic!("test tried to publish a new Serai TX") }, &spec, @@ -311,7 +311,7 @@ async fn dkg_test() { handle_new_blocks::<_, _, _, _, _, _, LocalP2p>( &mut scanner_db, &keys[0], - |_, _, _, _, _| async { + |_, _, _, _| async { panic!("provided TX caused recognized_id to be called after DKG confirmation") }, &processors, diff --git a/coordinator/src/tributary/handle.rs b/coordinator/src/tributary/handle.rs index bb9152c9..4012c798 100644 --- a/coordinator/src/tributary/handle.rs +++ b/coordinator/src/tributary/handle.rs @@ -27,7 +27,6 @@ use crate::{ processors::Processors, tributary::{ Transaction, TributarySpec, SeraiBlockNumber, Topic, DataSpecification, DataSet, Accumulation, - nonce_decider::NonceDecider, dkg_confirmer::DkgConfirmer, scanner::{RecognizedIdType, RIDTrait}, FatallySlashed, DkgShare, PlanIds, ConfirmationNonces, AttemptDb, DataDb, @@ -498,11 +497,6 @@ pub(crate) async fn handle_application_tx< genesis, Topic::SubstrateSign(SubstrateSignableId::CosigningSubstrateBlock(hash)), ); - NonceDecider::handle_substrate_signable( - txn, - genesis, - SubstrateSignableId::CosigningSubstrateBlock(hash), - ); let block_number = SeraiBlockNumber::get(txn, hash) .expect("CosignSubstrateBlock yet didn't save Serai block number"); @@ -528,9 +522,7 @@ pub(crate) async fn handle_application_tx< genesis, Topic::SubstrateSign(SubstrateSignableId::Batch(batch)), ); - let nonce = - NonceDecider::handle_substrate_signable(txn, genesis, SubstrateSignableId::Batch(batch)); - recognized_id(spec.set(), genesis, RecognizedIdType::Batch, batch.to_vec(), nonce).await; + recognized_id(spec.set(), genesis, RecognizedIdType::Batch, batch.to_vec()).await; } Transaction::SubstrateBlock(block) => { @@ -539,10 +531,9 @@ pub(crate) async fn handle_application_tx< despite us not providing that transaction", ); - let nonces = NonceDecider::handle_substrate_block(txn, genesis, &plan_ids); - for (nonce, id) in nonces.into_iter().zip(plan_ids.into_iter()) { + for id in plan_ids.into_iter() { AttemptDb::recognize_topic(txn, genesis, Topic::Sign(id)); - recognized_id(spec.set(), genesis, RecognizedIdType::Plan, id.to_vec(), nonce).await; + recognized_id(spec.set(), genesis, RecognizedIdType::Plan, id.to_vec()).await; } } @@ -569,7 +560,6 @@ pub(crate) async fn handle_application_tx< ) { Accumulation::Ready(DataSet::Participating(mut preprocesses)) => { unflatten(spec, &mut preprocesses); - NonceDecider::selected_for_signing_substrate(txn, genesis, data.plan); processors .send( spec.set().network, @@ -645,7 +635,6 @@ pub(crate) async fn handle_application_tx< ) { Accumulation::Ready(DataSet::Participating(mut preprocesses)) => { unflatten(spec, &mut preprocesses); - NonceDecider::selected_for_signing_plan(txn, genesis, data.plan); processors .send( spec.set().network, diff --git a/coordinator/src/tributary/mod.rs b/coordinator/src/tributary/mod.rs index 8e0dee5f..e129df8f 100644 --- a/coordinator/src/tributary/mod.rs +++ b/coordinator/src/tributary/mod.rs @@ -35,9 +35,6 @@ use tributary::{ mod db; pub use db::*; -mod nonce_decider; -pub use nonce_decider::*; - mod dkg_confirmer; mod handle; @@ -191,8 +188,8 @@ impl Debug for SignData ReadWrite for SignData { - fn read(reader: &mut R) -> io::Result { +impl SignData { + fn read(reader: &mut R, nonce: u32) -> io::Result { let plan = Id::decode(&mut scale::IoReader(&mut *reader)) .map_err(|_| io::Error::other("invalid plan in SignData"))?; @@ -217,7 +214,7 @@ impl ReadWrite for SignDat all_data }; - let signed = Signed::read(reader)?; + let signed = Signed::read_without_nonce(reader, nonce)?; Ok(SignData { plan, attempt, data, signed }) } @@ -242,7 +239,7 @@ impl ReadWrite for SignDat writer.write_all(data)?; } - self.signed.write(writer) + self.signed.write_without_nonce(writer) } } @@ -403,7 +400,7 @@ impl ReadWrite for Transaction { commitments }; - let signed = Signed::read(reader)?; + let signed = Signed::read_without_nonce(reader, 0)?; Ok(Transaction::DkgCommitments(attempt, commitments, signed)) } @@ -440,7 +437,7 @@ impl ReadWrite for Transaction { let mut confirmation_nonces = [0; 64]; reader.read_exact(&mut confirmation_nonces)?; - let signed = Signed::read(reader)?; + let signed = Signed::read_without_nonce(reader, 1)?; Ok(Transaction::DkgShares { attempt, shares, confirmation_nonces, signed }) } @@ -465,7 +462,8 @@ impl ReadWrite for Transaction { let mut blame = vec![0; u16::from_le_bytes(blame_len).into()]; reader.read_exact(&mut blame)?; - let signed = Signed::read(reader)?; + // This shares a nonce with DkgConfirmed as only one is expected + let signed = Signed::read_without_nonce(reader, 2)?; Ok(Transaction::InvalidDkgShare { attempt, @@ -484,7 +482,7 @@ impl ReadWrite for Transaction { let mut confirmation_share = [0; 32]; reader.read_exact(&mut confirmation_share)?; - let signed = Signed::read(reader)?; + let signed = Signed::read_without_nonce(reader, 2)?; Ok(Transaction::DkgConfirmed(attempt, confirmation_share, signed)) } @@ -509,11 +507,11 @@ impl ReadWrite for Transaction { Ok(Transaction::SubstrateBlock(u64::from_le_bytes(block))) } - 8 => SignData::read(reader).map(Transaction::SubstratePreprocess), - 9 => SignData::read(reader).map(Transaction::SubstrateShare), + 8 => SignData::read(reader, 0).map(Transaction::SubstratePreprocess), + 9 => SignData::read(reader, 1).map(Transaction::SubstrateShare), - 10 => SignData::read(reader).map(Transaction::SignPreprocess), - 11 => SignData::read(reader).map(Transaction::SignShare), + 10 => SignData::read(reader, 0).map(Transaction::SignPreprocess), + 11 => SignData::read(reader, 1).map(Transaction::SignShare), 12 => { let mut plan = [0; 32]; @@ -557,7 +555,7 @@ impl ReadWrite for Transaction { for commitments in commitments { writer.write_all(commitments)?; } - signed.write(writer) + signed.write_without_nonce(writer) } Transaction::DkgShares { attempt, shares, confirmation_nonces, signed } => { @@ -586,7 +584,7 @@ impl ReadWrite for Transaction { } writer.write_all(confirmation_nonces)?; - signed.write(writer) + signed.write_without_nonce(writer) } Transaction::InvalidDkgShare { attempt, accuser, faulty, blame, signed } => { @@ -602,14 +600,14 @@ impl ReadWrite for Transaction { writer.write_all(&blame_len.to_le_bytes())?; writer.write_all(blame.as_ref().unwrap_or(&vec![]))?; - signed.write(writer) + signed.write_without_nonce(writer) } Transaction::DkgConfirmed(attempt, share, signed) => { writer.write_all(&[4])?; writer.write_all(&attempt.to_le_bytes())?; writer.write_all(share)?; - signed.write(writer) + signed.write_without_nonce(writer) } Transaction::CosignSubstrateBlock(block) => { @@ -663,21 +661,21 @@ impl TransactionTrait for Transaction { 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::DkgCommitments(attempt, _, signed) => TransactionKind::Signed((b"dkg", attempt).encode(), signed), + Transaction::DkgShares { attempt, signed, .. } => TransactionKind::Signed((b"dkg", attempt).encode(), signed), + Transaction::InvalidDkgShare { attempt, signed, .. } => TransactionKind::Signed((b"dkg", attempt).encode(), signed), + Transaction::DkgConfirmed(attempt, _, signed) => TransactionKind::Signed((b"dkg", attempt).encode(), signed), Transaction::CosignSubstrateBlock(_) => TransactionKind::Provided("cosign"), Transaction::Batch(_, _) => TransactionKind::Provided("batch"), Transaction::SubstrateBlock(_) => TransactionKind::Provided("serai"), - Transaction::SubstratePreprocess(data) => TransactionKind::Signed(&data.signed), - Transaction::SubstrateShare(data) => TransactionKind::Signed(&data.signed), + Transaction::SubstratePreprocess(data) => TransactionKind::Signed((b"substrate", data.0.plan, data.0.attempt).encode(), &data.signed), + Transaction::SubstrateShare(data) => TransactionKind::Signed((b"substrate", data.0.plan, data.0.attempt).encode(), &data.signed), - Transaction::SignPreprocess(data) => TransactionKind::Signed(&data.signed), - Transaction::SignShare(data) => TransactionKind::Signed(&data.signed), + Transaction::SignPreprocess(data) => TransactionKind::Signed((b"sign", data.0.plan, data.0.attempt).encode(), &data.signed), + Transaction::SignShare(data) => TransactionKind::Signed((b"sign", data.0.plan, data.0.attempt).encode(), &data.signed), Transaction::SignCompleted { .. } => TransactionKind::Unsigned, } } @@ -729,7 +727,6 @@ impl Transaction { rng: &mut R, genesis: [u8; 32], key: &Zeroizing<::F>, - nonce: u32, ) { fn signed(tx: &mut Transaction) -> &mut Signed { match tx { @@ -756,7 +753,27 @@ impl Transaction { let signed_ref = signed(self); signed_ref.signer = Ristretto::generator() * key.deref(); - signed_ref.nonce = nonce; + + signed_ref.nonce = match tx { + Transaction::RemoveParticipant(_) => panic!("signing RemoveParticipant"), + + Transaction::DkgCommitments(_, _, _) => 0, + Transaction::DkgShares { .. } => 1, + Transaction::InvalidDkgShare { .. } => 2, + Transaction::DkgConfirmed(_, _, _) => 2, + + Transaction::CosignSubstrateBlock(_) => panic!("signing CosignSubstrateBlock"), + + Transaction::Batch(_, _) => panic!("signing Batch"), + Transaction::SubstrateBlock(_) => panic!("signing SubstrateBlock"), + + Transaction::SubstratePreprocess(_) => 0, + Transaction::SubstrateShare(_) => 1, + + Transaction::SignPreprocess(_) => 0, + Transaction::SignShare(_) => 1, + Transaction::SignCompleted { .. } => panic!("signing SignCompleted"), + }; let sig_nonce = Zeroizing::new(::F::random(rng)); signed(self).signature.R = ::generator() * sig_nonce.deref(); diff --git a/coordinator/src/tributary/nonce_decider.rs b/coordinator/src/tributary/nonce_decider.rs deleted file mode 100644 index 409280f3..00000000 --- a/coordinator/src/tributary/nonce_decider.rs +++ /dev/null @@ -1,123 +0,0 @@ -use serai_db::{Get, DbTxn, create_db}; - -use processor_messages::coordinator::SubstrateSignableId; - -use crate::tributary::Transaction; - -use scale::Encode; - -const SUBSTRATE_CODE: u8 = 0; -const SUBSTRATE_SIGNING_CODE: u8 = 1; -const PLAN_CODE: u8 = 2; -const PLAN_SIGNING_CODE: u8 = 3; - -create_db!( - NonceDeciderDb { - NextNonceDb: (genesis: [u8; 32]) -> u32, - ItemNonceDb: (genesis: [u8; 32], code: u8, id: &[u8]) -> u32, - } -); - -impl NextNonceDb { - pub fn allocate_nonce(txn: &mut impl DbTxn, genesis: [u8; 32]) -> u32 { - let next = Self::get(txn, genesis).unwrap_or(3); - Self::set(txn, genesis, &(next + 1)); - next - } -} - -/// Decides the nonce which should be used for a transaction on a Tributary. -/// -/// Deterministically builds a list of nonces to use based on the on-chain events and expected -/// transactions in response. Enables rebooting/rebuilding validators with full safety. -pub struct NonceDecider; -impl NonceDecider { - pub fn handle_substrate_signable( - txn: &mut impl DbTxn, - genesis: [u8; 32], - id: SubstrateSignableId, - ) -> u32 { - let nonce_for = NextNonceDb::allocate_nonce(txn, genesis); - ItemNonceDb::set(txn, genesis, SUBSTRATE_CODE, &id.encode(), &nonce_for); - nonce_for - } - - pub fn handle_substrate_block( - txn: &mut impl DbTxn, - genesis: [u8; 32], - plans: &[[u8; 32]], - ) -> Vec { - let mut res = Vec::with_capacity(plans.len()); - for plan in plans { - let nonce_for = NextNonceDb::allocate_nonce(txn, genesis); - ItemNonceDb::set(txn, genesis, PLAN_CODE, plan, &nonce_for); - res.push(nonce_for); - } - res - } - - // TODO: The processor won't yield shares for this if the signing protocol aborts. We need to - // detect when we're expecting shares for an aborted protocol and insert a dummy transaction - // there. - pub fn selected_for_signing_substrate( - txn: &mut impl DbTxn, - genesis: [u8; 32], - id: SubstrateSignableId, - ) { - let nonce_for = NextNonceDb::allocate_nonce(txn, genesis); - ItemNonceDb::set(txn, genesis, SUBSTRATE_SIGNING_CODE, &id.encode(), &nonce_for); - } - - // TODO: Same TODO as selected_for_signing_substrate - pub fn selected_for_signing_plan(txn: &mut impl DbTxn, genesis: [u8; 32], plan: [u8; 32]) { - let nonce_for = NextNonceDb::allocate_nonce(txn, genesis); - ItemNonceDb::set(txn, genesis, PLAN_SIGNING_CODE, &plan, &nonce_for); - } - - pub fn nonce(getter: &impl Get, genesis: [u8; 32], tx: &Transaction) -> Option> { - match tx { - Transaction::RemoveParticipant(_) => None, - - Transaction::DkgCommitments(attempt, _, _) => { - assert_eq!(*attempt, 0); - Some(Some(0)) - } - Transaction::DkgShares { attempt, .. } => { - 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)) - } - - Transaction::CosignSubstrateBlock(_) => None, - - Transaction::Batch(_, _) => None, - Transaction::SubstrateBlock(_) => None, - Transaction::SubstratePreprocess(data) => { - assert_eq!(data.attempt, 0); - Some(ItemNonceDb::get(getter, genesis, SUBSTRATE_CODE, &data.plan.encode())) - } - Transaction::SubstrateShare(data) => { - assert_eq!(data.attempt, 0); - Some(ItemNonceDb::get(getter, genesis, SUBSTRATE_SIGNING_CODE, &data.plan.encode())) - } - Transaction::SignPreprocess(data) => { - assert_eq!(data.attempt, 0); - Some(ItemNonceDb::get(getter, genesis, PLAN_CODE, &data.plan.encode())) - } - Transaction::SignShare(data) => { - assert_eq!(data.attempt, 0); - Some(ItemNonceDb::get(getter, genesis, PLAN_SIGNING_CODE, &data.plan.encode())) - } - Transaction::SignCompleted { .. } => None, - } - } -} diff --git a/coordinator/src/tributary/scanner.rs b/coordinator/src/tributary/scanner.rs index 8c8653d6..f7f6df79 100644 --- a/coordinator/src/tributary/scanner.rs +++ b/coordinator/src/tributary/scanner.rs @@ -35,10 +35,10 @@ pub enum RecognizedIdType { } pub(crate) trait RIDTrait: - Clone + Fn(ValidatorSet, [u8; 32], RecognizedIdType, Vec, u32) -> FRid + Clone + Fn(ValidatorSet, [u8; 32], RecognizedIdType, Vec) -> FRid { } -impl, u32) -> FRid> +impl) -> FRid> RIDTrait for F { } diff --git a/coordinator/tributary/src/transaction.rs b/coordinator/tributary/src/transaction.rs index 221db619..fdee7f2b 100644 --- a/coordinator/tributary/src/transaction.rs +++ b/coordinator/tributary/src/transaction.rs @@ -81,6 +81,32 @@ impl ReadWrite for Signed { } } +impl Signed { + fn read_without_nonce(reader: &mut R, nonce: u32) -> io::Result { + let signer = Ristretto::read_G(reader)?; + + let mut signature = SchnorrSignature::::read(reader)?; + if signature.R.is_identity().into() { + // Anyone malicious could remove this and try to find zero signatures + // We should never produce zero signatures though meaning this should never come up + // If it does somehow come up, this is a decent courtesy + signature.zeroize(); + Err(io::Error::other("signature nonce was identity"))?; + } + + Ok(Signed { signer, nonce, signature }) + } + + fn write_without_nonce(&self, writer: &mut W) -> io::Result<()> { + // This is either an invalid signature or a private key leak + if self.signature.R.is_identity().into() { + Err(io::Error::other("signature nonce was identity"))?; + } + writer.write_all(&self.signer.to_bytes())?; + self.signature.write(writer) + } +} + #[allow(clippy::large_enum_variant)] #[derive(Clone, PartialEq, Eq, Debug)] pub enum TransactionKind<'a> {