Refactor election ballot, vote routing, and confirmation reporting - #5159
Refactor election ballot, vote routing, and confirmation reporting#5159pwojcikdev wants to merge 18 commits into
Conversation
e59aa32 to
605c64b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Route cleanup has concurrency windows that can leave current vote targets unrouted or recreate routes to erased elections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extracts deterministic consensus bookkeeping into election_ballot and updates routing, status reporting, and confirmation consumers.
Changes:
- Adds ballot and representative-tier abstractions with focused tests.
- Refactors election lifecycle, fork admission, vote retention, and routing.
- Consolidates confirmation notifications into
block_confirmation_info.
File summaries
| File | Description |
|---|---|
nano/slow_test/node.cpp |
Updates vote-count expectations. |
nano/secure/rep_tiers.hpp |
Declares shared representative tiers. |
nano/secure/rep_tiers.cpp |
Implements tier classification. |
nano/secure/fwd.hpp |
Forwards election_ballot. |
nano/secure/election_ballot.hpp |
Defines ballot state and API. |
nano/secure/election_ballot.cpp |
Implements voting, tallying, and fork admission. |
nano/secure/CMakeLists.txt |
Builds new secure components. |
nano/rpc_test/rpc.cpp |
Updates confirmation RPC expectations. |
nano/qt/qt.cpp |
Migrates Qt confirmation observer. |
nano/node/websocket.hpp |
Updates confirmation interfaces. |
nano/node/websocket.cpp |
Consumes consolidated confirmation data. |
nano/node/vote_with_weight_info.hpp |
Removes superseded vote-info type. |
nano/node/vote_router.hpp |
Adds election-indexed routes. |
nano/node/vote_router.cpp |
Implements route ownership and cleanup. |
nano/node/rpc_callbacks.cpp |
Migrates HTTP confirmation callbacks. |
nano/node/rep_tiers.hpp |
Reuses shared tier definitions. |
nano/node/rep_tiers.cpp |
Centralizes tier boundary calculations. |
nano/node/node_observers.hpp |
Adds consolidated confirmation observer. |
nano/node/node_observers.cpp |
Updates observer diagnostics. |
nano/node/local_vote_history.cpp |
Uses canonical final-vote detection. |
nano/node/json_handler.cpp |
Updates status and confirmation RPC handling. |
nano/node/ipc/ipc_broker.cpp |
Migrates IPC confirmation events. |
nano/node/fwd.hpp |
Updates status and confirmation declarations. |
nano/node/election.hpp |
Integrates ballot-owned consensus state. |
nano/node/election.cpp |
Refactors election voting and lifecycle behavior. |
nano/node/election_status.hpp |
Defines status and confirmation payloads. |
nano/node/election_status.cpp |
Implements status formatting. |
nano/node/confirmation_solicitor.cpp |
Uses canonical final timestamps. |
nano/node/CMakeLists.txt |
Updates node build sources. |
nano/node/active_elections.hpp |
Extends cementing results with confirmation type. |
nano/node/active_elections.cpp |
Refactors publication, cleanup, and notifications. |
nano/lib/vote.hpp |
Clarifies vote timestamp constants. |
nano/lib/vote.cpp |
Uses the final timestamp constant. |
nano/lib/stats_enums.hpp |
Aligns statistics with confirmation types. |
nano/core_test/vote_router.cpp |
Tests route ownership cleanup. |
nano/core_test/vote_processor.cpp |
Updates synthetic-vote expectations. |
nano/core_test/rep_tiers.cpp |
Tests tier boundaries. |
nano/core_test/recently_cache.cpp |
Updates status fixtures. |
nano/core_test/online_reps.cpp |
Uses renamed block query. |
nano/core_test/node.cpp |
Updates election and observer tests. |
nano/core_test/network.cpp |
Updates vote-count expectations. |
nano/core_test/ledger.cpp |
Updates ballot and publication tests. |
nano/core_test/election.cpp |
Adds lifecycle and retained-route coverage. |
nano/core_test/election_scheduler.cpp |
Uses the new extended-status API. |
nano/core_test/election_ballot.cpp |
Adds comprehensive ballot tests. |
nano/core_test/conflicts.cpp |
Updates initial vote expectations. |
nano/core_test/CMakeLists.txt |
Registers new tests. |
nano/core_test/active_elections.cpp |
Updates fork and retained-vote coverage. |
Review details
- Files reviewed: 48/48 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Disconnect routes for both held and previously evicted blocks | ||
| node.vote_router.disconnect (election); |
There was a problem hiding this comment.
Fixed in 4a8be89. erase_election now cancels the election before disconnecting its routes, so a publish or vote already dispatched to it is refused and can no longer register a route after the disconnect. The tick cleanup path already erased sealed elections; this closes the rollback, RPC and wallet erase paths. Stats and logs keep reporting the state the election was erased in. Covered by active_elections.erase_seals_live_election, and election.sealed_state_transitions pins that cancelling a confirmed or expired election leaves its state unchanged.
| // Stop routing an unheld hash once no current vote references it | ||
| if (previous_vote && previous_vote->hash != block_hash && !ballot.contains_block (previous_vote->hash) && !ballot.has_vote_for (previous_vote->hash)) | ||
| { | ||
| node.vote_router.disconnect (previous_vote->hash); | ||
| } |
There was a problem hiding this comment.
Fixed in 4a8be89. An accepted vote for a hash the election does not hold now reconnects its route before the previous target is pruned, so a vote dispatched before the release ends up routed regardless of which side wins the election mutex. The same window at eviction time in publish is closed by the same reconnect, since the late vote restores the route itself. Tests election.in_flight_vote_restores_released_route and election.in_flight_vote_restores_evicted_route stage the late delivery by calling the election directly once the route is gone.
|
Here is Claude code review: Blocking-ish
Neither connect nor disconnect(hash) checks route ownership. Two fixes, either works: seal the election inside erase_election (smallest change, and it also makes the post-erase election->blocks() snapshot meaningful), or make both router calls election-aware (disconnect(hash, election) erases only if that election owns the route; connect refuses when a different election owns it). The connect half of this pre-exists on develop, but the new disconnect(hash) from election::vote widens the surface.
Behavior changes that need explicit sign-off / release notes These all look correct, but they're consensus- and API-visible, and the PR body understates a few:
Cooldown thresholds themselves are unchanged: calculate_rep_tier reproduces >5% → 1s, >1% → 5s, else 15s exactly, and the rep_tiers::calculate_tiers fallthrough preserves the old set membership. Copilot's second comment Real, but low severity and self-healing — I wouldn't block on it. The window (router releases its shared lock before election::vote) can leave has_vote_for(hash) == true with no route; subsequent votes for that same hash fall through to the vote cache. The rep's next vote for any held hash still routes and overwrites the entry. Worth a comment documenting the accepted looseness rather than reconnecting every unheld target. Minor
|
A bare contains does not say what the election contains.
605c64b to
4a8be89
Compare
Store vote routes in a multi-index keyed by block hash and election ownership. Removing an election can now erase all of its routes directly without querying the election under the router lock, while weak ownership still allows abandoned routes to expire.
Move vote tracking, tallies, winner selection, and fork replacement into a standalone, thoroughly tested component. Remove the synthetic null vote and make equal-weight tally ordering deterministic. A single private status_locked helper composes the public status copy from the ballot and the log lines read the ballot counts directly, with the status accessors paired as get_status/get_extended_status over status_locked/extended_status_locked. The status copy itself moves behind the class, tests query the winner instead of reaching into it. The vote broadcast path re-evaluates unconditionally, so the outgoing vote references the freshly retallied leader.
…ion mutex A concurrent republish of an evicted block could reinsert and reconnect it after the election mutex was released but before its old route was disconnected. The late disconnect would then erase the fresh route even though the election held the block again. Keep the election mutex held through route removal so block replacement and routing remain ordered.
Insert only consults cached_tally on the full-ballot path, yet publish paid the vote cache scan, the weight lookups and an unlock/relock for every new fork. Try the plain insert first and fall back to the scan and a retry only on rejection; this also folds the separate contains_block pre-check into insert's updated outcome.
The ballot consults the cooldown only when throttling a rep's follow-up vote, so first-time votes, replays and final-vote admissions paid the online stake read for nothing. Gate the computation on an existing recorded vote, matching the shape the code had before the ballot extraction.
The cooldown thresholds restated the rep tier percentages independently, so tuning one silently diverged from the other. A calculate_rep_tier free function now holds the single definition, used by both the tier calculation and the vote cooldown mapping.
The election kept a full election_status member, mixing the last-evaluation winner mirror and tally cache with the one-shot confirmation timestamps, while status_locked overwrote half the fields on every read. Keep the last evaluation result and the confirmation timestamps as dedicated members and compose the status purely on demand from the ballot. The status struct becomes a plain aggregate and a live election always reports ongoing, dropping the write-only stopped assignment.
election.hpp carried the extended status while the plain status lived alone in election_status.hpp. Gather the type enum and both structs there, with their object stream support and enum helpers in a matching source file, and nest the composed status inside the extended report. election.hpp keeps only the election machinery.
A full ballot admitted an incoming block only on the externally observed cached tally, so the votes deliberately retained for an evicted fork did not back its own return. Take the stronger of the two sources; they may both count the same rep, so they must not add.
An evicted fork still needs its route while a representative's current vote references it. Retain that route so later votes reach the election and recorded support can readmit the block. Remove it when the last such vote moves away, and disconnect an unvoted fork at eviction. This bounds the route set by the held blocks plus current vote targets. Connect admitted forks while holding the election mutex so replacement and route updates stay ordered. Whole-election cleanup uses the router's election index.
The winner always references a held block and is the only block the election broadcasts or votes for. Once the participation gate is met, it follows the heaviest held block. After the same gate, the leader tracks the heaviest voted-for hash overall and may therefore be unheld. Quorum now accounts for every current vote target: the participation gate sums all recorded vote weight, and the runner-up may be unheld. When the leader's block becomes available to a full ballot, it bypasses the weight comparison and replaces the weakest non-winner.
The election stamped a placeholder ongoing type into every composed status, while the meaningful values were decided later by the cementing path and guarded by leak asserts in the observers. The type classifies how a block got cemented, not the election, so carry it beside the status: block_cemented picks it, the block observers receive it as a dedicated confirmation_type argument, and the status struct drops the field. The dead ongoing and stopped values go with it, and the IPC broker stops overwriting the confirmation type it just computed.
The block observers took seven loose arguments and every subscriber redeclared the full row to use one or two of them. Gather the status, confirmation type, votes, account, amount and subtype flags into a block_confirmation_info struct, notify with it, and name the observer block_confirmed after the event it reports. The websocket builder and broadcast take the payload directly, dropping the block and account arguments that duplicated fields already inside it.
4a8be89 to
97d5584
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It substantially changes consensus selection, concurrency-sensitive routing, and confirmation lifecycle behavior despite strong test coverage.
Review details
- Files reviewed: 48/48 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Test Results for Commit 9ffc30cPull Request 5159: Results Test Case Results
Last updated: 2026-09-03 22:32:11 UTC |
The vote router looks a route up under its lock and releases it before delivering the vote, so a vote can reach an election after the route it was dispatched through has been pruned: either the last supporting vote for an evicted fork moved away, or a publish evicted the unsupported block. The election then records a current vote for an unheld hash that nothing routes, and later votes for it fall back to the vote cache. An accepted vote for an unheld hash now routes it again, so both orderings converge on the same routes. Erasing a live election left it able to accept blocks and votes already dispatched to it and to register routes after its disconnect. The erase now cancels the election before disconnecting it, and the cancel reports the state it found so stats and logs describe the election as it was erased.
97d5584 to
9ffc30c
Compare
Summary
Before this change,
electionmixed the core consensus rules—vote ordering, fork admission and replacement, tallying, winner selection, and quorum—with node-facing orchestration such as routing, pacing, status reporting, and notifications. This made the most sensitive behavior difficult to reason about in isolation and expensive to test without a full node fixture.This change extracts those rules into a deterministic, self-contained
election_ballot, with representative weights, timing, cooldown, and quorum threshold supplied by the caller. The consensus behavior can therefore be exercised directly with focused, comprehensive unit tests, whileelectionremains responsible for coordinating node services and side effects. The accompanying routing, lifecycle, and confirmation refactors align the surrounding code with that new ownership boundary.Changes
election_ballotto own representative votes, held blocks, tallying, replay and cooldown decisions, and winner selection. Remove the synthetic null vote and deterministically order equal-weight forks by hash.confirmation_typefrom election status, and replace the multi-argument block-confirmation observer withblock_confirmation_info.Test coverage
Validation
.ai/ai_build.sh🤖 Generated with Claude Code