Skip to content

Commit d93f9d6

Browse files
asmaastarkwareasmaastarkware
committed
apollo_consensus,apollo_protobuf: use BlockNumber for Vote.height
1 parent 0045f6c commit d93f9d6

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
@@ -25,14 +25,15 @@ lazy_static! {
2525
ProposalInit { proposer: *PROPOSER_ID, ..Default::default() };
2626
static ref TIMEOUTS: TimeoutsConfig = TimeoutsConfig::default();
2727
}
28+
const HEIGHT_0: BlockNumber = BlockNumber(0);
2829
fn get_proposal_init_for_height(height: BlockNumber) -> ProposalInit {
2930
ProposalInit { height, ..*PROPOSAL_INIT }
3031
}
3132

3233
#[test]
3334
fn proposer() {
3435
let mut shc = SingleHeightConsensus::new(
35-
BlockNumber(0),
36+
HEIGHT_0,
3637
false,
3738
*PROPOSER_ID,
3839
VALIDATORS.to_vec(),
@@ -62,9 +63,12 @@ fn proposer() {
6263
));
6364

6465
// Receive two prevotes from other validators to reach prevote quorum.
65-
let _ = shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_1)).unwrap();
66-
let ret =
67-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
66+
let _ = shc
67+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_1))
68+
.unwrap();
69+
let ret = shc
70+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
71+
.unwrap();
6872
// Expect a precommit broadcast request present.
6973
let mut reqs = match ret {
7074
ShcReturn::Requests(r) => r,
@@ -77,10 +81,12 @@ fn proposer() {
7781
));
7882

7983
// Now provide precommit votes to reach decision.
80-
let _ =
81-
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_1)).unwrap();
82-
let decision =
83-
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
84+
let _ = shc
85+
.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_1))
86+
.unwrap();
87+
let decision = shc
88+
.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
89+
.unwrap();
8490
match decision {
8591
ShcReturn::Decision(d) => assert_eq!(d.block, BLOCK.id),
8692
_ => panic!("expected decision"),
@@ -90,10 +96,9 @@ fn proposer() {
9096
#[test_case(false; "single_proposal")]
9197
#[test_case(true; "repeat_proposal")]
9298
fn validator(repeat_proposal: bool) {
93-
const HEIGHT: BlockNumber = BlockNumber(0);
94-
let proposal_init = get_proposal_init_for_height(HEIGHT);
99+
let proposal_init = get_proposal_init_for_height(HEIGHT_0);
95100
let mut shc = SingleHeightConsensus::new(
96-
HEIGHT,
101+
HEIGHT_0,
97102
false,
98103
*VALIDATOR_ID_1,
99104
VALIDATORS.to_vec(),
@@ -135,9 +140,12 @@ fn validator(repeat_proposal: bool) {
135140
}
136141

137142
// Reach prevote quorum with two other validators.
138-
let _ = shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
139-
let ret =
140-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_3)).unwrap();
143+
let _ = shc
144+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
145+
.unwrap();
146+
let ret = shc
147+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_3))
148+
.unwrap();
141149
// Expect a precommit broadcast request present.
142150
let mut reqs = match ret {
143151
ShcReturn::Requests(r) => r,
@@ -149,9 +157,12 @@ fn validator(repeat_proposal: bool) {
149157
);
150158

151159
// Now provide precommit votes to reach decision.
152-
let _ = shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), 0, 0, *PROPOSER_ID)).unwrap();
153-
let decision =
154-
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
160+
let _ = shc
161+
.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *PROPOSER_ID))
162+
.unwrap();
163+
let decision = shc
164+
.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
165+
.unwrap();
155166
match decision {
156167
ShcReturn::Decision(d) => assert_eq!(d.block, BLOCK.id),
157168
_ => panic!("expected decision"),
@@ -161,10 +172,9 @@ fn validator(repeat_proposal: bool) {
161172
#[test_case(true; "repeat")]
162173
#[test_case(false; "equivocation")]
163174
fn vote_twice(same_vote: bool) {
164-
const HEIGHT: BlockNumber = BlockNumber(0);
165-
let proposal_init = get_proposal_init_for_height(HEIGHT);
175+
let proposal_init = get_proposal_init_for_height(HEIGHT_0);
166176
let mut shc = SingleHeightConsensus::new(
167-
HEIGHT,
177+
HEIGHT_0,
168178
false,
169179
*VALIDATOR_ID_1,
170180
VALIDATORS.to_vec(),
@@ -182,9 +192,9 @@ fn vote_twice(same_vote: bool) {
182192

183193
// Duplicate prevote should be ignored.
184194
let _ =
185-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT.0, 0, *PROPOSER_ID)).unwrap();
195+
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *PROPOSER_ID)).unwrap();
186196
let res = shc
187-
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT.0, 0, *VALIDATOR_ID_2))
197+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
188198
.unwrap();
189199
// On quorum of prevotes, expect a precommit broadcast request.
190200
let mut reqs = match res {
@@ -198,16 +208,16 @@ fn vote_twice(same_vote: bool) {
198208
));
199209

