From dc2656a5381cbc423ed27f0c4058a6b1445cc375 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Thu, 24 Aug 2023 18:52:31 -0400 Subject: [PATCH] Don't bind to the entire batch, solely the network and ID This avoids needing to know the Batch in advance, avoiding a race condition. --- processor/src/main.rs | 9 ++-- processor/src/substrate_signer.rs | 58 ++++++++++--------------- processor/src/tests/substrate_signer.rs | 2 +- 3 files changed, 29 insertions(+), 40 deletions(-) diff --git a/processor/src/main.rs b/processor/src/main.rs index d6dd00cb..9bc167b3 100644 --- a/processor/src/main.rs +++ b/processor/src/main.rs @@ -373,9 +373,10 @@ async fn handle_coordinator_msg( // See TributaryMutable's struct definition for why this block is safe let KeyConfirmed { substrate_keys, network_keys } = tributary_mutable.key_gen.confirm(txn, set, key_pair).await; - tributary_mutable - .substrate_signers - .insert(substrate_keys.group_key().to_bytes(), SubstrateSigner::new(substrate_keys)); + tributary_mutable.substrate_signers.insert( + substrate_keys.group_key().to_bytes(), + SubstrateSigner::new(N::NETWORK, substrate_keys), + ); let key = network_keys.group_key(); @@ -516,7 +517,7 @@ async fn boot( let (substrate_keys, network_keys) = key_gen.keys(key); let substrate_key = substrate_keys.group_key(); - let substrate_signer = SubstrateSigner::new(substrate_keys); + let substrate_signer = SubstrateSigner::new(N::NETWORK, substrate_keys); // We don't have to load any state for this since the Scanner will re-fire any events // necessary substrate_signers.insert(substrate_key.to_bytes(), substrate_signer); diff --git a/processor/src/substrate_signer.rs b/processor/src/substrate_signer.rs index f541f4b2..92766864 100644 --- a/processor/src/substrate_signer.rs +++ b/processor/src/substrate_signer.rs @@ -19,11 +19,27 @@ use frost_schnorrkel::Schnorrkel; use log::{info, debug, warn}; -use serai_client::in_instructions::primitives::{Batch, SignedBatch, batch_message}; +use serai_client::{ + primitives::NetworkId, + in_instructions::primitives::{Batch, SignedBatch, batch_message}, +}; use messages::{sign::SignId, coordinator::*}; use crate::{Get, DbTxn, Db}; +// Generate an ID unique to a Batch +// TODO: Fork SignId to BatchSignId in order to just use the 5-byte encoding, not the hash of the +// 5-byte encoding +fn sign_id(network: NetworkId, id: u32) -> [u8; 32] { + let mut transcript = RecommendedTranscript::new(b"Serai Processor Batch Sign ID"); + transcript.append_message(b"network", network.encode()); + transcript.append_message(b"id", id.to_le_bytes()); + + let mut res = [0; 32]; + res.copy_from_slice(&transcript.challenge(b"id")[.. 32]); + res +} + #[derive(Debug)] pub enum SubstrateSignerEvent { ProcessorMessage(ProcessorMessage), @@ -37,16 +53,6 @@ impl SubstrateSignerDb { D::key(b"SUBSTRATE_SIGNER", dst, key) } - fn sign_id_key(id: u32) -> Vec { - Self::sign_key(b"sign_id", id.to_le_bytes()) - } - fn save_sign_id(txn: &mut D::Transaction<'_>, id: u32, sign_id: [u8; 32]) { - txn.put(Self::sign_id_key(id), sign_id); - } - fn sign_id(txn: &mut D::Transaction<'_>, id: u32) -> [u8; 32] { - txn.get(Self::sign_id_key(id)).expect("completing a batch we never started").try_into().unwrap() - } - fn completed_key(id: [u8; 32]) -> Vec { Self::sign_key(b"completed", id) } @@ -75,6 +81,7 @@ impl SubstrateSignerDb { pub struct SubstrateSigner { db: PhantomData, + network: NetworkId, keys: ThresholdKeys, signable: HashMap<[u8; 32], Batch>, @@ -96,10 +103,11 @@ impl fmt::Debug for SubstrateSigner { } impl SubstrateSigner { - pub fn new(keys: ThresholdKeys) -> SubstrateSigner { + pub fn new(network: NetworkId, keys: ThresholdKeys) -> SubstrateSigner { SubstrateSigner { db: PhantomData, + network, keys, signable: HashMap::new(), @@ -214,28 +222,8 @@ impl SubstrateSigner { } pub async fn sign(&mut self, txn: &mut D::Transaction<'_>, batch: Batch) { - // Generate a unique ID to sign with - let id = { - // TODO: Add this to in-instructions primitives - let mut transcript = RecommendedTranscript::new(b"Serai Processor Batch ID"); - transcript.domain_separate(b"header"); - transcript.append_message(b"network", batch.network.encode()); - transcript.append_message(b"id", batch.id.to_le_bytes()); - transcript.append_message(b"block", batch.block.0); - - transcript.domain_separate(b"instructions"); - for instruction in &batch.instructions { - // TODO: Either properly transcript this or simply encode the entire batch (since SCALE is - // canonical) - transcript.append_message(b"instruction", instruction.encode()); - } - - let mut res = [0; 32]; - res.copy_from_slice(&transcript.challenge(b"id")[.. 32]); - res - }; - SubstrateSignerDb::::save_sign_id(txn, batch.id, id); - + debug_assert_eq!(self.network, batch.network); + let id = sign_id(batch.network, batch.id); if SubstrateSignerDb::::completed(txn, id) { debug!("Sign batch order for ID we've already completed signing"); // See batch_signed for commentary on why this simply returns @@ -370,7 +358,7 @@ impl SubstrateSigner { // block behind it, which will trigger starting the Batch // TODO: There is a race condition between the Scanner recognizing the block and the Batch // having signing started - let sign_id = SubstrateSignerDb::::sign_id(txn, id); + let sign_id = sign_id(self.network, id); // Stop trying to sign for this batch SubstrateSignerDb::::complete(txn, sign_id); diff --git a/processor/src/tests/substrate_signer.rs b/processor/src/tests/substrate_signer.rs index 2d1effc8..fa5f9523 100644 --- a/processor/src/tests/substrate_signer.rs +++ b/processor/src/tests/substrate_signer.rs @@ -53,7 +53,7 @@ async fn test_substrate_signer() { let keys = keys.remove(&i).unwrap(); t = keys.params().t(); - let mut signer = SubstrateSigner::::new(keys); + let mut signer = SubstrateSigner::::new(NetworkId::Monero, keys); let mut db = MemDb::new(); let mut txn = db.txn(); signer.sign(&mut txn, batch.clone()).await;