fix: prune retained attestation state#408
Conversation
|
Warning Review limit reached
More reviews will be available in 26 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 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 | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
randygrok
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| boundary := k.attestationRetentionBoundary(ctx, k.GetCurrentEpoch(ctx)) | ||
| if boundary != nil && boundary.prunesHeight(msg.Height) { | ||
| return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "attestation height %d is below retention window (min %d)", msg.Height, boundary.firstRetainedHeight) |
There was a problem hiding this comment.
The write cutoff advances a full epoch before pruning does.
Attest uses GetCurrentEpoch() on every block, but pruning only runs in processEpochEnd() on the last block of the epoch. With EpochLength > 1, the first block of a new epoch starts rejecting heights that are still retained for the rest of that epoch. For example, with EpochLength=10 and PruneAfter=2, block 50 rejects height 25 (min=30), but PruneOldBitmaps will not delete height 25 until block 59.
Base the validation boundary on the last completed pruning epoch, or only advance it on epoch-boundary blocks, and add a regression test with EpochLength > 1.
🤖 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 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.
| @@ -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. | |||
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.
| 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.
1 participant
Summary
MsgAttestheight validation.Validation
go test -count=1 ./modules/network/keeperSummary by CodeRabbit
New Features
Documentation