200210
// Precommit handling towards decision.
201-
let first_vote = precommit(Some(BLOCK.id.0), HEIGHT.0, 0, *PROPOSER_ID);
211+
let first_vote = precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *PROPOSER_ID);
202212
let _ = shc.handle_vote(&leader_fn, first_vote.clone()).unwrap();
203213
let second_vote = if same_vote {
204214
first_vote.clone()
205215
} else {
206-
precommit(Some(Felt::TWO), HEIGHT.0, 0, *PROPOSER_ID)
216+
precommit(Some(Felt::TWO), HEIGHT_0, 0, *PROPOSER_ID)
207217
};
208218
let _ = shc.handle_vote(&leader_fn, second_vote.clone()).unwrap();
209219
let decision =
210-
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT.0, 0, *VALIDATOR_ID_3));
220+
shc.handle_vote(&leader_fn, precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_3));
211221
match decision {
212222
Ok(ShcReturn::Decision(d)) => assert_eq!(d.block, BLOCK.id),
213223
_ => panic!("Expected decision"),
@@ -217,7 +227,7 @@ fn vote_twice(same_vote: bool) {
217227
#[test]
218228
fn rebroadcast_votes() {
219229
let mut shc = SingleHeightConsensus::new(
220-
BlockNumber(0),
230+
HEIGHT_0,
221231
false,
222232
*PROPOSER_ID,
223233
VALIDATORS.to_vec(),
@@ -241,9 +251,12 @@ fn rebroadcast_votes() {
241251
));
242252

243253
// Receive two prevotes from other validators to reach prevote quorum at round 0.
244-
let _ = shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_1)).unwrap();
245-
let ret =
246-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
254+
let _ = shc
255+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_1))
256+
.unwrap();
257+
let ret = shc
258+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
259+
.unwrap();
247260
// Expect a precommit broadcast at round 0.
248261
let reqs = match ret {
249262
ShcReturn::Requests(r) => r,
@@ -256,8 +269,8 @@ fn rebroadcast_votes() {
256269

257270
// Advance with NIL precommits from peers (no decision) -> expect scheduling of precommit
258271
// timeout.
259-
let _ = shc.handle_vote(&leader_fn, precommit(None, 0, 0, *VALIDATOR_ID_1)).unwrap();
260-
let _ = shc.handle_vote(&leader_fn, precommit(None, 0, 0, *VALIDATOR_ID_2)).unwrap();
272+
let _ = shc.handle_vote(&leader_fn, precommit(None, HEIGHT_0, 0, *VALIDATOR_ID_1)).unwrap();
273+
let _ = shc.handle_vote(&leader_fn, precommit(None, HEIGHT_0, 0, *VALIDATOR_ID_2)).unwrap();
261274

262275
// Timeout at precommit(0) -> expect a prevote broadcast for round 1.
263276
let ret = shc.handle_event(&leader_fn, StateMachineEvent::TimeoutPrecommit(0)).unwrap();
@@ -271,9 +284,12 @@ fn rebroadcast_votes() {
271284
)));
272285

