ValidatorSet::sort_validators fails to deduplicate addresses and Validator violates Ord contract

[Yudis-bit]

Problem

At commit 6e764023ee6515fe70573e123ed2db912a7207b4, ValidatorSet::sort_validators in crates/types/src/validator_set.rs attempts in-place sorting and deduplication. However, it sorts primarily by descending voting power before calling vals.dedup(). Because dedup() only eliminates consecutive elements equal under PartialEq, duplicate validator address entries with different voting powers remain non-adjacent and are never removed. Even when voting powers match, any difference in public keys causes PartialEq to evaluate to false, retaining both entries.

This leaves multiple entries with the same address in ValidatorSet. total_voting_power() iterates over every entry and sums their voting power, whereas vote accumulation in consensus engines keys voters by unique Address and ValidatorSet::get_by_address() only yields the first match. This phantom voting power inflates the denominator of the 2/3 + 1 supermajority calculation with uncastable voting weight. If duplicate power exceeds 1/3 of the aggregate, the consensus round cannot achieve supermajority even if all valid validators precommit, producing an unrecoverable chain halt.

Additionally, Validator derives PartialEq across address, public_key, and voting_power, but manually implements Ord solely by comparing self.address.cmp(&other.address). This violates the standard library Ord contract where a.cmp(&b) == Equal must hold if and only if a == b. When two entries share an address but carry different voting powers, cmp returns Equal while PartialEq returns false, causing undefined ordering behavior in sorted slices and standard collections.

Furthermore, aggregate voting power overflow is checked only during total_voting_power(), deferring integer overflow panics to runtime execution inside consensus state transitions instead of validating at construction in ValidatorSet::new.

Reproduction

Run the following test case against crates/types/src/validator_set.rs:

#[test]
fn sort_validators_deduplicates_by_address() {
    let mut rng = StdRng::seed_from_u64(0x42);
    let sk = PrivateKey::generate(&mut rng);

    let v1 = Validator::new(sk.public_key(), 10);
    let v2 = Validator::new(sk.public_key(), 20);

    let vs = ValidatorSet::new(vec![v1.clone(), v2.clone()]);
    assert_eq!(vs.len(), 1);
    assert_eq!(vs.total_voting_power(), 20);
}

On current main, this assertion fails:

assertion `left == right` failed
  left: 2
 right: 1

The validator set retains both entries, and total_voting_power() evaluates to 30 instead of 20.

Expected Behavior

ValidatorSet::new must ensure validator address identities are unique in the resulting set, keeping the highest voting power entry for any address and reflecting only true castable power in total_voting_power(). Validator must implement total ordering consistent with equality, and aggregate voting power overflow must fail fast during construction.

Proposed Fix

Derive PartialOrd and Ord directly on Validator. In ValidatorSet::sort_validators, sort by descending voting power with Validator::cmp tie-breaking, then retain unique addresses in linear time using a HashSet<Address>. Validate non-overflow of total voting power at construction time in ValidatorSet::new.

I have the fix and regression test suite ready on a local branch. Could a maintainer please assign this issue to me so I can submit a PR? Thanks!

[Yudis-bit]

Requesting assignment to @Yudis-bit for this issue, per CONTRIBUTING.md.

The fix and regression tests are implemented and verified in Yudis-bit:fix/validator-set-dedup-ord (commit c93d26e). All 181 unit tests and 31 integration tests pass cleanly under cargo test -p arc-consensus-types, alongside passing cargo clippy -p arc-consensus-types -- -D warnings and
ustfmt --check.

Could a maintainer please assign this issue to me so the PR can be submitted? Thanks!

[romac]

Thanks for the detailed report, @Yudis-bit!

This can't happen in production: ValidatorRegistry.registerValidator is owner-only and rejects duplicate public keys, and validator addresses are derived from those keys, so the active set the consensus layer reads never contains two entries with the same address. We're still hardening ValidatorSet::new against duplicate addresses as defense-in-depth and you can expect to see this roll out in the next few releases.

[osr21]

Thanks, this production-path clarification is useful. In the current source at 6e76402, get_signing_validator_set queries getActiveValidatorSet at H−1 and the ABI decoder passes the entries to ValidatorSet::new. ValidatorRegistry.registerValidator is onlyOwner and rejects an already-registered public-key hash (registry guard; consensus lookup; decoder). This supports the conclusion that the reported same-key/different-power reproduction cannot enter through the normal registry path, assuming the deployed registry matches this source.

Two code-level points still need scope: Validator derives Eq over address, public_key, and voting_power, while Ord::cmp compares only address; and ValidatorSet::new does not validate aggregate voting power, so total_voting_power can still panic on overflow when called. Will the planned hardening cover these too, and will duplicate addresses be rejected or normalized under a defined rule? Also, should @Yudis-bit submit the ready branch, or wait for the internal fix?