From 562982c322ddc460d3ad9c7ee5a4c2981ba308c1 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Mon, 18 Nov 2024 12:37:20 -0800 Subject: [PATCH 1/4] Use tenure extend timestamp to determine if enough time has passed for a tenure extend Signed-off-by: Jacinta Ferrant --- stacks-signer/src/chainstate.rs | 15 +++--- stacks-signer/src/signerdb.rs | 46 ++++++++++++++++++- stacks-signer/src/v0/signer.rs | 81 ++++++++++++++------------------- 3 files changed, 84 insertions(+), 58 deletions(-) diff --git a/stacks-signer/src/chainstate.rs b/stacks-signer/src/chainstate.rs index 66bf173941..5a6aa04200 100644 --- a/stacks-signer/src/chainstate.rs +++ b/stacks-signer/src/chainstate.rs @@ -337,9 +337,14 @@ impl SortitionsView { // in tenure extends, we need to check: // (1) if this is the most recent sortition, an extend is allowed if it changes the burnchain view // (2) if this is the most recent sortition, an extend is allowed if enough time has passed to refresh the block limit + let sortition_consensus_hash = proposed_by.state().consensus_hash; let changed_burn_view = - tenure_extend.burn_view_consensus_hash != proposed_by.state().consensus_hash; - let enough_time_passed = Self::tenure_time_passed_block_lim()?; + tenure_extend.burn_view_consensus_hash != sortition_consensus_hash; + let enough_time_passed = get_epoch_time_secs() + > signer_db.get_tenure_extend_timestamp( + self.config.tenure_idle_timeout, + &sortition_consensus_hash, + ); if !changed_burn_view && !enough_time_passed { warn!( "Miner block proposal contains a tenure extend, but the burnchain view has not changed and enough time has not passed to refresh the block limit. Considering proposal invalid."; @@ -658,12 +663,6 @@ impl SortitionsView { } } - /// Has the current tenure lasted long enough to extend the block limit? - pub fn tenure_time_passed_block_lim() -> Result { - // TODO - Ok(false) - } - /// Fetch a new view of the recent sortitions pub fn fetch_view( config: ProposalEvalConfig, diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index bc163ee686..7e3659ff12 100644 --- a/stacks-signer/src/signerdb.rs +++ b/stacks-signer/src/signerdb.rs @@ -16,9 +16,10 @@ use std::fmt::Display; use std::path::Path; -use std::time::SystemTime; +use std::time::{Duration, SystemTime}; use blockstack_lib::chainstate::nakamoto::NakamotoBlock; +use blockstack_lib::chainstate::stacks::TransactionPayload; use blockstack_lib::util_lib::db::{ query_row, query_rows, sqlite_open, table_exists, tx_begin_immediate, u64_to_sql, Error as DBError, @@ -814,7 +815,7 @@ impl SignerDb { } /// Return the all globally accepted block in a tenure (identified by its consensus hash). - pub fn get_globally_accepted_blocks( + fn get_globally_accepted_blocks( &self, tenure: &ConsensusHash, ) -> Result, DBError> { @@ -826,6 +827,47 @@ impl SignerDb { .map(|info| serde_json::from_str(info).map_err(DBError::from)) .collect() } + + /// Compute the tenure extend timestamp based on the tenure start and already consumed idle time of the + /// globally accepted blocks of the provided tenure (identified by the cosnensus hash) + pub fn get_tenure_extend_timestamp( + &self, + tenure_idle_timeout: Duration, + consensus_hash: &ConsensusHash, + ) -> u64 { + // We do not know our tenure start timestamp until we find the last processed tenure change transaction for the given consensus hash. + // We may not even have it in our database, in which case, we should use the oldest known block in the tenure. + // If we have no blocks known for this tenure, we will assume it has only JUST started and calculate + // our tenure extend timestamp based on the epoch time in secs. + let mut tenure_start_timestamp = None; + let mut tenure_process_time_ms = 0; + // Note that the globally accepted blocks are already returned in descending order of stacks height, therefore by newest block to oldest block + for block_info in self + .get_globally_accepted_blocks(consensus_hash) + .unwrap_or_default() + .iter() + { + // Always use the oldest block as our tenure start timestamp + tenure_start_timestamp = Some(block_info.proposed_time); + tenure_process_time_ms += block_info.validation_time_ms.unwrap_or(0); + + if block_info + .block + .txs + .first() + .map(|tx| matches!(tx.payload, TransactionPayload::TenureChange(_))) + .unwrap_or(false) + { + // Tenure change found. No more blocks should count towards this tenure's processing time. + break; + } + } + + tenure_start_timestamp + .unwrap_or(get_epoch_time_secs()) + .saturating_add(tenure_idle_timeout.as_secs()) + .saturating_sub(tenure_process_time_ms / 1000) + } } fn try_deserialize(s: Option) -> Result, DBError> diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index e28703f56f..d6ae24b83d 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -18,11 +18,10 @@ use std::sync::mpsc::Sender; use std::time::{Duration, Instant}; use blockstack_lib::chainstate::nakamoto::{NakamotoBlock, NakamotoBlockHeader}; -use blockstack_lib::chainstate::stacks::TransactionPayload; use blockstack_lib::net::api::postblock_proposal::{ BlockValidateOk, BlockValidateReject, BlockValidateResponse, }; -use clarity::types::chainstate::{ConsensusHash, StacksPrivateKey}; +use clarity::types::chainstate::StacksPrivateKey; use clarity::types::{PrivateKey, StacksEpochId}; use clarity::util::hash::MerkleHashFunc; use clarity::util::secp256k1::Secp256k1PublicKey; @@ -303,7 +302,10 @@ impl Signer { BlockResponse::accepted( block_info.signer_signature_hash(), signature, - self.calculate_tenure_extend_timestamp(&block_info.block.header.consensus_hash), + self.signer_db.get_tenure_extend_timestamp( + self.proposal_config.tenure_idle_timeout, + &block_info.block.header.consensus_hash, + ), ) } else { debug!("{self}: Rejecting block {}", block_info.block.block_id()); @@ -312,7 +314,10 @@ impl Signer { RejectCode::RejectedInPriorRound, &self.private_key, self.mainnet, - self.calculate_tenure_extend_timestamp(&block_info.block.header.consensus_hash), + self.signer_db.get_tenure_extend_timestamp( + self.proposal_config.tenure_idle_timeout, + &block_info.block.header.consensus_hash, + ), ) }; Some(response) @@ -415,7 +420,8 @@ impl Signer { RejectCode::ConnectivityIssues, &self.private_key, self.mainnet, - self.calculate_tenure_extend_timestamp( + self.signer_db.get_tenure_extend_timestamp( + self.proposal_config.tenure_idle_timeout, &block_proposal.block.header.consensus_hash, ), )) @@ -432,7 +438,8 @@ impl Signer { RejectCode::SortitionViewMismatch, &self.private_key, self.mainnet, - self.calculate_tenure_extend_timestamp( + self.signer_db.get_tenure_extend_timestamp( + self.proposal_config.tenure_idle_timeout, &block_proposal.block.header.consensus_hash, ), )) @@ -451,7 +458,10 @@ impl Signer { RejectCode::NoSortitionView, &self.private_key, self.mainnet, - self.calculate_tenure_extend_timestamp(&block_proposal.block.header.consensus_hash), + self.signer_db.get_tenure_extend_timestamp( + self.proposal_config.tenure_idle_timeout, + &block_proposal.block.header.consensus_hash, + ), )) }; @@ -588,7 +598,10 @@ impl Signer { let accepted = BlockAccepted::new( block_info.signer_signature_hash(), signature, - self.calculate_tenure_extend_timestamp(&block_info.block.header.consensus_hash), + self.signer_db.get_tenure_extend_timestamp( + self.proposal_config.tenure_idle_timeout, + &block_info.block.header.consensus_hash, + ), ); // have to save the signature _after_ the block info self.handle_block_signature(stacks_client, &accepted); @@ -643,7 +656,10 @@ impl Signer { block_validate_reject.clone(), &self.private_key, self.mainnet, - self.calculate_tenure_extend_timestamp(&block_info.block.header.consensus_hash), + self.signer_db.get_tenure_extend_timestamp( + self.proposal_config.tenure_idle_timeout, + &block_info.block.header.consensus_hash, + ), ); self.signer_db .insert_block(&block_info) @@ -741,7 +757,10 @@ impl Signer { RejectCode::ConnectivityIssues, &self.private_key, self.mainnet, - self.calculate_tenure_extend_timestamp(&block_proposal.block.header.consensus_hash), + self.signer_db.get_tenure_extend_timestamp( + self.proposal_config.tenure_idle_timeout, + &block_proposal.block.header.consensus_hash, + ), ); if let Err(e) = block_info.mark_locally_rejected() { warn!("{self}: Failed to mark block as locally rejected: {e:?}",); @@ -1124,7 +1143,10 @@ impl Signer { RejectCode::TestingDirective, &self.private_key, self.mainnet, - self.calculate_tenure_extend_timestamp(&block_proposal.block.header.consensus_hash), + self.signer_db.get_tenure_extend_timestamp( + self.proposal_config.tenure_idle_timeout, + &block_proposal.block.header.consensus_hash, + ), )) } else { None @@ -1143,41 +1165,4 @@ impl Signer { warn!("{self}: Failed to send mock signature to stacker-db: {e:?}",); } } - - /// Calculate the tenure extend timestamp based on the tenure start and already consumed idle time. - fn calculate_tenure_extend_timestamp(&self, consensus_hash: &ConsensusHash) -> u64 { - // We do not know our tenure start timestamp until we find the last processed tenure change transaction for the given consensus hash. - // We may not even have it in our database, in which case, we should use the oldest known block in the tenure. - // If we have no blocks known for this tenure, we will assume it has only JUST started and calculate - // our tenure extend timestamp based on the epoch time in secs. - let mut tenure_start_timestamp = None; - let mut tenure_process_time_ms = 0; - // Note that the globally accepted blocks are already returned in descending order of stacks height, therefore by newest block to oldest block - for block_info in self - .signer_db - .get_globally_accepted_blocks(consensus_hash) - .unwrap_or_default() - .iter() - { - // Always use the oldest block as our tenure start timestamp - tenure_start_timestamp = Some(block_info.proposed_time); - tenure_process_time_ms += block_info.validation_time_ms.unwrap_or(0); - - if block_info - .block - .txs - .first() - .map(|tx| matches!(tx.payload, TransactionPayload::TenureChange(_))) - .unwrap_or(false) - { - // Tenure change found. No more blocks should count towards this tenure's processing time. - break; - } - } - - tenure_start_timestamp - .unwrap_or(get_epoch_time_secs()) - .saturating_add(self.proposal_config.tenure_idle_timeout.as_secs()) - .saturating_sub(tenure_process_time_ms / 1000) - } } From 9eeb3e85f9890b8176b112bdcf4af154a8ff5c47 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Mon, 18 Nov 2024 14:29:13 -0800 Subject: [PATCH 2/4] CRC: add comment about get globally accepted blocks in descending order Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signerdb.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index 7e3659ff12..3ad886cd22 100644 --- a/stacks-signer/src/signerdb.rs +++ b/stacks-signer/src/signerdb.rs @@ -814,7 +814,7 @@ impl SignerDb { )) } - /// Return the all globally accepted block in a tenure (identified by its consensus hash). + /// Return the all globally accepted block in a tenure (identified by its consensus hash) in stacks height descending order fn get_globally_accepted_blocks( &self, tenure: &ConsensusHash, From 1665a1a082a79c262bee8f177f9e8a8161df4477 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 19 Nov 2024 10:33:25 -0800 Subject: [PATCH 3/4] CRC: fix typo in comment Signed-off-by: Jacinta Ferrant --- stacks-signer/src/signerdb.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stacks-signer/src/signerdb.rs b/stacks-signer/src/signerdb.rs index 3ad886cd22..c7bd9f9a6a 100644 --- a/stacks-signer/src/signerdb.rs +++ b/stacks-signer/src/signerdb.rs @@ -829,7 +829,7 @@ impl SignerDb { } /// Compute the tenure extend timestamp based on the tenure start and already consumed idle time of the - /// globally accepted blocks of the provided tenure (identified by the cosnensus hash) + /// globally accepted blocks of the provided tenure (identified by the consensus hash) pub fn get_tenure_extend_timestamp( &self, tenure_idle_timeout: Duration, From 4b77c10f01b826bab3bb07ebfedef0f67dbfb11a Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 19 Nov 2024 10:33:38 -0800 Subject: [PATCH 4/4] Miner forking test fix attempt Signed-off-by: Jacinta Ferrant --- testnet/stacks-node/src/tests/signer/v0.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index 2952fcbae3..e3d7ba98b5 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -1866,10 +1866,15 @@ fn miner_forking() { info!("Flushing any pending commits to enable custom winner selection"); let burn_height_before = get_burn_height(); + let blocks_before = test_observer::get_blocks().len(); + let nakamoto_blocks_count_before = get_nakamoto_headers(&conf).len(); next_block_and( &mut signer_test.running_nodes.btc_regtest_controller, 30, - || Ok(get_burn_height() > burn_height_before), + || { + Ok(get_burn_height() > burn_height_before + && test_observer::get_blocks().len() > blocks_before) + }, ) .unwrap(); @@ -2047,11 +2052,14 @@ fn miner_forking() { }) .expect("Timed out waiting for miner 1 to RBF its old commit op"); + let blocks_before = test_observer::get_blocks().len(); info!("Mine RL1 Tenure"); - signer_test - .running_nodes - .btc_regtest_controller - .build_next_block(1); + next_block_and( + &mut signer_test.running_nodes.btc_regtest_controller, + 30, + || Ok(test_observer::get_blocks().len() > blocks_before), + ) + .unwrap(); // fetch the current sortition info let sortdb = conf.get_burnchain().open_sortition_db(true).unwrap(); @@ -2090,14 +2098,16 @@ fn miner_forking() { let peer_1_height = get_chain_info(&conf).stacks_tip_height; let peer_2_height = get_chain_info(&conf_node_2).stacks_tip_height; + let nakamoto_blocks_count = get_nakamoto_headers(&conf).len(); info!("Peer height information"; "peer_1" => peer_1_height, "peer_2" => peer_2_height, "pre_naka_height" => pre_nakamoto_peer_1_height); + info!("Nakamoto blocks count before test: {nakamoto_blocks_count_before}, Nakamoto blocks count now: {nakamoto_blocks_count}"); assert_eq!(peer_1_height, peer_2_height); let nakamoto_blocks_count = get_nakamoto_headers(&conf).len(); assert_eq!( peer_1_height - pre_nakamoto_peer_1_height, - u64::try_from(nakamoto_blocks_count).unwrap() - 1, // subtract 1 for the first Nakamoto block + u64::try_from(nakamoto_blocks_count - nakamoto_blocks_count_before).unwrap(), // subtract 1 for the first Nakamoto block "There should be no forks in this test" );