273286
// Reach prevote quorum at round 1 with two other validators.
274-
let _ = shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 1, *VALIDATOR_ID_2)).unwrap();
275-
let ret =
276-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 1, *VALIDATOR_ID_3)).unwrap();
287+
let _ = shc
288+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 1, *VALIDATOR_ID_2))
289+
.unwrap();
290+
let ret = shc
291+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 1, *VALIDATOR_ID_3))
292+
.unwrap();
277293
let reqs = match ret {
278294
ShcReturn::Requests(r) => r,
279295
_ => panic!("expected requests"),
@@ -286,15 +302,15 @@ fn rebroadcast_votes() {
286302
// Rebroadcast with older vote (round 0) - should be ignored (no broadcast, no task).
287303
let ret = shc.handle_event(
288304
&leader_fn,
289-
StateMachineEvent::VoteBroadcasted(precommit(Some(BLOCK.id.0), 0, 0, *PROPOSER_ID)),
305+
StateMachineEvent::VoteBroadcasted(precommit(Some(BLOCK.id.0), HEIGHT_0, 0, *PROPOSER_ID)),
290306
);
291307
match ret.unwrap() {
292308
ShcReturn::Requests(r) => assert!(r.is_empty()),
293309
_ => panic!("expected requests"),
294310
}
295311

296312
// Rebroadcast with current round (round 1) - should broadcast.
297-
let rebroadcast_vote = precommit(Some(BLOCK.id.0), 0, 1, *PROPOSER_ID);
313+
let rebroadcast_vote = precommit(Some(BLOCK.id.0), HEIGHT_0, 1, *PROPOSER_ID);
298314
let ret = shc
299315
.handle_event(&leader_fn, StateMachineEvent::VoteBroadcasted(rebroadcast_vote.clone()))
300316
.unwrap();
@@ -312,7 +328,7 @@ fn rebroadcast_votes() {
312328
#[test]
313329
fn repropose() {
314330
let mut shc = SingleHeightConsensus::new(
315-
BlockNumber(0),
331+
HEIGHT_0,
316332
false,
317333
*PROPOSER_ID,
318334
VALIDATORS.to_vec(),
@@ -336,17 +352,19 @@ fn repropose() {
336352
_ => panic!("expected requests"),
337353
}
338354
// A single prevote from another validator does not yet cause quorum.
339-
let ret =
340-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_1)).unwrap();
355+
let ret = shc
356+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_1))
357+
.unwrap();
341358
// No new requests are expected at this point.
342359
match ret {
343360
ShcReturn::Requests(reqs) => assert!(reqs.is_empty()),
344361
_ => panic!("expected requests"),
345362
}
346363
// Reaching prevote quorum with a second external prevote; proposer will broadcast a precommit
347364
// and schedule a prevote timeout for round 0.
348-
let ret =
349-
shc.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), 0, 0, *VALIDATOR_ID_2)).unwrap();
365+
let ret = shc
366+
.handle_vote(&leader_fn, prevote(Some(BLOCK.id.0), HEIGHT_0, 0, *VALIDATOR_ID_2))
367+
.unwrap();
350368
// Expect ScheduleTimeoutPrevote{round:0} and BroadcastVote(Precommit).
351369
match ret {
352370
ShcReturn::Requests(reqs) => {
@@ -362,8 +380,8 @@ fn repropose() {
362380
}
363381
// receiving Nil precommit requests and then decision on new round; just assert no panic and
364382
// decisions arrive after quorum.
365-
let _ = shc.handle_vote(&leader_fn, precommit(None, 0, 0, *VALIDATOR_ID_1)).unwrap();
366-
let ret = shc.handle_vote(&leader_fn, precommit(None, 0, 0, *VALIDATOR_ID_2)).unwrap();
383+
let _ = shc.handle_vote(&leader_fn, precommit(None, HEIGHT_0, 0, *VALIDATOR_ID_1)).unwrap();
384+
let ret = shc.handle_vote(&leader_fn, precommit(None, HEIGHT_0, 0, *VALIDATOR_ID_2)).unwrap();
367385
// assert that ret is ScheduleTimeoutPrecommit
368386
match ret {
369387
ShcReturn::Requests(reqs) => {

‎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)