Skip to content

Commit cc10135

Browse files
asmaastarkwareasmaastarkware
authored
apollo_consensus: use BlockNumber for Vote.height (#10507)
1 parent 5685012 commit cc10135

File tree

13 files changed

+198
-162
lines changed

13 files changed

+198
-162
lines changed

‎crates/apollo_consensus/src/manager.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ impl<ContextT: ConsensusContext> ConsensusCache<ContextT> {
238238

239239
/// Caches a vote for a future height.
240240
fn cache_future_vote(&mut self, vote: Vote) {
241-
self.future_votes.entry(BlockNumber(vote.height)).or_default().push(vote);
241+
self.future_votes.entry(vote.height).or_default().push(vote);
242242
}
243243

244244
/// Caches a proposal for a future height.
@@ -666,7 +666,7 @@ impl<ContextT: ConsensusContext> MultiHeightManager<ContextT> {
666666
// TODO(matan): We need to figure out an actual caching strategy under 2 constraints:
667667
// 1. Malicious - must be capped so a malicious peer can't DoS us.
668668
// 2. Parallel proposals - we may send/receive a proposal for (H+1, 0).
669-
match message.height.cmp(&height.0) {
669+
match message.height.cmp(&height) {
670670
std::cmp::Ordering::Greater => {
671671
if self.should_cache_vote(&height, 0, &message) {
672672
trace!("Cache message for a future height. {:?}", message);
@@ -825,7 +825,7 @@ impl<ContextT: ConsensusContext> MultiHeightManager<ContextT> {
825825
&self,
826826
current_height: &BlockNumber,
827827
current_round: Round,
828-
msg_height: u64,
828+
msg_height: BlockNumber,
829829
msg_round: Round,
830830
msg_description: &str,
831831
) -> bool {
@@ -845,7 +845,7 @@ impl<ContextT: ConsensusContext> MultiHeightManager<ContextT> {
845845
msg_description,
846846
msg_height,
847847
msg_round,
848-
current_height.0,
848+
current_height,
849849
current_round,
850850
limits.future_height_limit,
851851
limits.future_height_round_limit,
@@ -865,7 +865,7 @@ impl<ContextT: ConsensusContext> MultiHeightManager<ContextT> {
865865
self.should_cache_msg(
866866
current_height,
867867
current_round,
868-
proposal.height.0,
868+
proposal.height,
869869
proposal.round,
870870
"proposal",
871871
)

‎crates/apollo_consensus/src/manager_test.rs‎

Lines changed: 100 additions & 99 deletions
Large diffs are not rendered by default.

‎crates/apollo_consensus/src/simulation_network_receiver_test.rs‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use apollo_network_types::network_types::BroadcastedMessageMetadata;
66
use apollo_protobuf::consensus::Vote;
77
use apollo_test_utils::{get_rng, GetTestInstance};
88
use futures::{SinkExt, StreamExt};
9+
use starknet_api::block::BlockNumber;
910
use test_case::test_case;
1011

1112
use super::NetworkReceiver;
@@ -31,7 +32,10 @@ async fn test_invalid(distinct_messages: bool) {
3132
let mut invalid_messages = 0;
3233

3334
for height in 0..1000 {
34-
let msg = Vote { height: if distinct_messages { height } else { 0 }, ..Default::default() };
35+
let msg = Vote {
36+
height: if distinct_messages { BlockNumber(height) } else { BlockNumber(0) },
37+
..Default::default()
38+
};
3539
let broadcasted_message_metadata =
3640
BroadcastedMessageMetadata::get_test_instance(&mut get_rng());
3741
mock_network
@@ -62,7 +66,10 @@ async fn test_drops(distinct_messages: bool) {
6266
let mut num_received = 0;
6367

6468
for height in 0..1000 {
65-
let msg = Vote { height: if distinct_messages { height } else { 0 }, ..Default::default() };
69+
let msg = Vote {
70+
height: if distinct_messages { BlockNumber(height) } else { BlockNumber(0) },
71+
..Default::default()
72+
};
6673
let broadcasted_message_metadata =
6774
BroadcastedMessageMetadata::get_test_instance(&mut get_rng());
6875
mock_network

‎crates/apollo_consensus/src/single_height_consensus_test.rs‎

Lines changed: 61 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,15 @@ lazy_static! {
2626
ProposalInit { proposer: *PROPOSER_ID, ..Default::default() };
2727
static ref TIMEOUTS: TimeoutsConfig = TimeoutsConfig::default();
2828
}
29+
const HEIGHT_0: BlockNumber = BlockNumber(0);
2930
fn get_proposal_init_for_height(height: BlockNumber) -> ProposalInit {
3031
ProposalInit { height, ..*PROPOSAL_INIT }
3132
}
3233

3334
#[test]
3435
fn proposer() {
3536
let mut shc = SingleHeightConsensus::new(
36-
BlockNumber(0),
37+
HEIGHT_0,
3738
false,
3839
*PROPOSER_ID,
3940
VALIDATORS.to_vec(),
@@ -58,9 +59,12 @@ fn proposer() {
5859
});
5960

6061
// Receive two prevotes from other validators to reach prevote quorum.
61-
let _ = shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_1)).unwrap();
62-
let ret =
63-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
62+
let _ = shc
63+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_1))
64+
.unwrap();
65+
let ret = shc
66+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
67+
.unwrap();
6468
// Expect a precommit broadcast request present.
6569
assert_matches!(ret, ShcReturn::Requests(mut reqs) => {
6670
assert_matches!(reqs.pop_front(), Some(SMRequest::ScheduleTimeout(Step::Prevote, 0)));
@@ -69,10 +73,12 @@ fn proposer() {
6973
});
7074

7175
// Now provide precommit votes to reach decision.
72-
let _ =
73-
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_1)).unwrap();
74-
let decision =
75-
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
76+
let _ = shc
77+
.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_1))
78+
.unwrap();
79+
let decision = shc
80+
.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
81+
.unwrap();
7682
match decision {
7783
ShcReturn::Decision(d) => assert_eq!(d.block, BLOCK.id),
7884
_ => panic!("expected decision"),
@@ -82,10 +88,9 @@ fn proposer() {
8288
#[test_case(false; "single_proposal")]
8389
#[test_case(true; "repeat_proposal")]
8490
fn validator(repeat_proposal: bool) {
85-
const HEIGHT: BlockNumber = BlockNumber(0);
86-
let proposal_init = get_proposal_init_for_height(HEIGHT);
91+
let proposal_init = get_proposal_init_for_height(HEIGHT_0);
8792
let mut shc = SingleHeightConsensus::new(
88-
HEIGHT,
93+
HEIGHT_0,
8994
false,
9095
*VALIDATOR_ID_1,
9196
VALIDATORS.to_vec(),
@@ -120,9 +125,12 @@ fn validator(repeat_proposal: bool) {
120125
}
121126

122127
// Reach prevote quorum with two other validators.
123-
let _ = shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
124-
let ret =
125-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_3)).unwrap();
128+
let _ = shc
129+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
130+
.unwrap();
131+
let ret = shc
132+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_3))
133+
.unwrap();
126134
// Expect a precommit broadcast request present.
127135
assert_matches!(ret, ShcReturn::Requests(mut reqs) => {
128136
assert_matches!(reqs.pop_front(), Some(SMRequest::ScheduleTimeout(Step::Prevote, 0)));
@@ -131,9 +139,12 @@ fn validator(repeat_proposal: bool) {
131139
});
132140

133141
// Now provide precommit votes to reach decision.
134-
let _ = shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), 0, 0, *PROPOSER_ID)).unwrap();
135-
let decision =
136-
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
142+
let _ = shc
143+
.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *PROPOSER_ID))
144+
.unwrap();
145+
let decision = shc
146+
.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
147+
.unwrap();
137148
match decision {
138149
ShcReturn::Decision(d) => assert_eq!(d.block, BLOCK.id),
139150
_ => panic!("expected decision"),
@@ -143,10 +154,9 @@ fn validator(repeat_proposal: bool) {
143154
#[test_case(true; "repeat")]
144155
#[test_case(false; "equivocation")]
145156
fn vote_twice(same_vote: bool) {
146-
const HEIGHT: BlockNumber = BlockNumber(0);
147-
let proposal_init = get_proposal_init_for_height(HEIGHT);
157+
let proposal_init = get_proposal_init_for_height(HEIGHT_0);
148158
let mut shc = SingleHeightConsensus::new(
149-
HEIGHT,
159+
HEIGHT_0,
150160
false,
151161
*VALIDATOR_ID_1,
152162
VALIDATORS.to_vec(),
@@ -163,9 +173,9 @@ fn vote_twice(same_vote: bool) {
163173
.unwrap();
164174

165175
let _ =
166-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT.0, 0, *PROPOSER_ID)).unwrap();
176+
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *PROPOSER_ID)).unwrap();
167177
let res = shc
168-
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT.0, 0, *VALIDATOR_ID_2))
178+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
169179
.unwrap();
170180
// On quorum of prevotes, expect a precommit broadcast request.
171181
assert_matches!(res, ShcReturn::Requests(mut reqs) => {
@@ -175,12 +185,12 @@ fn vote_twice(same_vote: bool) {
175185
});
176186

177187
// Precommit handling towards decision.
178-
let first_vote = precommit(Some(BLOCK.id.0), HEIGHT.0, 0, *PROPOSER_ID);
188+
let first_vote = precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *PROPOSER_ID);
179189
let _ = shc.handle_vote(&leader_fn, first_vote.clone()).unwrap();
180190
let second_vote = if same_vote {
181191
first_vote.clone()
182192
} else {
183-
precommit(Some(Felt::TWO), HEIGHT.0, 0, *PROPOSER_ID)
193+
precommit(Some(Felt::TWO), HEIGHT_0, 0, *PROPOSER_ID)
184194
};
185195
// When same_vote is true, this is a duplicate precommit from PROPOSER_ID (same as first_vote).
186196
// When same_vote is false, this is an equivocation (different vote from PROPOSER_ID).
@@ -192,7 +202,7 @@ fn vote_twice(same_vote: bool) {
192202
let res = shc.handle_vote(&leader_fn, second_vote.clone()).unwrap();
193203
assert_matches!(res, ShcReturn::Requests(r) if r.is_empty());
194204
let decision =
195-
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT.0, 0, *VALIDATOR_ID_3));
205+
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_3));
196206
match decision {
197207
Ok(ShcReturn::Decision(d)) => assert_eq!(d.block, BLOCK.id),
198208
_ => panic!("Expected decision"),
@@ -202,7 +212,7 @@ fn vote_twice(same_vote: bool) {
202212
#[test]
203213
fn rebroadcast_votes() {
204214
let mut shc = SingleHeightConsensus::new(
205-
BlockNumber(0),
215+
HEIGHT_0,
206216
false,
207217
*PROPOSER_ID,
208218
VALIDATORS.to_vec(),
@@ -222,9 +232,12 @@ fn rebroadcast_votes() {
222232
});
223233

224234
// Receive two prevotes from other validators to reach prevote quorum at round 0.
225-
let _ = shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_1)).unwrap();
226-
let ret =
227-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
235+
let _ = shc
236+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_1))
237+
.unwrap();
238+
let ret = shc
239+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
240+
.unwrap();
228241
// Expect a precommit broadcast at round 0.
229242
assert_matches!(ret, ShcReturn::Requests(mut reqs) => {
230243
assert_matches!(reqs.pop_front(), Some(SMRequest::ScheduleTimeout(Step::Prevote, 0)));
@@ -234,8 +247,8 @@ fn rebroadcast_votes() {
234247

235248
// Advance with NIL precommits from peers (no decision) -> expect scheduling of precommit
236249
// timeout.
237-
let _ = shc.handle_vote(&leader_fn, precommit(None, 0, 0, *VALIDATOR_ID_1)).unwrap();
238-
let _ = shc.handle_vote(&leader_fn, precommit(None, 0, 0, *VALIDATOR_ID_2)).unwrap();
250+
let _ = shc.handle_vote(&leader_fn, precommit(None, HEIGHT_0, 0, *VALIDATOR_ID_1)).unwrap();
251+
let _ = shc.handle_vote(&leader_fn, precommit(None, HEIGHT_0, 0, *VALIDATOR_ID_2)).unwrap();
239252

240253
// Timeout at precommit(0) -> expect a prevote broadcast for round 1.
241254
let ret = shc.handle_event(&leader_fn, StateMachineEvent::TimeoutPrecommit(0)).unwrap();
@@ -246,9 +259,12 @@ fn rebroadcast_votes() {
246259
});
247260

248261
// Reach prevote quorum at round 1 with two other validators.
249-
let _ = shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 1, *VALIDATOR_ID_2)).unwrap();
250-
let ret =
251-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 1, *VALIDATOR_ID_3)).unwrap();
262+
let _ = shc
263+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 1, *VALIDATOR_ID_2))
264+
.unwrap();
265+
let ret = shc
266+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 1, *VALIDATOR_ID_3))
267+
.unwrap();
252268
assert_matches!(ret, ShcReturn::Requests(mut reqs) => {
253269
assert_matches!(reqs.pop_front(), Some(SMRequest::ScheduleTimeout(Step::Prevote, 1)));
254270
assert_matches!(reqs.pop_front(), Some(SMRequest::BroadcastVote(v)) if v.vote_type == VoteType::Precommit && v.round == 1);
@@ -258,12 +274,12 @@ fn rebroadcast_votes() {
258274
// Rebroadcast with older vote (round 0) - should be ignored (no broadcast, no task).
259275
let ret = shc.handle_event(
260276
&leader_fn,
261-
StateMachineEvent::VoteBroadcasted(precommit(Some(BLOCK.id.0), 0, 0, *PROPOSER_ID)),
277+
StateMachineEvent::VoteBroadcasted(precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *PROPOSER_ID)),
262278
);
263279
assert_matches!(ret.unwrap(), ShcReturn::Requests(r) if r.is_empty());
264280

265281
// Rebroadcast with current round (round 1) - should broadcast.
266-
let rebroadcast_vote = precommit(Some(BLOCK.id.0), 0, 1, *PROPOSER_ID);
282+
let rebroadcast_vote = precommit(Some(BLOCK.id.0), HEIGHT_0, 1, *PROPOSER_ID);
267283
let ret = shc
268284
.handle_event(&leader_fn, StateMachineEvent::VoteBroadcasted(rebroadcast_vote.clone()))
269285
.unwrap();
@@ -276,7 +292,7 @@ fn rebroadcast_votes() {
276292
#[test]
277293
fn repropose() {
278294
let mut shc = SingleHeightConsensus::new(
279-
BlockNumber(0),
295+
HEIGHT_0,
280296
false,
281297
*PROPOSER_ID,
282298
VALIDATORS.to_vec(),
@@ -295,14 +311,16 @@ fn repropose() {
295311
assert!(reqs.is_empty());
296312
});
297313
// A single prevote from another validator does not yet cause quorum.
298-
let ret =
299-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_1)).unwrap();
314+
let ret = shc
315+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_1))
316+
.unwrap();
300317
// No new requests are expected at this point.
301318
assert_matches!(ret, ShcReturn::Requests(reqs) if reqs.is_empty());
302319
// Reaching prevote quorum with a second external prevote; proposer will broadcast a precommit
303320
// and schedule a prevote timeout for round 0.
304-
let ret =
305-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
321+
let ret = shc
322+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
323+
.unwrap();
306324
// Expect ScheduleTimeout(Step::Prevote, 0) and BroadcastVote(Precommit).
307325
assert_matches!(ret, ShcReturn::Requests(mut reqs) => {
308326
assert_matches!(reqs.pop_front(), Some(SMRequest::ScheduleTimeout(Step::Prevote, 0)));
@@ -311,8 +329,8 @@ fn repropose() {
311329
});
312330
// receiving Nil precommit requests and then decision on new round; just assert no panic and
313331
// decisions arrive after quorum.
314-
let _ = shc.handle_vote(&leader_fn, precommit(None, 0, 0, *VALIDATOR_ID_1)).unwrap();
315-
let ret = shc.handle_vote(&leader_fn, precommit(None, 0, 0, *VALIDATOR_ID_2)).unwrap();
332+
let _ = shc.handle_vote(&leader_fn, precommit(None, HEIGHT_0, 0, *VALIDATOR_ID_1)).unwrap();
333+
let ret = shc.handle_vote(&leader_fn, precommit(None, HEIGHT_0, 0, *VALIDATOR_ID_2)).unwrap();
316334
// assert that ret is ScheduleTimeoutPrecommit
317335
assert_matches!(ret, ShcReturn::Requests(mut reqs) => {
318336
assert_matches!(reqs.pop_front(), Some(SMRequest::ScheduleTimeout(Step::Precommit, 0)));

‎crates/apollo_consensus/src/state_machine.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ impl StateMachine {
198198
) -> VecDeque<SMRequest> {
199199
let vote = Vote {
200200
vote_type,
201-
height: self.height.0,
201+
height: self.height,
202202
round: self.round,
203203
proposal_commitment,
204204
voter: self.id,

‎crates/apollo_consensus/src/state_machine_test.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ fn mk_vote(
2828
proposal_id: Option<ProposalCommitment>,
2929
voter: ValidatorId,
3030
) -> Vote {
31-
Vote { vote_type, height: HEIGHT.0, round, proposal_commitment: proposal_id, voter }
31+
Vote { vote_type, height: HEIGHT, round, proposal_commitment: proposal_id, voter }
3232
}
3333

3434
struct TestWrapper<LeaderFn: Fn(Round) -> ValidatorId> {

‎crates/apollo_consensus/src/test_utils.rs‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,17 +101,27 @@ mock! {
101101
}
102102
}
103103

104-
pub fn prevote(block_felt: Option<Felt>, height: u64, round: u32, voter: ValidatorId) -> Vote {
104+
pub fn prevote(
105+
block_felt: Option<Felt>,
106+
height: BlockNumber,
107+
round: u32,
108+
voter: ValidatorId,
109+
) -> Vote {
105110
let proposal_commitment = block_felt.map(ProposalCommitment);
106111
Vote { vote_type: VoteType::Prevote, height, round, proposal_commitment, voter }
107112
}
108113

109-
pub fn precommit(block_felt: Option<Felt>, height: u64, round: u32, voter: ValidatorId) -> Vote {
114+
pub fn precommit(
115+
block_felt: Option<Felt>,
116+
height: BlockNumber,
117+
round: u32,
118+
voter: ValidatorId,
119+
) -> Vote {
110120
let proposal_commitment = block_felt.map(ProposalCommitment);
111121
Vote { vote_type: VoteType::Precommit, height, round, proposal_commitment, voter }
112122
}
113-
pub fn proposal_init(height: u64, round: u32, proposer: ValidatorId) -> ProposalInit {
114-
ProposalInit { height: BlockNumber(height), round, proposer, ..Default::default() }
123+
pub fn proposal_init(height: BlockNumber, round: u32, proposer: ValidatorId) -> ProposalInit {
124+
ProposalInit { height, round, proposer, ..Default::default() }
115125
}
116126

117127
#[derive(Debug)]

‎crates/apollo_consensus_orchestrator/src/sequencer_consensus_context.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,6 @@ impl ConsensusContext for SequencerConsensusContext {
494494
let transactions;
495495
let block_info;
496496
{
497-
let height = BlockNumber(height);
498497
let mut proposals = self.valid_proposals.lock().unwrap();
499498
(block_info, transactions, proposal_id) =
500499
proposals.get_proposal(&height, &block).clone();
@@ -549,7 +548,7 @@ impl ConsensusContext for SequencerConsensusContext {
549548
let sequencer = SequencerContractAddress(block_info.builder);
550549

551550
let block_header_without_hash = BlockHeaderWithoutHash {
552-
block_number: BlockNumber(height),
551+
block_number: height,
553552
l1_gas_price,
554553
l1_data_gas_price,
555554
l2_gas_price,

0 commit comments

Comments
 (0)