From 83a032fa65ec7c4776f7869fd574716e53352a96 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Mon, 9 Dec 2024 13:53:10 -0500 Subject: [PATCH] Added block_proposal_max_age_secs signer configuration to drop old proposals without processing Signed-off-by: Jacinta Ferrant --- .github/workflows/bitcoin-tests.yml | 1 + stacks-signer/CHANGELOG.md | 2 + stacks-signer/src/client/mod.rs | 1 + stacks-signer/src/config.rs | 12 ++ stacks-signer/src/runloop.rs | 1 + stacks-signer/src/v0/signer.rs | 20 ++++ testnet/stacks-node/src/tests/signer/v0.rs | 123 +++++++++++++++++++-- 7 files changed, 152 insertions(+), 8 deletions(-) diff --git a/.github/workflows/bitcoin-tests.yml b/.github/workflows/bitcoin-tests.yml index 41e14b4458..2786fc20b1 100644 --- a/.github/workflows/bitcoin-tests.yml +++ b/.github/workflows/bitcoin-tests.yml @@ -127,6 +127,7 @@ jobs: - tests::signer::v0::continue_after_fast_block_no_sortition - tests::signer::v0::block_validation_response_timeout - tests::signer::v0::tenure_extend_after_bad_commit + - tests::signer::v0::block_proposal_max_age_rejections - tests::nakamoto_integrations::burn_ops_integration_test - tests::nakamoto_integrations::check_block_heights - tests::nakamoto_integrations::clarity_burn_state diff --git a/stacks-signer/CHANGELOG.md b/stacks-signer/CHANGELOG.md index a332b344ce..0b0937cde8 100644 --- a/stacks-signer/CHANGELOG.md +++ b/stacks-signer/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE ## [Unreleased] +- Introduced the `block_proposal_max_age_secs` configuration option for signers, enabling them to automatically ignore block proposals that exceed the specified age in seconds. + ### Added ### Changed diff --git a/stacks-signer/src/client/mod.rs b/stacks-signer/src/client/mod.rs index 37706368dc..ffb03af027 100644 --- a/stacks-signer/src/client/mod.rs +++ b/stacks-signer/src/client/mod.rs @@ -413,6 +413,7 @@ pub(crate) mod tests { block_proposal_timeout: config.block_proposal_timeout, tenure_last_block_proposal_timeout: config.tenure_last_block_proposal_timeout, block_proposal_validation_timeout: config.block_proposal_validation_timeout, + block_proposal_max_age_secs: config.block_proposal_max_age_secs, } } diff --git a/stacks-signer/src/config.rs b/stacks-signer/src/config.rs index 57c90ab0eb..3b1fd7ec3e 100644 --- a/stacks-signer/src/config.rs +++ b/stacks-signer/src/config.rs @@ -38,6 +38,7 @@ const BLOCK_PROPOSAL_TIMEOUT_MS: u64 = 600_000; const BLOCK_PROPOSAL_VALIDATION_TIMEOUT_MS: u64 = 120_000; const DEFAULT_FIRST_PROPOSAL_BURN_BLOCK_TIMING_SECS: u64 = 60; const DEFAULT_TENURE_LAST_BLOCK_PROPOSAL_TIMEOUT_SECS: u64 = 30; +const DEFAULT_BLOCK_PROPOSAL_MAX_AGE_SECS: u64 = 600; #[derive(thiserror::Error, Debug)] /// An error occurred parsing the provided configuration @@ -135,6 +136,8 @@ pub struct SignerConfig { pub tenure_last_block_proposal_timeout: Duration, /// How much time to wait for a block proposal validation response before marking the block invalid pub block_proposal_validation_timeout: Duration, + /// The maximum age of a block proposal in seconds that will be processed by the signer + pub block_proposal_max_age_secs: u64, } /// The parsed configuration for the signer @@ -171,6 +174,8 @@ pub struct GlobalConfig { /// How long to wait for a response from a block proposal validation response from the node /// before marking that block as invalid and rejecting it pub block_proposal_validation_timeout: Duration, + /// The maximum age of a block proposal that will be processed by the signer + pub block_proposal_max_age_secs: u64, } /// Internal struct for loading up the config file @@ -206,6 +211,8 @@ struct RawConfigFile { /// How long to wait (in millisecs) for a response from a block proposal validation response from the node /// before marking that block as invalid and rejecting it pub block_proposal_validation_timeout_ms: Option, + /// The maximum age of a block proposal (in secs) that will be processed by the signer. + pub block_proposal_max_age_secs: Option, } impl RawConfigFile { @@ -297,6 +304,10 @@ impl TryFrom for GlobalConfig { .unwrap_or(BLOCK_PROPOSAL_VALIDATION_TIMEOUT_MS), ); + let block_proposal_max_age_secs = raw_data + .block_proposal_max_age_secs + .unwrap_or(DEFAULT_BLOCK_PROPOSAL_MAX_AGE_SECS); + Ok(Self { node_host: raw_data.node_host, endpoint, @@ -312,6 +323,7 @@ impl TryFrom for GlobalConfig { chain_id: raw_data.chain_id, tenure_last_block_proposal_timeout, block_proposal_validation_timeout, + block_proposal_max_age_secs, }) } } diff --git a/stacks-signer/src/runloop.rs b/stacks-signer/src/runloop.rs index c8f6041478..2a15e75449 100644 --- a/stacks-signer/src/runloop.rs +++ b/stacks-signer/src/runloop.rs @@ -285,6 +285,7 @@ impl, T: StacksMessageCodec + Clone + Send + Debug> RunLo block_proposal_timeout: self.config.block_proposal_timeout, tenure_last_block_proposal_timeout: self.config.tenure_last_block_proposal_timeout, block_proposal_validation_timeout: self.config.block_proposal_validation_timeout, + block_proposal_max_age_secs: self.config.block_proposal_max_age_secs, })) } diff --git a/stacks-signer/src/v0/signer.rs b/stacks-signer/src/v0/signer.rs index b537cfae8a..4c2a4b3732 100644 --- a/stacks-signer/src/v0/signer.rs +++ b/stacks-signer/src/v0/signer.rs @@ -92,6 +92,8 @@ pub struct Signer { pub block_proposal_validation_timeout: Duration, /// The current submitted block proposal and its submission time pub submitted_block_proposal: Option<(BlockProposal, Instant)>, + /// Maximum age of a block proposal in seconds before it is dropped without processing + pub block_proposal_max_age_secs: u64, } impl std::fmt::Display for Signer { @@ -284,6 +286,7 @@ impl From for Signer { proposal_config, submitted_block_proposal: None, block_proposal_validation_timeout: signer_config.block_proposal_validation_timeout, + block_proposal_max_age_secs: signer_config.block_proposal_max_age_secs, } } } @@ -331,6 +334,23 @@ impl Signer { return; } + if block_proposal + .block + .header + .timestamp + .saturating_add(self.block_proposal_max_age_secs) + < get_epoch_time_secs() + { + // Block is too old. Drop it with a warning. Don't even bother broadcasting to the node. + warn!("{self}: Received a block proposal that is more than {} secs old. Ignoring...", self.block_proposal_max_age_secs; + "block_id" => %block_proposal.block.block_id(), + "block_height" => block_proposal.block.header.chain_length, + "burn_height" => block_proposal.burn_height, + "timestamp" => block_proposal.block.header.timestamp, + ); + return; + } + // TODO: should add a check to ignore an old burn block height if we know its outdated. Would require us to store the burn block height we last saw on the side. // the signer needs to be able to determine whether or not the block they're about to sign would conflict with an already-signed Stacks block let signer_signature_hash = block_proposal.block.header.signer_signature_hash(); diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index 9c9fee200f..b8cc15fce8 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -43,6 +43,7 @@ use stacks::net::api::postblock_proposal::{ValidateRejectCode, TEST_VALIDATE_STA use stacks::net::relay::fault_injection::set_ignore_block; use stacks::types::chainstate::{StacksAddress, StacksBlockId, StacksPrivateKey, StacksPublicKey}; use stacks::types::PublicKey; +use stacks::util::get_epoch_time_secs; use stacks::util::hash::{hex_bytes, Hash160, MerkleHashFunc}; use stacks::util::secp256k1::{Secp256k1PrivateKey, Secp256k1PublicKey}; use stacks::util_lib::boot::boot_code_id; @@ -811,14 +812,8 @@ fn reloads_signer_set_in() { let sender_addr = tests::to_addr(&sender_sk); let send_amt = 100; let send_fee = 180; - let mut signer_test: SignerTest = SignerTest::new_with_config_modifications( - num_signers, - vec![(sender_addr, send_amt + send_fee)], - |_config| {}, - |_| {}, - None, - None, - ); + let mut signer_test: SignerTest = + SignerTest::new(num_signers, vec![(sender_addr, send_amt + send_fee)]); setup_epoch_3_reward_set( &signer_test.running_nodes.conf, @@ -8574,3 +8569,115 @@ fn tenure_extend_after_2_bad_commits() { run_loop_2_thread.join().unwrap(); signer_test.shutdown(); } + +#[test] +#[ignore] +/// Test the block_proposal_max_age_secs signer configuration option. It should reject blocks that are +/// invalid but within the max age window, otherwise it should simply drop the block without further processing. +/// +/// Test Setup: +/// The test spins up five stacks signers, one miner Nakamoto node, and a corresponding bitcoind. +/// +/// Test Execution: +/// The stacks node is advanced to epoch 3.0 reward set calculation to ensure the signer set is determined. +/// An invalid block proposal with a recent timestamp is forcibly written to the miner's slot to simulate the miner proposing a block. +/// The signers process the invalid block and broadcast a block response rejection to the respective .signers-XXX-YYY contract. +/// A second block proposal with an outdated timestamp is then submitted to the miner's slot to simulate the miner proposing a very old block. +/// The test confirms no further block rejection response is submitted to the .signers-XXX-YYY contract. +/// +/// Test Assertion: +/// - Each signer successfully rejects the recent invalid block proposal. +/// - No signer submits a block proposal response for the outdated block proposal. +/// - The stacks tip does not advance +fn block_proposal_max_age_rejections() { + if env::var("BITCOIND_TEST") != Ok("1".into()) { + return; + } + + tracing_subscriber::registry() + .with(fmt::layer()) + .with(EnvFilter::from_default_env()) + .init(); + + info!("------------------------- Test Setup -------------------------"); + let num_signers = 5; + let mut signer_test: SignerTest = SignerTest::new_with_config_modifications( + num_signers, + vec![], + |config| { + config.block_proposal_max_age_secs = 30; + }, + |_| {}, + None, + None, + ); + signer_test.boot_to_epoch_3(); + let short_timeout = Duration::from_secs(30); + + // Make sure no other block approvals are in the system. + test_observer::clear(); + info!("------------------------- Send Block Proposal To Signers -------------------------"); + let info_before = get_chain_info(&signer_test.running_nodes.conf); + let mut block = NakamotoBlock { + header: NakamotoBlockHeader::empty(), + txs: vec![], + }; + // First propose a stale block that is older than the block_proposal_max_age_secs + block.header.timestamp = get_epoch_time_secs().saturating_sub( + signer_test.signer_configs[0] + .block_proposal_max_age_secs + .saturating_add(1), + ); + let _block_signer_signature_hash_1 = block.header.signer_signature_hash(); + signer_test.propose_block(block.clone(), short_timeout); + + // Next propose a recent invalid block + block.header.timestamp = get_epoch_time_secs(); + let block_signer_signature_hash_2 = block.header.signer_signature_hash(); + signer_test.propose_block(block, short_timeout); + + info!("------------------------- Test Block Proposal Rejected -------------------------"); + // Verify the signers rejected only the SECOND block proposal. The first was not even processed. + wait_for(30, || { + let rejections: Vec<_> = test_observer::get_stackerdb_chunks() + .into_iter() + .flat_map(|chunk| chunk.modified_slots) + .map(|chunk| { + let Ok(message) = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice()) + else { + return None; + }; + assert!(matches!( + message, + SignerMessage::BlockResponse(BlockResponse::Rejected(_)) + )); + let SignerMessage::BlockResponse(BlockResponse::Rejected(BlockRejection { + reason_code, + signer_signature_hash, + signature, + .. + })) = message + else { + panic!("Received an unexpected block approval from the signer"); + }; + assert_eq!( + signer_signature_hash, block_signer_signature_hash_2, + "Received a rejection for an unexpected block: {signer_signature_hash}" + ); + assert!( + matches!(reason_code, RejectCode::SortitionViewMismatch), + "Received a rejection for an unexpected reason: {reason_code}" + ); + Some(signature) + }) + .collect(); + Ok(rejections.len() == num_signers) + }) + .expect("Timed out waiting for block rejections"); + + info!("------------------------- Test Peer Info-------------------------"); + assert_eq!(info_before, get_chain_info(&signer_test.running_nodes.conf)); + + info!("------------------------- Test Shutdown-------------------------"); + signer_test.shutdown(); +}