Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config option tenure_last_block_proposal_timeout_secs and do not allow reorgs at tenure boundary before it is exceeded #5425

Merged
merged 5 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/bitcoin-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ jobs:
- tests::signer::v0::locally_accepted_blocks_overriden_by_global_rejection
- tests::signer::v0::locally_rejected_blocks_overriden_by_global_acceptance
- tests::signer::v0::reorg_locally_accepted_blocks_across_tenures_succeeds
- tests::signer::v0::reorg_locally_accepted_blocks_across_tenures_fails
- tests::signer::v0::miner_recovers_when_broadcast_block_delay_across_tenures_occurs
- tests::signer::v0::multiple_miners_with_nakamoto_blocks
- tests::signer::v0::partial_tenure_fork
Expand All @@ -133,6 +134,7 @@ jobs:
- tests::nakamoto_integrations::utxo_check_on_startup_panic
- tests::nakamoto_integrations::utxo_check_on_startup_recover
- tests::nakamoto_integrations::v3_signer_api_endpoint
- tests::nakamoto_integrations::signer_chainstate
# TODO: enable these once v1 signer is supported by a new nakamoto epoch
# - tests::signer::v1::dkg
# - tests::signer::v1::sign_request_rejected
Expand Down
62 changes: 50 additions & 12 deletions stacks-signer/src/chainstate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@ use blockstack_lib::chainstate::stacks::TenureChangePayload;
use blockstack_lib::net::api::getsortition::SortitionInfo;
use blockstack_lib::util_lib::db::Error as DBError;
use clarity::types::chainstate::BurnchainHeaderHash;
use clarity::util::get_epoch_time_secs;
jferrant marked this conversation as resolved.
Show resolved Hide resolved
use slog::{slog_info, slog_warn};
use stacks_common::types::chainstate::{ConsensusHash, StacksPublicKey};
use stacks_common::util::hash::Hash160;
use stacks_common::{info, warn};

use crate::client::{ClientError, CurrentAndLastSortition, StacksClient};
use crate::config::SignerConfig;
use crate::signerdb::{BlockState, SignerDb};
use crate::signerdb::{BlockInfo, BlockState, SignerDb};

#[derive(thiserror::Error, Debug)]
/// Error type for the signer chainstate module
Expand Down Expand Up @@ -119,13 +120,17 @@ pub struct ProposalEvalConfig {
pub first_proposal_burn_block_timing: Duration,
/// Time between processing a sortition and proposing a block before the block is considered invalid
pub block_proposal_timeout: Duration,
/// Time to wait for the last block of a tenure to be globally accepted or rejected before considering
/// a new miner's block at the same height as valid.
pub tenure_last_block_proposal_timeout: Duration,
}

impl From<&SignerConfig> for ProposalEvalConfig {
fn from(value: &SignerConfig) -> Self {
Self {
first_proposal_burn_block_timing: value.first_proposal_burn_block_timing,
block_proposal_timeout: value.block_proposal_timeout,
tenure_last_block_proposal_timeout: value.tenure_last_block_proposal_timeout,
}
}
}
Expand Down Expand Up @@ -460,7 +465,36 @@ impl SortitionsView {
Ok(true)
}

/// Check if the tenure change block confirms the expected parent block (i.e., the last globally accepted block in the parent tenure)
/// Get the last block from the given tenure
/// Returns the last locally accepted block if it is not timed out, otherwise it will return the last globally accepted block.
fn get_tenure_last_block_info(
consensus_hash: &ConsensusHash,
signer_db: &SignerDb,
tenure_last_block_proposal_timeout: Duration,
) -> Result<Option<BlockInfo>, ClientError> {
// Get the last known block in the previous tenure
let last_locally_accepted_block = signer_db
.get_last_accepted_block(consensus_hash)
.map_err(|e| ClientError::InvalidResponse(e.to_string()))?;

if let Some(local_info) = last_locally_accepted_block {
if let Some(signed_over_time) = local_info.signed_self {
if signed_over_time + tenure_last_block_proposal_timeout.as_secs()
> get_epoch_time_secs()
{
// The last locally accepted block is not timed out, return it
return Ok(Some(local_info));
}
}
}
// The last locally accepted block is timed out, get the last globally accepted block
signer_db
.get_last_globally_accepted_block(consensus_hash)
.map_err(|e| ClientError::InvalidResponse(e.to_string()))
}

