Skip to content

fix(parlia): split the vote pool and complete vote admission checks - #491

Open
chee-chyuan wants to merge 22 commits into
developfrom
fix/vote-pool-cur-future
Open

chee-chyuan wants to merge 22 commits into
developfrom
fix/vote-pool-cur-future

Conversation

@chee-chyuan

@chee-chyuan chee-chyuan commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Final PR of the vote-ingestion series, after #486 (authentication + inbound cost bounds) and #489 (admission window). #492 is folded in here.

Summary

Ports go-bsc's curVotes/futureVotes split, which is the prerequisite for the two remaining admission checks:

  • Validator-set membership at admission. Membership resolves against the target's parent snapshot, so a vote for a block we do not hold cannot be checked at all. Upstream's answer is to hold such votes separately and check them at promotion. Without that, checking at ingest would discard every vote for a not-yet-imported block — routine, since votes are broadcast the moment a block is produced.
  • Per-target vote caps. The two limits exist because of the split: a future target's validator set is unresolvable, so non-validator votes cannot be filtered yet.

Ingest classifies by whether the target block is known, applies the matching cap, and origin-checks current votes. transfer_future_votes promotes in upstream's two phases — unconditionally past latest - 11, then by availability — running the origin check at promotion and removing promoted-but-invalid votes from the dedup set so a later legitimate copy is not mistaken for a duplicate. Pruning covers the future queue too.

justified_pair_for_hash is extracted so the pool and BscForkChoiceEngine derive the justified pair one way instead of two.

Departures from a literal port

Canonical vs verified presence. go-bsc tests verified presence; reth exposes no non-canonical header lookup, so a valid-but-not-yet-canonical target classifies as future. That defers its origin check rather than skipping it — conservative.

Promotion trigger. No chain-head subscription exists in reth's pool, so promotion piggybacks the vote-ingest path exactly as prune already does. Same cadence in practice.

Fail-open before the header provider registers. build_network calls ctx.start_network while the provider is registered later in build_consensus, so the node accepts peers before classification is possible. Those votes are held as future: uncapped, invisible to finality counting, and fully origin-checked at promotion. An earlier revision treated them as current, which was wrong — see below.

Review findings addressed

Three Hashdit reports, all confirmed rather than waved off, and two of them corrected reasoning I had defended earlier in the series.

Unauthenticated future votes could exhaust the per-target cap. A future vote is signature-checked and nothing more, and a signature proves the signer holds the key in the envelope — not that the key belongs to a validator. Any peer could mint keys, self-sign enough envelopes to fill a bucket, and have genuine validator votes refused permanently, since votes are broadcast once. This falsified an argument I had made repeatedly: that caps are safe once signature verification is in place. True for current votes, which are origin-checked; false for future votes, where a signature carries no authority.

Fixed in two steps: the cap was dropped for future votes, then restored conditionally. future_vote_sender_is_validator judges the sender against the snapshot at our own head — sound because the validator set changes only on epoch boundaries and the admission window caps a future target at head + 11. Senders absent from that set are rejected before a bucket is created; the cap applies only to senders we authenticated, and is skipped when the answer is genuinely undecidable (no snapshot, or an epoch boundary inside (head, target] — 11 heights in every 200).

Fail-open startup admitted unauthenticated votes as current. Correct and reachable, as above. One half of the claim did not hold and is noted for the record: maybe_notify_finality returns early without BEST_BLOCK_NUMBER_PROVIDER, which is set in the same function as the header provider, so fake finality was not reachable in that window. The cap-exclusion half was, which is enough.

Test key material. random_test_signer now generates a fresh keypair, so no key-shaped literal remains anywhere in the source. The obvious version would have been flaky: SecretKey::from_bytes validates against the BLS12-381 group order, so unmasked random bytes are rejected roughly four times in five. The scalar's top nibble is cleared to keep it provably below the order.

Also fixed here

Oversize pruning reclaimed nothing. MAX_VOTES_IN_POOL was a trigger, not a ceiling: it pruned relative to the incoming vote's target, so a flood aimed at recent heights freed nothing. Worse, target_number is attacker-supplied and was unbounded before #489, so a single vote claiming a target near u64::MAX produced an astronomically large prune height and evicted the entire pool. No panic accompanied it — [profile.release] sets no overflow-checks, so the addition inside prune wraps rather than trapping.

Now pruned relative to our head, with shed_future_votes as a fallback that releases future votes furthest-ahead first. Current votes cannot cause an overflow: they are origin-checked and capped per target, so the 267-block window bounds them at roughly 267 * 21.

extreme_target_number_cannot_wipe_the_pool covers it, verified discriminating by mutation — restoring the attacker-supplied derivation fails it with the ordinary-height vote gone.

Saturating arithmetic, in prune as well as the window predicate and the future-prune loop. Go wraps silently on uint64 overflow, so a faithful port of arithmetic over attacker-supplied heights carries a defect into Rust that is invisible in the original. Three sites in this series needed it — worth treating as a review heuristic for anything else ported from Parlia.

First-vote-only packet admission (folded in from #492)

Admitting only the first vote of a VotesPacket looks like a defect and is not. go-bsc does the same in eth/handler_bsc.go, and has since 3fd5b0c149 (bnb-chain/bsc#1741) — a commit that replaced a for _, vote := range votes { PutVote(vote) } loop and, in the same change, added the per-peer receive limit now in #486. A conformance review comparing against that removed loop concludes reth is diverging when it is matching.

Neither client tests this — go-bsc's testRecvVotes sends a single vote and passes identically under either behaviour — so the test pins it with the upstream provenance attached. Verified discriminating by mutation: whole-packet admission fails it 3 != 1.

Validation

  • cargo test --all -- --test-threads=1 — 545 passed, 0 failed
  • RUSTFLAGS="-D warnings" cargo clippy --workspace --tests --all-features — clean
  • 10-node devnet (4 reth / 6 geth), rolling restart, 214 blocks: origin rejections 0/0/0/0, cap rejections 0/0/0/0, only 1–2 transient "unverifiable" during startup, attestations climbing 34/31/22/31 → 50/54/48/47, finalized tracking head-1 throughout.

The reading that matters is the attestation growth: it is the direct evidence that classification puts votes in the current pool rather than stranding them in future, which was the main risk of this change.

Caveat: that devnet run predates the conditional-cap rework, the startup-routing fix and two rebases. The code paths are unchanged but the measured binary does not correspond to this SHA, so it is worth a re-run before merge if the evidence needs to match the commit.

Series status

With #486 and #489 merged and this PR, the vote-ingestion workstream from the BEP conformance sweep is complete. Two items were closed as non-gaps during the work rather than implemented: blob retention (both clients target 19.2 days; reth's implementation lives in bnb-chain/reth, which is why grepping reth-bsc found nothing) and first-vote-only admission, above.

🤖 Generated with Claude Code

@hashdit-bot

hashdit-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown

Pull Request Review

This Rust blockchain-consensus PR splits the Parlia vote pool into current and future queues, adds per-target caps, promotes future votes when their targets become available, and deduplicates votes before BLS verification. It also adds validator/source checks, extends pruning and finality notification to promoted votes, and centralizes snapshot-based justified-pair lookup.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] Unauthenticated future votes can exhaust the per-target cap and exclude validator votes

File: src/consensus/parlia/vote_pool.rs

Future votes are capped at 50 before validator membership can be checked. Because any remote peer can generate its own BLS key and submit a correctly self-signed envelope for an unknown target, an attacker who learns the target hash can fill future_votes[target_hash] with 50 non-validator votes. Subsequent legitimate validator votes are rejected by is_at_capacity; invalid entries are removed only when promotion occurs, after the target becomes available, so votes rejected while the bucket was full are not recovered. Repeating this race for new targets can delay or prevent quorum and finality.

Recommendation: Do not let unverifiable future votes permanently exclude validator votes. Consider a replaceable/bounded per-peer staging queue, reserve capacity for votes received after membership becomes verifiable, or immediately re-open admission after promotion and request/regossip missing votes. Add an adversarial test that fills a future target with validly signed non-validator votes before legitimate validator votes arrive and verifies that quorum can still be reached.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

chee-chyuan added a commit that referenced this pull request Sep 2, 2026
Capping a bucket whose contents cannot be authenticated turns the cap into a
censorship tool, so drop it for future votes and keep it only for current ones.

A future vote is signature-checked and nothing more, because membership resolves
against the target's parent snapshot and we do not hold the target. A valid
signature proves the signer holds the key named in the envelope — not that the
key belongs to a validator. So any peer can mint keys, self-sign 50 envelopes
for a target hash it has seen and we have not, and fill the bucket. Genuine
validator votes for that target are then refused, and nothing re-sends them:
votes are broadcast once. When the block arrives the junk is discarded at
promotion, but the real votes are already gone, so that target can never reach
local quorum.

Reported by Hashdit Bot on #491 and confirmed. It also corrects an argument
made earlier in this series: that per-target caps are safe once signature
verification is in place. That holds for current votes, which are origin-checked,
and not for future votes, where a signature carries no authority.

Uncapping the future bucket trades permanent loss of good votes for temporary
retention of useless ones. Memory is still bounded three other ways: the
(head-256, head+11] admission window bounds how many distinct targets can be
addressed, MAX_VOTES_IN_POOL bounds the pool overall, and the per-peer receive
budget bounds the rate. Junk is reclaimed by promotion and by pruning.

go-bsc appears to share the weakness: basicVerify applies maxFutureVoteAmount-
PerBlock with only vote.Verify() behind it, VerifyVote runs solely for current
votes, and core/vote/vote_pool.go has no per-peer accounting for future votes.
This departs from upstream deliberately, in the safe direction, and should be
raised there rather than treated as reth-bsc-specific.

The complete fix, ahead of both clients, is per-peer fairness for
unauthenticated votes; that needs peer identity plumbed into the ingest path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown

Pull Request Review

This PR changes the Rust-based BSC/Parlia consensus vote pool to deduplicate votes before BLS verification and split stored votes into current and future queues. It adds origin validation during admission or promotion, current-vote per-target caps, future-vote promotion/pruning, and a shared helper for deriving justified snapshot pairs.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] Uncapped future votes allow remote memory and CPU denial of service

