From 1384084dd152d0c72bab68f48c3f055aa05ab339 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 23 Feb 2024 15:07:13 -0500 Subject: [PATCH] Do not trigger DKG if we have an approved DKG key set in the contract Signed-off-by: Jacinta Ferrant --- .github/workflows/bitcoin-tests.yml | 4 +- stacks-signer/src/coordinator.rs | 7 +- stacks-signer/src/runloop.rs | 12 ++-- stacks-signer/src/signer.rs | 58 ++++++++------- testnet/stacks-node/src/tests/signer.rs | 96 +++++++++++++++---------- 5 files changed, 101 insertions(+), 76 deletions(-) diff --git a/.github/workflows/bitcoin-tests.yml b/.github/workflows/bitcoin-tests.yml index c618172fd..e2447a360 100644 --- a/.github/workflows/bitcoin-tests.yml +++ b/.github/workflows/bitcoin-tests.yml @@ -76,9 +76,9 @@ jobs: - tests::nakamoto_integrations::block_proposal_api_endpoint - tests::nakamoto_integrations::miner_writes_proposed_block_to_stackerdb - tests::nakamoto_integrations::correct_burn_outs - - tests::signer::stackerdb_dkg_sign + - tests::signer::stackerdb_dkg + - tests::signer::stackerdb_sign - tests::signer::stackerdb_block_proposal - - tests::signer::stackerdb_reward_cycle_transitions - tests::signer::stackerdb_filter_bad_transactions steps: ## Setup test environment diff --git a/stacks-signer/src/coordinator.rs b/stacks-signer/src/coordinator.rs index e0c6c0f7d..2c23fd0b3 100644 --- a/stacks-signer/src/coordinator.rs +++ b/stacks-signer/src/coordinator.rs @@ -89,7 +89,12 @@ impl CoordinatorSelector { } new_index } else { - self.coordinator_index.saturating_add(1) % self.coordinator_ids.len() + let mut new_index = self.coordinator_index.saturating_add(1); + if new_index == self.coordinator_ids.len() { + // We have exhausted all potential coordinators. Go back to the start + new_index = 0; + } + new_index }; self.coordinator_id = *self .coordinator_ids diff --git a/stacks-signer/src/runloop.rs b/stacks-signer/src/runloop.rs index a2f710c60..8dab88d01 100644 --- a/stacks-signer/src/runloop.rs +++ b/stacks-signer/src/runloop.rs @@ -194,11 +194,13 @@ impl RunLoop { signer.coordinator.state = CoordinatorState::Idle; signer.state = SignerState::Idle; } - retry_with_exponential_backoff(|| { - signer - .update_dkg(&self.stacks_client, current_reward_cycle) - .map_err(backoff::Error::transient) - })?; + if signer.approved_aggregate_public_key.is_none() { + retry_with_exponential_backoff(|| { + signer + .update_dkg(&self.stacks_client, current_reward_cycle) + .map_err(backoff::Error::transient) + })?; + } } if self.stacks_signers.is_empty() { info!("Signer is not registered for the current {current_reward_cycle} or next {next_reward_cycle} reward cycles. Waiting for confirmed registration..."); diff --git a/stacks-signer/src/signer.rs b/stacks-signer/src/signer.rs index 6541bc05b..6247f83a5 100644 --- a/stacks-signer/src/signer.rs +++ b/stacks-signer/src/signer.rs @@ -144,6 +144,8 @@ pub struct Signer { pub tx_fee_ustx: u64, /// The coordinator info for the signer pub coordinator_selector: CoordinatorSelector, + /// The approved key registered to the contract + pub approved_aggregate_public_key: Option, } impl From for Signer { @@ -212,6 +214,7 @@ impl From for Signer { reward_cycle: signer_config.reward_cycle, tx_fee_ustx: signer_config.tx_fee_ustx, coordinator_selector, + approved_aggregate_public_key: None, } } } @@ -233,7 +236,11 @@ impl Signer { fn execute_command(&mut self, stacks_client: &StacksClient, command: &Command) { match command { Command::Dkg => { - //TODO: check if we already have an aggregate key stored in the contract. + if self.approved_aggregate_public_key.is_some() { + // We do not enforce a block contain any transactions except the aggregate votes when it is NOT already set + debug!("Signer #{}: Already have an aggregate key for reward cycle {}. Ignoring DKG command.", self.signer_id, self.reward_cycle); + return; + } // If we do, we should not start a new DKG let vote_round = match retry_with_exponential_backoff(|| { stacks_client @@ -270,6 +277,11 @@ impl Signer { is_taproot, merkle_root, } => { + if self.approved_aggregate_public_key.is_none() { + // We cannot sign a block if we do not have an approved aggregate public key + debug!("Signer #{}: Cannot sign a block without an approved aggregate public key. Ignore it.", self.signer_id); + return; + } let signer_signature_hash = block.header.signer_signature_hash(); let block_info = self .blocks @@ -624,14 +636,9 @@ impl Signer { block: &NakamotoBlock, current_reward_cycle: u64, ) -> bool { - let aggregate_key = retry_with_exponential_backoff(|| { - stacks_client - .get_approved_aggregate_key(self.reward_cycle) - .map_err(backoff::Error::transient) - }) - .unwrap_or(None); - if aggregate_key.is_some() { + if self.approved_aggregate_public_key.is_some() { // We do not enforce a block contain any transactions except the aggregate votes when it is NOT already set + // TODO: should be only allow special cased transactions during prepare phase before a key is set? debug!("Signer #{}: Already have an aggregate key for reward cycle {}. Skipping transaction verification...", self.signer_id, self.reward_cycle); return true; } @@ -1064,12 +1071,6 @@ impl Signer { current_reward_cycle: u64, ) -> Result<(), ClientError> { let txid = new_transaction.txid(); - let aggregate_key = retry_with_exponential_backoff(|| { - stacks_client - .get_approved_aggregate_key(self.reward_cycle) - .map_err(backoff::Error::transient) - }) - .unwrap_or(None); if epoch > StacksEpochId::Epoch30 { debug!("Signer #{}: Received a DKG result while in epoch 3.0. Broadcast the transaction only to stackerDB.", self.signer_id); } else if epoch == StacksEpochId::Epoch25 { @@ -1086,7 +1087,7 @@ impl Signer { // For all Pox-4 epochs onwards, broadcast the results also to stackerDB for other signers/miners to observe // TODO: Should we even store transactions if not in prepare phase? Should the miner just ignore all signer transactions if not in prepare phase? let txid = new_transaction.txid(); - let new_transactions = if aggregate_key.is_some() { + let new_transactions = if self.approved_aggregate_public_key.is_some() { // We do not enforce a block contain any transactions except the aggregate votes when it is NOT already set info!( "Signer #{}: Already has an aggregate key for reward cycle {}. Do not broadcast the transaction ({txid:?}).", @@ -1233,26 +1234,23 @@ impl Signer { current_reward_cycle: u64, ) -> Result<(), ClientError> { let reward_cycle = self.reward_cycle; - let new_aggregate_public_key = stacks_client.get_approved_aggregate_key(reward_cycle)?; - let old_aggregate_public_key = self.coordinator.get_aggregate_public_key(); - if new_aggregate_public_key.is_some() - && old_aggregate_public_key != new_aggregate_public_key - { + self.approved_aggregate_public_key = + stacks_client.get_approved_aggregate_key(reward_cycle)?; + if self.approved_aggregate_public_key.is_some() { // TODO: this will never work as is. We need to have stored our party shares on the side etc for this particular aggregate key. // Need to update state to store the necessary info, check against it to see if we have participated in the winning round and // then overwrite our value accordingly. Otherwise, we will be locked out of the round and should not participate. - debug!( - "Signer #{}: Received a new aggregate public key ({new_aggregate_public_key:?}) for reward cycle {reward_cycle}. Overwriting its internal aggregate key ({old_aggregate_public_key:?})", - self.signer_id - ); self.coordinator - .set_aggregate_public_key(new_aggregate_public_key); - } + .set_aggregate_public_key(self.approved_aggregate_public_key); + // We have an approved aggregate public key. Do nothing further + debug!( + "Signer #{}: Have updated DKG value to {:?}.", + self.signer_id, self.approved_aggregate_public_key + ); + return Ok(()); + }; let coordinator_id = self.coordinator_selector.get_coordinator().0; - if new_aggregate_public_key.is_none() - && self.signer_id == coordinator_id - && self.state == State::Idle - { + if self.signer_id == coordinator_id && self.state == State::Idle { debug!( "Signer #{}: Checking if old transactions exist", self.signer_id diff --git a/testnet/stacks-node/src/tests/signer.rs b/testnet/stacks-node/src/tests/signer.rs index 8a3798eea..bb58956d4 100644 --- a/testnet/stacks-node/src/tests/signer.rs +++ b/testnet/stacks-node/src/tests/signer.rs @@ -761,8 +761,59 @@ fn setup_stx_btc_node( #[test] #[ignore] /// Test the signer can respond to external commands to perform DKG -/// and sign a block with both taproot and non-taproot signatures -fn stackerdb_dkg_sign() { +fn stackerdb_dkg() { + if env::var("BITCOIND_TEST") != Ok("1".into()) { + return; + } + + tracing_subscriber::registry() + .with(fmt::layer()) + .with(EnvFilter::from_default_env()) + .init(); + + info!("------------------------- Test Setup -------------------------"); + let timeout = Duration::from_secs(200); + let mut signer_test = SignerTest::new(10); + info!("Boot to epoch 3.0 reward calculation..."); + boot_to_epoch_3_reward_set( + &signer_test.running_nodes.conf, + &signer_test.running_nodes.blocks_processed, + &signer_test.signer_stacks_private_keys, + &signer_test.signer_stacks_private_keys, + &mut signer_test.running_nodes.btc_regtest_controller, + ); + + info!("Pox 4 activated and at epoch 3.0 reward set calculation (2nd block of its prepare phase)! Ready for signers to perform DKG and Sign!"); + // First wait for the automatically triggered DKG to complete + let key = signer_test.wait_for_dkg(timeout); + + info!("------------------------- Test DKG -------------------------"); + let reward_cycle = signer_test.get_current_reward_cycle().saturating_add(1); + let coordinator_sender = signer_test.get_coordinator_sender(reward_cycle); + + // Determine the coordinator of the current node height + info!("signer_runloop: spawn send commands to do dkg"); + let dkg_now = Instant::now(); + coordinator_sender + .send(RunLoopCommand { + reward_cycle, + command: SignerCommand::Dkg, + }) + .expect("failed to send DKG command"); + let new_key = signer_test.wait_for_dkg(timeout); + let dkg_elapsed = dkg_now.elapsed(); + assert_ne!(new_key, key); + + info!("DKG Time Elapsed: {:.2?}", dkg_elapsed); + // TODO: look into this. Cannot get this to NOT hang unless I wait a bit... + std::thread::sleep(Duration::from_secs(1)); + signer_test.shutdown(); +} + +#[test] +#[ignore] +/// Test the signer can respond to external commands to perform DKG +fn stackerdb_sign() { if env::var("BITCOIND_TEST") != Ok("1".into()) { return; } @@ -811,41 +862,12 @@ fn stackerdb_dkg_sign() { let timeout = Duration::from_secs(200); let mut signer_test = SignerTest::new(10); - info!("Boot to epoch 3.0 reward calculation..."); - boot_to_epoch_3_reward_set( - &signer_test.running_nodes.conf, - &signer_test.running_nodes.blocks_processed, - &signer_test.signer_stacks_private_keys, - &signer_test.signer_stacks_private_keys, - &mut signer_test.running_nodes.btc_regtest_controller, - ); - - info!("Pox 4 activated and at epoch 3.0 reward set calculation (2nd block of its prepare phase)! Ready for signers to perform DKG and Sign!"); - - // First wait for the automatically triggered DKG to complete - let key = signer_test.wait_for_dkg(timeout); - - info!("------------------------- Test DKG -------------------------"); - - // We are voting for the NEXT reward cycle hence the + 1; - let reward_cycle = signer_test.get_current_reward_cycle().saturating_add(1); - let coordinator_sender = signer_test.get_coordinator_sender(reward_cycle); - - let dkg_now = Instant::now(); - coordinator_sender - .send(RunLoopCommand { - reward_cycle, - command: SignerCommand::Dkg, - }) - .expect("failed to send DKG command"); - let new_key = signer_test.wait_for_dkg(timeout); - let dkg_elapsed = dkg_now.elapsed(); - assert_ne!(new_key, key); + let key = signer_test.boot_to_epoch_3(timeout); info!("------------------------- Test Sign -------------------------"); - + let reward_cycle = signer_test.get_current_reward_cycle(); // Determine the coordinator of the current node height - info!("signer_runloop: spawn send commands to do dkg and then sign"); + info!("signer_runloop: spawn send commands to do sign"); let sign_command = RunLoopCommand { reward_cycle, command: SignerCommand::Sign { @@ -874,10 +896,10 @@ fn stackerdb_dkg_sign() { let schnorr_proofs = signer_test.wait_for_taproot_signatures(timeout); for frost_signature in frost_signatures { - assert!(frost_signature.verify(&new_key, &msg)); + assert!(frost_signature.verify(&key, &msg)); } for schnorr_proof in schnorr_proofs { - let tweaked_key = tweaked_public_key(&new_key, None); + let tweaked_key = tweaked_public_key(&key, None); assert!( schnorr_proof.verify(&tweaked_key.x(), &msg), "Schnorr proof verification failed" @@ -923,8 +945,6 @@ fn stackerdb_dkg_sign() { } else { panic!("Received unexpected message: {:?}", &signer_message); } - - info!("DKG Time Elapsed: {:.2?}", dkg_elapsed); info!("Sign Time Elapsed: {:.2?}", sign_elapsed); signer_test.shutdown(); }