UniqueVotesWithSameTarget caller contract: cmpVotes must be seat-index-aware

[dnadales]

Problem

PR #2020 made UniqueVotesWithSameTarget a properly opaque carrier whose construction checks for duplicates, replacing the silent-dedup hazard at the type level (option (a) of the original forgeCert dedup concern).
The remaining concern is at the caller side: uniqueness is parameterized by the cmpVotes :: Vote crypto committee -> Vote crypto committee -> Ordering function the caller supplies, so the duplicate-rejection guarantee is only as strong as that comparator.

For WFALS and EveryoneVotes the underlying hazard is two votes sharing a SeatIndex but differing in signature / VRF output:

• WFALS.implForgeCert (WFALS.hs:456) calls NEMap.fromAscList voters while signatures are aggregated separately.
  If two voters entries share a SeatIndex, the cert's seat-index map has fewer entries than the signature aggregate, and downstream verifyCert fails with the opaque "number of keys and signatures do not match" error.
• EveryoneVotes.implForgeCert (EveryoneVotes.hs:265) has the same shape via NESet.fromList voters.

The Peras integration site that builds UniqueVotesWithSameTarget from inbound votes must therefore choose a cmpVotes that returns EQ for any two votes that would collide at the seat-index level (or, more conservatively, on underlying voter identity).

Proposed fix

• Locate the call site that builds UniqueVotesWithSameTarget for the Peras voting committee.
• Confirm cmpVotes orders EQ on shared SeatIndex for both persistent and non-persistent variants of WFALS and EveryoneVotes Vote constructors.
• Add a property test that generates votes with seeded seat-index collisions and asserts ensureUniqueVotesWithSameTarget returns Left (DuplicateVotes …) for them.

References

• ouroboros-consensus PR #1975 reviews r3131282442 and r3131290517
• Closing of option (a) in PR #2020

[tbagrel1]

uniqueness is parameterized by the cmpVotes :: Vote crypto committee -> Vote crypto committee -> Ordering function the caller supplies, so the duplicate-rejection guarantee is only as strong as that comparator.

This seems fair, and is documented in the docstring:

  -- | How to compare votes by ID, where EQ means that two votes have the same
  -- ID and are either total duplicates, or are equivocating (i.e., they have
  -- the same ID but a different target)
  (Vote crypto committee -> Vote crypto committee -> Ordering) ->

The security-critical path here is verifyCert, and in that function, the voters are represented as a NE (Map SeatIndex (Maybe (VRFOutput crypto))), so we can't have duplicate or equivocating votes here.

The worse thing that could happen, if someone deliberatly misuse ensureUniqueVotesWithSameTarget (by using a faulty ordering function), is that they would forge a cert that would later be considered invalid by verifyCert because the signature would have been computed with a duplicate while the voter map used to derive the aggregate verififcation key wouldn't have the duplicate (since it's a map). So in that case, we would get a proper typed InvalidCertSignature, not a untyped error, and the cert would be rejected by the receiving party.

However, I agree that we should try to catch the error ASAP, and add some detection in forgeCert to ensure that the NEMap.fromAscList voters is the same size as the voters/voteSignatures lists. I don't think this warrants a dedicated typed error variant though; because this is not a case that can conditionally happen at runtime in normal circumstances (the failure case should have been handled by ensureUniqueVotesWithSameTarget already). If this case can ever be produced, it means the implementation of the ordering function in ensureUniqueVotesWithSameTarget is faulty.

Actions needed:

1. Add assert/error in forgeCert (for both WFALS/EveryoneVotes) to ensure that NEMap.fromAscList voters is the same size as the voters/voteSignatures lists so it becomes impossible to build a malformed cert
   (FTR, verifyCert has no way to do the same check directly, but if there is a mismatch between the number of deduped voters, and the number of signatures aggregated into the cert signature, then aggregate signature verification will correctly fail and thus we will reject the malformed cert)
2. Add property-based tests for UniqueVotesWithSameTarget constructors
   Note that these tests will only ensure that the no-dup invariant will hold with a well-formed ordering function. Since votes are abstract, and what constitutes a vote ID depends on the committee selection scheme, we can't really enforce anything about this ordering function (although this may change with Properly define the Equivocating class and implement it for votes and certs #232)

[agustinmista]

About (2), I have already implemented prop_checkUniqueVotesWithSameTarget here, so not sure there's much else to do there --- we could create a variant that uses a buggy ordering function, but that would be morally like checking that False => False, so I don't see much value in that.

[qnikst]

I've implement very simple version that relies on assert, that was added in both versions of the forgeCert invocation.
I hope it's the solution we need.