File: src/consensus/parlia/vote_pool.rs

The newly introduced future-vote queue deliberately has no per-target cap and accepts votes from arbitrary self-generated BLS keys because validator membership cannot yet be checked. The claimed global MAX_VOTES_IN_POOL bound is not enforced as a hard limit: exceeding it merely calls prune(target_number - 256), which generally cannot remove recent future votes admitted within the (head-256, head+11] window. An attacker can therefore continuously submit distinct, correctly self-signed envelopes for unknown target hashes, causing unbounded memory growth; eventual promotion also verifies the entire attacker-controlled bucket while holding the global write lock, enabling CPU exhaustion and blocking legitimate vote processing.

Recommendation: Enforce a real hard global limit before insertion and introduce per-peer quotas or fair allocation for unauthenticated future votes. Also bound promotion work per invocation and avoid performing an unbounded number of origin/snapshot checks while holding the pool write lock; reserve capacity for distinct peers or known validators so an attacker cannot crowd out legitimate votes.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

@chee-chyuan

Copy link
Copy Markdown
Contributor Author

Confirmed and fixed in 462c255 — thanks, this was a real finding.

The mechanism holds. verify_vote_envelope proves the signer holds the key named in the envelope; it does not prove that key belongs to a validator. For a future vote that is the only check available, because membership resolves against the target's parent snapshot and we don't hold the target. So any peer can mint keys, self-sign 50 envelopes for a target hash it has seen and we haven't, and fill the bucket. Genuine validator votes are then refused, and nothing re-sends them — votes are broadcast once. The junk is discarded at promotion, but the real votes are already gone, so that target can never reach local quorum.

It also corrects an argument made earlier in this PR series: that per-target caps are safe once signature verification is in place. That holds for current votes, which are origin-checked. It does not hold for future votes, where a signature carries no authority.

Fix: drop the cap for future votes only, keep it for current votes. Current votes have passed the origin check, so their cap can only ever refuse surplus. Uncapping the future bucket trades permanent loss of good votes for temporary retention of useless ones — and memory is still bounded three other ways: the (head-256, head+11] admission window bounds how many distinct targets can be addressed, MAX_VOTES_IN_POOL bounds the pool overall, and #488's per-peer receive budget bounds the rate. Junk is reclaimed by promotion and by pruning.

This likely affects go-bsc as well, and is worth raising upstream. In core/vote/vote_pool.go, basicVerify applies maxFutureVoteAmountPerBlock = 50 with only vote.Verify() behind it; VerifyVote runs solely for current votes (if !isFutureVote); and there is no per-peer accounting for future votes anywhere in that file. The vector appears identical. This PR now departs from upstream deliberately, in the safe direction, rather than porting the weakness.

On the suggested remedies: the per-peer staging queue is the right idea and the only one that fully works — it needs peer identity plumbed into the ingest path, which reth-bsc's put_vote does not currently have, so it's follow-up work ahead of both clients. Reserving capacity for post-verifiability votes doesn't help, since membership never becomes verifiable while the target is unknown, and there's no regossip mechanism in either client to re-request lost votes. Two other obvious-looking options also fail: a per-(target, vote_address) cap of 1 is defeated by minting more keys, and FIFO eviction instead of rejection is defeated by rate, since a single peer's 68 votes/sec budget outpaces 21 validators sending one vote each.

Test: future_votes_are_uncapped_and_current_votes_are_capped drives the pool directly — 200 future votes for one target all retained, current votes still stopping at MAX_CUR_VOTE_AMOUNT_PER_BLOCK. It exercises the pool internals rather than put_vote, because no unit test can register the header provider, so the ingest path always classifies as current.

An adversarial end-to-end test of the kind suggested would need peer-level injection against a live cluster; noting it as not covered rather than claiming it is.

chee-chyuan added a commit that referenced this pull request Sep 2, 2026
…ing reclaim

Two follow-ups to the Hashdit finding on #491.

1. Check whether a future vote's sender is a validator at all

The previous commit dropped the future-vote cap because membership could not be
resolved for a target we do not hold. That was too absolute. `verify_vote_origin`
needs the target's *parent* snapshot, which we lack — but the validator set only
changes on epoch boundaries, and the admission window caps a future target at
head+11, so the set at our own head is the set that will govern the target.

`future_vote_sender_is_validator` looks the sender up in that set and rejects it
outright when absent. An attacker's minted key is in no validator set, so the
junk is refused before a bucket is created. It returns None, and the vote is
admitted uncapped, only when the answer is genuinely unknowable: no snapshot
available, or an epoch boundary inside (head, target] where the governing set
may differ. That window is 11 heights in every 200.

With senders authenticated the cap is safe again, so it returns for exactly
those votes and stays absent for the undecidable ones. A cap only censors when
it sits over contents we cannot vouch for.

2. Make the oversize path actually free memory

MAX_VOTES_IN_POOL was a trigger, not a ceiling. It pruned relative to the
*incoming* vote's target, so a flood aimed at recent heights pruned below
target-256 and reclaimed nothing: every entry was still inside the window. The
valve could fire repeatedly and release zero.

Prune relative to our head instead, and if that is not enough, shed future
votes furthest-ahead first via `shed_future_votes`. Current votes cannot be the
cause of an overflow — they are origin-checked and capped per target, so the
267-block window bounds them at roughly 267 * 21 entries — so any excess is
future votes, which are the less trustworthy half by construction.

go-bsc applies its future cap unconditionally and has no equivalent shedding
path. Both departures are deliberate and in the safe direction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown

Pull Request Review

This Rust BSC/Parlia consensus change splits the vote pool into current and future queues, deduplicates votes before BLS verification, and adds origin validation, per-target limits, promotion, pruning, and global shedding behavior. It also centralizes justified-pair snapshot lookup and exposes whether the global header provider has been registered.

Sensitive Content

Private Key / Seed Phrase / Mnemonic / Secret Material:

  • 0000...0001 (BLS private key) in src/consensus/parlia/vote_pool.rs — Deterministic scalar-one BLS private key constructed in the newly added duplicate-vote test. It appears to be test-only key material rather than a production credential.

Security Issues

No serious security issues detected.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

@hashdit-bot

hashdit-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown

Pull Request Review

This Rust/BSC consensus change splits the Parlia vote pool into current and future vote queues, adds per-target and global capacity handling, promotes future votes when targets become available, and deduplicates votes before BLS verification. It also centralizes justified-pair lookup and adds a header-provider readiness check, along with extensive vote-pool tests.

Sensitive Content

Private Key / Seed Phrase / Mnemonic / Secret Material:

  • 0000...0001 (BLS test private key) in src/consensus/parlia/vote_pool.rs — A deterministic BLS private key with integer value 1 is constructed in the newly added duplicate-vote test. It is test-only key material, not a production credential.

Security Issues

🟠 [HIGH] Fail-open startup path admits and counts unauthenticated votes as current

File: src/consensus/parlia/vote_pool.rs

When the header provider is not registered, can_classify is false, so every signature-valid vote is classified as current while verify_vote_origin is skipped. The vote is inserted into cur_votes and immediately passed to maybe_notify_finality; it is never revalidated after the provider becomes available. An attacker can self-generate BLS keys and signatures during this startup window, causing non-validator votes to count toward finality and potentially filling the 21-vote current-target cap to exclude legitimate validator votes.

