Extract vote eligibility logic into voting_policy#5044
Merged
pwojcikdev merged 1 commit intonanocurrency:developfrom Mar 24, 2026
Merged
Extract vote eligibility logic into voting_policy#5044pwojcikdev merged 1 commit intonanocurrency:developfrom
pwojcikdev merged 1 commit intonanocurrency:developfrom
Conversation
647ed1b to
e5e0a54
Compare
Test Results for Commit 52346f4Pull Request 5044: Results Test Case Results
Last updated: 2026-03-24 16:45:21 UTC |
e5e0a54 to
cdc93d1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes vote eligibility checks and vote construction/signing behind a new nano::voting_policy, replacing duplicated logic previously split across vote_generator and request_aggregator. This improves consistency and makes vote-safety invariants (especially around final_vote and fork-safe replies) easier to reason about.
Changes:
- Introduces
nano::voting_policyandnano::vote_permitto unify normal/final/reply eligibility checks and vote signing. - Refactors
vote_generatorandrequest_aggregatorto delegate eligibility decisions and signing tovoting_policy. - Adds comprehensive unit tests covering eligibility, fork safety, and signing behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nano/secure/voting_policy.hpp | Declares vote_type, vote_permit, and voting_policy public API for eligibility + signing. |
| nano/secure/voting_policy.cpp | Implements eligibility rules and centralized vote signing logic. |
| nano/secure/fwd.hpp | Adds forward declarations for vote_permit, voting_policy, and vote_type. |
| nano/secure/CMakeLists.txt | Adds new voting policy sources to the secure library target. |
| nano/node/vote_generator.hpp | Updates generator interfaces/types to operate on vote_permit and use voting_policy. |
| nano/node/vote_generator.cpp | Replaces internal eligibility/signing logic with voting_policy calls. |
| nano/node/request_aggregator.hpp | Injects voting_policy dependency. |
| nano/node/request_aggregator.cpp | Uses voting_policy::reply_final to decide final vote reply eligibility. |
| nano/node/node.hpp | Stores/owns a voting_policy instance on the node. |
| nano/node/node.cpp | Constructs/injects voting_policy into vote generators and request aggregator. |
| nano/core_test/voting_policy.cpp | Adds extensive test coverage for eligibility, fork safety, and signing. |
| nano/core_test/CMakeLists.txt | Adds voting_policy.cpp to the core test target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cdc93d1 to
3fdfcf7
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Consolidate vote eligibility checks previously scattered across vote_generator and request_aggregator into a dedicated voting_policy class. The vote_permit type ensures callers cannot bypass the policy check.
3fdfcf7 to
52346f4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Vote eligibility checks were previously scattered across
vote_generatorandrequest_aggregator, with each component independently deciding whether a block should receive a normal or final vote. This made it difficult to reason about vote safety as a whole, and easy to introduce inconsistencies when modifying one path but not the other.This PR introduces a
voting_policyclass that serves as the single source of truth for all vote eligibility decisions and vote construction. The class exposes three eligibility methods (vote_normal,vote_final,reply_final) and asignmethod that constructs and signs votes. Avote_permittype with a private constructor ensures that callers cannot bypass the policy.The key safety invariants enforced by the policy are: normal votes require all block dependencies to be cemented and refuse to vote if a conflicting final vote record exists for the same root, final votes additionally claim a persistent slot via
final_vote.putto prevent double-voting on forks, and the reply path (reply_final) is read-only and returns the recorded hash rather than the queried block's hash to avoid accidentally endorsing a fork during vote replies.vote_generatorandrequest_aggregatorare updated to delegate all eligibility and signing decisions tovoting_policy, removing their duplicated logic. The vote construction code that previously lived insidevote_generator::broadcastandvote_generator::replyis now handled byvoting_policy::sign.