Skip to content

Commit be8256c

Browse files
BowTiedRadoneBowTiedRadone
committed
Address suggestions from code review
This commit addresses: - #6007 (comment) - #6007 (comment) - #6007 (comment) - #6007 (comment) - #6007 (comment)
1 parent acea311 commit be8256c

File tree

9 files changed

+131
-68
lines changed

9 files changed

+131
-68
lines changed

‎testnet/stacks-node/src/tests/signer/commands/bitcoin_mining.rs

Lines changed: 55 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use std::sync::atomic::Ordering;
12
use std::sync::{Arc, Mutex};
23

34
use madhouse::{Command, CommandWrapper};
@@ -6,27 +7,45 @@ use stacks::chainstate::stacks::TenureChangeCause;
67
use tracing::info;
78

89
use super::context::{SignerTestContext, SignerTestState};
9-
use crate::tests::signer::v0::{
10-
get_chain_info_wrapper, wait_for_block_pushed_by_miner_key, MultipleMinerTest,
11-
};
10+
use crate::tests::neon_integrations::get_chain_info;
11+
use crate::tests::signer::v0::{wait_for_block_pushed_by_miner_key, MultipleMinerTest};
1212

13-
pub struct MineBitcoinBlockTenureChangePrimaryMiner {
13+
pub struct MineBitcoinBlockTenureChangeMiner1 {
1414
miners: Arc<Mutex<MultipleMinerTest>>,
1515
}
1616

17-
impl MineBitcoinBlockTenureChangePrimaryMiner {
17+
impl MineBitcoinBlockTenureChangeMiner1 {
1818
pub fn new(miners: Arc<Mutex<MultipleMinerTest>>) -> Self {
1919
Self { miners }
2020
}
2121
}
2222

23-
impl Command<SignerTestState, SignerTestContext> for MineBitcoinBlockTenureChangePrimaryMiner {
23+
impl Command<SignerTestState, SignerTestContext> for MineBitcoinBlockTenureChangeMiner1 {
2424
fn check(&self, state: &SignerTestState) -> bool {
25+
let (conf_1, _) = self.miners.lock().unwrap().get_node_configs();
26+
let burn_height = get_chain_info(&conf_1).burn_block_height;
27+
let miner_1_submitted_commit_last_burn_height = self
28+
.miners
29+
.lock()
30+
.unwrap()
31+
.get_primary_submitted_commit_last_burn_height()
32+
.0
33+
.load(Ordering::SeqCst);
34+
let miner_2_submitted_commit_last_burn_height = self
35+
.miners
36+
.lock()
37+
.unwrap()
38+
.get_secondary_submitted_commit_last_burn_height()
39+
.0
40+
.load(Ordering::SeqCst);
41+
2542
info!(
26-
"Checking: Miner 1 mining Bitcoin block and tenure change tx. Result: {:?}",
27-
state.is_booted_to_nakamoto
43+
"Checking: Miner 1 mining Bitcoin block and tenure change tx. Result: {:?} && {:?} && {:?}",
44+
state.is_booted_to_nakamoto, burn_height == miner_1_submitted_commit_last_burn_height, burn_height > miner_2_submitted_commit_last_burn_height
2845
);
2946
state.is_booted_to_nakamoto
47+
&& burn_height == miner_1_submitted_commit_last_burn_height
48+
&& burn_height > miner_2_submitted_commit_last_burn_height
3049
}
3150

3251
fn apply(&self, _state: &mut SignerTestState) {
@@ -63,7 +82,7 @@ impl Command<SignerTestState, SignerTestContext> for MineBitcoinBlockTenureChang
6382
mined_block_height
6483
);
6584

66-
let info_after = get_chain_info_wrapper(&conf_1);
85+
let info_after = get_chain_info(&conf_1);
6786
assert_eq!(info_after.stacks_tip, miner_1_block.header.block_hash());
6887
assert_eq!(info_after.stacks_tip_height, mined_block_height);
6988
assert_eq!(mined_block_height, stacks_height_before + 1);
@@ -77,28 +96,47 @@ impl Command<SignerTestState, SignerTestContext> for MineBitcoinBlockTenureChang
7796
ctx: Arc<SignerTestContext>,
7897
) -> impl Strategy<Value = CommandWrapper<SignerTestState, SignerTestContext>> {
7998
Just(CommandWrapper::new(
80-
MineBitcoinBlockTenureChangePrimaryMiner::new(ctx.miners.clone()),
99+
MineBitcoinBlockTenureChangeMiner1::new(ctx.miners.clone()),
81100
))
82101
}
83102
}
84103

85-
pub struct MineBitcoinBlockTenureChangeSecondaryMiner {
104+
pub struct MineBitcoinBlockTenureChangeMiner2 {
86105
miners: Arc<Mutex<MultipleMinerTest>>,
87106
}
88107

89-
impl MineBitcoinBlockTenureChangeSecondaryMiner {
108+
impl MineBitcoinBlockTenureChangeMiner2 {
90109
pub fn new(miners: Arc<Mutex<MultipleMinerTest>>) -> Self {
91110
Self { miners }
92111
}
93112
}
94113

95-
impl Command<SignerTestState, SignerTestContext> for MineBitcoinBlockTenureChangeSecondaryMiner {
114+
impl Command<SignerTestState, SignerTestContext> for MineBitcoinBlockTenureChangeMiner2 {
96115
fn check(&self, state: &SignerTestState) -> bool {
116+
let (conf_1, _) = self.miners.lock().unwrap().get_node_configs();
117+
let burn_height = get_chain_info(&conf_1).burn_block_height;
118+
let miner_1_submitted_commit_last_burn_height = self
119+
.miners
120+
.lock()
121+
.unwrap()
122+
.get_primary_submitted_commit_last_burn_height()
123+
.0
124+
.load(Ordering::SeqCst);
125+
let miner_2_submitted_commit_last_burn_height = self
126+
.miners
127+
.lock()
128+
.unwrap()
129+
.get_secondary_submitted_commit_last_burn_height()
130+
.0
131+
.load(Ordering::SeqCst);
132+
97133
info!(
98-
"Checking: Miner 2 mining Bitcoin block and tenure change tx. Result: {:?}",
99-
state.is_booted_to_nakamoto
134+
"Checking: Miner 2 mining Bitcoin block and tenure change tx. Result: {:?} && {:?} && {:?}",
135+
state.is_booted_to_nakamoto, burn_height == miner_1_submitted_commit_last_burn_height, burn_height > miner_2_submitted_commit_last_burn_height
100136
);
101137
state.is_booted_to_nakamoto
138+
&& burn_height == miner_2_submitted_commit_last_burn_height
139+
&& burn_height > miner_1_submitted_commit_last_burn_height
102140
}
103141

104142
fn apply(&self, _state: &mut SignerTestState) {
@@ -128,7 +166,7 @@ impl Command<SignerTestState, SignerTestContext> for MineBitcoinBlockTenureChang
128166

129167
let mined_block_height = secondary_miner_block.header.chain_length;
130168

131-
let info_after = get_chain_info_wrapper(&conf_2);
169+
let info_after = get_chain_info(&conf_2);
132170
assert_eq!(
133171
info_after.stacks_tip,
134172
secondary_miner_block.header.block_hash()
@@ -145,7 +183,7 @@ impl Command<SignerTestState, SignerTestContext> for MineBitcoinBlockTenureChang
145183
ctx: Arc<SignerTestContext>,
146184
) -> impl Strategy<Value = CommandWrapper<SignerTestState, SignerTestContext>> {
147185
Just(CommandWrapper::new(
148-
MineBitcoinBlockTenureChangeSecondaryMiner::new(ctx.miners.clone()),
186+
MineBitcoinBlockTenureChangeMiner2::new(ctx.miners.clone()),
149187
))
150188
}
151189
}

‎testnet/stacks-node/src/tests/signer/commands/block_commit.rs

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,22 +6,24 @@ use proptest::prelude::{Just, Strategy};
66
use super::context::{SignerTestContext, SignerTestState};
77
use crate::tests::signer::v0::MultipleMinerTest;
88

9-
pub struct SubmitBlockCommitSecondaryMiner {
9+
pub struct SubmitBlockCommitMiner2 {
1010
miners: Arc<Mutex<MultipleMinerTest>>,
1111
}
1212

13-
impl SubmitBlockCommitSecondaryMiner {
13+
impl SubmitBlockCommitMiner2 {
1414
pub fn new(miners: Arc<Mutex<MultipleMinerTest>>) -> Self {
1515
Self { miners }
1616
}
1717
}
1818

19-
impl Command<SignerTestState, SignerTestContext> for SubmitBlockCommitSecondaryMiner {
19+
impl Command<SignerTestState, SignerTestContext> for SubmitBlockCommitMiner2 {
2020
fn check(&self, state: &SignerTestState) -> bool {
2121
info!(
2222
"Checking: Submitting block commit miner 2. Result: {:?}",
2323
state.is_secondary_miner_skip_commit_op
2424
);
25+
// Ensure Miner 2's automatic commit ops are paused. If not, this may
26+
// result in no commit being submitted.
2527
state.is_secondary_miner_skip_commit_op
2628
}
2729

@@ -36,34 +38,36 @@ impl Command<SignerTestState, SignerTestContext> for SubmitBlockCommitSecondaryM
3638
}
3739

3840
fn label(&self) -> String {
39-
"SUBMIT_BLOCK_COMMIT_SECONDARY_MINER".to_string()
41+
"SUBMIT_BLOCK_COMMIT_MINER_2".to_string()
4042
}
4143

4244
fn build(
4345
ctx: Arc<SignerTestContext>,
4446
) -> impl Strategy<Value = CommandWrapper<SignerTestState, SignerTestContext>> {
45-
Just(CommandWrapper::new(SubmitBlockCommitSecondaryMiner::new(
47+
Just(CommandWrapper::new(SubmitBlockCommitMiner2::new(
4648
ctx.miners.clone(),
4749
)))
4850
}
4951
}
5052

51-
pub struct SubmitBlockCommitPrimaryMiner {
53+
pub struct SubmitBlockCommitMiner1 {
5254
miners: Arc<Mutex<MultipleMinerTest>>,
5355
}
5456

55-
impl SubmitBlockCommitPrimaryMiner {
57+
impl SubmitBlockCommitMiner1 {
5658
pub fn new(miners: Arc<Mutex<MultipleMinerTest>>) -> Self {
5759
Self { miners }
5860
}
5961
}
6062

61-
impl Command<SignerTestState, SignerTestContext> for SubmitBlockCommitPrimaryMiner {
63+
impl Command<SignerTestState, SignerTestContext> for SubmitBlockCommitMiner1 {
6264
fn check(&self, state: &SignerTestState) -> bool {
6365
info!(
6466
"Checking: Submitting block commit miner 1. Result: {:?}",
6567
state.is_primary_miner_skip_commit_op
6668
);
69+
// Ensure Miner 1's automatic commit ops are paused. If not, this may
70+
// result in no commit being submitted.
6771
state.is_primary_miner_skip_commit_op
6872
}
6973

@@ -78,13 +82,13 @@ impl Command<SignerTestState, SignerTestContext> for SubmitBlockCommitPrimaryMin
7882
}
7983

8084
fn label(&self) -> String {
81-
"SUBMIT_BLOCK_COMMIT_PRIMARY_MINER".to_string()
85+
"SUBMIT_BLOCK_COMMIT_MINER_1".to_string()
8286
}
8387

8488
fn build(
8589
ctx: Arc<SignerTestContext>,
8690
) -> impl Strategy<Value = CommandWrapper<SignerTestState, SignerTestContext>> {
87-
Just(CommandWrapper::new(SubmitBlockCommitPrimaryMiner::new(
91+
Just(CommandWrapper::new(SubmitBlockCommitMiner1::new(
8892
ctx.miners.clone(),
8993
)))
9094
}

‎testnet/stacks-node/src/tests/signer/commands/block_wait.rs

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,17 @@ use proptest::prelude::{Just, Strategy};
77
use super::context::{SignerTestContext, SignerTestState};
88
use crate::tests::signer::v0::{wait_for_block_pushed_by_miner_key, MultipleMinerTest};
99

10-
pub struct WaitForBlockFromMiner1 {
10+
pub struct WaitForTenureChangeBlockFromMiner1 {
1111
miners: Arc<Mutex<MultipleMinerTest>>,
1212
}
1313

14-
impl WaitForBlockFromMiner1 {
14+
impl WaitForTenureChangeBlockFromMiner1 {
1515
pub fn new(miners: Arc<Mutex<MultipleMinerTest>>) -> Self {
1616
Self { miners }
1717
}
1818
}
1919

20-
impl Command<SignerTestState, SignerTestContext> for WaitForBlockFromMiner1 {
20+
impl Command<SignerTestState, SignerTestContext> for WaitForTenureChangeBlockFromMiner1 {
2121
fn check(&self, state: &SignerTestState) -> bool {
2222
info!(
2323
"Checking: Waiting for Nakamoto block from miner 1. Result: {:?}",
@@ -53,29 +53,29 @@ impl Command<SignerTestState, SignerTestContext> for WaitForBlockFromMiner1 {
5353
}
5454

5555
fn label(&self) -> String {
56-
"WAIT_FOR_BLOCK_FROM_MINER_1".to_string()
56+
"WAIT_FOR_TENURE_CHANGE_BLOCK_FROM_MINER_1".to_string()
5757
}
5858

5959
fn build(
6060
ctx: Arc<SignerTestContext>,
6161
) -> impl Strategy<Value = CommandWrapper<SignerTestState, SignerTestContext>> {
62-
Just(CommandWrapper::new(WaitForBlockFromMiner1::new(
63-
ctx.miners.clone(),
64-
)))
62+
Just(CommandWrapper::new(
63+
WaitForTenureChangeBlockFromMiner1::new(ctx.miners.clone()),
64+
))
6565
}
6666
}
6767

68-
pub struct WaitForBlockFromMiner2 {
68+
pub struct WaitForTenureChangeBlockFromMiner2 {
6969
miners: Arc<Mutex<MultipleMinerTest>>,
7070
}
7171

72-
impl WaitForBlockFromMiner2 {
72+
impl WaitForTenureChangeBlockFromMiner2 {
7373
pub fn new(miners: Arc<Mutex<MultipleMinerTest>>) -> Self {
7474
Self { miners }
7575
}
7676
}
7777

78-
impl Command<SignerTestState, SignerTestContext> for WaitForBlockFromMiner2 {
78+
impl Command<SignerTestState, SignerTestContext> for WaitForTenureChangeBlockFromMiner2 {
7979
fn check(&self, state: &SignerTestState) -> bool {
8080
info!(
8181
"Checking: Waiting for Nakamoto block from miner 2. Result: {:?}",
@@ -112,14 +112,14 @@ impl Command<SignerTestState, SignerTestContext> for WaitForBlockFromMiner2 {
112112
}
113113

114114
fn label(&self) -> String {
115-
"WAIT_FOR_BLOCK_FROM_MINER_2".to_string()
115+
"WAIT_FOR_TENURE_CHANGE_BLOCK_FROM_MINER_2".to_string()
116116
}
117117

118118
fn build(
119119
ctx: Arc<SignerTestContext>,
120120
) -> impl Strategy<Value = CommandWrapper<SignerTestState, SignerTestContext>> {
121-
Just(CommandWrapper::new(WaitForBlockFromMiner2::new(
122-
ctx.miners.clone(),
123-
)))
121+
Just(CommandWrapper::new(
122+
WaitForTenureChangeBlockFromMiner2::new(ctx.miners.clone()),
123+
))
124124
}
125125
}

‎testnet/stacks-node/src/tests/signer/commands/boot.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ use madhouse::{Command, CommandWrapper};
44
use proptest::prelude::{Just, Strategy};
55

66
use super::context::{SignerTestContext, SignerTestState};
7-
use crate::tests::signer::v0::{get_chain_info_wrapper, MultipleMinerTest};
7+
use crate::tests::neon_integrations::get_chain_info;
8+
use crate::tests::signer::v0::MultipleMinerTest;
89

910
pub struct BootToEpoch3 {
1011
miners: Arc<Mutex<MultipleMinerTest>>,
@@ -31,7 +32,7 @@ impl Command<SignerTestState, SignerTestContext> for BootToEpoch3 {
3132
self.miners.lock().unwrap().boot_to_epoch_3();
3233

3334
let (conf_1, _) = self.miners.lock().unwrap().get_node_configs();
34-
let burn_block_height = get_chain_info_wrapper(&conf_1).burn_block_height;
35+
let burn_block_height = get_chain_info(&conf_1).burn_block_height;
3536

3637
assert_eq!(burn_block_height, 231);
3738

‎testnet/stacks-node/src/tests/signer/commands/commit_ops.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,19 +6,19 @@ use stacks::util::tests::TestFlag;
66

77
use super::context::{SignerTestContext, SignerTestState};
88

9-
pub struct SkipCommitOpPrimaryMiner {
9+
pub struct SkipCommitOpMiner1 {
1010
miner_1_skip_commit_flag: TestFlag<bool>,
1111
}
1212

13-
impl SkipCommitOpPrimaryMiner {
13+
impl SkipCommitOpMiner1 {
1414
pub fn new(miner_1_skip_commit_flag: TestFlag<bool>) -> Self {
1515
Self {
1616
miner_1_skip_commit_flag,
1717
}
1818
}
1919
}
2020

21-
impl Command<SignerTestState, SignerTestContext> for SkipCommitOpPrimaryMiner {
21+
impl Command<SignerTestState, SignerTestContext> for SkipCommitOpMiner1 {
2222
fn check(&self, state: &SignerTestState) -> bool {
2323
info!(
2424
"Checking: Skipping commit operations for miner 1. Result: {:?}",
@@ -36,31 +36,31 @@ impl Command<SignerTestState, SignerTestContext> for SkipCommitOpPrimaryMiner {
3636
}
3737

3838
fn label(&self) -> String {
39-
"SKIP_COMMIT_OP_PRIMARY_MINER".to_string()
39+
"SKIP_COMMIT_OP_MINER_1".to_string()
4040
}
4141

4242
fn build(
4343
ctx: Arc<SignerTestContext>,
4444
) -> impl Strategy<Value = CommandWrapper<SignerTestState, SignerTestContext>> {
45-
Just(CommandWrapper::new(SkipCommitOpPrimaryMiner::new(
45+
Just(CommandWrapper::new(SkipCommitOpMiner1::new(
4646
ctx.miners.lock().unwrap().get_primary_skip_commit_flag(),
4747
)))
4848
}
4949
}
5050

51-
pub struct SkipCommitOpSecondaryMiner {
51+
pub struct SkipCommitOpMiner2 {
5252
miner_2_skip_commit_flag: TestFlag<bool>,
5353
}
5454

55-
impl SkipCommitOpSecondaryMiner {
55+
impl SkipCommitOpMiner2 {
5656
pub fn new(miner_2_skip_commit_flag: TestFlag<bool>) -> Self {
5757
Self {
5858
miner_2_skip_commit_flag,
5959
}
6060
}
6161
}
6262

63-
impl Command<SignerTestState, SignerTestContext> for SkipCommitOpSecondaryMiner {
63+
impl Command<SignerTestState, SignerTestContext> for SkipCommitOpMiner2 {
6464
fn check(&self, state: &SignerTestState) -> bool {
6565
info!(
6666
"Checking: Skipping commit operations for miner 2. Result: {:?}",
@@ -78,13 +78,13 @@ impl Command<SignerTestState, SignerTestContext> for SkipCommitOpSecondaryMiner
7878
}
7979

8080
fn label(&self) -> String {
81-
"SKIP_COMMIT_OP_SECONDARY_MINER".to_string()
81+
"SKIP_COMMIT_OP_MINER_2".to_string()
8282
}
8383

8484
fn build(
8585
ctx: Arc<SignerTestContext>,
8686
) -> impl Strategy<Value = CommandWrapper<SignerTestState, SignerTestContext>> {
87-
Just(CommandWrapper::new(SkipCommitOpSecondaryMiner::new(
87+
Just(CommandWrapper::new(SkipCommitOpMiner2::new(
8888
ctx.miners.lock().unwrap().get_secondary_skip_commit_flag(),
8989
)))
9090
}

0 commit comments

Comments
 (0)