Recommendation: Do not treat votes as current when the provider is unavailable. Buffer them in a separate unclassified/future queue and perform the full target, source, and validator-origin checks before promotion or finality counting. Alternatively, delay network vote admission until the header and snapshot providers are initialized; also ensure any votes accepted during initialization are explicitly revalidated afterward.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

chee-chyuan added a commit that referenced this pull request Sep 3, 2026
The header provider is registered in BscConsensusBuilder::build_consensus, but
build_network has already called ctx.start_network, so the node accepts peers
before classification is possible. Votes arriving in that window were treated
as current: inserted into cur_votes with the origin check skipped, counted by
maybe_notify_finality, charged against the 21-per-target cap, and never
revalidated once the provider appeared. A peer connected in that window could
self-sign non-validator votes and have them counted toward finality, or fill a
target's cap and exclude real validator votes.

Route them to the future pool instead. There they are uncapped, so they cannot
crowd out validator votes; they return 0 from insert, so they never reach
finality notification; and they are fully origin-checked at promotion once the
provider exists, which happens as soon as the head advances.

This reverses a call made earlier in the series. Routing to current was chosen
on the reasoning that future votes could never be promoted without the provider
and finality would stall. That was wrong: the window ends, and promotion then
processes the backlog. The stall was temporary and self-healing; treating
unauthenticated votes as validated was not.

Reported by Hashdit Bot on #491.

Test-side effects: put_vote_unchecked now places directly into the current pool,
since no unit test can register the provider and the real path would otherwise
route everything to future. Tests whose subject is admission rather than
placement use a new test-only fetch_any_vote_by_block_hash covering both pools;
production callers still see only current votes, which are origin-checked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chee-chyuan added a commit that referenced this pull request Sep 3, 2026
The header provider is registered in BscConsensusBuilder::build_consensus, but
build_network has already called ctx.start_network, so the node accepts peers
before classification is possible. Votes arriving in that window were treated
as current: inserted into cur_votes with the origin check skipped, counted by
maybe_notify_finality, charged against the 21-per-target cap, and never
revalidated once the provider appeared. A peer connected in that window could
self-sign non-validator votes and have them counted toward finality, or fill a
target's cap and exclude real validator votes.

Route them to the future pool instead. There they are uncapped, so they cannot
crowd out validator votes; they return 0 from insert, so they never reach
finality notification; and they are fully origin-checked at promotion once the
provider exists, which happens as soon as the head advances.

This reverses a call made earlier in the series. Routing to current was chosen
on the reasoning that future votes could never be promoted without the provider
and finality would stall. That was wrong: the window ends, and promotion then
processes the backlog. The stall was temporary and self-healing; treating
unauthenticated votes as validated was not.

Reported by Hashdit Bot on #491.

Test-side effects: put_vote_unchecked now places directly into the current pool,
since no unit test can register the provider and the real path would otherwise
route everything to future. Tests whose subject is admission rather than
placement use a new test-only fetch_any_vote_by_block_hash covering both pools;
production callers still see only current votes, which are origin-checked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

Pull Request Review

This Rust BSC/Parlia consensus change splits the vote pool into current and future queues, deduplicates votes before BLS verification, adds origin validation, per-target limits, promotion, pruning, and global shedding logic. It also centralizes justified-pair lookup, exposes header-provider readiness, and updates tests to account for votes held in either queue.

Sensitive Content

Private Key / Seed Phrase / Mnemonic / Secret Material:

  • 0000...0001 (BLS test private key) in src/consensus/parlia/vote_pool.rs — A deterministic 32-byte BLS private key is newly constructed in tests using a zeroed array with its final byte set to 1.

Security Issues

🟠 [HIGH] Unauthenticated future-vote flooding creates algorithmic denial of service and vote eviction

File: src/consensus/parlia/vote_pool.rs

This blockchain consensus client admits arbitrarily many self-signed future votes whenever future_vote_sender_is_validator returns None, including around epoch boundaries or while snapshots are unavailable. An attacker can use distinct target hashes to fill the 65,536-entry pool; every subsequent insertion then invokes shed_future_votes, which copies and sorts the entire future-target queue while holding the vote-pool write lock. This provides sustained O(n log n) work per accepted vote and also allows unauthenticated buckets to compete with and potentially evict legitimate future validator votes, risking consensus/finality availability.

Recommendation: Apply strict per-peer and global admission quotas to undecidable future votes, bound the number of target buckets, and reserve capacity or eviction priority for authenticated validator votes. Replace full-queue sorting on every overflow with an incrementally maintained max-priority structure or amortized batch eviction, and add adversarial tests covering repeated overflow with tens of thousands of distinct unknown target hashes.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

@chee-chyuan

Copy link
Copy Markdown
Contributor Author

Confirmed and fixed in 1fed653. Thanks — this one is real too, and I checked the reachability rather than assuming it.

The window exists. set_header_provider runs in BscConsensusBuilder::build_consensus, but build_network has already called ctx.start_network(...), and the components builder wires .network(...) before .consensus(...). So the node is accepting peers before classification is possible. With --trusted-peers configured, connections are immediate, so the gap is reachable in practice rather than only in principle.

One half of the claim doesn't hold, for what it's worth. maybe_notify_finality returns early without BEST_BLOCK_NUMBER_PROVIDER, which set_header_provider sets in the same function as the header provider — so fake finality was not reachable in that window. The cap-exclusion half was, and that is enough on its own.

Fix: route unclassifiable votes to the future pool, not current — which is your first recommendation, and it needs no new queue since the split already provides one. There they are uncapped, so they cannot crowd out validator votes; they return 0 from insert, so they never reach finality notification; and they are fully origin-checked at promotion once the provider appears, which happens as soon as the head advances. That also answers the "never revalidated" point: promotion is the revalidation.

This reverses a call made earlier in this PR. Routing to current was chosen on the reasoning that future votes could never be promoted while the provider was missing, so finality would stall. That was wrong — the window ends and promotion then drains the backlog. The stall was temporary and self-healing; treating unauthenticated votes as validated was not.

Test: unclassifiable_votes_are_held_as_future_not_current asserts the vote is absent from the current pool, present overall, and uncapped. Unit tests cannot register the header provider, so that startup state is exactly what a unit test naturally reproduces.

On the flagged key material: 0000...0001 is a BLS scalar of integer value 1, constructed inline in tests to get a deterministic keypair. Not a credential, and it corresponds to no funded or privileged account.

@chee-chyuan
chee-chyuan force-pushed the fix/vote-height-window branch from 831bb4d to 93f2d39 Compare September 3, 2026 05:51
chee-chyuan added a commit that referenced this pull request Sep 3, 2026
Capping a bucket whose contents cannot be authenticated turns the cap into a
censorship tool, so drop it for future votes and keep it only for current ones.

A future vote is signature-checked and nothing more, because membership resolves
against the target's parent snapshot and we do not hold the target. A valid
signature proves the signer holds the key named in the envelope — not that the
key belongs to a validator. So any peer can mint keys, self-sign 50 envelopes
for a target hash it has seen and we have not, and fill the bucket. Genuine
validator votes for that target are then refused, and nothing re-sends them:
votes are broadcast once. When the block arrives the junk is discarded at
promotion, but the real votes are already gone, so that target can never reach
local quorum.

Reported by Hashdit Bot on #491 and confirmed. It also corrects an argument
made earlier in this series: that per-target caps are safe once signature
verification is in place. That holds for current votes, which are origin-checked,
and not for future votes, where a signature carries no authority.

Uncapping the future bucket trades permanent loss of good votes for temporary
retention of useless ones. Memory is still bounded three other ways: the
(head-256, head+11] admission window bounds how many distinct targets can be
addressed, MAX_VOTES_IN_POOL bounds the pool overall, and the per-peer receive
budget bounds the rate. Junk is reclaimed by promotion and by pruning.

go-bsc appears to share the weakness: basicVerify applies maxFutureVoteAmount-
PerBlock with only vote.Verify() behind it, VerifyVote runs solely for current
votes, and core/vote/vote_pool.go has no per-peer accounting for future votes.
This departs from upstream deliberately, in the safe direction, and should be
raised there rather than treated as reth-bsc-specific.

The complete fix, ahead of both clients, is per-peer fairness for
unauthenticated votes; that needs peer identity plumbed into the ingest path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chee-chyuan added a commit that referenced this pull request Sep 3, 2026
…ing reclaim

Two follow-ups to the Hashdit finding on #491.

1. Check whether a future vote's sender is a validator at all

