fix: prune retained attestation state#408
Open
randygrok wants to merge 4 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements attestation state retention and pruning for the network module. A new retention boundary abstraction computes which epochs and block heights should be retained based on the ChangesAttestation State Retention and Pruning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/network/keeper/msg_server.go (1)
55-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject negative attestation heights before they become unprunable state.
When
currentEpoch <= PruneAfter,boundaryisnil, so an authorized attester can still writemsg.Height < 0. Downstream,PruneOldBitmapsonly clearsAttestationBitmapandStoredAttestationInfoover[0, firstRetainedHeight), so those negative-height entries never age out.Suggested fix
currentHeight := ctx.BlockHeight() maxFutureHeight := currentHeight + 1 + if msg.Height < 0 { + return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "attestation height must be non-negative: %d", msg.Height) + } if msg.Height > maxFutureHeight { return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "attestation height %d exceeds max allowed height %d", msg.Height, maxFutureHeight) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/network/keeper/msg_server.go` around lines 55 - 69, Add an explicit rejection for negative attestation heights: before using attestationRetentionBoundary(k.attestationRetentionBoundary) or calling boundary.prunesHeight, check if msg.Height < 0 and return an sdkerrors.ErrInvalidRequest explaining the height is negative; this ensures when currentEpoch <= PruneAfter (boundary == nil) callers cannot write negative heights that PruneOldBitmaps (which only trims [0, firstRetainedHeight)) will never remove from AttestationBitmap/StoredAttestationInfo. Use the same error wrapping style as the existing checks so the check sits alongside the current maxFutureHeight and boundary.prunesHeight validations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 67-69: The Attest validation uses
k.attestationRetentionBoundary(ctx, k.GetCurrentEpoch(ctx)) causing the write
cutoff to advance at the start of a new epoch before pruning runs in
processEpochEnd; change the boundary calculation to use the last
completed/pruned epoch (e.g., compute lastCompletedEpoch :=
k.GetCurrentEpoch(ctx) - 1 or only advance when ctx.BlockHeight() is the epoch
boundary) and pass that into attestationRetentionBoundary so Attest rejects only
heights actually pruned by PruneOldBitmaps; update attestationRetentionBoundary
call sites (including Attest) accordingly and add a regression test exercising
EpochLength > 1 demonstrating block-start rejects do not occur until the
epoch-end prune has run.
In `@modules/network/README.md`:
- Line 130: The README's checkpoint event description is out of sync with the
actual EventSoftCheckpoint emitted by emitCheckpointHashes: update the
documentation text to list the real event fields (height, validator_hash,
commit_hash, soft_confirmed) and remove/replace references to block_hash,
attester bitmap, and voting power, or alternatively change emitCheckpointHashes
(EventSoftCheckpoint emission in abci.go) to emit the documented fields; refer
to the function emitCheckpointHashes and the EventSoftCheckpoint usage to ensure
the docs exactly match the emitted event structure.
---
Outside diff comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 55-69: Add an explicit rejection for negative attestation heights:
before using attestationRetentionBoundary(k.attestationRetentionBoundary) or
calling boundary.prunesHeight, check if msg.Height < 0 and return an
sdkerrors.ErrInvalidRequest explaining the height is negative; this ensures when
currentEpoch <= PruneAfter (boundary == nil) callers cannot write negative
heights that PruneOldBitmaps (which only trims [0, firstRetainedHeight)) will
never remove from AttestationBitmap/StoredAttestationInfo. Use the same error
wrapping style as the existing checks so the check sits alongside the current
maxFutureHeight and boundary.prunesHeight validations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f9cade7-fe32-4715-a7ef-6810375c3f74
📒 Files selected for processing (5)
modules/network/README.mdmodules/network/keeper/abci.gomodules/network/keeper/keeper.gomodules/network/keeper/msg_server.gomodules/network/keeper/msg_server_test.go
| @@ -128,7 +128,20 @@ The module's `EndBlocker` is executed at the end of every block and performs the | |||
|
|
|||
| 1. **Quorum Evaluation**: It iterates through recent blocks that have received attestations and checks if the cumulative voting power of the attesters has reached the required quorum. | |||
| 2. **Checkpoint Emission**: If quorum is met for a block, it emits an `EventSoftCheckpoint` with the `height`, `block_hash`, a bitmap of the participating attesters, and the total voting power. | |||
Contributor
There was a problem hiding this comment.
Critical: Event field documentation doesn't match implementation.
The documentation claims the checkpoint event includes block_hash, a bitmap of participating attesters, and total voting power. However, the actual event emitted by emitCheckpointHashes (abci.go:131-148) includes different fields:
- Actual:
height,validator_hash,commit_hash,soft_confirmed - Documented:
height,block_hash, bitmap, voting power
This mismatch will mislead consumers trying to parse checkpoint events.
📝 Proposed fix to align documentation with actual event
-2. **Checkpoint Emission**: If quorum is met for a block, it emits an `EventSoftCheckpoint` with the `height`, `block_hash`, a bitmap of the participating attesters, and the total voting power.
+2. **Checkpoint Emission**: If quorum is met for a block, it emits a `checkpoint` event with the `height`, `validator_hash`, `commit_hash`, and `soft_confirmed` status.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 2. **Checkpoint Emission**: If quorum is met for a block, it emits an `EventSoftCheckpoint` with the `height`, `block_hash`, a bitmap of the participating attesters, and the total voting power. | |
| 2. **Checkpoint Emission**: If quorum is met for a block, it emits a `checkpoint` event with the `height`, `validator_hash`, `commit_hash`, and `soft_confirmed` status. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 130-130: Spaces after list markers
Expected: 1; Actual: 2
(MD030, list-marker-space)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/network/README.md` at line 130, The README's checkpoint event
description is out of sync with the actual EventSoftCheckpoint emitted by
emitCheckpointHashes: update the documentation text to list the real event
fields (height, validator_hash, commit_hash, soft_confirmed) and remove/replace
references to block_hash, attester bitmap, and voting power, or alternatively
change emitCheckpointHashes (EventSoftCheckpoint emission in abci.go) to emit
the documented fields; refer to the function emitCheckpointHashes and the
EventSoftCheckpoint usage to ensure the docs exactly match the emitted event
structure.
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
1 participant
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.
Summary
MsgAttestheight validation.Validation
go test -count=1 ./modules/network/keeperSummary by CodeRabbit
PruneAfter) to improve the out-of-the-box retention window.Closes #399