test(types): pin Vote/Proposal sign-bytes length disjointness

[anir0y]

Summary

Adds a regression test pinning an invariant that consensus signing currently relies on implicitly.

This is a hardening/robustness contribution, not a vulnerability report. I could not construct a
collision against the code as it stands today — the property holds. The concern is that it holds by
accident rather than by construction, so an ordinary future refactor could remove it silently.

The invariant

Vote::to_sign_bytes and Proposal::to_sign_bytes hand raw SSZ to the signer with no domain
separator:

// vote.rs / proposal.rs
self.as_ssz_bytes().into()

Both flow into the same sign_bytes() in the remote signer, so nothing structurally prevents a
signature over one type from validating as the other. Today they cannot collide because their
encoded lengths are disjoint:

type	encoded lengths
Vote	43, 75
Proposal	74, 78

SszRound (Option<u32>) and SszNilOrVal (Option<ValueId>) are variable-length, so both are
variable-size SSZ containers with offset tables — and Proposal carries two variable Round
fields to Vote's one, giving fixed parts of 68 vs 37 bytes. That size difference is what keeps
the length sets apart. (to_sign_bytes asserts round.is_defined() in both types, so only value /
pol_round vary.)

That is an emergent property of the current field sets. Adding one fixed field to Vote, or making
Value variable-length, could make the sets overlap — and nothing in the test suite would notice.

Notably, ValidatorProof::signing_bytes already does this correctly:

bytes.extend_from_slice(POV_SEPARATOR);                             // b"PoV"
bytes.extend_from_slice(&(public_key.len() as u32).to_be_bytes());  // length prefix

An explicit domain tag plus length prefixes. Vote and Proposal have neither.

Why a test and not a domain separator

Prefixing a domain tag in to_sign_bytes would be the stronger fix, but it changes the signed
preimage and is therefore consensus-breaking — every node would have to upgrade in lockstep or
signatures stop validating. That is a coordinated-rollout decision for maintainers, not something to
slip into a drive-by PR.

This test is the non-breaking half: it changes no runtime behaviour and makes the invariant explicit
and enforced, so if the domain separator is ever wanted, the failure will say so at the right moment.

The test

crates/types/src/proposal.rs, in the existing tests module. It computes both length sets through
the real to_sign_bytes and asserts they do not intersect. The failure message names the fix:

Vote and Proposal signed-byte lengths overlap at 43 bytes (vote=[43, 75], proposal=[74, 78]).
Cross-type signature replay may be possible; add an explicit domain separator to
`to_sign_bytes` for both types.

Verification

$ cargo test -p arc-consensus-types --lib test_vote_and_proposal_sign_bytes_lengths_are_disjoint
test proposal::tests::test_vote_and_proposal_sign_bytes_lengths_are_disjoint ... ok
test result: ok. 1 passed; 0 failed

I also ran a negative control — temporarily inverting the comparison so the assertion should trip —
to confirm the test can actually fail rather than being vacuously green. It failed with the message
above, then passed again once reverted.

Scope

• One file, test-only. No runtime or wire-format change.
• ValidatorProof is unaffected and already safe by construction.

[Copilot]

Pull request overview

Adds a regression test to make explicit (and enforce via CI) an implicit consensus safety invariant: Vote::to_sign_bytes() and Proposal::to_sign_bytes() currently produce SSZ payloads whose lengths are disjoint, preventing cross-type signature replay despite lacking a domain separator.

Changes:

• Adds a unit test asserting Vote and Proposal signed-byte length sets do not intersect.
• Documents the rationale and expected mitigation (domain separation) if the invariant ever breaks.

Suppressed comments (1)

crates/types/src/proposal.rs:202