The previous commit dropped the future-vote cap because membership could not be
resolved for a target we do not hold. That was too absolute. `verify_vote_origin`
needs the target's *parent* snapshot, which we lack — but the validator set only
changes on epoch boundaries, and the admission window caps a future target at
head+11, so the set at our own head is the set that will govern the target.

`future_vote_sender_is_validator` looks the sender up in that set and rejects it
outright when absent. An attacker's minted key is in no validator set, so the
junk is refused before a bucket is created. It returns None, and the vote is
admitted uncapped, only when the answer is genuinely unknowable: no snapshot
available, or an epoch boundary inside (head, target] where the governing set
may differ. That window is 11 heights in every 200.

With senders authenticated the cap is safe again, so it returns for exactly
those votes and stays absent for the undecidable ones. A cap only censors when
it sits over contents we cannot vouch for.

2. Make the oversize path actually free memory

MAX_VOTES_IN_POOL was a trigger, not a ceiling. It pruned relative to the
*incoming* vote's target, so a flood aimed at recent heights pruned below
target-256 and reclaimed nothing: every entry was still inside the window. The
valve could fire repeatedly and release zero.

Prune relative to our head instead, and if that is not enough, shed future
votes furthest-ahead first via `shed_future_votes`. Current votes cannot be the
cause of an overflow — they are origin-checked and capped per target, so the
267-block window bounds them at roughly 267 * 21 entries — so any excess is
future votes, which are the less trustworthy half by construction.

go-bsc applies its future cap unconditionally and has no equivalent shedding
path. Both departures are deliberate and in the safe direction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chee-chyuan added a commit that referenced this pull request Sep 3, 2026
The header provider is registered in BscConsensusBuilder::build_consensus, but
build_network has already called ctx.start_network, so the node accepts peers
before classification is possible. Votes arriving in that window were treated
as current: inserted into cur_votes with the origin check skipped, counted by
maybe_notify_finality, charged against the 21-per-target cap, and never
revalidated once the provider appeared. A peer connected in that window could
self-sign non-validator votes and have them counted toward finality, or fill a
target's cap and exclude real validator votes.

Route them to the future pool instead. There they are uncapped, so they cannot
crowd out validator votes; they return 0 from insert, so they never reach
finality notification; and they are fully origin-checked at promotion once the
provider exists, which happens as soon as the head advances.

This reverses a call made earlier in the series. Routing to current was chosen
on the reasoning that future votes could never be promoted without the provider
and finality would stall. That was wrong: the window ends, and promotion then
processes the backlog. The stall was temporary and self-healing; treating
unauthenticated votes as validated was not.

Reported by Hashdit Bot on #491.

Test-side effects: put_vote_unchecked now places directly into the current pool,
since no unit test can register the provider and the real path would otherwise
route everything to future. Tests whose subject is admission rather than
placement use a new test-only fetch_any_vote_by_block_hash covering both pools;
production callers still see only current votes, which are origin-checked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chee-chyuan
chee-chyuan force-pushed the fix/vote-pool-cur-future branch from 1fed653 to 3cd32a8 Compare September 3, 2026 05:55
@hashdit-bot

hashdit-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

Pull Request Review

This Rust BSC/Parlia consensus change splits the vote pool into current and future queues, adds admission and promotion-time validator/source checks, per-target limits, global shedding, and pruning for future votes. It also centralizes justified-pair lookup, exposes header-provider readiness, and updates tests and vote-production assertions for the split pool.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] One validator can fill a future-target cap and censor all legitimate votes

File: src/consensus/parlia/vote_pool.rs

Future-vote capacity counts envelopes rather than unique validators. A single malicious validator can submit 50 distinct, correctly signed envelopes for an unavailable target while varying fields such as the source pair; future_vote_sender_is_validator authenticates the validator but does not validate those fields, and conflict_detect does not reject insertion. Once the bucket reaches 50, genuine validator votes are rejected; during promotion the attacker's malformed votes can all fail the source check, potentially preventing finality for that target.

Recommendation: Limit each future target to one retained vote per authenticated validator, or replace an existing vote from the same validator rather than consuming another slot. Do not treat membership alone as proof that every envelope from that validator is surplus-safe.

🟠 [HIGH] Legitimate votes for unavailable targets across a past epoch boundary are rejected using the wrong validator set

File: src/consensus/parlia/vote_pool.rs

future_vote_sender_is_validator only returns None when target_number / epoch > head_number / epoch. However, targets are classified as future based on hash availability, not whether their number is ahead of the head, and the admission window permits targets up to 256 blocks behind. For an unavailable or non-canonical target in an earlier epoch, the function compares its signer against the current head's validator set and may permanently reject a validator that was valid for the target's parent snapshot.

Recommendation: Use the current-head validator set only when the target is at or above the head and no epoch boundary lies between them. For targets behind the head whose governing snapshot cannot be resolved, return None and defer membership validation until promotion, or resolve membership from the target's parent on a provider that supports non-canonical headers.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

@chee-chyuan
chee-chyuan force-pushed the fix/vote-height-window branch from 93f2d39 to 0c8ea90 Compare September 3, 2026 06:20
chee-chyuan added a commit that referenced this pull request Sep 3, 2026
Capping a bucket whose contents cannot be authenticated turns the cap into a
censorship tool, so drop it for future votes and keep it only for current ones.

A future vote is signature-checked and nothing more, because membership resolves
against the target's parent snapshot and we do not hold the target. A valid
signature proves the signer holds the key named in the envelope — not that the
key belongs to a validator. So any peer can mint keys, self-sign 50 envelopes
for a target hash it has seen and we have not, and fill the bucket. Genuine
validator votes for that target are then refused, and nothing re-sends them:
votes are broadcast once. When the block arrives the junk is discarded at
promotion, but the real votes are already gone, so that target can never reach
local quorum.

Reported by Hashdit Bot on #491 and confirmed. It also corrects an argument
made earlier in this series: that per-target caps are safe once signature
verification is in place. That holds for current votes, which are origin-checked,
and not for future votes, where a signature carries no authority.

Uncapping the future bucket trades permanent loss of good votes for temporary
retention of useless ones. Memory is still bounded three other ways: the
(head-256, head+11] admission window bounds how many distinct targets can be
addressed, MAX_VOTES_IN_POOL bounds the pool overall, and the per-peer receive
budget bounds the rate. Junk is reclaimed by promotion and by pruning.

go-bsc appears to share the weakness: basicVerify applies maxFutureVoteAmount-
PerBlock with only vote.Verify() behind it, VerifyVote runs solely for current
votes, and core/vote/vote_pool.go has no per-peer accounting for future votes.
This departs from upstream deliberately, in the safe direction, and should be
raised there rather than treated as reth-bsc-specific.

The complete fix, ahead of both clients, is per-peer fairness for
unauthenticated votes; that needs peer identity plumbed into the ingest path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chee-chyuan added a commit that referenced this pull request Sep 3, 2026
…ing reclaim

Two follow-ups to the Hashdit finding on #491.

1. Check whether a future vote's sender is a validator at all

The previous commit dropped the future-vote cap because membership could not be
resolved for a target we do not hold. That was too absolute. `verify_vote_origin`
needs the target's *parent* snapshot, which we lack — but the validator set only
changes on epoch boundaries, and the admission window caps a future target at
head+11, so the set at our own head is the set that will govern the target.

`future_vote_sender_is_validator` looks the sender up in that set and rejects it
outright when absent. An attacker's minted key is in no validator set, so the
junk is refused before a bucket is created. It returns None, and the vote is
admitted uncapped, only when the answer is genuinely unknowable: no snapshot
available, or an epoch boundary inside (head, target] where the governing set
may differ. That window is 11 heights in every 200.

With senders authenticated the cap is safe again, so it returns for exactly
those votes and stays absent for the undecidable ones. A cap only censors when
it sits over contents we cannot vouch for.

2. Make the oversize path actually free memory

MAX_VOTES_IN_POOL was a trigger, not a ceiling. It pruned relative to the
*incoming* vote's target, so a flood aimed at recent heights pruned below
target-256 and reclaimed nothing: every entry was still inside the window. The
valve could fire repeatedly and release zero.

Prune relative to our head instead, and if that is not enough, shed future
votes furthest-ahead first via `shed_future_votes`. Current votes cannot be the
cause of an overflow — they are origin-checked and capped per target, so the
267-block window bounds them at roughly 267 * 21 entries — so any excess is
future votes, which are the less trustworthy half by construction.

