From bccdabb53d335fc5537335bb613c8e0435082330 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Thu, 24 Aug 2023 20:30:50 -0400 Subject: [PATCH] Use a single Substrate signer, per intentions in #227 Removes key from Update as well, since it's no longer variable. --- coordinator/src/main.rs | 2 +- processor/messages/src/lib.rs | 2 +- processor/src/main.rs | 49 ++++++++++++++---------------- processor/src/scanner.rs | 9 ++---- processor/src/tests/addresses.rs | 6 ++-- processor/src/tests/scanner.rs | 3 +- processor/src/tests/wallet.rs | 6 ++-- tests/processor/src/tests/batch.rs | 3 -- 8 files changed, 33 insertions(+), 47 deletions(-) diff --git a/coordinator/src/main.rs b/coordinator/src/main.rs index eb5e237d..54b98c97 100644 --- a/coordinator/src/main.rs +++ b/coordinator/src/main.rs @@ -569,7 +569,7 @@ pub async fn handle_processors( } }, ProcessorMessage::Substrate(inner_msg) => match inner_msg { - processor_messages::substrate::ProcessorMessage::Update { key: _, batch } => { + processor_messages::substrate::ProcessorMessage::Update { batch } => { assert_eq!( batch.batch.network, msg.network, "processor sent us a batch for a different network than it was for", diff --git a/processor/messages/src/lib.rs b/processor/messages/src/lib.rs index c23bfe3d..312a7294 100644 --- a/processor/messages/src/lib.rs +++ b/processor/messages/src/lib.rs @@ -176,7 +176,7 @@ pub mod substrate { #[derive(Clone, PartialEq, Eq, Debug, Zeroize, Serialize, Deserialize)] pub enum ProcessorMessage { - Update { key: [u8; 32], batch: SignedBatch }, + Update { batch: SignedBatch }, } } diff --git a/processor/src/main.rs b/processor/src/main.rs index 9bc167b3..e254c2fa 100644 --- a/processor/src/main.rs +++ b/processor/src/main.rs @@ -157,8 +157,8 @@ struct TributaryMutable { // Substrate may mark tasks as completed, invalidating any existing mutable borrows. // The safety of this follows as written above. - // TODO: There should only be one SubstrateSigner at a time (see #277) - substrate_signers: HashMap<[u8; 32], SubstrateSigner>, + // There should only be one SubstrateSigner at a time (see #277) + substrate_signer: Option>, } // Items which are mutably borrowed by Substrate. @@ -308,7 +308,9 @@ async fn handle_coordinator_msg( } CoordinatorMessage::Coordinator(msg) => { - tributary_mutable.substrate_signers.get_mut(msg.key()).unwrap().handle(txn, msg).await; + if let Some(substrate_signer) = tributary_mutable.substrate_signer.as_mut() { + substrate_signer.handle(txn, msg).await; + } } CoordinatorMessage::Substrate(msg) => { @@ -317,7 +319,7 @@ async fn handle_coordinator_msg( // This is the first key pair for this network so no block has been finalized yet let activation_number = if context.network_latest_finalized_block.0 == [0; 32] { assert!(tributary_mutable.signers.is_empty()); - assert!(tributary_mutable.substrate_signers.is_empty()); + assert!(tributary_mutable.substrate_signer.is_none()); assert!(substrate_mutable.schedulers.is_empty()); // Wait until a network's block's time exceeds Serai's time @@ -373,10 +375,9 @@ 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(N::NETWORK, substrate_keys), - ); + // TODO2: Don't immediately set this, set it once it's active + tributary_mutable.substrate_signer = + Some(SubstrateSigner::new(N::NETWORK, substrate_keys)); let key = network_keys.group_key(); @@ -408,9 +409,9 @@ async fn handle_coordinator_msg( // We now have to acknowledge every block for this key up to the acknowledged block let outputs = substrate_mutable.scanner.ack_up_to_block(txn, key, block_id).await; // Since this block was acknowledged, we no longer have to sign the batch for it - for batch_id in batches { - for (_, signer) in tributary_mutable.substrate_signers.iter_mut() { - signer.batch_signed(txn, batch_id); + if let Some(substrate_signer) = tributary_mutable.substrate_signer.as_mut() { + for batch_id in batches { + substrate_signer.batch_signed(txn, batch_id); } } @@ -506,7 +507,7 @@ async fn boot( let (mut scanner, active_keys) = Scanner::new(network.clone(), raw_db.clone()); let mut schedulers = HashMap::, Scheduler>::new(); - let mut substrate_signers = HashMap::new(); + let mut substrate_signer = None; let mut signers = HashMap::new(); let main_db = MainDb::new(raw_db.clone()); @@ -516,11 +517,10 @@ async fn boot( let (substrate_keys, network_keys) = key_gen.keys(key); - let substrate_key = substrate_keys.group_key(); - 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); + // TODO2: This uses most recent as signer, use the active one + substrate_signer = Some(SubstrateSigner::new(N::NETWORK, substrate_keys)); let mut signer = Signer::new(network.clone(), network_keys); @@ -554,7 +554,7 @@ async fn boot( ( main_db, - TributaryMutable { key_gen, substrate_signers, signers }, + TributaryMutable { key_gen, substrate_signer, signers }, SubstrateMutable { scanner, schedulers }, ) } @@ -603,7 +603,7 @@ async fn run(mut raw_db: D, network: N, mut } } - for (key, signer) in tributary_mutable.substrate_signers.iter_mut() { + if let Some(signer) = tributary_mutable.substrate_signer.as_mut() { while let Some(msg) = signer.events.pop_front() { match msg { SubstrateSignerEvent::ProcessorMessage(msg) => { @@ -612,7 +612,6 @@ async fn run(mut raw_db: D, network: N, mut SubstrateSignerEvent::SignedBatch(batch) => { coordinator .send(ProcessorMessage::Substrate(messages::substrate::ProcessorMessage::Update { - key: *key, batch, })) .await; @@ -665,7 +664,7 @@ async fn run(mut raw_db: D, network: N, mut let mut txn = raw_db.txn(); match msg.unwrap() { - ScannerEvent::Block { key, block, outputs } => { + ScannerEvent::Block { block, outputs } => { let mut block_hash = [0; 32]; block_hash.copy_from_slice(block.as_ref()); // TODO: Move this out from Scanner now that the Scanner no longer handles batches @@ -735,13 +734,11 @@ async fn run(mut raw_db: D, network: N, mut for batch in batches { info!("created batch {} ({} instructions)", batch.id, batch.instructions.len()); - // TODO: Don't reload both sets of keys in full just to get the Substrate public key - tributary_mutable - .substrate_signers - .get_mut(tributary_mutable.key_gen.keys(&key).0.group_key().to_bytes().as_slice()) - .unwrap() - .sign(&mut txn, batch) - .await; + if let Some(substrate_signer) = tributary_mutable.substrate_signer.as_mut() { + substrate_signer + .sign(&mut txn, batch) + .await; + } } }, diff --git a/processor/src/scanner.rs b/processor/src/scanner.rs index 51404431..6c0fb5e7 100644 --- a/processor/src/scanner.rs +++ b/processor/src/scanner.rs @@ -22,11 +22,7 @@ use crate::{ #[derive(Clone, Debug)] pub enum ScannerEvent { // Block scanned - Block { - key: ::G, - block: >::Id, - outputs: Vec, - }, + Block { block: >::Id, outputs: Vec }, // Eventuality completion found on-chain Completed([u8; 32], >::Id), } @@ -507,7 +503,8 @@ impl Scanner { txn.commit(); // Send all outputs - if !scanner.emit(ScannerEvent::Block { key, block: block_id, outputs }) { + // TODO2: Fire this with all outputs for all keys, not for each key + if !scanner.emit(ScannerEvent::Block { block: block_id, outputs }) { return; } // Write this number as scanned so we won't re-fire these outputs diff --git a/processor/src/tests/addresses.rs b/processor/src/tests/addresses.rs index d3b23ddb..00a88805 100644 --- a/processor/src/tests/addresses.rs +++ b/processor/src/tests/addresses.rs @@ -51,8 +51,7 @@ async fn spend( network.mine_block().await; } match timeout(Duration::from_secs(30), scanner.events.recv()).await.unwrap().unwrap() { - ScannerEvent::Block { key: this_key, block: _, outputs } => { - assert_eq!(this_key, key); + ScannerEvent::Block { block: _, outputs } => { assert_eq!(outputs.len(), 1); // Make sure this is actually a change output assert_eq!(outputs[0].kind(), OutputType::Change); @@ -89,8 +88,7 @@ pub async fn test_addresses(network: N) { // Verify the Scanner picked them up let outputs = match timeout(Duration::from_secs(30), scanner.events.recv()).await.unwrap().unwrap() { - ScannerEvent::Block { key: this_key, block, outputs } => { - assert_eq!(this_key, key); + ScannerEvent::Block { block, outputs } => { assert_eq!(block, block_id); assert_eq!(outputs.len(), 1); assert_eq!(outputs[0].kind(), OutputType::Branch); diff --git a/processor/src/tests/scanner.rs b/processor/src/tests/scanner.rs index 4f20ed0a..45d7abef 100644 --- a/processor/src/tests/scanner.rs +++ b/processor/src/tests/scanner.rs @@ -55,8 +55,7 @@ pub async fn test_scanner(network: N) { let verify_event = |mut scanner: ScannerHandle| async { let outputs = match timeout(Duration::from_secs(30), scanner.events.recv()).await.unwrap().unwrap() { - ScannerEvent::Block { key, block, outputs } => { - assert_eq!(key, keys.group_key()); + ScannerEvent::Block { block, outputs } => { assert_eq!(block, block_id); assert_eq!(outputs.len(), 1); assert_eq!(outputs[0].kind(), OutputType::External); diff --git a/processor/src/tests/wallet.rs b/processor/src/tests/wallet.rs index 3eebe6fc..8b22685c 100644 --- a/processor/src/tests/wallet.rs +++ b/processor/src/tests/wallet.rs @@ -36,8 +36,7 @@ pub async fn test_wallet(network: N) { let block_id = block.id(); match timeout(Duration::from_secs(30), scanner.events.recv()).await.unwrap().unwrap() { - ScannerEvent::Block { key: this_key, block, outputs } => { - assert_eq!(this_key, key); + ScannerEvent::Block { block, outputs } => { assert_eq!(block, block_id); assert_eq!(outputs.len(), 1); (block_id, outputs) @@ -109,8 +108,7 @@ pub async fn test_wallet(network: N) { } match timeout(Duration::from_secs(30), scanner.events.recv()).await.unwrap().unwrap() { - ScannerEvent::Block { key: this_key, block: block_id, outputs: these_outputs } => { - assert_eq!(this_key, key); + ScannerEvent::Block { block: block_id, outputs: these_outputs } => { assert_eq!(block_id, block.id()); assert_eq!(these_outputs, outputs); } diff --git a/tests/processor/src/tests/batch.rs b/tests/processor/src/tests/batch.rs index 41e0adf3..a843cb99 100644 --- a/tests/processor/src/tests/batch.rs +++ b/tests/processor/src/tests/batch.rs @@ -122,11 +122,8 @@ pub(crate) async fn sign_batch( if preprocesses.contains_key(&i) { match coordinator.recv_message().await { messages::ProcessorMessage::Substrate(messages::substrate::ProcessorMessage::Update { - key, batch: this_batch, }) => { - assert_eq!(key.as_slice(), &id.key); - if batch.is_none() { assert!(PublicKey::from_raw(id.key.clone().try_into().unwrap()) .verify(&batch_message(&this_batch.batch), &this_batch.signature));