• This test is meant to guard against cross-type replay for any Vote, but vote_lens only covers the Prevote variant. Including both new_prevote and new_precommit makes the invariant explicit for all votes (and will catch any future change where VoteType encoding affects SSZ length).

        let vote_lens: Vec<usize> = [NilOrVal::Nil, NilOrVal::Val(ValueId::new(block_hash))]
            .into_iter()
            .map(|value| {
                Vote::new_prevote(height, round, value, validator_address)
                    .to_sign_bytes()

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[osr21]

Verified this against current main — the claims hold up, and the framing is honest. Details:

The premise is accurate. Vote::to_sign_bytes (vote.rs, which also strips extension before encoding) and Proposal::to_sign_bytes (proposal.rs) both hand raw as_ssz_bytes() to the signer, and both flow into the same sign_bytes() in remote-signer/src/provider.rs (and the same sign() in signer/src/local.rs) with no domain tag. The contrast case is real too — ValidatorProof::signing_bytes with POV_SEPARATOR = b"PoV" plus u32 length prefixes — though worth noting it lives upstream in malachite core-types/src/validator_proof.rs, not in this repo. That cuts in the PR's favor: a future domain-separator fix for Vote/Proposal would be entirely arc-node-side (crates/types), no upstream coordination needed beyond the network-upgrade rollout itself.

The length sets check out arithmetically from the struct definitions:

• Vote fixed part: typ 1 + height 8 + round offset 4 + value offset 4 + validator_address 20 = 37. With round defined (5 bytes: selector + u32) and value Nil (1) / Val (1+32): 43 / 75. ✓
• Proposal fixed part: height 8 + round offset 4 + value 32 + pol_round offset 4 + validator_address 20 = 68. With round defined (5) and pol_round Nil (1) / Some (5): 74 / 78. ✓

{43, 75} ∩ {74, 78} = ∅, as claimed.

Coverage of the variable dimensions is complete, which matters for a pinning test: VoteType is a fixed 1-byte field so prevote/precommit can't change length, extension is stripped from the preimage (and #[ssz(skip_serializing)] besides) so it can't reintroduce variance, and the round.is_defined() asserts mean only value/pol_round genuinely vary — exactly the four encodings sampled. And pinning disjointness rather than the exact values is the right call: a benign field addition that keeps the sets disjoint passes; only a change that actually creates cross-type ambiguity fails.

The "test now, separator later" split is correct. Changing the preimage is consensus-breaking (every validator/follower must flip together or signature verification forks), so a drive-by PR is the wrong vehicle for the real fix — and this PR says so instead of pretending otherwise. Also checked for duplication: no existing test in crates/types covers this, and no open PR/issue touches sign-bytes domain separation.

One optional extension, take or leave: the same signing key domain has a third preimage family — ValidatorProof — and its non-collision with Vote/Proposal currently also rests on an unstated argument (the b"PoV" first bytes vs. Vote's leading vote-type byte / Proposal's leading height bytes). A companion assertion pinning that would close out the whole "one key, three preimage shapes" surface in the same style. Fine as a follow-up rather than scope creep here.

Disclosure: I reviewed statically (struct-level length derivation above); I didn't execute the test in a full workspace build, so CI is the authority on the run itself.

[osr21]

Confirming Copilot's inline comment on the doc wording — it's right, and worth fixing because the test is sound but the explanation it pins into the codebase is not.

Counting variable-length fields per container (a field is variable iff it encodes behind a 4-byte offset):

container	variable fields	offsets	fixed part
Vote	round (SszRound), value (SszNilOrVal)	2	typ 1 + height 8 + 2×offset 8 + address 20 = 37
Proposal	round, pol_round (both SszRound)	2	height 8 + 2×offset 8 + value 32 + address 20 = 68

So the offset tables are the same size (two 4-byte offsets each), contrary to the doc comment's "Vote has one variable field pair and Proposal has two, so their SSZ offset tables differ in size." The 31-byte fixed-part gap comes from two things the doc doesn't name: Proposal.value is a fixed inline 32-byte Value (vs. Vote.value living behind an offset), minus Vote's 1-byte typ. The variable payloads then differ the way Copilot describes — Vote's value contributes 1 or 33 bytes, Proposal's pol_round 1 or 5.

Why the wording matters more than usual here: this doc comment is the failure-time explanation a future maintainer will read when the assertion fires. If it tells them to look at offset-table sizes, they'll be debugging the wrong mechanism. Suggested replacement for the relevant sentence:

Today the two cannot collide because their encoded lengths are disjoint — Proposal inlines a fixed 32-byte Value while Vote carries its value behind an offset, so Proposal's fixed part is 68 bytes to Vote's 37, and the small variable payloads (1–33 bytes for Vote's value, 1–5 for Proposal's pol_round) can't bridge the gap.

(Same fix applies to the PR description's table paragraph.)

On Copilot's suppressed suggestion to also sample new_precommit: with the current structs it's a no-op — typ is a fixed 1-byte field, so prevote/precommit encode to identical lengths. But it costs one array element and makes the test robust against a future VoteType encoding change rather than assuming one, which is exactly the spirit of a pinning test. I'd take it.

Neither point changes the verdict: assertion logic, length sets, and the non-breaking framing all still hold — this is a doc-accuracy fix plus an optional one-line hardening.