go-bsc applies its future cap unconditionally and has no equivalent shedding
path. Both departures are deliberate and in the safe direction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chee-chyuan added a commit that referenced this pull request Sep 3, 2026
The header provider is registered in BscConsensusBuilder::build_consensus, but
build_network has already called ctx.start_network, so the node accepts peers
before classification is possible. Votes arriving in that window were treated
as current: inserted into cur_votes with the origin check skipped, counted by
maybe_notify_finality, charged against the 21-per-target cap, and never
revalidated once the provider appeared. A peer connected in that window could
self-sign non-validator votes and have them counted toward finality, or fill a
target's cap and exclude real validator votes.

Route them to the future pool instead. There they are uncapped, so they cannot
crowd out validator votes; they return 0 from insert, so they never reach
finality notification; and they are fully origin-checked at promotion once the
provider exists, which happens as soon as the head advances.

This reverses a call made earlier in the series. Routing to current was chosen
on the reasoning that future votes could never be promoted without the provider
and finality would stall. That was wrong: the window ends, and promotion then
processes the backlog. The stall was temporary and self-healing; treating
unauthenticated votes as validated was not.

Reported by Hashdit Bot on #491.

Test-side effects: put_vote_unchecked now places directly into the current pool,
since no unit test can register the provider and the real path would otherwise
route everything to future. Tests whose subject is admission rather than
placement use a new test-only fetch_any_vote_by_block_hash covering both pools;
production callers still see only current votes, which are origin-checked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chee-chyuan
chee-chyuan force-pushed the fix/vote-pool-cur-future branch from 3cd32a8 to f6b46c4 Compare September 3, 2026 06:20
@hashdit-bot

hashdit-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

Pull Request Review

This Rust blockchain-consensus PR splits Parlia votes into current and future pools, adds target-aware admission caps, validator/origin checks, promotion, pruning, and bounded shedding for future votes. It also centralizes justified-pair lookup, exposes header-provider readiness, and updates tests and vote-producer assertions for the new pool behavior.

Sensitive Content

No sensitive content detected.

Security Issues

No serious security issues detected.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

@chee-chyuan chee-chyuan changed the title feat(parlia): split the vote pool into current and future votes (D4b + D1) feat(parlia): split the vote pool into current and future votes Sep 4, 2026
@chee-chyuan
chee-chyuan force-pushed the fix/vote-height-window branch from 0c8ea90 to 139401b Compare September 4, 2026 02:10
chee-chyuan added a commit that referenced this pull request Sep 4, 2026
Capping a bucket whose contents cannot be authenticated turns the cap into a
censorship tool, so drop it for future votes and keep it only for current ones.

A future vote is signature-checked and nothing more, because membership resolves
against the target's parent snapshot and we do not hold the target. A valid
signature proves the signer holds the key named in the envelope — not that the
key belongs to a validator. So any peer can mint keys, self-sign 50 envelopes
for a target hash it has seen and we have not, and fill the bucket. Genuine
validator votes for that target are then refused, and nothing re-sends them:
votes are broadcast once. When the block arrives the junk is discarded at
promotion, but the real votes are already gone, so that target can never reach
local quorum.

Reported by Hashdit Bot on #491 and confirmed. It also corrects an argument
made earlier in this series: that per-target caps are safe once signature
verification is in place. That holds for current votes, which are origin-checked,
and not for future votes, where a signature carries no authority.

Uncapping the future bucket trades permanent loss of good votes for temporary
retention of useless ones. Memory is still bounded three other ways: the
(head-256, head+11] admission window bounds how many distinct targets can be
addressed, MAX_VOTES_IN_POOL bounds the pool overall, and the per-peer receive
budget bounds the rate. Junk is reclaimed by promotion and by pruning.

go-bsc appears to share the weakness: basicVerify applies maxFutureVoteAmount-
PerBlock with only vote.Verify() behind it, VerifyVote runs solely for current
votes, and core/vote/vote_pool.go has no per-peer accounting for future votes.
This departs from upstream deliberately, in the safe direction, and should be
raised there rather than treated as reth-bsc-specific.

The complete fix, ahead of both clients, is per-peer fairness for
unauthenticated votes; that needs peer identity plumbed into the ingest path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chee-chyuan added a commit that referenced this pull request Sep 4, 2026
…ing reclaim

Two follow-ups to the Hashdit finding on #491.

1. Check whether a future vote's sender is a validator at all

The previous commit dropped the future-vote cap because membership could not be
resolved for a target we do not hold. That was too absolute. `verify_vote_origin`
needs the target's *parent* snapshot, which we lack — but the validator set only
changes on epoch boundaries, and the admission window caps a future target at
head+11, so the set at our own head is the set that will govern the target.

`future_vote_sender_is_validator` looks the sender up in that set and rejects it
outright when absent. An attacker's minted key is in no validator set, so the
junk is refused before a bucket is created. It returns None, and the vote is
admitted uncapped, only when the answer is genuinely unknowable: no snapshot
available, or an epoch boundary inside (head, target] where the governing set
may differ. That window is 11 heights in every 200.

With senders authenticated the cap is safe again, so it returns for exactly
those votes and stays absent for the undecidable ones. A cap only censors when
it sits over contents we cannot vouch for.

2. Make the oversize path actually free memory

MAX_VOTES_IN_POOL was a trigger, not a ceiling. It pruned relative to the
*incoming* vote's target, so a flood aimed at recent heights pruned below
target-256 and reclaimed nothing: every entry was still inside the window. The
valve could fire repeatedly and release zero.

Prune relative to our head instead, and if that is not enough, shed future
votes furthest-ahead first via `shed_future_votes`. Current votes cannot be the
cause of an overflow — they are origin-checked and capped per target, so the
267-block window bounds them at roughly 267 * 21 entries — so any excess is
future votes, which are the less trustworthy half by construction.

go-bsc applies its future cap unconditionally and has no equivalent shedding
path. Both departures are deliberate and in the safe direction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chee-chyuan and others added 2 commits September 9, 2026 20:56
`justified_pair_for_hash` returned a snapshot's `vote_data` verbatim. Before the
chain justifies anything that pair is all zeroes, which is the *absence* of an
attestation, not a justified block living at the zero hash.

Nothing can ever cite that hash. Our own producers substitute genesis in this
state — `vote_producer.rs` before signing, `assemble_vote_attestation` before
building — and go-bsc's `GetJustifiedNumberAndHash` returns
`chain.GetHeaderByNumber(0).Hash()` whenever `snap.Attestation == nil`, with
`VerifyVote` comparing against exactly that. So every vote on the wire cites
genesis while `verify_vote_origin` expected zero, and every vote was rejected
for source mismatch.

The rejection is self-locking: leaving the state needs an attestation, and an
attestation can only be assembled from the votes being rejected. A network
starting at genesis with only reth validators therefore never reaches finality
at all, and cannot recover.

The zero pair itself predates this PR — the fork-choice engine has always read
`vote_data` raw — but it was inert there, because that caller keeps only the
number and discards the hash. #491 added the first consumer that compares the
hash, which is what made it load-bearing.

Split the decision into `justified_pair_of`, with the genesis lookup passed as a
lazy closure: a chain that has justified something never pays for the lookup,
and the branch is unit-testable without a registered header provider (only
`set_header_provider` installs one, and it is a process-wide `OnceLock`).

Verified on a fresh 10-validator all-reth devnet. Before: head 2613, `finalized`
null on every node, 16,468 origin-check rejections on node0, all ten distinct
rejected senders present in the validator set. After, on the same chain with
only the binary swapped: `finalized` tracks head-1 within seconds, zero
rejections. A clean genesis run also bootstraps, finalizing from block ~203 once
the epoch switch publishes vote addresses.

Raised by will-2012 in review of #491.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Promotion ran only from `put_vote_inner`, so a vote arriving was the only thing
that could move future votes into the current pool. Promotion needs a *block*,
not a vote, and the two do not coincide:

- `put_vote` returns at its dedup check before reaching `put_vote_inner`, so a
  re-relayed copy is not an event at all.
- A node that received every vote for `N` before importing `N` has nothing left
  to arrive. The next new vote targets `N+1`, which does not exist yet if we are
  the proposer of `N+1` — the exact moment we need `N`'s votes to assemble its
  attestation.

So the votes sat unpromoted through precisely the window the cur/future split
exists to serve. It is not validator-only either: `get_finalized_number_and_hash`
reads `cur_votes` on every node to advance finalized to head-1, so a full node
silently loses that lead the same way.

go-bsc drives this off `highestVerifiedBlock`. The equivalent choke point here is
`BscForkChoiceEngine::update_forkchoice`, which every node reaches on every
import path; the call sits after the canonical head is chosen and before the
justified and finalized reads that consume `cur_votes`.

