Skip to content

Commit 1d0f97c

Browse files
Merge pull request #6850 from jacinta-stacks/bugfix/wait-for-validation-before-issuing-signature
Do not prematurely issue a response if still waiting on block validate response
2 parents aa54197 + 394d3af commit 1d0f97c

File tree

6 files changed

+342
-46
lines changed

6 files changed

+342
-46
lines changed

stacks-node/src/tests/signer/v0/mod.rs

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ use crate::{nakamoto_node, BitcoinRegtestController, BurnchainController, Config
119119

120120
pub mod late_block_proposal;
121121
pub mod reorg;
122+
pub mod signers_wait_for_validation;
122123
pub mod tenure_extend;
123124
pub mod tx_replay;
124125

@@ -2297,7 +2298,12 @@ fn end_of_tenure() {
22972298
);
22982299

22992300
info!("------------------------- Test Block Validation Stalled -------------------------");
2300-
TEST_VALIDATE_STALL.set(true);
2301+
TEST_VALIDATE_STALL.set(vec![signer_test
2302+
.running_nodes
2303+
.conf
2304+
.connection_options
2305+
.auth_token
2306+
.clone()]);
23012307

23022308
let proposals_before = proposed_blocks.load(Ordering::SeqCst);
23032309
let info = signer_test.get_peer_info();
@@ -2357,7 +2363,7 @@ fn end_of_tenure() {
23572363

23582364
info!("Unpausing block validation and waiting for block to be processed");
23592365
// Disable the stall and wait for the block to be processed
2360-
TEST_VALIDATE_STALL.set(false);
2366+
TEST_VALIDATE_STALL.set(vec![]);
23612367
wait_for(short_timeout.as_secs(), || {
23622368
let processed_now = get_chain_info(&signer_test.running_nodes.conf).stacks_tip_height;
23632369
Ok(processed_now > blocks_before)
@@ -4578,7 +4584,12 @@ fn block_validation_response_timeout() {
45784584
info!("------------------------- Test Mine and Verify Confirmed Nakamoto Block -------------------------");
45794585
signer_test.mine_and_verify_confirmed_naka_block(timeout, num_signers, true);
45804586
info!("------------------------- Test Block Validation Stalled -------------------------");
4581-
TEST_VALIDATE_STALL.set(true);
4587+
TEST_VALIDATE_STALL.set(vec![signer_test
4588+
.running_nodes
4589+
.conf
4590+
.connection_options
4591+
.auth_token
4592+
.clone()]);
45824593
let validation_stall_start = Instant::now();
45834594

45844595
let proposals_before = block_proposals.load(Ordering::SeqCst);
@@ -4682,7 +4693,7 @@ fn block_validation_response_timeout() {
46824693
let info_before = info_after;
46834694
info!("Unpausing block validation");
46844695
// Disable the stall and wait for the block to be processed successfully
4685-
TEST_VALIDATE_STALL.set(false);
4696+
TEST_VALIDATE_STALL.set(vec![]);
46864697
wait_for(30, || {
46874698
let info = get_chain_info(&signer_test.running_nodes.conf);
46884699
Ok(info.stacks_tip_height > info_before.stacks_tip_height)
@@ -6279,7 +6290,12 @@ fn signer_can_accept_rejected_block() {
62796290
TEST_IGNORE_ALL_BLOCK_PROPOSALS.set(vec![ignoring_signer]);
62806291

62816292
// Stall block validation so we can ensure the timing we want to test
6282-
TEST_VALIDATE_STALL.set(true);
6293+
TEST_VALIDATE_STALL.set(vec![signer_test
6294+
.running_nodes
6295+
.conf
6296+
.connection_options
6297+
.auth_token
6298+
.clone()]);
62836299

62846300
// submit a tx so that the miner will mine a block
62856301
let transfer_tx = make_stacks_transfer_serialized(
@@ -6306,7 +6322,7 @@ fn signer_can_accept_rejected_block() {
63066322
TEST_REJECT_ALL_BLOCK_PROPOSAL.set(vec![]);
63076323

63086324
// Unstall the other signers
6309-
TEST_VALIDATE_STALL.set(false);
6325+
TEST_VALIDATE_STALL.set(vec![]);
63106326

63116327
info!(
63126328
"Block proposed, submitting another transaction that should not get included in the block"

stacks-node/src/tests/signer/v0/reorg.rs

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,12 @@ fn reorg_attempts_count_towards_miner_validity() {
133133
info!("------------------------- Test Mine Block N -------------------------");
134134
let chain_before = get_chain_info(&signer_test.running_nodes.conf);
135135
// Stall validation so signers will be unable to process the tenure change block for Tenure B.
136-
TEST_VALIDATE_STALL.set(true);
136+
TEST_VALIDATE_STALL.set(vec![signer_test
137+
.running_nodes
138+
.conf
139+
.connection_options
140+
.auth_token
141+
.clone()]);
137142
test_observer::clear();
138143
// submit a tx so that the miner will mine an extra block
139144
let sender_nonce = 0;
@@ -185,7 +190,7 @@ fn reorg_attempts_count_towards_miner_validity() {
185190
assert_ne!(block_proposal_n, block_proposal_n_prime);
186191
let chain_before = get_chain_info(&signer_test.running_nodes.conf);
187192
TEST_MINE_SKIP.set(true);
188-
TEST_VALIDATE_STALL.set(false);
193+
TEST_VALIDATE_STALL.set(vec![]);
189194

190195
info!("------------------------- Advance Tip to Block N -------------------------");
191196
wait_for(30, || {
@@ -321,7 +326,12 @@ fn reorg_attempts_activity_timeout_exceeded() {
321326
let chain_start = get_chain_info(&signer_test.running_nodes.conf);
322327
// Stall validation so signers will be unable to process the tenure change block for Tenure B.
323328
// And so the incoming miner proposes a block N' (the reorging block).
324-
TEST_VALIDATE_STALL.set(true);
329+
TEST_VALIDATE_STALL.set(vec![signer_test
330+
.running_nodes
331+
.conf
332+
.connection_options
333+
.auth_token
334+
.clone()]);
325335
test_observer::clear();
326336
// submit a tx so that the miner will mine an extra block
327337
let sender_nonce = 0;
@@ -361,7 +371,7 @@ fn reorg_attempts_activity_timeout_exceeded() {
361371
block_proposal_n.header.signer_signature_hash()
362372
);
363373
// Allow block N validation to finish, but don't broadcast it yet
364-
TEST_VALIDATE_STALL.set(false);
374+
TEST_VALIDATE_STALL.set(vec![]);
365375
TEST_PAUSE_BLOCK_BROADCAST.set(true);
366376
let reward_cycle = signer_test.get_current_reward_cycle();
367377
wait_for_block_global_acceptance_from_signers(
@@ -1327,7 +1337,14 @@ fn no_reorg_due_to_successive_block_validation_ok() {
13271337
debug!("Miner 1 mined block N: {block_n_signature_hash}");
13281338

13291339
info!("------------------------- Pause Block Validation Response of N+1 -------------------------");
1330-
TEST_VALIDATE_STALL.set(true);
1340+
// Both miners have the same auth token
1341+
TEST_VALIDATE_STALL.set(vec![miners
1342+
.signer_test
1343+
.running_nodes
1344+
.conf
1345+
.connection_options
1346+
.auth_token
1347+
.clone()]);
13311348
let rejections_before_2 = rl2_rejections.load(Ordering::SeqCst);
13321349
let blocks_before = test_observer::get_blocks().len();
13331350
let blocks_processed_before_1 = blocks_mined1.load(Ordering::SeqCst);
@@ -1383,7 +1400,7 @@ fn no_reorg_due_to_successive_block_validation_ok() {
13831400

13841401
info!("------------------------- Unpause Block Validation Response of N+1 -------------------------");
13851402

1386-
TEST_VALIDATE_STALL.set(false);
1403+
TEST_VALIDATE_STALL.set(vec![]);
13871404

13881405
// Verify that the node accepted the proposed N+1, sending back a validate ok response
13891406
wait_for(30, || {
Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,239 @@
1+
// Copyright (C) 2026 Stacks Open Internet Foundation
2+
//
3+
// This program is free software: you can redistribute it and/or modify
4+
// it under the terms of the GNU General Public License as published by
5+
// the Free Software Foundation, either version 3 of the License, or
6+
// (at your option) any later version.
7+
//
8+
// This program is distributed in the hope that it will be useful,
9+
// but WITHOUT ANY WARRANTY; without even the implied warranty of
10+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11+
// GNU General Public License for more details.
12+
//
13+
// You should have received a copy of the GNU General Public License
14+
// along with this program. If not, see <http://www.gnu.org/licenses/>.
15+
use std::env;
16+
17+
use libsigner::v0::messages::{BlockResponse, SignerMessage};
18+
use stacks::chainstate::burn::db::sortdb::SortitionDB;
19+
use stacks::chainstate::stacks::TenureChangeCause;
20+
use stacks::codec::StacksMessageCodec;
21+
use stacks::net::api::postblock_proposal::TEST_VALIDATE_STALL;
22+
use tracing_subscriber::layer::SubscriberExt;
23+
use tracing_subscriber::util::SubscriberInitExt;
24+
use tracing_subscriber::{fmt, EnvFilter};
25+
26+
use crate::tests::nakamoto_integrations::wait_for;
27+
use crate::tests::neon_integrations::test_observer;
28+
use crate::tests::signer::v0::{
29+
wait_for_block_pre_commits_from_signers, wait_for_block_pushed_by_miner_key, MultipleMinerTest,
30+
};
31+
32+
#[test]
33+
#[ignore]
34+
/// Test that signers don't issue signatures until they have validated the block
35+
///
36+
/// This test verifies a race condition where a signer receives enough pre-commits
37+
/// to exceed the 70% threshold before receiving its own block validation response.
38+
/// The signer should NOT issue a signature until it has confirmed the block is valid.
39+
///
40+
/// Test Setup:
41+
/// - Distribute signers across two miners (4 on miner 1, 1 on miner 2)
42+
/// - Signers on different miners use different validation endpoints
43+
///
44+
/// Test Execution:
45+
/// 1. Propose a block to all signers
46+
/// 2. Pause block validation on miner 2 (the single signer)
47+
/// 3. 4 signers on miner 1 issue pre-commits, pushing threshold over 70%
48+
/// 4. The single signer on miner 2 receives all pre-commits but its validation is stalled
49+
/// 5. Verify the single signer does NOT issue a signature until validation completes
50+
/// 6. Resume validation and confirm the block is accepted
51+
///
52+
/// Test Assertion:
53+
/// The signer waits for its own validation before issuing a signature, preventing
54+
/// race conditions where it could sign before discovering the block is invalid.
55+
fn signer_waits_for_validation_before_signing() {
56+
if env::var("BITCOIND_TEST") != Ok("1".into()) {
57+
return;
58+
}
59+
60+
tracing_subscriber::registry()
61+
.with(fmt::layer())
62+
.with(EnvFilter::from_default_env())
63+
.init();
64+
65+
info!("------------------------- Test Setup -------------------------");
66+
67+
// Create a multiple miner test with 5 signers
68+
// They will be distributed: 4 to miner 1, 1 to miner 2
69+
let num_signers = 5;
70+
let node_2_auth = "node_2".to_string();
71+
let node_1_auth = "node_1".to_string();
72+
let mut miners = MultipleMinerTest::new_with_signer_dist(
73+
num_signers,
74+
0,
75+
|config| {
76+
if config.endpoint.port() % 5 == 0 {
77+
config.auth_password = node_2_auth.clone();
78+
} else {
79+
config.auth_password = node_1_auth.clone();
80+
}
81+
},
82+
|config| {
83+
config.burnchain.pox_reward_length = Some(30);
84+
config.connection_options.auth_token = Some(node_1_auth.clone());
85+
},
86+
|config| {
87+
config.connection_options.auth_token = Some(node_2_auth.clone());
88+
},
89+
// Distribute signers so first 4 go to node 1, last 1 goes to node 2
90+
|port| if port % 5 == 0 { 1 } else { 0 },
91+
None,
92+
);
93+
let all_signers = miners.signer_test.signer_test_pks();
94+
let signer_configs = &miners.signer_test.signer_configs;
95+
let (conf_1, conf_2) = miners.get_node_configs();
96+
let mut approving_signers = Vec::new();
97+
let mut stalled_signer = Vec::new();
98+
for (config, signer_pk) in signer_configs.iter().zip(all_signers.iter()) {
99+
if config.node_host == conf_2.node.rpc_bind {
100+
stalled_signer.push(signer_pk.clone());
101+
} else {
102+
approving_signers.push(signer_pk.clone());
103+
}
104+
}
105+
assert_eq!(
106+
stalled_signer.len(),
107+
1,
108+
"Expected exactly one signer to be assigned to miner 2"
109+
);
110+
let (miner_pk_1, _miner_pk_2) = miners.get_miner_public_keys();
111+
112+
miners.pause_commits_miner_2();
113+
miners.boot_to_epoch_3();
114+
115+
// Make sure we know which miner will win in the stalled block
116+
miners.pause_commits_miner_1();
117+
info!("------------------------- Mine First Block N -------------------------");
118+
119+
let sortdb = SortitionDB::open(
120+
&conf_1.get_burn_db_file_path(),
121+
false,
122+
conf_1.get_burnchain().pox_constants,
123+
)
124+
.unwrap();
125+
// Mine an initial block to establish state
126+
miners
127+
.mine_bitcoin_block_and_tenure_change_tx(&sortdb, TenureChangeCause::BlockFound, 30)
128+
.expect("Failed to mine BTC block followed by tenure change tx");
129+
miners.submit_commit_miner_1(&sortdb);
130+
miners.signer_test.check_signer_states_normal();
131+
132+
let info_before = miners.get_peer_info();
133+
info!("------------------------- Stall Validation on Miner 2 -------------------------");
134+
// Stall block validation submission on the signer connected to miner 2
135+
// This prevents that signer from validating the next proposed block
136+
TEST_VALIDATE_STALL.set(vec![Some(node_2_auth)]);
137+
138+
info!("------------------------- Mine Block N+1 with Stalled Validation -------------------------");
139+
// Mine a new tenure which will issue a block proposal to all signers for its tenure change.
140+
miners.signer_test.mine_bitcoin_block();
141+
142+
// The 4 signers on miner 1 should have validated and sent pre-commits
143+
// The 1 signer on miner 2 should be waiting for validation and should NOT have issued a signature
144+
let block =
145+
wait_for_block_pushed_by_miner_key(30, info_before.stacks_tip_height + 1, &miner_pk_1)
146+
.expect("Failed to mine block N+1");
147+
let signer_signature_hash = block.header.signer_signature_hash();
148+
info!("------------------------- Mined {signer_signature_hash}. Checking for Pre-Commits for {} Signers-------------------------", approving_signers.len());
149+
wait_for_block_pre_commits_from_signers(30, &signer_signature_hash, &approving_signers)
150+
.expect("Failed to receive pre-commits from approving signers");
151+
// We only wait a small amount of time for each of these checks since we already received block commits from everyone else.
152+
let stalled_pk = stalled_signer[0].clone();
153+
assert!(
154+
wait_for(15, || {
155+
for chunk in test_observer::get_stackerdb_chunks()
156+
.into_iter()
157+
.flat_map(|chunk| chunk.modified_slots)
158+
{
159+
let message = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice())
160+
.expect("Failed to deserialize SignerMessage");
161+
162+
let pk = chunk.recover_pk().expect("Failed to recover pk");
163+
if stalled_pk != pk {
164+
continue;
165+
}
166+
167+
match message {
168+
SignerMessage::BlockPreCommit(pre_commit) => {
169+
assert_ne!(
170+
signer_signature_hash, pre_commit,
171+
"Stalled signer should not have issued a pre-commit yet."
172+
);
173+
}
174+
SignerMessage::BlockResponse(BlockResponse::Accepted(acceptance)) => {
175+
assert_ne!(
176+
signer_signature_hash, acceptance.signer_signature_hash,
177+
"Stalled signer should not have accepted the block yet"
178+
);
179+
}
180+
SignerMessage::BlockResponse(BlockResponse::Rejected(rejection)) => {
181+
assert_ne!(
182+
signer_signature_hash, rejection.signer_signature_hash,
183+
"Stalled signer should not have rejected the block yet"
184+
);
185+
}
186+
_ => {}
187+
}
188+
}
189+
Ok(false)
190+
})
191+
.is_err(),
192+
"Stalled signer issued a pre-commit or response before validation completed"
193+
);
194+
// At this point, we should NOT have received a signature from the stalled signer
195+
// because it hasn't yet validated the block. We're checking this happened correctly
196+
// by the fact that we can now resume validation without issues.
197+
info!("------------------------- Resume Validation -------------------------");
198+
TEST_VALIDATE_STALL.set(vec![]); // Unset the stall condition
199+
info!("------------------------- Wait for Block Response -------------------------");
200+
// Now that validation is resumed, the stalled signer should issue a response
201+
let mut found_commit = false;
202+
let mut found_accept = false;
203+
wait_for(15, || {
204+
for chunk in test_observer::get_stackerdb_chunks()
205+
.into_iter()
206+
.flat_map(|chunk| chunk.modified_slots)
207+
{
208+
let message = SignerMessage::consensus_deserialize(&mut chunk.data.as_slice())
209+
.expect("Failed to deserialize SignerMessage");
210+
211+
let pk = chunk.recover_pk().expect("Failed to recover pk");
212+
if stalled_pk != pk {
213+
continue;
214+
}
215+
match message {
216+
SignerMessage::BlockPreCommit(pre_commit) => {
217+
if signer_signature_hash == pre_commit {
218+
found_commit = true;
219+
}
220+
}
221+
SignerMessage::BlockResponse(BlockResponse::Accepted(acceptance)) => {
222+
if signer_signature_hash == acceptance.signer_signature_hash {
223+
found_accept = true;
224+
}
225+
}
226+
SignerMessage::BlockResponse(BlockResponse::Rejected(rejection)) => {
227+
assert_ne!(
228+
signer_signature_hash, rejection.signer_signature_hash,
229+
"Stalled signer should not have rejected the block"
230+
);
231+
}
232+
_ => {}
233+
}
234+
}
235+
Ok(found_accept && found_commit)
236+
})
237+
.expect("Stalled signer did not issue a pre-commit and acceptance after validation resumed");
238+
miners.shutdown();
239+
}

0 commit comments

Comments
 (0)