[Bug] imprecision in test function is_round_reached()

[bendyarm]

🐛 Bug Report

This bug does not affect mainnet. It is a bug in a test function
that may cause false positives or negatives from the tests that use it.
However, there are other more complete test suites that
are being run and passed, so this bug is minor.

The bug is in this function:

    // Checks if at least 2f + 1 nodes have reached the given round.
    pub fn is_round_reached(&self, round: u64) -> bool {
        let quorum_threshold = self.validators.len() / 2 + 1;
        self.validators.values().filter(|v| v.primary.current_round() >= round).count() >= quorum_threshold
    }

There are 12 tests that use this test function permalink here, but this function is not doing what it claims.

I presume validators here are equal (not stake-weighted).
If there are N validators, f should be
(N - 1)/3 (where / means integer division rounding down),
and quorum threshold should. be N - f (not 2f + 1 as mentioned in the comment),
which can be computed from N as 2N/3 + 1 (same meaning of /, integer division).

For example:
For 4 validators, this function will return 3, which is correct.
For 5 validators, this function will return 3, which is incorrect.
When there are 5 validators, the quorum threshold should be 4.

See also the definition of quorum_threshold() in snarkVM
ledger/committee/src/lib.rs
permalink
(note the comment at the permalink still has 2f + 1, but that should be fixed with this PR.
The code does the correct calculation.

[ungaro]

I've submitted a pull request to address this issue:

Fix: Correct quorum threshold calculation in is_round_reached() #3384

This PR updates the quorum threshold calculation to ensure proper Byzantine fault tolerance. The new implementation correctly calculates the quorum as N - f where N is the number of validators and f is the maximum number of faulty validators that can be tolerated.

The PR includes:

• Updated implementation of is_round_reached()
• Corrected comments explaining the Byzantine fault tolerance calculation
• Comparison of results between the old and new implementations

Thanks

@bendyarm @d0cd

[CX555536]

It is a bug in a test function that may cause false positives or negatives from the tests that use it.
However, there are other more complete test suites that are being run and passed, so this bug is minor.

[ungaro]

It is a bug in a test function that may cause false positives or negatives from the tests that use it. However, there are other more complete test suites that are being run and passed, so this bug is minor.

Thank you for the feedback. I appreciate the context about the bug's impact and the existence of more comprehensive test suites. As I'm getting familiar with the codebase, I wanted to focus on addressing some smaller issues first. Will try to do more high impact PR's soon.

[niklaslong]

Just commenting to confirm the impact is minor here—we run these cases with N = 4 and so the threshold is correct despite the imprecise calculation.