Promotion also did provider and snapshot lookups while holding the pool write
lock that every incoming vote contends for, which made it unwise to run more
often. Split it into three phases: `promotion_candidates` collects eligible
targets and clones their envelopes under the read lock, the header and origin
checks run with no lock held, and `apply_promotion` applies the verdicts under
the write lock.

Two consequences of the split are handled explicitly. A vote landing between the
verdicts and the write has no verdict of its own, so `promote_judged` leaves it
future and re-queues its target for the next pass rather than guessing. And
because `apply_promotion` pops from the heap it re-queues into, re-queues are
collected and pushed after the loop — inline, the same entry would be popped
again on the same pass, forever.

The ingest path keeps calling promotion as a backstop for the window before the
fork-choice engine is registered, but now before taking the write lock.

Raised by will-2012 in review of #491.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chee-chyuan
chee-chyuan dismissed stale reviews from will-2012 and joey0612 via 34307c1 September 9, 2026 13:30
@hashdit-bot

hashdit-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

Pull Request Review

This Rust BSC/Parlia node change splits vote storage into current and future pools, adds validator-origin checks, per-target caps, promotion on block import, safer pruning, and bounded future-vote shedding. It also centralizes justified-pair derivation, handles startup classification conservatively, and adds extensive tests for vote admission, promotion, overflow resistance, rate limiting, and first-vote-only packet handling.

Sensitive Content

No sensitive content detected.

Security Issues

No serious security issues detected.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

…fore

`promote_future_votes` ran at the top of `update_forkchoice`, before the update
was dispatched. At that point the block that triggered the call is not visible
to the provider: reth canonicalizes in `on_forkchoice_updated` and never in
`on_new_payload`, and `ConsistentProvider::header` resolves a hash only against
the canonical in-memory chain (`head_block.block_on_chain`) or the database, so
a block that is executed but not yet canonical is in neither.

Promotion therefore judged every vote for the just-imported block "still future"
and skipped it, leaving the next proposer to read an empty `cur_votes` — the
exact lag the change was written to remove. Moving the call after the update, and
running it only when the update succeeded, makes the block visible.

Measured on a 10-validator all-reth devnet, two fresh chains, same binary except
for the call site. Share of promotion passes that resolved nothing:

  before the update:  393/1091, 405/1145, 265/787   (~35%)
  after the update:    51/590,   49/548,   55/635   (~8.7%)

A direct probe over the same run recorded the head's visibility on either side of
the update: of 474 genuinely new blocks, 391 were invisible before it and visible
after. Finality also reached lag 0 on some nodes in the second arm, which the
first never did.

Found by an automated review pass over the PR; the placement it corrects was
suggested in review and looked right by inspection.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

Pull Request Review

This PR refactors the Parlia vote pool into separate current and future queues, adds validator-origin admission checks, per-target caps, promotion on canonical block import, safer pruning, and bounded future-vote shedding. It also centralizes justified-pair derivation, adds provider-registration detection, and expands tests covering packet admission, startup behavior, promotion, validator-set boundaries, caps, and overflow-resistant pruning.

Sensitive Content

No sensitive content detected.

Security Issues

No serious security issues detected.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

chee-chyuan and others added 2 commits September 10, 2026 08:04
Two problems this PR introduced, both on paths that only exist because of the
current/future split.

**Origin rejections were not remembered.** `REJECTED_VOTES` was populated only on
BLS failure, so a vote refused by the origin check left no trace: it never
entered `received_votes` either, so a replay missed both caches and paid for
another BLS verification. Rate limiting bounds that to the per-peer budget, but
before this PR such an envelope was admitted once and deduped thereafter, so it
is a regression.

Caching every refusal would be wrong, though. `verify_vote_origin` conflated two
different outcomes behind one `bool`: an envelope that contradicts the chain
(wrong target height, wrong source pair, sender absent from the governing set),
and one we simply cannot judge yet because a snapshot is missing. The first is a
property of the envelope and can be cached; the second must not be, or a vote a
later copy could prove valid is blacklisted. Return `OriginVerdict` and cache
only `Rejected`. The future-sender refusal is cacheable on the same grounds:
`future_vote_sender_is_validator` answers `Some` only when no validator-set
change lies between our head and the target, so the verdict cannot flip.

**Promotion was unbounded.** It now runs on every import, and it clones each
eligible target's votes and origin-checks them — a provider read and two snapshot
reads per vote. Future buckets whose sender could not be authenticated are
deliberately uncapped (capping them is a censorship lever, see
`MAX_FUTURE_VOTE_AMOUNT_PER_BLOCK`), so one target can hold tens of thousands of
votes and a flood would land all of that work between importing one block and
producing the next.

Budget each pass at 64 targets and 512 votes. Nothing is dropped: a partially
judged bucket keeps its remainder in the future pool and stays queued, which
`promote_judged` already handles, so a large backlog drains across passes at
roughly one pass per block.

Found by an automated review pass over the PR.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ull window

Two smaller gaps on the promotion path.

**Delay metrics skipped promoted votes.** `put_vote_inner` reports every admitted
vote to `block_stats::on_vote_received`, but promotion only called
`maybe_notify_finality`. Promotion is the sole path by which a future vote reaches
a block's vote total, so first- and majority-vote delay were under-reported in
proportion to how many votes a node classifies as future — worst on the nodes
whose measurements matter most.

**Future votes were discarded 244 blocks early.** A candidate was treated as
expired once its target fell `head + 11` behind, and an expired target whose block
we do not hold is judged immediately, which drops it. But not holding a block does
not make it invalid: it can be a valid block on a branch we have not seen, and
go-bsc keeps such votes for the full 256-block window. Discarding at 11 loses the
evidence if the node later reorgs onto that branch. Expire at
`LOWER_LIMIT_OF_VOTE_BLOCK_NUMBER` instead, which is where `prune` would drop them
anyway.

The check stays rather than being left entirely to `prune`, because `prune` runs
on the vote-ingest path while promotion now runs on every import, so promotion can
still meet entries that prune has not reached.

Found by an automated review pass over the PR.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown

Pull Request Review

This PR refactors the Rust BSC/Parlia consensus vote pool into current and future queues, adds validator-origin checks, per-target and global capacity controls, bounded promotion, safer pruning, and shared justified-pair resolution. It also triggers future-vote promotion after successful fork-choice updates and adds regression tests for vote admission, promotion, pruning, packet handling, and validator-set boundaries.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] Historical future votes are authenticated against the wrong validator set

File: src/consensus/parlia/vote_pool.rs

future_vote_sender_is_validator always checks membership against the current head snapshot, while validator_set_swaps_within(head, target, ...) only detects forward boundaries. For an unavailable or non-canonical target below the head, target < head makes the boundary predicate return false even if the validator set changed between the target and the head. A legitimate validator from the target's governing set can consequently receive Some(false), causing its vote to be permanently rejected and cached; this can impair finality or reorganization handling around validator-set changes.

Recommendation: Only use current-head membership as a prefilter when target_number >= head_number and no validator-set swap occurs in (head, target]. For historical targets, return None and retain the vote for later target-parent snapshot verification, or explicitly resolve membership from the target's governing snapshot when available.

🟠 [HIGH] Unresolvable targets can starve promotable votes indefinitely

File: src/consensus/parlia/vote_pool.rs

promotion_candidates takes the first 64 eligible entries encountered in the binary heap's internal storage, including targets whose blocks remain unavailable. Such unresolved entries are requeued without any rotation or retry backoff, so the same entries can consume every promotion pass. An attacker able to submit future votes while sender membership is undecidable, such as during startup or a validator-set boundary, can populate enough crafted unknown targets to keep newly canonical targets out of the candidate set, delaying their votes from reaching finality and attestation processing until the blockers expire.

Recommendation: Make promotion scheduling fair across targets. Use a round-robin work queue or cursor, rotate unresolved entries behind other eligible targets, and ensure repeatedly unavailable targets cannot consume every per-pass candidate slot. Add a regression test with more than 64 unresolved eligible targets preceding a known target and assert that the known target is promoted within a bounded number of passes.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

The previous commit moved the promotion sweep's expiry from `head - 11` to
`head - 256`, on the claim that go-bsc retains unresolvable future votes for the
full window. It does not.

`transferVotesFromFutureToCur` (core/vote/vote_pool.go) sweeps
`TargetNumber + upperLimitOfVoteBlockNumber < latestBlockNumber` unconditionally,
with no header check, and lets `VerifyVote` inside `transfer` drop what cannot be
verified. `prune` touches only `curVotes`; upstream never prunes `futureVotes` by
the 256-block bound, because this sweep is what clears them.

So the original `head + 11` bound was already an exact match for upstream, and
the change was a regression: unresolvable future votes would have lingered 245
blocks longer, occupying the pool and being re-examined by every promotion pass.

The block-stats half of the previous commit stands; only the bound is reverted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown

