diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index 50eda721..5197b986 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -190,7 +190,6 @@ pub async fn scan_tributaries( } } -#[allow(clippy::type_complexity)] pub async fn heartbeat_tributaries( p2p: P, tributaries: Arc>>, @@ -217,7 +216,6 @@ pub async fn heartbeat_tributaries( } } -#[allow(clippy::type_complexity)] pub async fn handle_p2p( our_key: ::G, p2p: P, @@ -364,7 +362,6 @@ pub async fn publish_transaction( } } -#[allow(clippy::type_complexity)] pub async fn handle_processors( mut db: D, key: Zeroizing<::F>, diff --git a/coordinator/src/tributary/mod.rs b/coordinator/src/tributary/mod.rs index 1b718e76..5ddeb231 100644 --- a/coordinator/src/tributary/mod.rs +++ b/coordinator/src/tributary/mod.rs @@ -232,6 +232,7 @@ pub enum Transaction { SignPreprocess(SignData), SignShare(SignData), + SignCompleted([u8; 32], Vec, Signed), } impl ReadWrite for Transaction { @@ -304,6 +305,19 @@ impl ReadWrite for Transaction { 6 => SignData::read(reader).map(Transaction::SignPreprocess), 7 => SignData::read(reader).map(Transaction::SignShare), + 8 => { + let mut plan = [0; 32]; + reader.read_exact(&mut plan)?; + + let mut tx_len = [0]; + reader.read_exact(&mut tx_len)?; + let mut tx = vec![0; usize::from(tx_len[0])]; + reader.read_exact(&mut tx)?; + + let signed = Signed::read(reader)?; + Ok(Transaction::SignCompleted(plan, tx, signed)) + } + _ => Err(io::Error::new(io::ErrorKind::Other, "invalid transaction type")), } } @@ -376,6 +390,13 @@ impl ReadWrite for Transaction { writer.write_all(&[7])?; data.write(writer) } + Transaction::SignCompleted(plan, tx, signed) => { + writer.write_all(&[8])?; + writer.write_all(plan)?; + writer.write_all(&[u8::try_from(tx.len()).expect("tx hash length exceed 255 bytes")])?; + writer.write_all(tx)?; + signed.write(writer) + } } } } @@ -394,6 +415,7 @@ impl TransactionTrait for Transaction { Transaction::SignPreprocess(data) => TransactionKind::Signed(&data.signed), Transaction::SignShare(data) => TransactionKind::Signed(&data.signed), + Transaction::SignCompleted(_, _, signed) => TransactionKind::Signed(signed), } } @@ -451,6 +473,7 @@ impl Transaction { Transaction::SignPreprocess(ref mut data) => &mut data.signed, Transaction::SignShare(ref mut data) => &mut data.signed, + Transaction::SignCompleted(_, _, ref mut signed) => signed, } } diff --git a/coordinator/src/tributary/scanner.rs b/coordinator/src/tributary/scanner.rs index 204c2a0d..2464409d 100644 --- a/coordinator/src/tributary/scanner.rs +++ b/coordinator/src/tributary/scanner.rs @@ -297,6 +297,7 @@ async fn handle_block( .await; } } + Transaction::SignCompleted(_, _, _) => todo!(), } TributaryDb::::handle_event(&mut txn, hash, event_id); diff --git a/coordinator/tributary/src/transaction.rs b/coordinator/tributary/src/transaction.rs index f45387fd..95852835 100644 --- a/coordinator/tributary/src/transaction.rs +++ b/coordinator/tributary/src/transaction.rs @@ -1,11 +1,15 @@ use core::fmt::Debug; use std::{io, collections::HashMap}; +use zeroize::Zeroize; use thiserror::Error; use blake2::{Digest, Blake2b512}; -use ciphersuite::{group::GroupEncoding, Ciphersuite, Ristretto}; +use ciphersuite::{ + group::{Group, GroupEncoding}, + Ciphersuite, Ristretto, +}; use schnorr::SchnorrSignature; use crate::{TRANSACTION_SIZE_LIMIT, ReadWrite}; @@ -48,12 +52,23 @@ impl ReadWrite for Signed { Err(io::Error::new(io::ErrorKind::Other, "nonce exceeded limit"))?; } - let signature = SchnorrSignature::::read(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::new(io::ErrorKind::Other, "signature nonce was identity"))?; + } Ok(Signed { signer, nonce, signature }) } fn write(&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::new(io::ErrorKind::Other, "signature nonce was identity"))?; + } writer.write_all(&self.signer.to_bytes())?; writer.write_all(&self.nonce.to_le_bytes())?; self.signature.write(writer) diff --git a/docs/coordinator/Coordinator.md b/docs/coordinator/Coordinator.md index a61f0306..49638961 100644 --- a/docs/coordinator/Coordinator.md +++ b/docs/coordinator/Coordinator.md @@ -36,5 +36,10 @@ transaction containing the signed batch to the Serai blockchain. # Sign Completed -On `sign::ProcessorMessage::Completed`, the coordinator broadcasts the -contained information to all validators. +On `sign::ProcessorMessage::Completed`, the coordinator makes a tributary +transaction containing the transaction hash the signing process was supposedly +completed with. + +Due to rushing adversaries, the actual transaction completing the plan may be +distinct on-chain. These messages solely exist to coordinate the signing +process, not to determine chain state. diff --git a/processor/messages/src/lib.rs b/processor/messages/src/lib.rs index 2831f8ff..b6c25597 100644 --- a/processor/messages/src/lib.rs +++ b/processor/messages/src/lib.rs @@ -98,7 +98,6 @@ pub mod sign { // Signed share for the specified signing protocol. Share { id: SignId, share: Vec }, // Completed a signing protocol already. - // TODO: Move this to SignId Completed { key: Vec, id: [u8; 32], tx: Vec }, } } @@ -246,9 +245,13 @@ impl CoordinatorMessage { sign::CoordinatorMessage::Preprocesses { id, .. } => (0, bincode::serialize(id).unwrap()), sign::CoordinatorMessage::Shares { id, .. } => (1, bincode::serialize(id).unwrap()), sign::CoordinatorMessage::Reattempt { id } => (2, bincode::serialize(id).unwrap()), - // TODO: This doesn't embed the attempt. Accordingly, multiple distinct completions will - // be flattened. This isn't acceptable. - sign::CoordinatorMessage::Completed { id, .. } => (3, id.to_vec()), + // The coordinator should report all reported completions to the processor + // Accordingly, the intent is a combination of plan ID and actual TX + // While transaction alone may suffice, that doesn't cover cross-chain TX ID conflicts, + // which are possible + sign::CoordinatorMessage::Completed { id, tx, .. } => { + (3, bincode::serialize(&(id, tx)).unwrap()) + } }; let mut res = vec![COORDINATOR_UID, TYPE_SIGN_UID, sub];