apollo_consensus: add faulty nodes to the sim test

[asmaastarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[asmaastarkware]

• apollo_consensus: use instance fields instead of constants in sim test #11035
• apollo_consensus: add faulty nodes to the sim test #10964 👈 (View in Graphite)
• apollo_consensus: update consensus sim test to simulate network msg drops #10869 : 1 other dependent PR (#11016 )
• apollo_consensus: add honest nodes consensus simulation #10868
• main-v0.14.1-committer

This stack of pull requests is managed by Graphite. Learn more about stacking.

[matanl-starkware]

@matanl-starkware reviewed 1 file and all commit messages, and made 10 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).

---

crates/apollo_consensus/src/simulation_test.rs line 99 at r1 (raw file):

/// Generates a "fake" commitment for equivocation/conflict attacks.
fn fake_commitment_for_round(round: Round) -> ProposalCommitment {

Consider using existing proposal_commitment_for_round() fn, with an additional parameter

Code quote:

fake_commitment_for_round

---

crates/apollo_consensus/src/simulation_test.rs line 134 at r1 (raw file):

    round_stats: HashMap<(Round, ProposalCommitment), (HashSet<Vote>, bool)>,
    /// Tracks which voters have already voted for each round (to detect conflicts/duplicates).
    voter_first_vote: HashSet<(Round, ValidatorId)>,

Wouldn't it be easier to keep a HashMap with the value of commitment, to easily count equivocators?

Code quote:

HashSet<(Round, ValidatorId)>

---

crates/apollo_consensus/src/simulation_test.rs line 139 at r1 (raw file):

impl DiscreteEventSimulation {
    fn new(total_nodes: usize, honest_nodes: usize, seed: u64, keep_ratio: f64) -> Self {
        assert!(honest_nodes <= total_nodes, "honest_nodes must be <= total_nodes");

You can also verify: 0<= keep_ratio <= 1

---

crates/apollo_consensus/src/simulation_test.rs line 173 at r1 (raw file):

    /// Uses deterministic randomness based on node index and round.
    fn get_fault_type(&mut self, node_idx: usize, round: Round) -> FaultType {
        let node_idx_u64 = u64::try_from(node_idx).unwrap_or(0);

It should never be a problem, unless there's a bug.

Suggestion:

unwrap()

---

crates/apollo_consensus/src/simulation_test.rs line 184 at r1 (raw file):

        // Randomly select a fault type
        let fault_types = [

Suggestion:

const FAULT_TYPES: &[FaultType] = &[

---

crates/apollo_consensus/src/simulation_test.rs line 191 at r1 (raw file):

            FaultType::NonValidator,
        ];
        let idx = fault_rng.gen_range(0..fault_types.len());

use rand::seq::SliceRandom;

Suggestion:

let fault = *FAULT_TYPES.choose(&mut fault_rng).unwrap();

---

crates/apollo_consensus/src/simulation_test.rs line 273 at r1 (raw file):

        // Handle leader proposal (only if leader is honest and not node 0)
        if leader_idx != 0 && leader_idx < self.honest_nodes {

Define as const

Code quote:

0

---

crates/apollo_consensus/src/simulation_test.rs line 273 at r1 (raw file):

        // Handle leader proposal (only if leader is honest and not node 0)
        if leader_idx != 0 && leader_idx < self.honest_nodes {

If self.honest_nodes = 2, you want to allow leader_idx [1,2] to propose.

Suggestion:

<= 

---

crates/apollo_consensus/src/simulation_test.rs line 315 at r1 (raw file):

                self.schedule_prevote_and_precommit(node_id, round, proposal_commitment);
                let conflicting_commitment = Some(fake_commitment_for_round(round));
                self.schedule_prevote_and_precommit(node_id, round, conflicting_commitment);

Order of voting matters?
Consider sometimes sending the erroneous vote first

Code quote:

                self.schedule_prevote_and_precommit(node_id, round, proposal_commitment);
                let conflicting_commitment = Some(fake_commitment_for_round(round));
                self.schedule_prevote_and_precommit(node_id, round, conflicting_commitment);

---

crates/apollo_consensus/src/simulation_test.rs line 352 at r1 (raw file):

        if !self.validators.contains(&vote.voter) {
            return;
        }

Since you know how it was implemented, you can compare the id vs. TOTAL_NODES.
Current check is O(n)

Code quote:

        if !self.validators.contains(&vote.voter) {
            return;
        }

[asmaastarkware]

@asmaastarkware made 8 comments.
Reviewable status: 0 of 1 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware).

---

crates/apollo_consensus/src/simulation_test.rs line 99 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Consider using existing proposal_commitment_for_round() fn, with an additional parameter

Done.

---

crates/apollo_consensus/src/simulation_test.rs line 134 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Wouldn't it be easier to keep a HashMap with the value of commitment, to easily count equivocators?

For equivocation we count the first vote not the second.. the value doesn't matter

---

crates/apollo_consensus/src/simulation_test.rs line 139 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

You can also verify: 0<= keep_ratio <= 1

Done.

---

crates/apollo_consensus/src/simulation_test.rs line 173 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

It should never be a problem, unless there's a bug.

Done.

---

crates/apollo_consensus/src/simulation_test.rs line 273 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Define as const

Done.

---

crates/apollo_consensus/src/simulation_test.rs line 273 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

If self.honest_nodes = 2, you want to allow leader_idx [1,2] to propose.

Done.

---

crates/apollo_consensus/src/simulation_test.rs line 315 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Order of voting matters?
Consider sometimes sending the erroneous vote first

Done.

---

crates/apollo_consensus/src/simulation_test.rs line 352 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Since you know how it was implemented, you can compare the id vs. TOTAL_NODES.
Current check is O(n)

voter is a ContractAddress :(

[matanl-starkware]

@matanl-starkware reviewed 1 file and all commit messages, made 3 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).

---

crates/apollo_consensus/src/simulation_test.rs line 352 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

voter is a ContractAddress :(

Maybe add TODO (keep validators as set)?

---

crates/apollo_consensus/src/simulation_test.rs line 309 at r2 (raw file):

            FaultType::EquivocatorRealFirst => {
                self.schedule_prevote_and_precommit(node_id, round, proposal_commitment);
                let conflicting_commitment = Some(proposal_commitment_for_round(round, true));

Consider moving outside the match, for clarity.

Code quote:

let conflicting_commitment = Some(proposal_commitment_for_round(round, true));

[matanl-starkware]

@matanl-starkware reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @dafnamatsry).

[matanl-starkware]

@matanl-starkware reviewed 1 file and all commit messages, made 3 comments, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry).

---

crates/apollo_consensus/src/simulation_test.rs line 310 at r8 (raw file):

                let proposal_tick = (round_start_tick
                    + self.rng.gen_range(PROPOSAL_ARRIVAL_DELAY_RANGE))
                .min(round_start_tick + ROUND_DURATION);

No need to leave some spare time for the votes?

Code quote:

ROUND_DURATION

---

crates/apollo_consensus/src/simulation_test.rs line 434 at r8 (raw file):

        if let Some(commitment) = commitment {
            let key = (round, commitment);
            let entry = self.round_stats.entry(key).or_insert_with(|| (HashSet::new(), false));

Consider extracting to a helper function to be also used in track_precommit

Code quote:

            let key = (round, commitment);
            let entry = self.round_stats.entry(key).or_insert_with(|| (HashSet::new(), false));

---

crates/apollo_consensus/src/simulation_test.rs line 593 at r8 (raw file):

            if total_precommits >= THRESHOLD {
                Some((*r, *commitment, precommits.clone()))

I'm not sure the clone is necessary

Code quote:

.clone()

[dafnamatsry]

@dafnamatsry reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @asmaastarkware).