Pull Request Review

This PR refactors the Rust BSC/Parlia vote pool into separate current and future queues, adding validator-origin checks, per-target limits, bounded promotion, safer pruning, and shared justified-pair derivation. It also promotes future votes after successful fork-choice updates and adds tests covering admission, promotion, pool shedding, epoch boundaries, and first-vote-only packet handling.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] Promotion pass target limit can be bypassed, enabling block-import DoS

File: src/consensus/parlia/vote_pool.rs

Although promotion_candidates selects at most MAX_PROMOTION_TARGETS_PER_PASS targets, apply_promotion subsequently pops every queued target whose height is at or below latest, including targets absent from resolved, and then pushes all deferred entries back into the heap. An attacker who fills the future pool with many distinct target hashes during an undecidable-authentication window can therefore force each promotion pass to perform tens of thousands of heap pops and pushes while holding the global vote-pool write lock. Once those targets expire, this work can recur on the block-import path, potentially exhausting CPU and delaying consensus processing.

Recommendation: Make the write phase operate only on the bounded set of candidate target hashes. Avoid draining and rebuilding all eligible queue entries; for example, select/pop at most MAX_PROMOTION_TARGETS_PER_PASS entries under the lock, perform verification outside the lock, and reinsert only those bounded entries that remain unresolved. Add a stress test asserting that both candidate selection and application touch no more than the configured per-pass target limit when the future queue contains many eligible targets.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

The per-pass budget added in f199138 bounded only the expensive half.
`promotion_candidates` stopped at `MAX_PROMOTION_TARGETS_PER_PASS`, but
`apply_promotion` went on popping every queued target at or below the head and
pushing back whatever it could not resolve. An attacker who fills the future pool
with distinct target hashes — which is possible while sender authentication is
undecidable, since those buckets are deliberately uncapped — forces tens of
thousands of heap operations on every import, under the write lock that every
incoming vote contends for. Reported by Hashdit Bot on #491.

Bounding the pops alone would have been worse than the bug. Selection walked the
heap in iteration order, which is unordered, while application pops in ascending
target order, so the budgeted targets were an arbitrary subset and the full drain
was the only reason the write phase ever reached them. Capping the loop would
have quietly stopped promoting anything.

So both halves now agree on the same set: selection takes the `k` *smallest*
eligible targets via a bounded max-heap — O(n log k), read lock only, no
allocation beyond `k` — and application pops at most `k`, which are exactly those
targets. Anything a concurrent insert slips in front of them is simply handled by
the next pass.

`apply_promotion` returns the number of targets it examined so the bound is
assertable, and two tests cover it: a pass over 20x the budget where nothing
resolves must still examine exactly `k` and keep every deferred entry, and a
spread-height pool must select the `k` smallest so the write phase promotes all
of what was judged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown

Pull Request Review

This PR restructures the Parlia vote pool into current and future queues, adds validator-origin and source checks, per-target limits, bounded promotion, safer pruning, and global pool shedding. It also promotes future votes after canonical fork-choice updates, centralizes justified-pair derivation, and adds extensive tests for admission, promotion, overflow, packet handling, and denial-of-service boundaries.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] Oversized future pool causes repeated full-pool sorting and rebuilding under the write lock

File: src/consensus/parlia/vote_pool.rs

When future-vote authentication is undecidable, attacker-created votes are intentionally admitted without a per-target cap. Once the pool exceeds MAX_VOTES_IN_POOL, every subsequent insertion can call shed_future_votes(1), which copies and sorts the entire future target queue and then rebuilds it while holding the global vote-pool write lock. By using one vote per distinct target hash, a remote attacker can keep roughly 65,000 targets resident and force O(n log n) sorting plus O(n) queue reconstruction for every additional vote. This condition is reachable during provider startup and around validator-set swaps, and per-peer limits can be bypassed with multiple peers, potentially stalling vote ingestion and block-import promotion.

Recommendation: Enforce a cheap hard ceiling before inserting an unauthenticated future vote and use an eviction structure that removes the furthest-ahead target in O(log n), without sorting and rebuilding the full queue. Also consider limiting the number of distinct future target hashes and shedding enough entries in batches to create headroom rather than triggering eviction on every subsequent vote.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

`PromotionCandidate::target_number` and `PromotionApplied::examined` were read
only from tests, so the lib build tripped `dead_code`, which CI turns into an
error via `-D warnings`. My local run was `clippy --lib` without that flag and
only grepped for errors, so it passed while CI failed.

Both fields describe something worth seeing in production rather than only in
assertions, so put them to use instead of annotating them away: trace the target
height when a candidate stays future, and record the per-pass budget consumption
as a histogram. Sitting at the cap means a backlog is draining across passes, or
that someone is filling the future pool faster than promotion retires it.

Verified with the CI invocation this time:
`RUSTFLAGS="-D warnings" cargo clippy --workspace --tests --all-features`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown

Pull Request Review

This PR splits Parlia votes into current and future pools, adds validator/source admission checks, bounded promotion, per-target caps, safe pruning, and oversized-pool shedding. It also triggers future-vote promotion after successful fork-choice updates, centralizes justified-pair resolution, and adds regression tests for vote admission, promotion, rate limiting, and first-vote-only packet handling.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] A single validator can exhaust the future-vote cap and exclude honest votes

File: src/consensus/parlia/vote_pool.rs

The future bucket cap counts envelopes rather than distinct validator addresses. Before promotion, future_vote_sender_is_validator only establishes that the sender belongs to the current validator set; it does not validate the source pair. A Byzantine validator can therefore sign 50 unique envelopes for the same target by varying source fields, fill MAX_FUTURE_VOTE_AMOUNT_PER_BLOCK, and cause later honest validator votes for that target to be rejected. Because votes are not re-sent, this can suppress quorum/finality for affected targets.

Recommendation: Enforce at most one retained future vote per vote_address and target, or otherwise reserve capacity by unique validator identity rather than envelope count. Conflicting votes from the same validator should not consume additional capacity; alternatively, defer the cap until full origin/source validation is possible.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

`shed_future_votes` collects every future queue entry, sorts it, drops the
furthest-ahead targets, then rebuilds the queue — O(n log n) plus a full
reconstruction, under the global write lock. The call site asked it to release
exactly the overflow, so once the pool sat at its ceiling the *next* admitted vote
overflowed again and paid the whole cost. With one vote per distinct target hash
an attacker can keep ~65k targets resident, which is reachable while sender
authentication is undecidable, since those buckets are deliberately uncapped.
Reported by Hashdit Bot on #491.

Shed down to `MAX_VOTES_IN_POOL - SHED_HEADROOM` instead, so the pass runs about
once per 4096 admissions rather than once per vote.

Dropping buckets leaves their queue entries behind, which the full rebuild used to
clean up. That rebuild now runs rarely, so `apply_promotion` discards an entry
whose bucket is gone rather than deferring it — deferring re-queues it on every
pass, forever. The same applies to entries orphaned by `prune`.

Kept the sort and rebuild rather than restructuring the queue: extracting the
furthest-ahead target in O(log n) needs a different data structure for the future
side, and with the cost amortised that is a change worth making deliberately
rather than under a review deadline.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown

Pull Request Review

This PR splits Parlia vote storage into current and future pools, adds validator-origin checks, per-target caps, bounded promotion, safer pruning/shedding, and promotes future votes after canonical fork-choice updates. It also centralizes justified-pair derivation, preserves first-vote-only packet admission, and adds extensive tests for admission, promotion, overflow, and validator-set boundary behavior.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] Older future votes can be permanently rejected using the wrong validator set

File: src/consensus/parlia/vote_pool.rs

future_vote_sender_is_validator authenticates a future vote against the validator set at the current head and relies on validator_set_swaps_within(head, target, ...) to return None when the governing set may differ. However, the predicate only checks whether swaps_upto(target) > swaps_upto(head). For an admissible target behind the head, this is always false even when a validator-set swap occurred between the target and head. A legitimate vote for an older non-canonical target can therefore be classified as coming from a non-validator, rejected, and placed in REJECTED_VOTES, preventing that envelope from being accepted if the target later becomes canonical during a reorganization. Because votes are not resent, this can impair consensus finality around validator-set transitions.

Recommendation: Handle targets behind the head explicitly. Treat membership as undecidable whenever the target and head are governed by different validator sets, for example by comparing swap counts with swaps_upto(target) != swaps_upto(head) rather than >, or conservatively return None for older targets unless their governing snapshot can be resolved. Add a regression test where target < head and a validator-set swap lies in (target, head].


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