/// Check if the tenure change block confirms the expected parent block
/// (i.e., the last locally accepted block in the parent tenure, or if that block is timed out, the last globally accepted block in the parent tenure)
/// It checks the local DB first, and if the block is not present in the local DB, it asks the
/// Stacks node for the highest processed block header in the given tenure (and then caches it
/// in the DB).
Expand All @@ -473,24 +507,27 @@ impl SortitionsView {
reward_cycle: u64,
signer_db: &mut SignerDb,
client: &StacksClient,
tenure_last_block_proposal_timeout: Duration,
) -> Result<bool, ClientError> {
// If the tenure change block confirms the expected parent block, it should confirm at least one more block than the last globally accepted block in the parent tenure.
let last_globally_accepted_block = signer_db
.get_last_globally_accepted_block(&tenure_change.prev_tenure_consensus_hash)
.map_err(|e| ClientError::InvalidResponse(e.to_string()))?;
// If the tenure change block confirms the expected parent block, it should confirm at least one more block than the last accepted block in the parent tenure.
let last_block_info = Self::get_tenure_last_block_info(
&tenure_change.prev_tenure_consensus_hash,
signer_db,
tenure_last_block_proposal_timeout,
)?;

if let Some(global_info) = last_globally_accepted_block {
if let Some(info) = last_block_info {
// N.B. this block might not be the last globally accepted block across the network;
// it's just the highest one in this tenure that we know about. If this given block is
// no higher than it, then it's definitely no higher than the last globally accepted
// block across the network, so we can do an early rejection here.
if block.header.chain_length <= global_info.block.header.chain_length {
if block.header.chain_length <= info.block.header.chain_length {
warn!(
"Miner's block proposal does not confirm as many blocks as we expect";
"proposed_block_consensus_hash" => %block.header.consensus_hash,
"proposed_block_signer_sighash" => %block.header.signer_signature_hash(),
"proposed_chain_length" => block.header.chain_length,
"expected_at_least" => global_info.block.header.chain_length + 1,
"expected_at_least" => info.block.header.chain_length + 1,
);
return Ok(false);
}
Expand Down Expand Up @@ -558,6 +595,7 @@ impl SortitionsView {
reward_cycle,
signer_db,
client,
self.config.tenure_last_block_proposal_timeout,
)?;
if !confirms_expected_parent {
return Ok(false);
Expand All @@ -573,15 +611,15 @@ impl SortitionsView {
if !is_valid_parent_tenure {
return Ok(false);
}
let last_in_tenure = signer_db
let last_in_current_tenure = signer_db
.get_last_globally_accepted_block(&block.header.consensus_hash)
.map_err(|e| ClientError::InvalidResponse(e.to_string()))?;
if let Some(last_in_tenure) = last_in_tenure {
if let Some(last_in_current_tenure) = last_in_current_tenure {
warn!(
"Miner block proposal contains a tenure change, but we've already signed a block in this tenure. Considering proposal invalid.";
"proposed_block_consensus_hash" => %block.header.consensus_hash,
"proposed_block_signer_sighash" => %block.header.signer_signature_hash(),
"last_in_tenure_signer_sighash" => %last_in_tenure.block.header.signer_signature_hash(),
"last_in_tenure_signer_sighash" => %last_in_current_tenure.block.header.signer_signature_hash(),
);
return Ok(false);
}
Expand Down
1 change: 1 addition & 0 deletions stacks-signer/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,7 @@ pub(crate) mod tests {
db_path: config.db_path.clone(),
first_proposal_burn_block_timing: config.first_proposal_burn_block_timing,
block_proposal_timeout: config.block_proposal_timeout,
tenure_last_block_proposal_timeout: config.tenure_last_block_proposal_timeout,
}
}

Expand Down
19 changes: 18 additions & 1 deletion stacks-signer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use crate::client::SignerSlotID;
const EVENT_TIMEOUT_MS: u64 = 5000;
const BLOCK_PROPOSAL_TIMEOUT_MS: u64 = 600_000;
const DEFAULT_FIRST_PROPOSAL_BURN_BLOCK_TIMING_SECS: u64 = 60;
const DEFAULT_TENURE_LAST_BLOCK_PROPOSAL_TIMEOUT_SECS: u64 = 30;

#[derive(thiserror::Error, Debug)]
/// An error occurred parsing the provided configuration
Expand Down Expand Up @@ -128,6 +129,9 @@ pub struct SignerConfig {
pub first_proposal_burn_block_timing: Duration,
/// How much time to wait for a miner to propose a block following a sortition
pub block_proposal_timeout: Duration,
/// Time to wait for the last block of a tenure to be globally accepted or rejected
/// before considering a new miner's block at the same height as potentially valid.
pub tenure_last_block_proposal_timeout: Duration,
}

/// The parsed configuration for the signer
Expand Down Expand Up @@ -158,6 +162,9 @@ pub struct GlobalConfig {
pub block_proposal_timeout: Duration,
/// An optional custom Chain ID
pub chain_id: Option<u32>,
/// Time to wait for the last block of a tenure to be globally accepted or rejected
/// before considering a new miner's block at the same height as potentially valid.
pub tenure_last_block_proposal_timeout: Duration,
}

/// Internal struct for loading up the config file
Expand All @@ -180,13 +187,16 @@ struct RawConfigFile {
pub db_path: String,
/// Metrics endpoint
pub metrics_endpoint: Option<String>,
/// How much time must pass between the first block proposal in a tenure and the next bitcoin block
/// How much time must pass in seconds between the first block proposal in a tenure and the next bitcoin block
/// before a subsequent miner isn't allowed to reorg the tenure
pub first_proposal_burn_block_timing_secs: Option<u64>,
/// How much time to wait for a miner to propose a block following a sortition in milliseconds
pub block_proposal_timeout_ms: Option<u64>,
/// An optional custom Chain ID
pub chain_id: Option<u32>,
/// Time in seconds to wait for the last block of a tenure to be globally accepted or rejected
/// before considering a new miner's block at the same height as potentially valid.
pub tenure_last_block_proposal_timeout_secs: Option<u64>,
}

impl RawConfigFile {
Expand Down Expand Up @@ -266,6 +276,12 @@ impl TryFrom<RawConfigFile> for GlobalConfig {
.unwrap_or(BLOCK_PROPOSAL_TIMEOUT_MS),
);

let tenure_last_block_proposal_timeout = Duration::from_secs(
raw_data
.tenure_last_block_proposal_timeout_secs
.unwrap_or(DEFAULT_TENURE_LAST_BLOCK_PROPOSAL_TIMEOUT_SECS),
);

Ok(Self {
node_host: raw_data.node_host,
endpoint,
Expand All @@ -279,6 +295,7 @@ impl TryFrom<RawConfigFile> for GlobalConfig {
first_proposal_burn_block_timing,
block_proposal_timeout,
chain_id: raw_data.chain_id,
tenure_last_block_proposal_timeout,
})
}
}
Expand Down
1 change: 1 addition & 0 deletions stacks-signer/src/runloop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ impl<Signer: SignerTrait<T>, T: StacksMessageCodec + Clone + Send + Debug> RunLo
mainnet: self.config.network.is_mainnet(),
db_path: self.config.db_path.clone(),
block_proposal_timeout: self.config.block_proposal_timeout,
tenure_last_block_proposal_timeout: self.config.tenure_last_block_proposal_timeout,
}))
}

Expand Down
1 change: 1 addition & 0 deletions stacks-signer/src/tests/chainstate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ fn setup_test_environment(
config: ProposalEvalConfig {
first_proposal_burn_block_timing: Duration::from_secs(30),
block_proposal_timeout: Duration::from_secs(5),
tenure_last_block_proposal_timeout: Duration::from_secs(30),
},
};

Expand Down
9 changes: 6 additions & 3 deletions testnet/stacks-node/src/tests/nakamoto_integrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6369,6 +6369,7 @@ fn signer_chainstate() {
let proposal_conf = ProposalEvalConfig {
first_proposal_burn_block_timing: Duration::from_secs(0),
block_proposal_timeout: Duration::from_secs(100),
tenure_last_block_proposal_timeout: Duration::from_secs(30),
};
let mut sortitions_view =
SortitionsView::fetch_view(proposal_conf, &signer_client).unwrap();
Expand Down Expand Up @@ -6507,6 +6508,7 @@ fn signer_chainstate() {
let proposal_conf = ProposalEvalConfig {
first_proposal_burn_block_timing: Duration::from_secs(0),
block_proposal_timeout: Duration::from_secs(100),
tenure_last_block_proposal_timeout: Duration::from_secs(30),
};
let burn_block_height = SortitionDB::get_canonical_burn_chain_tip(sortdb.conn())
.unwrap()
Expand Down Expand Up @@ -6541,10 +6543,10 @@ fn signer_chainstate() {
valid: Some(true),
signed_over: true,
proposed_time: get_epoch_time_secs(),
signed_self: None,
signed_group: None,
signed_self: Some(get_epoch_time_secs()),
signed_group: Some(get_epoch_time_secs()),
ext: ExtraBlockInfo::None,
state: BlockState::Unprocessed,
state: BlockState::GloballyAccepted,
})
.unwrap();

Expand Down Expand Up @@ -6584,6 +6586,7 @@ fn signer_chainstate() {
let proposal_conf = ProposalEvalConfig {
first_proposal_burn_block_timing: Duration::from_secs(0),
block_proposal_timeout: Duration::from_secs(100),
tenure_last_block_proposal_timeout: Duration::from_secs(30),
};
let mut sortitions_view = SortitionsView::fetch_view(proposal_conf, &signer_client).unwrap();
let burn_block_height = SortitionDB::get_canonical_burn_chain_tip(sortdb.conn())
Expand Down
Loading
Loading