Skip to content

Match commit votes against the full LedgerInfo - #429

Draft
sausagee wants to merge 2 commits into
m1from
cursor/full-ledgerinfo-commit-vote-match-60b0
Draft

Match commit votes against the full LedgerInfo#429
sausagee wants to merge 2 commits into
m1from
cursor/full-ledgerinfo-commit-vote-match-60b0

Conversation

@sausagee

@sausagee sausagee commented Sep 6, 2026

Copy link
Copy Markdown

Description

Depends on / should land after #427 (clean-room deferred BLS decompression). This branch is stacked on cursor/defer-bls-decompression-1a4b. Full-LedgerInfo matching needs that deferred-signature shape so a well-formed but invalid payload can still be compared as ledger fields without G2 recovery.

Clean-room rewrite on this tree (not a cherry-pick, patch, or mechanical port from aptos-labs/aptos-core). Upstream intent was read only to identify the bug and the invariant (aptos-labs/aptos-core#20335): a BLS commit vote is valid only for the exact LedgerInfo it signed (commit_info plus consensus_data_hash).

Bug: BufferItem::add_signature_if_matched admitted a CommitVote into the partial proof when only commit_info matched. A vote signed over a different consensus_data_hash — a valid signature over a different message — could still aggregate. CommitVote::verify checks the vote against its own ledger_info and may set SignatureWithStatus::is_verified, which does not record which message was verified.

Invariant:

  • Adding a commit vote to an Executed or Signed partial proof requires the vote's entire LedgerInfo to equal the aggregator's data. Mismatch fails closed; the signature is not inserted.
  • The Ordered path still stores at most one vote per author after an ordered-only commit_info compare; create_signature_aggregator later admits only full-LedgerInfo matches (unchanged).
  • Aggregation recovery re-checks every stored signature against the aggregator's own data and does not treat a cached is_verified flag as proof of that binding. Unbound authors go to the pessimistic verify set.
  • A recovered certificate is verified against the aggregator's data before it is returned. Quorum voting-power checks are unchanged.
  • No unsafe. Consensus-facing maps stay BTreeMap.

How Has This Been Tested?

  • reject_vote_with_unrelated_consensus_data_hash_and_still_quorum (consensus/src/pipeline/buffer_item.rs) — a vote whose consensus_data_hash differs is rejected on both Executed and Signed items even though CommitVote::verify succeeds; an honest quorum still produces a certificate that passes verify_signatures.
  • signs_same_ledger_info_requires_consensus_data_hash (consensus/consensus-types/src/pipeline/commit_vote.rs) — helper requires both commit_info and consensus_data_hash.
  • verified_signature_for_other_ledger_info_is_dropped_on_recovery (types/src/ledger_info.rs) — a signature valid over a different LedgerInfo, already marked verified, is dropped on recovery; the remaining honest quorum still certifies.
  • cargo test -p aptos-consensus-types20 passed (includes the new helper test).
  • cargo test -p aptos-types222 passed / 3 ignored (includes the new recovery test and existing test_signature_aggregator).
  • cargo test -p aptos-consensus --lib319 passed / 6 ignored (includes the new mismatch/quorum test plus existing buffer_item happy/bad paths).

Aikido SAST was attempted on the new/modified files. The Aikido MCP is not authenticated in this environment (aikido_full_scan requires a browser sign-in), so the scan could not complete here.

Key Areas to Review

  • admit_into_partial_commit_proof — fail-closed full-LedgerInfo gate before add_signature.
  • SignatureAggregator::aggregate_and_verify recovery — always re-verify individuals against self.data, then verify the recovered aggregate.
  • Confirm Ordered-path behavior is unchanged (unverified cache + later full match).

Type of Change

  • Bug fix
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Other (specify): aptos-consensus, aptos-consensus-types, aptos-types commit-vote aggregation

Checklist

  • Clean-room implementation; no aptos-labs commit SHAs, patches, or copied function bodies
  • Depends on Defer BLS signature decompression until verification #427; should land after it
  • RUST_SECURE_CODING.md followed: no unsafe, BTreeMap for consensus maps, Result / fail-closed on mismatch
  • Named crate tests for changed crates
Open in Web Open in Cursor 

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

cursoragent and others added 2 commits September 6, 2026 05:07
Keep AggregateSignature and SignatureWithStatus in compressed 96-byte
wire form so LedgerInfo commit-vote matching is not blocked by G2
point recovery. Recover the group element only on verification and
aggregation paths. BCS/JSON encoding stays identical to bls12381::Signature.

This is a clean-room rewrite of the deferred-decompression invariant
for m1; it does not cherry-pick or port aptos-labs/aptos-core.

Co-authored-by: Young Yang Liauw <sausagee@users.noreply.github.com>
Admitting a commit vote into a partial proof now requires the vote's
entire LedgerInfo (commit_info plus consensus_data_hash). A signature
valid over a different consensus_data_hash is rejected. Aggregation
recovery re-checks each signature against the aggregator's data and
verifies the recovered certificate before returning it.

Co-authored-by: Young Yang Liauw <sausagee@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants