Feature/implement posv snapshot#20
Conversation
…loading and storing, and enhance vote management
…, and validation methods
…th dynamic epoch-based calculation
There was a problem hiding this comment.
Pull request overview
Implements PoSV snapshot persistence/reconstruction and voting application logic, integrating it into header verification and adding unit tests.
Changes:
- Implemented PoSV snapshot lifecycle (create/load/store/copy/apply) including voting + signer set updates.
- Added snapshot reconstruction in the verifier by replaying headers and persisting checkpoint snapshots.
- Added a comprehensive snapshot test suite and introduced new PoSV-specific error variables.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| consensus/posv/verifier.go | Implements snapshot retrieval/rebuild flow and adjusts timestamp error usage. |
| consensus/posv/snapshot.go | Adds full snapshot + voting logic, including signer sorting and in-turn calculation. |
| consensus/posv/snapshot_test.go | Introduces unit tests for snapshot creation, persistence, voting tallying, and header application. |
| consensus/posv/posv.go | Adds/renames errors used by snapshot/verifier logic. |
| consensus/errors.go | Removes duplicate/redundant license header lines. |
| common/slice.go | Removes redundant license header block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // at a checkpoint block without a parent (light client CHT), or we have piled | ||
| // up more headers than allowed to be reorged (chain reinit from a freezer), | ||
| // consider the checkpoint trusted and snapshot it. | ||
| if number == 0 || (number%c.config.Epoch == 0 && (len(headers) > params.FullImmutabilityThreshold || chain.GetHeaderByNumber(number-1) == nil)) { |
There was a problem hiding this comment.
Checkpoint handling elsewhere in this method uses (number+c.config.Gap)%c.config.Epoch == 0, but this trusted-checkpoint condition uses number%c.config.Epoch == 0. If Gap is non-zero, the logic will disagree on what constitutes a checkpoint, potentially treating non-checkpoint blocks as trusted checkpoints (or missing actual ones). Align this condition with the same (number+c.config.Gap)%c.config.Epoch rule.
| if number == 0 || (number%c.config.Epoch == 0 && (len(headers) > params.FullImmutabilityThreshold || chain.GetHeaderByNumber(number-1) == nil)) { | |
| if number == 0 || ((number+c.config.Gap)%c.config.Epoch == 0 && (len(headers) > params.FullImmutabilityThreshold || chain.GetHeaderByNumber(number-1) == nil)) { |
| copy := snap.copy() | ||
|
|
||
| // Verify the copy | ||
| if copy.Number != snap.Number { | ||
| t.Errorf("Expected number %d, got %d", snap.Number, copy.Number) | ||
| } | ||
|
|
||
| if len(copy.Signers) != len(snap.Signers) { | ||
| t.Errorf("Expected %d signers, got %d", len(snap.Signers), len(copy.Signers)) | ||
| } | ||
|
|
||
| if len(copy.Recents) != len(snap.Recents) { | ||
| t.Errorf("Expected %d recents, got %d", len(snap.Recents), len(copy.Recents)) | ||
| } | ||
|
|
||
| if len(copy.Votes) != len(snap.Votes) { | ||
| t.Errorf("Expected %d votes, got %d", len(snap.Votes), len(copy.Votes)) | ||
| } | ||
|
|
||
| // Modify the copy and ensure the original is unchanged | ||
| copy.Recents[100] = signers[0] | ||
| if len(snap.Recents) == len(copy.Recents) { |
There was a problem hiding this comment.
The local variable name copy shadows Go's predeclared copy function, which can make the test harder to read and can lead to accidental misuse later in the function. Rename it to something like snapCopy or cloned.
| copy := snap.copy() | |
| // Verify the copy | |
| if copy.Number != snap.Number { | |
| t.Errorf("Expected number %d, got %d", snap.Number, copy.Number) | |
| } | |
| if len(copy.Signers) != len(snap.Signers) { | |
| t.Errorf("Expected %d signers, got %d", len(snap.Signers), len(copy.Signers)) | |
| } | |
| if len(copy.Recents) != len(snap.Recents) { | |
| t.Errorf("Expected %d recents, got %d", len(snap.Recents), len(copy.Recents)) | |
| } | |
| if len(copy.Votes) != len(snap.Votes) { | |
| t.Errorf("Expected %d votes, got %d", len(snap.Votes), len(copy.Votes)) | |
| } | |
| // Modify the copy and ensure the original is unchanged | |
| copy.Recents[100] = signers[0] | |
| if len(snap.Recents) == len(copy.Recents) { | |
| snapCopy := snap.copy() | |
| // Verify the copy | |
| if snapCopy.Number != snap.Number { | |
| t.Errorf("Expected number %d, got %d", snap.Number, snapCopy.Number) | |
| } | |
| if len(snapCopy.Signers) != len(snap.Signers) { | |
| t.Errorf("Expected %d signers, got %d", len(snap.Signers), len(snapCopy.Signers)) | |
| } | |
| if len(snapCopy.Recents) != len(snap.Recents) { | |
| t.Errorf("Expected %d recents, got %d", len(snap.Recents), len(snapCopy.Recents)) | |
| } | |
| if len(snapCopy.Votes) != len(snap.Votes) { | |
| t.Errorf("Expected %d votes, got %d", len(snap.Votes), len(snapCopy.Votes)) | |
| } | |
| // Modify the copy and ensure the original is unchanged | |
| snapCopy.Recents[100] = signers[0] | |
| if len(snap.Recents) == len(snapCopy.Recents) { |
| // ErrInvalidTimestamp is returned if the timestamp of a block is lower than | ||
| // the previous block's timestamp + the minimum block period. | ||
| ErrInvalidTimestamp = errors.New("invalid timestamp") | ||
| errInvalidTimestamp = errors.New("invalid timestamp") |
There was a problem hiding this comment.
This changes ErrInvalidTimestamp from an exported identifier to an unexported one (errInvalidTimestamp). If external callers rely on posv.ErrInvalidTimestamp for error checks, this is a breaking API change. Consider keeping ErrInvalidTimestamp as an exported alias (possibly deprecated) that references the internal variable.
| // If the vote passed, update the list of signers | ||
| if tally := snap.Tally[header.Coinbase]; tally.Votes > len(snap.Signers)/2 { | ||
| if tally.Authorize { | ||
| snap.Signers[header.Coinbase] = struct{}{} | ||
| } else { | ||
| delete(snap.Signers, header.Coinbase) |
There was a problem hiding this comment.
The signer-set mutation logic here (especially deauthorizing signers) needs explicit tests for edge cases introduced by the new implementation. Add unit tests to ensure (1) the last signer cannot be removed, and (2) a signer cannot accumulate multiple concurrent votes for different addresses (a new vote should replace the previous one).
…ead of block number
1 participant
This pull request implements the snapshot and voting logic for the PoSV (Proof-of-Stake Voting) consensus engine, completing previously stubbed-out methods and integrating snapshot management into the consensus flow. It also introduces several bug fixes, code cleanups, and minor refactoring for type consistency. The most important changes are grouped below.
PoSV Snapshot and Voting Logic Implementation
Snapshotstruct methods inconsensus/posv/snapshot.go, including snapshot creation, loading, storing, copying, voting, unvoting, applying headers, and retrieving signers. This enables the PoSV engine to track authorized signers, tally votes, and update the set of signers as blocks are processed.inturnfunction to determine if a signer is "in-turn" for a given block, which is important for block production rules.Consensus Engine Integration
snapshotmethod inconsensus/posv/verifier.goto retrieve or reconstruct snapshots from memory, disk, or by replaying headers, ensuring the PoSV engine can verify blocks against the current set of authorized signers.Type and Naming Consistency
signersAscendingtype toSignersAscendinginconsensus/clique/snapshot.goand updates all usages to improve naming consistency and export the type for use in other packages, such as PoSV.clique.SignersAscendingfor sorting signers, ensuring consistency with the Clique consensus implementation.License Header Cleanup
New Error Types
consensus/posv/posv.gofor more granular error handling, such aserrInvalidVotingChainanderrRecentlySigned, which are used in the snapshot and voting logic.These changes collectively enable the PoSV consensus engine to maintain and update the set of authorized signers, process votes, and validate blocks according to the PoSV protocol.