From eb16d549e00933d28b9f0064a2338ad1a6b5aa2a Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Mon, 20 Jan 2025 13:02:25 +0000 Subject: [PATCH 01/14] move processing_delay to request_decider. Retry sending decisions --- signer/src/config/mod.rs | 7 ++ signer/src/main.rs | 1 + signer/src/message.rs | 12 ++++ signer/src/request_decider.rs | 43 ++++++++++++ signer/src/storage/in_memory.rs | 31 +++++++++ signer/src/storage/mod.rs | 9 +++ signer/src/storage/postgres.rs | 58 +++++++++++++++-- signer/src/testing/request_decider.rs | 7 ++ signer/src/transaction_coordinator.rs | 6 -- signer/tests/integration/postgres.rs | 65 +++++++++++++++++++ signer/tests/integration/request_decider.rs | 5 ++ .../integration/transaction_coordinator.rs | 3 + 12 files changed, 236 insertions(+), 11 deletions(-) diff --git a/signer/src/config/mod.rs b/signer/src/config/mod.rs index f2527fcc3..b58966255 100644 --- a/signer/src/config/mod.rs +++ b/signer/src/config/mod.rs @@ -229,6 +229,9 @@ pub struct SignerConfig { /// How many bitcoin blocks back from the chain tip the signer will /// look for requests. pub context_window: u16, + /// How many bitcoin blocks back from the chain tip the signer will + /// look for deposit decisions to retry to propagate. + pub deposit_decisions_retry_window: u16, /// The maximum duration of a signing round before the coordinator will /// time out and return an error. #[serde(deserialize_with = "duration_seconds_deserializer")] @@ -397,6 +400,7 @@ impl Settings { // after https://github.com/stacks-network/sbtc/issues/1004 gets // done. cfg_builder = cfg_builder.set_default("signer.context_window", 1000)?; + cfg_builder = cfg_builder.set_default("signer.deposit_decisions_retry_window", 3)?; cfg_builder = cfg_builder.set_default("signer.dkg_max_duration", 120)?; cfg_builder = cfg_builder.set_default("signer.bitcoin_presign_request_max_duration", 30)?; cfg_builder = cfg_builder.set_default("signer.signer_round_max_duration", 30)?; @@ -525,6 +529,7 @@ mod tests { assert_eq!(settings.signer.sbtc_bitcoin_start_height, Some(101)); assert_eq!(settings.signer.bootstrap_signatures_required, 2); assert_eq!(settings.signer.context_window, 1000); + assert_eq!(settings.signer.deposit_decisions_retry_window, 3); assert!(settings.signer.prometheus_exporter_endpoint.is_none()); assert_eq!( settings.signer.bitcoin_presign_request_max_duration, @@ -831,6 +836,7 @@ mod tests { .remove(parameter); }; remove_parameter("context_window"); + remove_parameter("deposit_decisions_retry_window"); remove_parameter("signer_round_max_duration"); remove_parameter("bitcoin_presign_request_max_duration"); remove_parameter("dkg_max_duration"); @@ -843,6 +849,7 @@ mod tests { let settings = Settings::new(Some(&new_config.path())).unwrap(); assert_eq!(settings.signer.context_window, 1000); + assert_eq!(settings.signer.deposit_decisions_retry_window, 3); assert_eq!( settings.signer.bitcoin_presign_request_max_duration, Duration::from_secs(30) diff --git a/signer/src/main.rs b/signer/src/main.rs index b407b994b..f52f8e57a 100644 --- a/signer/src/main.rs +++ b/signer/src/main.rs @@ -366,6 +366,7 @@ async fn run_request_decider(ctx: impl Context) -> Result<(), Error> { network, context: ctx.clone(), context_window: config.signer.context_window, + deposit_decisions_retry_window: config.signer.deposit_decisions_retry_window, blocklist_checker: BlocklistClient::new(&ctx), signer_private_key: config.signer.private_key, }; diff --git a/signer/src/message.rs b/signer/src/message.rs index e7f0eeaa3..26f7631fe 100644 --- a/signer/src/message.rs +++ b/signer/src/message.rs @@ -7,6 +7,7 @@ use crate::bitcoin::validation::TxRequestIds; use crate::keys::PublicKey; use crate::stacks::contracts::ContractCall; use crate::stacks::contracts::StacksTx; +use crate::storage::model; use crate::storage::model::BitcoinBlockHash; use crate::storage::model::StacksTxId; @@ -139,6 +140,17 @@ pub struct SignerDepositDecision { pub can_sign: bool, } +impl From for SignerDepositDecision { + fn from(signer: model::DepositSigner) -> Self { + Self { + txid: signer.txid.into(), + output_index: signer.output_index, + can_accept: signer.can_accept, + can_sign: signer.can_sign, + } + } +} + /// Represents a decision related to signer withdrawal. #[derive(Debug, Clone, PartialEq)] #[cfg_attr(feature = "testing", derive(fake::Dummy))] diff --git a/signer/src/request_decider.rs b/signer/src/request_decider.rs index 6c22babe9..940bb1b47 100644 --- a/signer/src/request_decider.rs +++ b/signer/src/request_decider.rs @@ -5,6 +5,8 @@ //! //! For more details, see the [`RequestDeciderEventLoop`] documentation. +use std::time::Duration; + use crate::block_observer::BlockObserver; use crate::blocklist_client::BlocklistChecker; use crate::context::Context; @@ -48,6 +50,9 @@ pub struct RequestDeciderEventLoop { pub signer_private_key: PrivateKey, /// How many bitcoin blocks back from the chain tip the signer will look for requests. pub context_window: u16, + /// How many bitcoin blocks back from the chain tip the signer will look for deposit + /// decisions to retry to propagate. + pub deposit_decisions_retry_window: u16, } /// This function defines which messages this event loop is interested @@ -118,6 +123,12 @@ where #[tracing::instrument(skip_all, fields(chain_tip = tracing::field::Empty))] async fn handle_new_requests(&mut self) -> Result<(), Error> { + let bitcoin_processing_delay = self.context.config().signer.bitcoin_processing_delay; + if bitcoin_processing_delay > Duration::ZERO { + tracing::debug!("sleeping before processing new requests"); + tokio::time::sleep(bitcoin_processing_delay).await; + } + let db = self.context.get_storage(); let chain_tip = db .get_bitcoin_canonical_chain_tip() @@ -129,6 +140,23 @@ where let span = tracing::Span::current(); span.record("chain_tip", tracing::field::display(chain_tip)); + // We retry the deposit decisions because some signers' bitcoin nodes might have + // been running behind and ignored the previous messages. + let deposit_decisions_to_retry = db + .get_deposit_signer_decisions( + &chain_tip, + self.deposit_decisions_retry_window, + &signer_public_key, + ) + .await?; + + let _ = self + .handle_deposit_decisions_to_retry(deposit_decisions_to_retry, &chain_tip) + .await + .inspect_err( + |error| tracing::warn!(%error, "error handling deposit decisions to retry"), + ); + let deposit_requests = db .get_pending_deposit_requests(&chain_tip, self.context_window, &signer_public_key) .await?; @@ -248,6 +276,20 @@ where Ok(()) } + /// Send the given deposit decisions to the other signers for redundancy. + #[tracing::instrument(skip_all)] + pub async fn handle_deposit_decisions_to_retry( + &mut self, + decisions: Vec, + chain_tip: &BitcoinBlockHash, + ) -> Result<(), Error> { + for decision in decisions.into_iter().map(SignerDepositDecision::from) { + self.send_message(decision, chain_tip).await?; + } + + Ok(()) + } + #[tracing::instrument(skip_all)] async fn handle_pending_withdrawal_request( &mut self, @@ -480,6 +522,7 @@ mod tests { testing::request_decider::TestEnvironment { context, context_window: 6, + deposit_decisions_retry_window: 1, num_signers: 7, signing_threshold: 5, test_model_parameters, diff --git a/signer/src/storage/in_memory.rs b/signer/src/storage/in_memory.rs index 7d0dae4e4..81476cba4 100644 --- a/signer/src/storage/in_memory.rs +++ b/signer/src/storage/in_memory.rs @@ -908,6 +908,37 @@ impl super::DbRead for SharedStore { .get(sighash) .map(|s| (s.will_sign, s.aggregate_key))) } + + async fn get_deposit_signer_decisions( + &self, + chain_tip: &model::BitcoinBlockHash, + context_window: u16, + signer_public_key: &PublicKey, + ) -> Result, Error> { + let store = self.lock().await; + let deposit_requests = store.get_deposit_requests(chain_tip, context_window); + let voted: HashSet<(model::BitcoinTxId, u32)> = store + .signer_to_deposit_request + .get(signer_public_key) + .cloned() + .unwrap_or(Vec::new()) + .into_iter() + .collect(); + + let result = deposit_requests + .into_iter() + .filter(|x| voted.contains(&(x.txid, x.output_index))) + .flat_map(|req| { + store + .deposit_request_to_signers + .get(&(req.txid, req.output_index)) + .cloned() + .unwrap_or_default() + }) + .collect(); + + Ok(result) + } } impl super::DbWrite for SharedStore { diff --git a/signer/src/storage/mod.rs b/signer/src/storage/mod.rs index 9b8af4e42..f8881af3a 100644 --- a/signer/src/storage/mod.rs +++ b/signer/src/storage/mod.rs @@ -124,6 +124,15 @@ pub trait DbRead { output_index: u32, ) -> impl Future, Error>> + Send; + /// Get all the deposit decisions for the given signer in the given window + /// of blocks. + fn get_deposit_signer_decisions( + &self, + chain_tip: &model::BitcoinBlockHash, + context_window: u16, + signer_public_key: &PublicKey, + ) -> impl Future, Error>> + Send; + /// Returns whether the given `signer_public_key` can provide signature /// shares for the deposit transaction. /// diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 0bae1bd1d..a78498d82 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -488,7 +488,7 @@ impl PgStore { JOIN sbtc_signer.bitcoin_transactions AS bt USING (txid) WHERE prevout_type = 'signers_input' ) - SELECT + SELECT bo.txid , bb.block_height FROM sbtc_signer.bitcoin_tx_outputs AS bo @@ -1796,7 +1796,7 @@ impl super::DbRead for PgStore { sqlx::query_as::<_, model::SweptDepositRequest>( " WITH RECURSIVE bitcoin_blockchain AS ( - SELECT + SELECT block_hash , block_height FROM bitcoin_blockchain_of($1, $2) @@ -1810,9 +1810,9 @@ impl super::DbRead for PgStore { JOIN bitcoin_blockchain as bb ON bb.block_hash = stacks_blocks.bitcoin_anchor WHERE stacks_blocks.block_hash = $3 - + UNION ALL - + SELECT parent.block_hash , parent.block_height @@ -1841,7 +1841,7 @@ impl super::DbRead for PgStore { LEFT JOIN completed_deposit_events AS cde ON cde.bitcoin_txid = deposit_req.txid AND cde.output_index = deposit_req.output_index - LEFT JOIN stacks_blockchain AS sb + LEFT JOIN stacks_blockchain AS sb ON sb.block_hash = cde.block_hash GROUP BY bc_trx.txid @@ -1921,6 +1921,54 @@ impl super::DbRead for PgStore { .await .map_err(Error::SqlxQuery) } + + async fn get_deposit_signer_decisions( + &self, + chain_tip: &model::BitcoinBlockHash, + context_window: u16, + signer_public_key: &PublicKey, + ) -> Result, Error> { + sqlx::query_as::<_, model::DepositSigner>( + r#" + WITH RECURSIVE context_window AS ( + -- Anchor member: Initialize the recursion with the chain tip + SELECT block_hash, block_height, parent_hash, created_at, 1 AS depth + FROM sbtc_signer.bitcoin_blocks + WHERE block_hash = $1 + + UNION ALL + + -- Recursive member: Fetch the parent block using the last block's parent_hash + SELECT parent.block_hash, parent.block_height, parent.parent_hash, + parent.created_at, last.depth + 1 + FROM sbtc_signer.bitcoin_blocks parent + JOIN context_window last ON parent.block_hash = last.parent_hash + WHERE last.depth < $2 + ), + transactions_in_window AS ( + SELECT transactions.txid + FROM context_window blocks_in_window + JOIN sbtc_signer.bitcoin_transactions transactions ON + transactions.block_hash = blocks_in_window.block_hash + ) + SELECT + deposit_signers.txid + , deposit_signers.output_index + , deposit_signers.signer_pub_key + , deposit_signers.can_sign + , deposit_signers.can_accept + FROM transactions_in_window transactions + JOIN sbtc_signer.deposit_signers USING (txid) + WHERE deposit_signers.signer_pub_key = $3 + "#, + ) + .bind(chain_tip) + .bind(i32::from(context_window)) + .bind(signer_public_key) + .fetch_all(&self.0) + .await + .map_err(Error::SqlxQuery) + } } impl super::DbWrite for PgStore { diff --git a/signer/src/testing/request_decider.rs b/signer/src/testing/request_decider.rs index a44e21241..c6fa4b07c 100644 --- a/signer/src/testing/request_decider.rs +++ b/signer/src/testing/request_decider.rs @@ -42,6 +42,7 @@ impl RequestDeciderEventLoopHarness { context: C, network: SignerNetwork, context_window: u16, + deposit_decisions_retry_window: u16, signer_private_key: PrivateKey, ) -> Self { Self { @@ -51,6 +52,7 @@ impl RequestDeciderEventLoopHarness { blocklist_checker: Some(()), signer_private_key, context_window, + deposit_decisions_retry_window, }, context, } @@ -122,6 +124,8 @@ pub struct TestEnvironment { pub context: C, /// Bitcoin context window pub context_window: u16, + /// Deposit decisions retry window + pub deposit_decisions_retry_window: u16, /// Num signers pub num_signers: usize, /// Signing threshold @@ -156,6 +160,7 @@ where self.context.clone(), signer_network, self.context_window, + self.deposit_decisions_retry_window, coordinator_signer_info.signer_private_key, ); @@ -238,6 +243,7 @@ where self.context.clone(), signer_network, self.context_window, + self.deposit_decisions_retry_window, coordinator_signer_info.signer_private_key, ); @@ -315,6 +321,7 @@ where ctx, net, self.context_window, + self.deposit_decisions_retry_window, signer_info.signer_private_key, ); diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 8eada8180..c15b3fcee 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -265,12 +265,6 @@ where return Ok(()); } - let bitcoin_processing_delay = self.context.config().signer.bitcoin_processing_delay; - if bitcoin_processing_delay > Duration::ZERO { - tracing::debug!("sleeping before processing new bitcoin block"); - tokio::time::sleep(bitcoin_processing_delay).await; - } - let bitcoin_chain_tip = self .context .get_storage() diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index 7fb7d4de4..103235426 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -1400,6 +1400,71 @@ async fn fetching_deposit_request_votes() { signer::testing::storage::drop_db(store).await; } +#[cfg_attr(not(feature = "integration-tests"), ignore)] +#[tokio::test] +async fn fetching_deposit_signer_decisions() { + let pg_store = testing::storage::new_test_database().await; + let mut rng = rand::rngs::StdRng::seed_from_u64(51); + + // This is just a sql test, where we use the `TestData` struct to help + // populate the database with test data. We set all the other + // unnecessary parameters to zero. + let num_signers = 3; + let test_model_params = testing::storage::model::Params { + num_bitcoin_blocks: 5, + num_stacks_blocks_per_bitcoin_block: 0, + num_deposit_requests_per_block: 1, + num_withdraw_requests_per_block: 0, + num_signers_per_request: num_signers, + }; + + let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); + + let test_data = TestData::generate(&mut rng, &signer_set, &test_model_params); + test_data.write_to(&pg_store).await; + + let chain_tip = test_data + .bitcoin_blocks + .iter() + .max_by_key(|x| (x.block_height, x.block_hash)) + .unwrap(); + + for signer_pub_key in signer_set.iter() { + for deposit in test_data.deposit_requests.iter() { + let decision = model::DepositSigner { + txid: deposit.txid, + output_index: deposit.output_index, + signer_pub_key: *signer_pub_key, + can_accept: true, + can_sign: true, + }; + pg_store + .write_deposit_signer_decision(&decision) + .await + .unwrap(); + } + } + + let signer_pub_key = signer_set.first().unwrap(); + + let deposit_decisions = pg_store + .get_deposit_signer_decisions(&chain_tip.block_hash, 3, signer_pub_key) + .await + .unwrap(); + + assert_eq!(deposit_decisions.len(), 3); + for deposit in test_data.deposit_requests[3..].iter() { + assert!(deposit_decisions + .iter() + .find(|decision| { + decision.txid == deposit.txid && decision.output_index == deposit.output_index + }) + .is_some()); + } + + signer::testing::storage::drop_db(pg_store).await; +} + /// For this test we check that when we get the votes for a withdrawal /// request for a specific aggregate key, that we get a vote for all public /// keys for the specific aggregate key. This includes "implicit" votes diff --git a/signer/tests/integration/request_decider.rs b/signer/tests/integration/request_decider.rs index ee8397cf4..8609ab56f 100644 --- a/signer/tests/integration/request_decider.rs +++ b/signer/tests/integration/request_decider.rs @@ -40,6 +40,7 @@ fn test_environment( >, > { let context_window = 6; + let deposit_decisions_retry_window = 1; let test_model_parameters = testing::storage::model::Params { num_bitcoin_blocks: 20, @@ -58,6 +59,7 @@ fn test_environment( context, num_signers, context_window, + deposit_decisions_retry_window, signing_threshold, test_model_parameters, } @@ -170,6 +172,7 @@ async fn handle_pending_deposit_request_address_script_pub_key() { network: network.connect(), context: ctx.clone(), context_window: 10000, + deposit_decisions_retry_window: 1, blocklist_checker: Some(()), signer_private_key: setup.aggregated_signer.keypair.secret_key().into(), }; @@ -255,6 +258,7 @@ async fn handle_pending_deposit_request_not_in_signing_set() { network: network.connect(), context: ctx.clone(), context_window: 10000, + deposit_decisions_retry_window: 1, blocklist_checker: Some(()), // We generate a new private key here so that we know (with very // high probability) that this signer is not in the signer set. @@ -334,6 +338,7 @@ async fn persist_received_deposit_decision_fetches_missing_deposit_requests() { network: network.spawn(), context: ctx.clone(), context_window: 10000, + deposit_decisions_retry_window: 1, blocklist_checker: Some(()), signer_private_key: PrivateKey::new(&mut rng), }; diff --git a/signer/tests/integration/transaction_coordinator.rs b/signer/tests/integration/transaction_coordinator.rs index b59e1296b..3b19a3252 100644 --- a/signer/tests/integration/transaction_coordinator.rs +++ b/signer/tests/integration/transaction_coordinator.rs @@ -1584,6 +1584,7 @@ async fn sign_bitcoin_transaction() { network: network.spawn(), context: ctx.clone(), context_window: 10000, + deposit_decisions_retry_window: 1, blocklist_checker: Some(()), signer_private_key: kp.secret_key().into(), }; @@ -2026,6 +2027,7 @@ async fn sign_bitcoin_transaction_multiple_locking_keys() { network: network.spawn(), context: ctx.clone(), context_window: 10000, + deposit_decisions_retry_window: 1, blocklist_checker: Some(()), signer_private_key: kp.secret_key().into(), }; @@ -2616,6 +2618,7 @@ async fn skip_smart_contract_deployment_and_key_rotation_if_up_to_date() { network: network.spawn(), context: ctx.clone(), context_window: 10000, + deposit_decisions_retry_window: 1, blocklist_checker: Some(()), signer_private_key: kp.secret_key().into(), }; From c1281427d0d44a7d7ce04c0253629e6d493d4fdc Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Wed, 22 Jan 2025 14:29:33 +0000 Subject: [PATCH 02/14] revert back bitcoin_processing_delay --- signer/src/request_decider.rs | 6 ------ signer/src/transaction_coordinator.rs | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/signer/src/request_decider.rs b/signer/src/request_decider.rs index 940bb1b47..0e29ff773 100644 --- a/signer/src/request_decider.rs +++ b/signer/src/request_decider.rs @@ -123,12 +123,6 @@ where #[tracing::instrument(skip_all, fields(chain_tip = tracing::field::Empty))] async fn handle_new_requests(&mut self) -> Result<(), Error> { - let bitcoin_processing_delay = self.context.config().signer.bitcoin_processing_delay; - if bitcoin_processing_delay > Duration::ZERO { - tracing::debug!("sleeping before processing new requests"); - tokio::time::sleep(bitcoin_processing_delay).await; - } - let db = self.context.get_storage(); let chain_tip = db .get_bitcoin_canonical_chain_tip() diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index c15b3fcee..8eada8180 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -265,6 +265,12 @@ where return Ok(()); } + let bitcoin_processing_delay = self.context.config().signer.bitcoin_processing_delay; + if bitcoin_processing_delay > Duration::ZERO { + tracing::debug!("sleeping before processing new bitcoin block"); + tokio::time::sleep(bitcoin_processing_delay).await; + } + let bitcoin_chain_tip = self .context .get_storage() From 68b7cf332471b08c2d1ea4a4718e81edfedceaf3 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Wed, 22 Jan 2025 14:35:50 +0000 Subject: [PATCH 03/14] remove unused import --- signer/src/request_decider.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/signer/src/request_decider.rs b/signer/src/request_decider.rs index 0e29ff773..b65171c3e 100644 --- a/signer/src/request_decider.rs +++ b/signer/src/request_decider.rs @@ -5,8 +5,6 @@ //! //! For more details, see the [`RequestDeciderEventLoop`] documentation. -use std::time::Duration; - use crate::block_observer::BlockObserver; use crate::blocklist_client::BlocklistChecker; use crate::context::Context; From ce69dc16769bef9c976422c550f848ec60859747 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Thu, 23 Jan 2025 14:38:31 +0000 Subject: [PATCH 04/14] address tests comments --- signer/tests/integration/postgres.rs | 41 +++++++++------------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index 103235426..5da789af0 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -1423,43 +1423,28 @@ async fn fetching_deposit_signer_decisions() { let test_data = TestData::generate(&mut rng, &signer_set, &test_model_params); test_data.write_to(&pg_store).await; - let chain_tip = test_data - .bitcoin_blocks - .iter() - .max_by_key(|x| (x.block_height, x.block_hash)) + let chain_tip = pg_store + .get_bitcoin_canonical_chain_tip() + .await + .unwrap() .unwrap(); - for signer_pub_key in signer_set.iter() { - for deposit in test_data.deposit_requests.iter() { - let decision = model::DepositSigner { - txid: deposit.txid, - output_index: deposit.output_index, - signer_pub_key: *signer_pub_key, - can_accept: true, - can_sign: true, - }; - pg_store - .write_deposit_signer_decision(&decision) - .await - .unwrap(); - } - } - let signer_pub_key = signer_set.first().unwrap(); let deposit_decisions = pg_store - .get_deposit_signer_decisions(&chain_tip.block_hash, 3, signer_pub_key) + .get_deposit_signer_decisions(&chain_tip, 3, signer_pub_key) .await .unwrap(); assert_eq!(deposit_decisions.len(), 3); - for deposit in test_data.deposit_requests[3..].iter() { - assert!(deposit_decisions - .iter() - .find(|decision| { - decision.txid == deposit.txid && decision.output_index == deposit.output_index - }) - .is_some()); + // Test data contains 5 deposit requests, we should get decisions for + // the last 3. + for deposit in test_data.deposit_requests[2..].iter() { + assert!(deposit_decisions.iter().any(|decision| { + decision.txid == deposit.txid + && decision.output_index == deposit.output_index + && decision.signer_pub_key == *signer_pub_key + })); } signer::testing::storage::drop_db(pg_store).await; From 5e294aafa312c45f902f73686cf9e10b3b687b51 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Thu, 23 Jan 2025 14:41:48 +0000 Subject: [PATCH 05/14] update new tests --- signer/tests/integration/transaction_coordinator.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/signer/tests/integration/transaction_coordinator.rs b/signer/tests/integration/transaction_coordinator.rs index b5294fe8e..de4a9dcb1 100644 --- a/signer/tests/integration/transaction_coordinator.rs +++ b/signer/tests/integration/transaction_coordinator.rs @@ -3417,6 +3417,7 @@ async fn test_conservative_initial_sbtc_limits() { context_window: 10000, blocklist_checker: Some(()), signer_private_key: kp.secret_key().into(), + deposit_decisions_retry_window: 1, }; let counter = start_count.clone(); tokio::spawn(async move { From f8df970a601ae46ee779ba6025449dbe97d44523 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Mon, 27 Jan 2025 00:18:14 +0000 Subject: [PATCH 06/14] add concatenate_blocks to TestData Params. fix test --- signer/src/api/new_block.rs | 5 ++ signer/src/request_decider.rs | 1 + signer/src/stacks/wallet.rs | 1 + signer/src/storage/postgres.rs | 45 ++++------ signer/src/testing/storage/model.rs | 15 +++- signer/src/transaction_coordinator.rs | 1 + signer/src/transaction_signer.rs | 1 + signer/tests/integration/emily.rs | 1 + signer/tests/integration/postgres.rs | 82 +++++++++++++++++-- signer/tests/integration/request_decider.rs | 1 + signer/tests/integration/rotate_keys.rs | 8 ++ .../integration/transaction_coordinator.rs | 3 + .../tests/integration/transaction_signer.rs | 1 + 13 files changed, 128 insertions(+), 37 deletions(-) diff --git a/signer/src/api/new_block.rs b/signer/src/api/new_block.rs index ae3a971ba..0894bb3d1 100644 --- a/signer/src/api/new_block.rs +++ b/signer/src/api/new_block.rs @@ -667,6 +667,7 @@ mod tests { num_deposit_requests_per_block: 1, num_withdraw_requests_per_block: 1, num_signers_per_request: 0, + concatenate_blocks: true, }; let db = ctx.inner_storage(); let test_data = TestData::generate(&mut rng, &[], &test_params); @@ -739,6 +740,7 @@ mod tests { num_deposit_requests_per_block: 1, num_withdraw_requests_per_block: 1, num_signers_per_request: 0, + concatenate_blocks: true, }; let db = ctx.inner_storage(); let test_data = TestData::generate(&mut rng, &[], &test_params); @@ -787,6 +789,7 @@ mod tests { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 2, num_signers_per_request: 0, + concatenate_blocks: true, }; let db = ctx.inner_storage(); @@ -859,6 +862,7 @@ mod tests { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 2, num_signers_per_request: 0, + concatenate_blocks: true, }; let db = ctx.inner_storage(); @@ -920,6 +924,7 @@ mod tests { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 2, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_params); diff --git a/signer/src/request_decider.rs b/signer/src/request_decider.rs index b65171c3e..f3d31397b 100644 --- a/signer/src/request_decider.rs +++ b/signer/src/request_decider.rs @@ -504,6 +504,7 @@ mod tests { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, + concatenate_blocks: false, }; let context = TestContext::builder() diff --git a/signer/src/stacks/wallet.rs b/signer/src/stacks/wallet.rs index 4f3e5036d..25257c238 100644 --- a/signer/src/stacks/wallet.rs +++ b/signer/src/stacks/wallet.rs @@ -677,6 +677,7 @@ mod tests { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_params); test_data.write_to(&db).await; diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index a78498d82..1a0674eb5 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -737,6 +737,9 @@ impl PgStore { } } +#[cfg(test)] +impl PgStore {} + impl From for PgStore { fn from(value: sqlx::PgPool) -> Self { Self(value) @@ -1930,36 +1933,22 @@ impl super::DbRead for PgStore { ) -> Result, Error> { sqlx::query_as::<_, model::DepositSigner>( r#" - WITH RECURSIVE context_window AS ( - -- Anchor member: Initialize the recursion with the chain tip - SELECT block_hash, block_height, parent_hash, created_at, 1 AS depth - FROM sbtc_signer.bitcoin_blocks - WHERE block_hash = $1 - - UNION ALL - - -- Recursive member: Fetch the parent block using the last block's parent_hash - SELECT parent.block_hash, parent.block_height, parent.parent_hash, - parent.created_at, last.depth + 1 - FROM sbtc_signer.bitcoin_blocks parent - JOIN context_window last ON parent.block_hash = last.parent_hash - WHERE last.depth < $2 - ), - transactions_in_window AS ( - SELECT transactions.txid - FROM context_window blocks_in_window - JOIN sbtc_signer.bitcoin_transactions transactions ON - transactions.block_hash = blocks_in_window.block_hash + WITH target_block AS ( + SELECT blocks.block_hash, blocks.created_at + FROM sbtc_signer.bitcoin_blockchain_of($1, $2) chain + JOIN sbtc_signer.bitcoin_blocks blocks USING (block_hash) + ORDER BY chain.block_height ASC + LIMIT 1 ) SELECT - deposit_signers.txid - , deposit_signers.output_index - , deposit_signers.signer_pub_key - , deposit_signers.can_sign - , deposit_signers.can_accept - FROM transactions_in_window transactions - JOIN sbtc_signer.deposit_signers USING (txid) - WHERE deposit_signers.signer_pub_key = $3 + ds.txid, + ds.output_index, + ds.signer_pub_key, + ds.can_sign, + ds.can_accept + FROM sbtc_signer.deposit_signers ds + WHERE ds.signer_pub_key = $3 + AND ds.created_at >= (SELECT created_at FROM target_block) "#, ) .bind(chain_tip) diff --git a/signer/src/testing/storage/model.rs b/signer/src/testing/storage/model.rs index 41e7a8e61..1425752d7 100644 --- a/signer/src/testing/storage/model.rs +++ b/signer/src/testing/storage/model.rs @@ -51,6 +51,8 @@ pub struct TestData { /// transaction outputs pub tx_outputs: Vec, + + bitcoin_blocks_ref: Vec, } impl TestData { @@ -66,8 +68,14 @@ impl TestData { let mut test_data = Self::new(); for _ in 0..params.num_bitcoin_blocks { - let (next_chunk, _) = test_data.new_block(rng, signer_keys, params, None); + let parent = if params.concatenate_blocks { + test_data.bitcoin_blocks_ref.last() + } else { + None + }; + let (next_chunk, block_ref) = test_data.new_block(rng, signer_keys, params, parent); test_data.push(next_chunk); + test_data.bitcoin_blocks_ref.push(block_ref); } test_data @@ -114,7 +122,7 @@ impl TestData { .collect(); let bitcoin_blocks = vec![block.clone()]; - + let bitcoin_blocks_refs = vec![BitcoinBlockRef::summarize(&block)]; ( Self { bitcoin_blocks, @@ -127,6 +135,7 @@ impl TestData { stacks_transactions: withdraw_data.stacks_transactions, transactions, tx_outputs: Vec::new(), + bitcoin_blocks_ref: bitcoin_blocks_refs, }, block.into(), ) @@ -515,6 +524,8 @@ pub struct Params { pub num_withdraw_requests_per_block: usize, /// The number of signers to hallucinate per request pub num_signers_per_request: usize, + /// Wheter to generate consecutive blocks or not + pub concatenate_blocks: bool, } impl BitcoinBlockRef { diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index c4f6893ed..2b56580b9 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -1840,6 +1840,7 @@ mod tests { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 7, + concatenate_blocks: false, }; let context = TestContext::builder() diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index f3cdf5076..e38409cc5 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -1001,6 +1001,7 @@ mod tests { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, + concatenate_blocks: true, }; let context = TestContext::builder() diff --git a/signer/tests/integration/emily.rs b/signer/tests/integration/emily.rs index d572e3fd9..5620f4698 100644 --- a/signer/tests/integration/emily.rs +++ b/signer/tests/integration/emily.rs @@ -80,6 +80,7 @@ where num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(rng, &signer_keys, &test_model_parameters); test_data.write_to(&storage).await; diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index 09a1dd9fc..cc0643be9 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -14,6 +14,7 @@ use futures::future::join_all; use futures::StreamExt; use rand::seq::IteratorRandom; use rand::seq::SliceRandom; +use time::OffsetDateTime; use signer::bitcoin::validation::DepositConfirmationStatus; use signer::bitcoin::MockBitcoinInteract; @@ -81,6 +82,7 @@ async fn should_be_able_to_query_bitcoin_blocks() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, 7); @@ -326,6 +328,7 @@ async fn should_return_the_same_pending_deposit_requests_as_in_memory_store() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); let test_data = TestData::generate(&mut rng, &signer_set, &test_model_params); @@ -490,6 +493,7 @@ async fn should_return_the_same_pending_withdraw_requests_as_in_memory_store() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 1, num_signers_per_request: 0, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -566,6 +570,7 @@ async fn should_return_the_same_pending_accepted_deposit_requests_as_in_memory_s num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let threshold = 4; @@ -634,6 +639,7 @@ async fn should_return_the_same_pending_accepted_withdraw_requests_as_in_memory_ // the threshold in order for the test to succeed with accepted // requests. num_signers_per_request: num_signers, + concatenate_blocks: false, }; let threshold = 4; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -771,6 +777,7 @@ async fn should_return_only_accepted_pending_deposits_that_are_within_reclaim_bo num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let threshold = 4; @@ -955,6 +962,7 @@ async fn should_return_the_same_last_key_rotation_as_in_memory_store() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 1, num_signers_per_request: 7, + concatenate_blocks: true, }; let num_signers = 7; let threshold = 4; @@ -1381,7 +1389,6 @@ async fn fetching_deposit_request_votes() { signer::testing::storage::drop_db(store).await; } -#[cfg_attr(not(feature = "integration-tests"), ignore)] #[tokio::test] async fn fetching_deposit_signer_decisions() { let pg_store = testing::storage::new_test_database().await; @@ -1397,30 +1404,81 @@ async fn fetching_deposit_signer_decisions() { num_deposit_requests_per_block: 1, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); - let test_data = TestData::generate(&mut rng, &signer_set, &test_model_params); + let mut test_data = TestData::generate(&mut rng, &signer_set, &test_model_params); test_data.write_to(&pg_store).await; + let signer_pub_key = signer_set.first().unwrap(); + + // move the oldest deposit request to the last block + // so that we change its created_at time to be the latest + test_data.deposit_requests.rotate_left(1); + let mut new_time_block = OffsetDateTime::now_utc() - time::Duration::minutes(15); + let mut new_time_request = OffsetDateTime::now_utc() - time::Duration::minutes(12); + // Update Bitcoin blocks + for (block, deposit) in test_data + .bitcoin_blocks + .iter() + .zip(test_data.deposit_requests.iter()) + { + let new_time_block_str = new_time_block + .format(&time::format_description::well_known::Rfc3339) + .unwrap(); + let new_time_request_str = new_time_request + .format(&time::format_description::well_known::Rfc3339) + .unwrap(); + + sqlx::query( + r#" + UPDATE sbtc_signer.bitcoin_blocks + SET created_at = $1::timestamptz + WHERE block_hash = $2 + "#, + ) + .bind(new_time_block_str) // Bind as string + .bind(block.block_hash) + .execute(pg_store.pool()) + .await + .unwrap(); + + sqlx::query( + r#" + UPDATE sbtc_signer.deposit_signers + SET created_at = $1::timestamptz + WHERE txid = $2 AND output_index = $3 AND signer_pub_key = $4 + "#, + ) + .bind(new_time_request_str) // Bind as string + .bind(deposit.txid) + .bind(i32::try_from(deposit.output_index).unwrap()) + .bind(signer_pub_key) + .execute(pg_store.pool()) + .await + .unwrap(); + + new_time_block += time::Duration::minutes(2); + new_time_request += time::Duration::minutes(2); + } + let chain_tip = pg_store .get_bitcoin_canonical_chain_tip() .await .unwrap() .unwrap(); - let signer_pub_key = signer_set.first().unwrap(); - let deposit_decisions = pg_store .get_deposit_signer_decisions(&chain_tip, 3, signer_pub_key) .await .unwrap(); - assert_eq!(deposit_decisions.len(), 3); + assert_eq!(deposit_decisions.len(), 4); // Test data contains 5 deposit requests, we should get decisions for - // the last 3. - for deposit in test_data.deposit_requests[2..].iter() { + // the last 4. + for deposit in test_data.deposit_requests[1..].iter() { assert!(deposit_decisions.iter().any(|decision| { decision.txid == deposit.txid && decision.output_index == deposit.output_index @@ -1559,6 +1617,7 @@ async fn block_in_canonical_bitcoin_blockchain_in_other_block_chain() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -1632,6 +1691,7 @@ async fn we_can_fetch_bitcoin_txs_from_db() { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -1843,6 +1903,7 @@ async fn is_known_bitcoin_block_hash_works() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2298,6 +2359,7 @@ async fn transaction_coordinator_test_environment( num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 7, + concatenate_blocks: false, }; let context = TestContext::builder() @@ -2404,6 +2466,7 @@ async fn deposit_report_with_only_deposit_request() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2503,6 +2566,7 @@ async fn deposit_report_with_deposit_request_reorged() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2577,6 +2641,7 @@ async fn deposit_report_with_deposit_request_spent() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2674,6 +2739,7 @@ async fn deposit_report_with_deposit_request_swept_but_swept_reorged() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2804,6 +2870,7 @@ async fn deposit_report_with_deposit_request_confirmed() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -3040,6 +3107,7 @@ async fn signer_utxo_reorg_suite(desc: ReorgDescription) { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, + concatenate_blocks: true, }; // Let's generate some dummy data and write it into the database. diff --git a/signer/tests/integration/request_decider.rs b/signer/tests/integration/request_decider.rs index 75dec1321..b55d7aaf8 100644 --- a/signer/tests/integration/request_decider.rs +++ b/signer/tests/integration/request_decider.rs @@ -48,6 +48,7 @@ fn test_environment( num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, + concatenate_blocks: false, }; let context = TestContext::builder() diff --git a/signer/tests/integration/rotate_keys.rs b/signer/tests/integration/rotate_keys.rs index 1c5469395..75b031c4d 100644 --- a/signer/tests/integration/rotate_keys.rs +++ b/signer/tests/integration/rotate_keys.rs @@ -174,6 +174,7 @@ async fn rotate_key_validation_happy_path() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -223,6 +224,7 @@ async fn rotate_key_validation_no_dkg() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -260,6 +262,7 @@ async fn rotate_key_validation_wrong_deployer() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -303,6 +306,7 @@ async fn rotate_key_validation_wrong_signing_set() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -352,6 +356,7 @@ async fn rotate_key_validation_wrong_aggregate_key() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -401,6 +406,7 @@ async fn rotate_key_validation_wrong_signatures_required() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -455,6 +461,7 @@ async fn rotate_key_validation_replay() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -493,6 +500,7 @@ async fn rotate_key_validation_replay() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; diff --git a/signer/tests/integration/transaction_coordinator.rs b/signer/tests/integration/transaction_coordinator.rs index 2571f33f0..5fcb8c585 100644 --- a/signer/tests/integration/transaction_coordinator.rs +++ b/signer/tests/integration/transaction_coordinator.rs @@ -818,6 +818,7 @@ async fn get_signer_public_keys_and_aggregate_key_falls_back() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_params); test_data.write_to(&db).await; @@ -926,6 +927,7 @@ async fn run_dkg_from_scratch() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_params); @@ -1143,6 +1145,7 @@ async fn run_subsequent_dkg() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_params); diff --git a/signer/tests/integration/transaction_signer.rs b/signer/tests/integration/transaction_signer.rs index 23029bced..72f008ee4 100644 --- a/signer/tests/integration/transaction_signer.rs +++ b/signer/tests/integration/transaction_signer.rs @@ -83,6 +83,7 @@ async fn get_signer_public_keys_and_aggregate_key_falls_back() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, + concatenate_blocks: true, }; let test_data = TestData::generate(&mut rng, &[], &test_params); test_data.write_to(&db).await; From 7877c2294ecd724a95b142c320064d3a89d7d437 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Mon, 27 Jan 2025 16:59:15 +0000 Subject: [PATCH 07/14] revert to old behaviour for concatenate_blocks --- signer/src/api/new_block.rs | 10 +++---- signer/src/stacks/wallet.rs | 2 +- signer/src/transaction_signer.rs | 2 +- signer/tests/integration/emily.rs | 2 +- signer/tests/integration/postgres.rs | 28 +++++++++---------- signer/tests/integration/rotate_keys.rs | 16 +++++------ .../integration/transaction_coordinator.rs | 6 ++-- .../tests/integration/transaction_signer.rs | 2 +- 8 files changed, 34 insertions(+), 34 deletions(-) diff --git a/signer/src/api/new_block.rs b/signer/src/api/new_block.rs index 0894bb3d1..ccea3c07e 100644 --- a/signer/src/api/new_block.rs +++ b/signer/src/api/new_block.rs @@ -667,7 +667,7 @@ mod tests { num_deposit_requests_per_block: 1, num_withdraw_requests_per_block: 1, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let db = ctx.inner_storage(); let test_data = TestData::generate(&mut rng, &[], &test_params); @@ -740,7 +740,7 @@ mod tests { num_deposit_requests_per_block: 1, num_withdraw_requests_per_block: 1, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let db = ctx.inner_storage(); let test_data = TestData::generate(&mut rng, &[], &test_params); @@ -789,7 +789,7 @@ mod tests { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 2, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let db = ctx.inner_storage(); @@ -862,7 +862,7 @@ mod tests { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 2, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let db = ctx.inner_storage(); @@ -924,7 +924,7 @@ mod tests { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 2, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); diff --git a/signer/src/stacks/wallet.rs b/signer/src/stacks/wallet.rs index 25257c238..f2a2394dd 100644 --- a/signer/src/stacks/wallet.rs +++ b/signer/src/stacks/wallet.rs @@ -677,7 +677,7 @@ mod tests { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); test_data.write_to(&db).await; diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index e38409cc5..10e9b2b30 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -1001,7 +1001,7 @@ mod tests { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let context = TestContext::builder() diff --git a/signer/tests/integration/emily.rs b/signer/tests/integration/emily.rs index 5620f4698..de30d4670 100644 --- a/signer/tests/integration/emily.rs +++ b/signer/tests/integration/emily.rs @@ -80,7 +80,7 @@ where num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(rng, &signer_keys, &test_model_parameters); test_data.write_to(&storage).await; diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index cc0643be9..5899977d4 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -82,7 +82,7 @@ async fn should_be_able_to_query_bitcoin_blocks() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, 7); @@ -328,7 +328,7 @@ async fn should_return_the_same_pending_deposit_requests_as_in_memory_store() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); let test_data = TestData::generate(&mut rng, &signer_set, &test_model_params); @@ -570,7 +570,7 @@ async fn should_return_the_same_pending_accepted_deposit_requests_as_in_memory_s num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let threshold = 4; @@ -777,7 +777,7 @@ async fn should_return_only_accepted_pending_deposits_that_are_within_reclaim_bo num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let threshold = 4; @@ -962,7 +962,7 @@ async fn should_return_the_same_last_key_rotation_as_in_memory_store() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 1, num_signers_per_request: 7, - concatenate_blocks: true, + concatenate_blocks: false, }; let num_signers = 7; let threshold = 4; @@ -1617,7 +1617,7 @@ async fn block_in_canonical_bitcoin_blockchain_in_other_block_chain() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -1691,7 +1691,7 @@ async fn we_can_fetch_bitcoin_txs_from_db() { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -1903,7 +1903,7 @@ async fn is_known_bitcoin_block_hash_works() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2466,7 +2466,7 @@ async fn deposit_report_with_only_deposit_request() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2566,7 +2566,7 @@ async fn deposit_report_with_deposit_request_reorged() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2641,7 +2641,7 @@ async fn deposit_report_with_deposit_request_spent() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2739,7 +2739,7 @@ async fn deposit_report_with_deposit_request_swept_but_swept_reorged() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2870,7 +2870,7 @@ async fn deposit_report_with_deposit_request_confirmed() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -3107,7 +3107,7 @@ async fn signer_utxo_reorg_suite(desc: ReorgDescription) { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + concatenate_blocks: false, }; // Let's generate some dummy data and write it into the database. diff --git a/signer/tests/integration/rotate_keys.rs b/signer/tests/integration/rotate_keys.rs index 75b031c4d..18851dea4 100644 --- a/signer/tests/integration/rotate_keys.rs +++ b/signer/tests/integration/rotate_keys.rs @@ -174,7 +174,7 @@ async fn rotate_key_validation_happy_path() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -224,7 +224,7 @@ async fn rotate_key_validation_no_dkg() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -262,7 +262,7 @@ async fn rotate_key_validation_wrong_deployer() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -306,7 +306,7 @@ async fn rotate_key_validation_wrong_signing_set() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -356,7 +356,7 @@ async fn rotate_key_validation_wrong_aggregate_key() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -406,7 +406,7 @@ async fn rotate_key_validation_wrong_signatures_required() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -461,7 +461,7 @@ async fn rotate_key_validation_replay() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -500,7 +500,7 @@ async fn rotate_key_validation_replay() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; diff --git a/signer/tests/integration/transaction_coordinator.rs b/signer/tests/integration/transaction_coordinator.rs index afb548c8f..8ec06724a 100644 --- a/signer/tests/integration/transaction_coordinator.rs +++ b/signer/tests/integration/transaction_coordinator.rs @@ -818,7 +818,7 @@ async fn get_signer_public_keys_and_aggregate_key_falls_back() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); test_data.write_to(&db).await; @@ -927,7 +927,7 @@ async fn run_dkg_from_scratch() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); @@ -1145,7 +1145,7 @@ async fn run_subsequent_dkg() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); diff --git a/signer/tests/integration/transaction_signer.rs b/signer/tests/integration/transaction_signer.rs index 72f008ee4..ecb4ac440 100644 --- a/signer/tests/integration/transaction_signer.rs +++ b/signer/tests/integration/transaction_signer.rs @@ -83,7 +83,7 @@ async fn get_signer_public_keys_and_aggregate_key_falls_back() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: true, + concatenate_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); test_data.write_to(&db).await; From 1c1f205e12d68987472a5b34fe2a265765852cf8 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Tue, 28 Jan 2025 10:27:26 +0000 Subject: [PATCH 08/14] remove refuse --- signer/src/storage/postgres.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 1a0674eb5..33db234de 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -737,9 +737,6 @@ impl PgStore { } } -#[cfg(test)] -impl PgStore {} - impl From for PgStore { fn from(value: sqlx::PgPool) -> Self { Self(value) From e35a700bc2a70f226fa2e052f52836564cb4bff0 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Tue, 28 Jan 2025 10:54:03 +0000 Subject: [PATCH 09/14] don't bail on retry fail --- signer/src/request_decider.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/signer/src/request_decider.rs b/signer/src/request_decider.rs index f3d31397b..9c2fa41dc 100644 --- a/signer/src/request_decider.rs +++ b/signer/src/request_decider.rs @@ -276,9 +276,10 @@ where chain_tip: &BitcoinBlockHash, ) -> Result<(), Error> { for decision in decisions.into_iter().map(SignerDepositDecision::from) { - self.send_message(decision, chain_tip).await?; + if let Err(error) = self.send_message(decision, chain_tip).await { + tracing::warn!(%error, "error sending deposit decision to retry, skipping"); + } } - Ok(()) } From 019c2afca9e94f7bab5c55728a94903a5f2122bd Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Tue, 28 Jan 2025 17:29:23 +0000 Subject: [PATCH 10/14] improve tests --- signer/src/api/new_block.rs | 10 +++--- signer/src/request_decider.rs | 2 +- signer/src/stacks/wallet.rs | 2 +- signer/src/testing/storage/model.rs | 20 ++++------- signer/src/transaction_coordinator.rs | 2 +- signer/src/transaction_signer.rs | 2 +- signer/tests/integration/emily.rs | 2 +- signer/tests/integration/postgres.rs | 36 +++++++++---------- signer/tests/integration/request_decider.rs | 2 +- signer/tests/integration/rotate_keys.rs | 16 ++++----- .../integration/transaction_coordinator.rs | 6 ++-- .../tests/integration/transaction_signer.rs | 2 +- 12 files changed, 48 insertions(+), 54 deletions(-) diff --git a/signer/src/api/new_block.rs b/signer/src/api/new_block.rs index ccea3c07e..2a1765561 100644 --- a/signer/src/api/new_block.rs +++ b/signer/src/api/new_block.rs @@ -667,7 +667,7 @@ mod tests { num_deposit_requests_per_block: 1, num_withdraw_requests_per_block: 1, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let db = ctx.inner_storage(); let test_data = TestData::generate(&mut rng, &[], &test_params); @@ -740,7 +740,7 @@ mod tests { num_deposit_requests_per_block: 1, num_withdraw_requests_per_block: 1, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let db = ctx.inner_storage(); let test_data = TestData::generate(&mut rng, &[], &test_params); @@ -789,7 +789,7 @@ mod tests { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 2, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let db = ctx.inner_storage(); @@ -862,7 +862,7 @@ mod tests { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 2, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let db = ctx.inner_storage(); @@ -924,7 +924,7 @@ mod tests { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 2, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); diff --git a/signer/src/request_decider.rs b/signer/src/request_decider.rs index 9c2fa41dc..df5c1df8a 100644 --- a/signer/src/request_decider.rs +++ b/signer/src/request_decider.rs @@ -505,7 +505,7 @@ mod tests { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let context = TestContext::builder() diff --git a/signer/src/stacks/wallet.rs b/signer/src/stacks/wallet.rs index f2a2394dd..a16e38c81 100644 --- a/signer/src/stacks/wallet.rs +++ b/signer/src/stacks/wallet.rs @@ -677,7 +677,7 @@ mod tests { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); test_data.write_to(&db).await; diff --git a/signer/src/testing/storage/model.rs b/signer/src/testing/storage/model.rs index 1425752d7..c2046a820 100644 --- a/signer/src/testing/storage/model.rs +++ b/signer/src/testing/storage/model.rs @@ -51,8 +51,6 @@ pub struct TestData { /// transaction outputs pub tx_outputs: Vec, - - bitcoin_blocks_ref: Vec, } impl TestData { @@ -66,16 +64,14 @@ impl TestData { R: rand::RngCore, { let mut test_data = Self::new(); - + let mut parent: Option = None; for _ in 0..params.num_bitcoin_blocks { - let parent = if params.concatenate_blocks { - test_data.bitcoin_blocks_ref.last() - } else { - None - }; - let (next_chunk, block_ref) = test_data.new_block(rng, signer_keys, params, parent); + let (next_chunk, block_ref) = + test_data.new_block(rng, signer_keys, params, parent.as_ref()); test_data.push(next_chunk); - test_data.bitcoin_blocks_ref.push(block_ref); + if params.consecutive_blocks { + parent = Some(block_ref); + } } test_data @@ -122,7 +118,6 @@ impl TestData { .collect(); let bitcoin_blocks = vec![block.clone()]; - let bitcoin_blocks_refs = vec![BitcoinBlockRef::summarize(&block)]; ( Self { bitcoin_blocks, @@ -135,7 +130,6 @@ impl TestData { stacks_transactions: withdraw_data.stacks_transactions, transactions, tx_outputs: Vec::new(), - bitcoin_blocks_ref: bitcoin_blocks_refs, }, block.into(), ) @@ -525,7 +519,7 @@ pub struct Params { /// The number of signers to hallucinate per request pub num_signers_per_request: usize, /// Wheter to generate consecutive blocks or not - pub concatenate_blocks: bool, + pub consecutive_blocks: bool, } impl BitcoinBlockRef { diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 2b56580b9..473ab2b4c 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -1840,7 +1840,7 @@ mod tests { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 7, - concatenate_blocks: false, + consecutive_blocks: false, }; let context = TestContext::builder() diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index 10e9b2b30..ebba4a02d 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -1001,7 +1001,7 @@ mod tests { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let context = TestContext::builder() diff --git a/signer/tests/integration/emily.rs b/signer/tests/integration/emily.rs index de30d4670..d1cccc320 100644 --- a/signer/tests/integration/emily.rs +++ b/signer/tests/integration/emily.rs @@ -80,7 +80,7 @@ where num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(rng, &signer_keys, &test_model_parameters); test_data.write_to(&storage).await; diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index 5899977d4..81ac28fd6 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -82,7 +82,7 @@ async fn should_be_able_to_query_bitcoin_blocks() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, 7); @@ -328,7 +328,7 @@ async fn should_return_the_same_pending_deposit_requests_as_in_memory_store() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); let test_data = TestData::generate(&mut rng, &signer_set, &test_model_params); @@ -493,7 +493,7 @@ async fn should_return_the_same_pending_withdraw_requests_as_in_memory_store() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 1, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -570,7 +570,7 @@ async fn should_return_the_same_pending_accepted_deposit_requests_as_in_memory_s num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let threshold = 4; @@ -639,7 +639,7 @@ async fn should_return_the_same_pending_accepted_withdraw_requests_as_in_memory_ // the threshold in order for the test to succeed with accepted // requests. num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let threshold = 4; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -777,7 +777,7 @@ async fn should_return_only_accepted_pending_deposits_that_are_within_reclaim_bo num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let threshold = 4; @@ -962,7 +962,7 @@ async fn should_return_the_same_last_key_rotation_as_in_memory_store() { num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 1, num_signers_per_request: 7, - concatenate_blocks: false, + consecutive_blocks: false, }; let num_signers = 7; let threshold = 4; @@ -1404,7 +1404,7 @@ async fn fetching_deposit_signer_decisions() { num_deposit_requests_per_block: 1, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: true, + consecutive_blocks: true, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -1617,7 +1617,7 @@ async fn block_in_canonical_bitcoin_blockchain_in_other_block_chain() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -1691,7 +1691,7 @@ async fn we_can_fetch_bitcoin_txs_from_db() { num_deposit_requests_per_block: 2, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -1903,7 +1903,7 @@ async fn is_known_bitcoin_block_hash_works() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2359,7 +2359,7 @@ async fn transaction_coordinator_test_environment( num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 7, - concatenate_blocks: false, + consecutive_blocks: false, }; let context = TestContext::builder() @@ -2466,7 +2466,7 @@ async fn deposit_report_with_only_deposit_request() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2566,7 +2566,7 @@ async fn deposit_report_with_deposit_request_reorged() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2641,7 +2641,7 @@ async fn deposit_report_with_deposit_request_spent() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2739,7 +2739,7 @@ async fn deposit_report_with_deposit_request_swept_but_swept_reorged() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -2870,7 +2870,7 @@ async fn deposit_report_with_deposit_request_confirmed() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; let signer_set = testing::wsts::generate_signer_set_public_keys(&mut rng, num_signers); @@ -3107,7 +3107,7 @@ async fn signer_utxo_reorg_suite(desc: ReorgDescription) { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: num_signers, - concatenate_blocks: false, + consecutive_blocks: false, }; // Let's generate some dummy data and write it into the database. diff --git a/signer/tests/integration/request_decider.rs b/signer/tests/integration/request_decider.rs index b55d7aaf8..79334b68e 100644 --- a/signer/tests/integration/request_decider.rs +++ b/signer/tests/integration/request_decider.rs @@ -48,7 +48,7 @@ fn test_environment( num_deposit_requests_per_block: 5, num_withdraw_requests_per_block: 5, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let context = TestContext::builder() diff --git a/signer/tests/integration/rotate_keys.rs b/signer/tests/integration/rotate_keys.rs index 18851dea4..8a1856b1a 100644 --- a/signer/tests/integration/rotate_keys.rs +++ b/signer/tests/integration/rotate_keys.rs @@ -174,7 +174,7 @@ async fn rotate_key_validation_happy_path() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -224,7 +224,7 @@ async fn rotate_key_validation_no_dkg() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -262,7 +262,7 @@ async fn rotate_key_validation_wrong_deployer() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -306,7 +306,7 @@ async fn rotate_key_validation_wrong_signing_set() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -356,7 +356,7 @@ async fn rotate_key_validation_wrong_aggregate_key() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -406,7 +406,7 @@ async fn rotate_key_validation_wrong_signatures_required() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -461,7 +461,7 @@ async fn rotate_key_validation_replay() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; @@ -500,7 +500,7 @@ async fn rotate_key_validation_replay() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_model_params); test_data.write_to(&mut db).await; diff --git a/signer/tests/integration/transaction_coordinator.rs b/signer/tests/integration/transaction_coordinator.rs index 8ec06724a..3d061bea2 100644 --- a/signer/tests/integration/transaction_coordinator.rs +++ b/signer/tests/integration/transaction_coordinator.rs @@ -818,7 +818,7 @@ async fn get_signer_public_keys_and_aggregate_key_falls_back() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); test_data.write_to(&db).await; @@ -927,7 +927,7 @@ async fn run_dkg_from_scratch() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); @@ -1145,7 +1145,7 @@ async fn run_subsequent_dkg() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); diff --git a/signer/tests/integration/transaction_signer.rs b/signer/tests/integration/transaction_signer.rs index ecb4ac440..f395961bd 100644 --- a/signer/tests/integration/transaction_signer.rs +++ b/signer/tests/integration/transaction_signer.rs @@ -83,7 +83,7 @@ async fn get_signer_public_keys_and_aggregate_key_falls_back() { num_deposit_requests_per_block: 0, num_withdraw_requests_per_block: 0, num_signers_per_request: 0, - concatenate_blocks: false, + consecutive_blocks: false, }; let test_data = TestData::generate(&mut rng, &[], &test_params); test_data.write_to(&db).await; From 5828645d07f0dabac437f240cbd47b58f75b2740 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Wed, 29 Jan 2025 09:51:05 +0000 Subject: [PATCH 11/14] fix get_deposit_signer_decisions in_memory --- signer/src/storage/in_memory.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/signer/src/storage/in_memory.rs b/signer/src/storage/in_memory.rs index 81476cba4..d976e585f 100644 --- a/signer/src/storage/in_memory.rs +++ b/signer/src/storage/in_memory.rs @@ -927,13 +927,19 @@ impl super::DbRead for SharedStore { let result = deposit_requests .into_iter() - .filter(|x| voted.contains(&(x.txid, x.output_index))) - .flat_map(|req| { + .filter_map(|request| { + if !voted.contains(&(request.txid, request.output_index)) { + return None; + } store .deposit_request_to_signers - .get(&(req.txid, req.output_index)) - .cloned() - .unwrap_or_default() + .get(&(request.txid, request.output_index)) + .and_then(|signers| { + signers + .iter() + .find(|signer| signer.signer_pub_key == *signer_public_key) + .cloned() + }) }) .collect(); From ada934c4a80fc518f1d5e4ef3e100b3076927885 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Wed, 29 Jan 2025 09:51:45 +0000 Subject: [PATCH 12/14] refactor fetching_deposit_signer_decisions test --- signer/tests/integration/postgres.rs | 52 +++++++++++++++------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index 81ac28fd6..79d3ecc6d 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -1417,42 +1417,47 @@ async fn fetching_deposit_signer_decisions() { // move the oldest deposit request to the last block // so that we change its created_at time to be the latest test_data.deposit_requests.rotate_left(1); - let mut new_time_block = OffsetDateTime::now_utc() - time::Duration::minutes(15); - let mut new_time_request = OffsetDateTime::now_utc() - time::Duration::minutes(12); + // We'll register each block with a 2 minute interval + // i.e. times -> [-15, -13, -11, -9, -7] + let mut new_time = OffsetDateTime::now_utc() - time::Duration::minutes(15); // Update Bitcoin blocks - for (block, deposit) in test_data - .bitcoin_blocks - .iter() - .zip(test_data.deposit_requests.iter()) - { - let new_time_block_str = new_time_block - .format(&time::format_description::well_known::Rfc3339) - .unwrap(); - let new_time_request_str = new_time_request + for block in test_data.bitcoin_blocks.iter() { + let new_time_str = new_time .format(&time::format_description::well_known::Rfc3339) .unwrap(); - sqlx::query( r#" - UPDATE sbtc_signer.bitcoin_blocks - SET created_at = $1::timestamptz - WHERE block_hash = $2 - "#, + UPDATE sbtc_signer.bitcoin_blocks + SET created_at = $1::timestamptz + WHERE block_hash = $2 + "#, ) - .bind(new_time_block_str) // Bind as string + .bind(new_time_str) // Bind as string .bind(block.block_hash) .execute(pg_store.pool()) .await .unwrap(); + new_time += time::Duration::minutes(2); + } + // Now we'll update the deposit. Each deposit will be updated so that it will + // arrive 1 minute after its corresponding block. + // With the exception of the first deposit which will be updated to arrive last. + // i.e. times -> [-12, -10, -8, -6, -4 (the first deposit)] + new_time = OffsetDateTime::now_utc() - time::Duration::minutes(12); + for deposit in test_data.deposit_requests.iter() { + let new_time_str = new_time + .format(&time::format_description::well_known::Rfc3339) + .unwrap(); + sqlx::query( r#" - UPDATE sbtc_signer.deposit_signers - SET created_at = $1::timestamptz - WHERE txid = $2 AND output_index = $3 AND signer_pub_key = $4 - "#, + UPDATE sbtc_signer.deposit_signers + SET created_at = $1::timestamptz + WHERE txid = $2 AND output_index = $3 AND signer_pub_key = $4 + "#, ) - .bind(new_time_request_str) // Bind as string + .bind(new_time_str) // Bind as string .bind(deposit.txid) .bind(i32::try_from(deposit.output_index).unwrap()) .bind(signer_pub_key) @@ -1460,8 +1465,7 @@ async fn fetching_deposit_signer_decisions() { .await .unwrap(); - new_time_block += time::Duration::minutes(2); - new_time_request += time::Duration::minutes(2); + new_time += time::Duration::minutes(2); } let chain_tip = pg_store From bbb4edab915b939d8bbbc289ea10c0b680714541 Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Wed, 29 Jan 2025 10:15:59 +0000 Subject: [PATCH 13/14] improve test comments --- signer/tests/integration/postgres.rs | 39 +++++++++++++++++----------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index 79d3ecc6d..14d22dc4f 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -1414,9 +1414,6 @@ async fn fetching_deposit_signer_decisions() { let signer_pub_key = signer_set.first().unwrap(); - // move the oldest deposit request to the last block - // so that we change its created_at time to be the latest - test_data.deposit_requests.rotate_left(1); // We'll register each block with a 2 minute interval // i.e. times -> [-15, -13, -11, -9, -7] let mut new_time = OffsetDateTime::now_utc() - time::Duration::minutes(15); @@ -1427,10 +1424,9 @@ async fn fetching_deposit_signer_decisions() { .unwrap(); sqlx::query( r#" - UPDATE sbtc_signer.bitcoin_blocks - SET created_at = $1::timestamptz - WHERE block_hash = $2 - "#, + UPDATE sbtc_signer.bitcoin_blocks + SET created_at = $1::timestamptz + WHERE block_hash = $2"#, ) .bind(new_time_str) // Bind as string .bind(block.block_hash) @@ -1440,10 +1436,24 @@ async fn fetching_deposit_signer_decisions() { new_time += time::Duration::minutes(2); } - // Now we'll update the deposit. Each deposit will be updated so that it will - // arrive 1 minute after its corresponding block. - // With the exception of the first deposit which will be updated to arrive last. - // i.e. times -> [-12, -10, -8, -6, -4 (the first deposit)] + + // Rotate deposits to test edge case: + // Move first deposit to be processed last (latest timestamp) + // This tests that a deposit decision can still be returned + // even when its associated block falls outside the context window + test_data.deposit_requests.rotate_left(1); + + // Now we'll update the deposits decisions. Each decision will be + // updated so that it will arrive 1 minute after its corresponding block. + // With the exception of the first one, which will be updated to arrive last. + // Block times: [-15, -13, -11, -9, -7] + // Decision times: [-12, -10, -8, -6, -4] + // ^ ^ ^ ^ ^ + // | | | | first deposit (moved to last) + // | | | fifth deposit + // | | forth deposit + // | third deposit + // second deposit new_time = OffsetDateTime::now_utc() - time::Duration::minutes(12); for deposit in test_data.deposit_requests.iter() { let new_time_str = new_time @@ -1452,10 +1462,9 @@ async fn fetching_deposit_signer_decisions() { sqlx::query( r#" - UPDATE sbtc_signer.deposit_signers - SET created_at = $1::timestamptz - WHERE txid = $2 AND output_index = $3 AND signer_pub_key = $4 - "#, + UPDATE sbtc_signer.deposit_signers + SET created_at = $1::timestamptz + WHERE txid = $2 AND output_index = $3 AND signer_pub_key = $4"#, ) .bind(new_time_str) // Bind as string .bind(deposit.txid) From 05fe03e349dacb78e68f6d5acbf652daeab1efaf Mon Sep 17 00:00:00 2001 From: Francesco Leacche Date: Wed, 29 Jan 2025 10:25:44 +0000 Subject: [PATCH 14/14] use inspect_err --- signer/src/request_decider.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/signer/src/request_decider.rs b/signer/src/request_decider.rs index df5c1df8a..f30745317 100644 --- a/signer/src/request_decider.rs +++ b/signer/src/request_decider.rs @@ -276,9 +276,12 @@ where chain_tip: &BitcoinBlockHash, ) -> Result<(), Error> { for decision in decisions.into_iter().map(SignerDepositDecision::from) { - if let Err(error) = self.send_message(decision, chain_tip).await { - tracing::warn!(%error, "error sending deposit decision to retry, skipping"); - } + let _ = self + .send_message(decision, chain_tip) + .await + .inspect_err(|error| { + tracing::warn!(%error, "error sending deposit decision to retry, skipping"); + }); } Ok(()) }