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.

Sign up for GitHub

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

Fix/bitcoind forking test signerdb state #5674

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Fix/bitcoind forking test signerdb state #5674

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e2f0e25
First pass fix
jferrant Jan 8, 2025
701ae1a
Use check_tenure_confirms_parent in check_block_against_signer_db_sta…
jferrant Jan 8, 2025
f999638
CRC: cleanup comment in check_block_against_signer_db_state
jferrant Jan 8, 2025
942b434
Merge branch 'develop' of https://github.com/stacks-network/stacks-co…
jferrant Jan 8, 2025
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: 1 addition & 1 deletion stacks-signer/src/chainstate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ impl SortitionsView {
///
/// The rationale here is that the signer DB can be out-of-sync with the node. For example,
/// the signer may have been added to an already-running node.
fn check_tenure_change_confirms_parent(
pub fn check_tenure_change_confirms_parent(
tenure_change: &TenureChangePayload,
block: &NakamotoBlock,
signer_db: &mut SignerDb,
Expand Down
77 changes: 0 additions & 77 deletions stacks-signer/src/signerdb.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,18 +733,6 @@ impl SignerDb {
try_deserialize(result)
}

/// Return the last accepted block the signer (highest stacks height). It will tie break a match based on which was more recently signed.
pub fn get_signer_last_accepted_block(&self) -> Result<Option<BlockInfo>, DBError> {
let query = "SELECT block_info FROM blocks WHERE state IN (?1, ?2) ORDER BY stacks_height DESC, signed_group DESC, signed_self DESC LIMIT 1";
let args = params![
&BlockState::GloballyAccepted.to_string(),
&BlockState::LocallyAccepted.to_string()
];
let result: Option<String> = query_row(&self.db, query, args)?;

try_deserialize(result)
}

/// Return the last accepted block in a tenure (identified by its consensus hash).
pub fn get_last_accepted_block(
&self,
Expand Down Expand Up @@ -1769,69 +1757,4 @@ mod tests {
< block_infos[0].proposed_time
);
}

#[test]
fn signer_last_accepted_block() {
let db_path = tmp_db_path();
let mut db = SignerDb::new(db_path).expect("Failed to create signer db");

let (mut block_info_1, _block_proposal_1) = create_block_override(|b| {
b.block.header.miner_signature = MessageSignature([0x01; 65]);
b.block.header.chain_length = 1;
b.burn_height = 1;
});

let (mut block_info_2, _block_proposal_2) = create_block_override(|b| {
b.block.header.miner_signature = MessageSignature([0x02; 65]);
b.block.header.chain_length = 2;
b.burn_height = 1;
});

let (mut block_info_3, _block_proposal_3) = create_block_override(|b| {
b.block.header.miner_signature = MessageSignature([0x02; 65]);
b.block.header.chain_length = 2;
b.burn_height = 4;
});
block_info_3
.mark_locally_accepted(false)
.expect("Failed to mark block as locally accepted");

db.insert_block(&block_info_1)
.expect("Unable to insert block into db");
db.insert_block(&block_info_2)
.expect("Unable to insert block into db");

assert!(db.get_signer_last_accepted_block().unwrap().is_none());

block_info_1
.mark_globally_accepted()
.expect("Failed to mark block as globally accepted");
db.insert_block(&block_info_1)
.expect("Unable to insert block into db");

assert_eq!(
db.get_signer_last_accepted_block().unwrap().unwrap(),
block_info_1
);

block_info_2
.mark_globally_accepted()
.expect("Failed to mark block as globally accepted");
block_info_2.signed_self = Some(get_epoch_time_secs());
db.insert_block(&block_info_2)
.expect("Unable to insert block into db");

assert_eq!(
db.get_signer_last_accepted_block().unwrap().unwrap(),
block_info_2
);

db.insert_block(&block_info_3)
.expect("Unable to insert block into db");

assert_eq!(
db.get_signer_last_accepted_block().unwrap().unwrap(),
block_info_3
);
}
}
92 changes: 57 additions & 35 deletions stacks-signer/src/v0/signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,60 +582,80 @@ impl Signer {
}
}

/// WARNING: Do NOT call this function PRIOR to check_proposal or block_proposal validation succeeds.
/// WARNING: This is an incomplete check. Do NOT call this function PRIOR to check_proposal or block_proposal validation succeeds.
///
/// Re-verify a block's chain length against the last signed block within signerdb.
/// This is required in case a block has been approved since the initial checks of the block validation endpoint.
fn check_block_against_signer_db_state(
&self,
&mut self,
stacks_client: &StacksClient,
proposed_block: &NakamotoBlock,
) -> Option<BlockResponse> {
let signer_signature_hash = proposed_block.header.signer_signature_hash();
let proposed_block_consensus_hash = proposed_block.header.consensus_hash;
// 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.
jferrant marked this conversation as resolved.
Show resolved Hide resolved
if let Some(tenure_change) = proposed_block.get_tenure_change_tx_payload() {
// Ensure that the tenure change block confirms the expected parent block
match SortitionsView::check_tenure_change_confirms_parent(
tenure_change,
proposed_block,
&mut self.signer_db,
stacks_client,
self.proposal_config.tenure_last_block_proposal_timeout,
) {
Ok(true) => {}
Ok(false) => {
return Some(
self.create_block_rejection(
RejectCode::SortitionViewMismatch,
proposed_block,
),
)
}
Err(e) => {
warn!("{self}: Error checking block proposal: {e}";
"signer_sighash" => %signer_signature_hash,
"block_id" => %proposed_block.block_id()
);
return Some(
self.create_block_rejection(RejectCode::ConnectivityIssues, proposed_block),
);
}
}
}

match self.signer_db.get_signer_last_accepted_block() {
// Ensure that the block is the last block in the chain of its current tenure.
match self
.signer_db
.get_last_accepted_block(&proposed_block_consensus_hash)
{
Ok(Some(last_block_info)) => {
if proposed_block.header.chain_length <= last_block_info.block.header.chain_length {
// We do not allow reorgs at any time within the same consensus hash OR of globally accepted blocks
let non_reorgable_block = last_block_info.block.header.consensus_hash
== proposed_block_consensus_hash
|| last_block_info.state == BlockState::GloballyAccepted;
// Is the reorg timeout requirement exceeded?
let reorg_timeout_exceeded = last_block_info
.signed_self
.map(|signed_over_time| {
signed_over_time.saturating_add(
self.proposal_config
.tenure_last_block_proposal_timeout
.as_secs(),
) <= get_epoch_time_secs()
})
.unwrap_or(false);
if non_reorgable_block || !reorg_timeout_exceeded {
warn!(
"Miner's block proposal does not confirm as many blocks as we expect";
"proposed_block_consensus_hash" => %proposed_block_consensus_hash,
"proposed_block_signer_sighash" => %signer_signature_hash,
"proposed_chain_length" => proposed_block.header.chain_length,
"expected_at_least" => last_block_info.block.header.chain_length + 1,
);
return Some(self.create_block_rejection(
RejectCode::SortitionViewMismatch,
proposed_block,
));
}
warn!(
"Miner's block proposal does not confirm as many blocks as we expect";
"proposed_block_consensus_hash" => %proposed_block_consensus_hash,
"proposed_block_signer_sighash" => %signer_signature_hash,
"proposed_chain_length" => proposed_block.header.chain_length,
"expected_at_least" => last_block_info.block.header.chain_length + 1,
);
return Some(self.create_block_rejection(
RejectCode::SortitionViewMismatch,
proposed_block,
));
}
None
}
Ok(_) => None,
Ok(_) => {}
Err(e) => {
warn!("{self}: Failed to check block against signer db: {e}";
"signer_sighash" => %signer_signature_hash,
"block_id" => %proposed_block.block_id()
);
Some(self.create_block_rejection(RejectCode::ConnectivityIssues, proposed_block))
return Some(
self.create_block_rejection(RejectCode::ConnectivityIssues, proposed_block),
);
}
}
None
}

/// Handle the block validate ok response. Returns our block response if we have one
Expand Down Expand Up @@ -684,7 +704,9 @@ impl Signer {
}
};

if let Some(block_response) = self.check_block_against_signer_db_state(&block_info.block) {
if let Some(block_response) =
self.check_block_against_signer_db_state(stacks_client, &block_info.block)
{
// The signer db state has changed. We no longer view this block as valid. Override the validation response.
if let Err(e) = block_info.mark_locally_rejected() {
if !block_info.has_reached_consensus() {
Expand Down
Loading