`future_vote_sender_is_validator` judges a future vote's sender against the set at
our own head, guarded by a predicate that asked only "does a validator-set swap
lie after the head". For any target at or below the head that is false, so the
guard never fired and the vote was judged against our head's set — which is not
the set that governs it when a swap lies in between.

Such targets are ordinary, not an edge case. A future vote is one whose block we
do not hold, which says nothing about its height: the admission window reaches
back to `head - 255`, so a vote for a valid block on a branch we have not seen is
routinely behind the head. A validator that left at the swap is absent from our
head's set, gets `Some(false)`, and is rejected — and since f199138 those
rejections are cached, so the envelope stays out even if that branch later becomes
canonical. Votes are never re-sent, so the evidence is gone. Reported by Hashdit
Bot on #491.

Compare the two governing sets instead, in either order. Membership for a target
is resolved from the target's *parent* snapshot — what `verify_vote_origin` uses,
and go-bsc's `VerifyVote` too — so the comparison is `head` against `target - 1`,
which also closes the off-by-one I left when fixing the boundary: a vote for the
swap block itself is governed by the outgoing set and is now judged rather than
declined.

`validator_set_swaps_within` becomes `governing_sets_differ`, and the differential
test against `SnapshotProvider::try_rebuild`'s predicate now covers every ordered
pair the admission window permits, behind the head as well as ahead of it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown

Pull Request Review

This Rust BSC/Parlia node change splits votes into current and future pools, adds validator-origin and source checks, per-target limits, bounded promotion, pruning, and overflow shedding. It promotes future votes after successful fork-choice updates, centralizes justified-pair derivation, and adds extensive tests for admission, promotion, resource bounds, startup behavior, and first-vote-only packet handling.

Sensitive Content

No sensitive content detected.

Security Issues

No serious security issues detected.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

Brings the branch up to date with develop (7 commits: #501, #502, #504, #498,
#499, #469, #506). None of them touch `vote_pool.rs` or `node/consensus.rs`, so
the vote-pool work merges without conflict.

Merged rather than rebased: will-2012 has three open review threads anchored to
lines in this branch, and rewriting the history would detach them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 15, 2026

Copy link
Copy Markdown

Pull Request Review

This PR restructures Parlia vote ingestion into separate current and future vote pools, adds validator-origin checks, per-target limits, bounded promotion, safer pruning/shedding, and shared justified-pair resolution. It also triggers future-vote promotion after successful fork-choice updates and adds extensive tests for admission, promotion, resource bounds, and first-vote-only packet handling.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] Unauthenticated votes can still make authenticated validator votes hit the future-vote cap

File: src/consensus/parlia/vote_pool.rs

The future-vote capacity check is selected using the incoming vote's authenticated flag, but it compares the cap against the total bucket length, including previously admitted unauthenticated votes. During startup or whenever validator membership is undecidable, an attacker can populate a future target bucket beyond 50 entries; once membership becomes available, a legitimate validator's authenticated vote for that target will see the already oversized bucket and be rejected. Because votes are broadcast only once, this can censor genuine votes and impair finality/liveness—the exact failure mode the conditional cap is intended to prevent.

Recommendation: Track authentication status per stored vote or maintain separate authenticated and undecidable counts. When admitting an authenticated future vote, enforce the cap only against authenticated validator votes; alternatively, revalidate and remove unauthenticated bucket entries before applying the cap, ensuring untrusted entries can never cause a legitimate vote to be refused.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

The future-vote cap is selected by the incoming vote's authentication but was
measured against the whole bucket. Buckets whose sender could not be
authenticated are deliberately uncapped, so anything admitted during an
undecidable window counted toward a cap it was exempt from: an attacker fills a
target past 50 with minted keys, and the first genuine validator vote for that
target is then refused. Votes are broadcast once and never re-sent, so that
evidence is gone — the censorship this conditional cap exists to prevent.
Reported by Hashdit Bot on #491.

The undecidable window is not just startup. `future_vote_sender_is_validator`
returns `None` whenever our head's set is not the set governing the target, which
is every vote whose window spans a validator-set swap, so the window reopens each
epoch.

Record authentication per vote and keep a per-bucket count of the authenticated
ones, then cap on that. A counter rather than a scan, because the bucket being
measured is the unbounded one and walking it on every admission would be its own
denial of service.

The existing capacity test missed this because it exercised an unauthenticated
bucket and an authenticated bucket separately, never one holding both. The
regression test uses a single target: a flood of 4x the cap must not refuse a
validator's vote, while the cap still closes after 50 authenticated ones.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 15, 2026

Copy link
Copy Markdown

Pull Request Review

This Rust BSC/Parlia consensus-node PR splits vote storage into current and future pools, adds validator-origin admission checks, bounded promotion, per-target caps, safer pruning, and overflow shedding. It also promotes future votes after successful fork-choice updates, centralizes justified-pair derivation, and adds extensive tests for admission, promotion, rate limiting, packet handling, and denial-of-service edge cases.

Sensitive Content

No sensitive content detected.

Security Issues

No serious security issues detected.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

The comments carried "Reported by Hashdit Bot on #491" / "Raised by will-2012 on
#491" tags from the review round. They date the code to one discussion and say
nothing a later reader needs: the reasoning they trail is what matters, and the
provenance already lives in the commit history and the PR.

Comment-only; the prose is re-wrapped where removing the sentence left a short
line. No behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 15, 2026

Copy link
Copy Markdown

Pull Request Review

This PR refactors Parlia vote ingestion into separate current and future pools, adds validator-origin checks, per-target caps, bounded promotion, safer pruning and overflow shedding, and promotes future votes after successful fork-choice updates. It also centralizes justified-pair resolution, preserves first-vote-only packet admission, and adds extensive regression and resource-bound tests.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] Future votes can be permanently rejected using the validator set from an unrelated fork

File: src/consensus/parlia/vote_pool.rs

future_vote_sender_is_validator assumes that equal validator-set boundary counts imply the canonical head and an unknown target's parent use the same validator set. For a non-canonical target on another branch, especially after that branch crossed an epoch transition with different state or epoch data, its governing validator set can differ even when governing_sets_differ returns false. A legitimate validator from that branch is then classified as Some(false), and put_vote_inner permanently inserts the envelope hash into REJECTED_VOTES; if the branch later becomes canonical, replaying the valid vote is still rejected. This can discard legitimate consensus votes during epoch-spanning reorganizations and impair finality.

Recommendation: Do not reject or permanently cache future votes based solely on the canonical head's validator set and block-number boundary arithmetic. Only authenticate against that set when ancestry proves the target's parent shares the relevant validator-set transition history; otherwise return None, retain the vote as undecidable, and perform authoritative validation against the target-parent snapshot during promotion.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

will-2012
will-2012 previously approved these changes Sep 15, 2026

@will-2012 will-2012 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ponytail skill maybe make it more simple and clean.

`future_vote_sender_is_validator` judges a future vote's sender against our own
head's set, guarded by `governing_sets_differ` — arithmetic over block heights.
That arithmetic cannot see which branch the target sits on. A fork that diverged
before the last epoch checkpoint elects its own validator set, and its
`miner_history_check_len` puts the swap boundaries at different heights again, so
equal boundary counts do not prove equal sets. A validator legitimate on that
branch can be answered `Some(false)` here.

Caching that refusal made it permanent: the envelope stayed out even if the
branch later became canonical, and votes are never re-sent. The refusal itself is
a deliberate trade — it keeps minted keys from creating a bucket — but treating a
branch-blind guess as a fact about the envelope is not.

Replays are not hypothetical. geth relays every verified vote to peers that lack
it, so a reth node peered with geth sees the same envelope repeatedly, and
`sync_pending_votes_to_peer` dumps pools to new peers. Without the cache a later
copy is judged afresh, by which time the head may have moved past the divergence.

go-bsc forms no such verdict at all — `VerifyVote` runs only for current votes —
and goes further: when verification fails at transfer it *removes* the hash from
`receivedVotes`, so a later copy is not mistaken for a duplicate. Caching stays
for `verify_vote_origin`'s rejections, which are judged against the target's own
parent snapshot and so are branch-correct, and for BLS failures.

The doc comment claimed this verdict "cannot start passing". That was true within
one chain and false across branches; corrected to state the limitation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hashdit-bot

hashdit-bot Bot commented Sep 15, 2026

Copy link
Copy Markdown

Pull Request Review

This PR restructures the Rust Parlia vote pool into current and future queues, adds validator-origin checks, per-target caps, bounded promotion, safer pruning, and overflow shedding. It also promotes future votes after successful fork-choice updates, centralizes justified-pair derivation, and adds extensive regression tests for admission, promotion, rate limiting, and packet handling.

Sensitive Content

No sensitive content detected.

Security Issues

No serious security issues detected.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

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.

3 participants