fix(network): add attestation height bounds validation#359
fix(network): add attestation height bounds validation#359
Conversation
The Attest handler previously accepted attestations for any height, allowing attackers to spam storage with attestations for arbitrarily far-future blocks (MaxInt64) or arbitrarily old blocks. This commit adds height bounds validation: - Upper bound: msg.Height <= ctx.BlockHeight() + 1 - Lower bound: msg.Height >= ctx.BlockHeight() - (PruneAfter * EpochLength) - Lower bound floor of 1 for early chain heights The retention window is derived from the existing PruneAfter and EpochLength params, keeping it consistent with the pruning configuration. Adds 7 table-driven test cases covering boundary conditions.
Summary of ChangesHello @alpe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security and stability of the network module by addressing a critical vulnerability related to attestation handling. It introduces necessary height bounds validation for attestations, effectively preventing malicious actors from exploiting the system through storage exhaustion attacks using future or stale block attestations. This change ensures that the system's storage footprint remains predictable and secure. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds pre-validation in Attest to enforce attestation height bounds: rejects attestations above current block height + 1 or below a retention-calculated minimum before any bitmap retrieval or signature storage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Keeper as Keeper/Attest
participant Chain as BlockContext/Params
participant Storage as AttestationStore
Client->>Keeper: Send Attest(msg{Height, Signatures})
Keeper->>Chain: Query current block height, PruneAfter, EpochLength
Chain-->>Keeper: Return height and params
Keeper-->>Keeper: Compute minHeight and maxFutureHeight
alt Height out of bounds
Keeper-->>Client: Return InvalidRequest (height out of range)
else Height within bounds
Keeper->>Storage: Retrieve or create bitmap for Height/Epoch
Storage-->>Keeper: Bitmap
Keeper->>Storage: Set bit, store signature, update quorum/epoch bitmap
Keeper-->>Client: Return success, emit events
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
🧹 Nitpick comments (3)
modules/network/keeper/msg_server.go (1)
33-52:GetParamsis called twice within the same handler.
k.GetParams(ctx)is invoked at line 33 (forSignMode) and again at line 52. Hoisting a singleparamsvariable to the top of the handler eliminates the duplicate store read.♻️ Proposed refactor
+ params := k.GetParams(ctx) - if k.GetParams(ctx).SignMode == types.SignMode_SIGN_MODE_CHECKPOINT && + if params.SignMode == types.SignMode_SIGN_MODE_CHECKPOINT && !k.IsCheckpointHeight(ctx, msg.Height) { return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "height %d is not a checkpoint", msg.Height) } // ... existing attester-set and index checks ... - params := k.GetParams(ctx) currentHeight := ctx.BlockHeight()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server.go` around lines 33 - 52, Handler calls k.GetParams(ctx) twice (once for SignMode check and again later); hoist a single params := k.GetParams(ctx) at the start of the handler and replace both k.GetParams(ctx) usages with that local variable (used for SignMode comparison and later param checks). Update references to SignMode (params.SignMode) and any later uses of params so you do one store read instead of two.modules/network/keeper/msg_server_test.go (2)
260-267: Non-deterministic ordering inGetLastValidators.Iterating
m.activeSet(a plain Go map) produces a non-deterministic slice order. Currently harmless because tests insert at most one validator, but any future multi-validator test could yield flaky results.♻️ Proposed fix: sort consistently (mirrors the existing `GetAllValidators` pattern)
func (m MockStakingKeeper) GetLastValidators(ctx context.Context) (validators []stakingtypes.Validator, err error) { - for _, validator := range m.activeSet { - if validator.IsBonded() { - validators = append(validators, validator) - } - } - return + for _, validator := range m.activeSet { + if validator.IsBonded() { + validators = append(validators, validator) + } + } + slices.SortFunc(validators, func(v1, v2 stakingtypes.Validator) int { + return strings.Compare(v1.OperatorAddress, v2.OperatorAddress) + }) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server_test.go` around lines 260 - 267, GetLastValidators iterates the map m.activeSet which yields non-deterministic ordering; change the implementation to collect bonded validators into a slice and then sort that slice deterministically (same approach used by GetAllValidators) before returning. Specifically, inside MockStakingKeeper.GetLastValidators gather validators where IsBonded() is true into a local slice and then sort the slice by a stable key such as Validator.OperatorAddress (or the same field used by GetAllValidators) using sort.Slice to ensure consistent order across runs.
127-182: No test forPruneAfter × EpochLengthoverflow inretentionWindow.The table has no case with extreme
PruneAfter/EpochLengthvalues that would trigger the overflow described inmsg_server.goline 55. Once that overflow is fixed, a regression test here would prevent silent reversion.🧪 Suggested additional test case
+ "overflow pruning window treated as max retention": { + // PruneAfter * EpochLength > math.MaxInt64 → should not reject a valid current-height attestation + blockHeight: 100, + attestH: 100, + pruneAfter: math.MaxUint64, + epochLength: 2, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server_test.go` around lines 127 - 182, Add a regression test case to TestAttestHeightBounds that exercises the overflow path in the retentionWindow calculation by supplying extreme values for pruneAfter and epochLength whose multiplication would overflow uint64 (e.g., very large pruneAfter or epochLength) and assert the handler returns the expected error (sdkerrors.ErrInvalidRequest); locate the retention logic in the code symbol retentionWindow (or the function in msg_server.go that computes pruneAfter * epochLength) and add a table entry similar to other cases labeled like "pruneAfter*epochLength overflow rejected" that sets blockHeight/attestH appropriately and expects an error to prevent silent reversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/network/keeper/msg_server_test.go`:
- Line 204: The call to keeper.SetParams(ctx, params) is unchecked so a failure
would let the test continue with defaults; update the test to assert the call
succeeds by capturing and checking its error return (e.g. using require.NoError
or t.Fatalf) immediately after invoking SetParams on keeper with ctx and params
so the test fails fast if params could not be applied.
In `@modules/network/keeper/msg_server.go`:
- Around line 55-56: The multiplication params.PruneAfter * params.EpochLength
is done in uint64 and then cast to int64, which can overflow and produce a
negative retentionWindow; update the logic in the msg server where
retentionWindow and minHeight are computed to first detect overflow by checking
if params.PruneAfter > math.MaxInt64/params.EpochLength (or similarly cast both
to int64 and compare with MaxInt64), and if an overflow would occur return an
error (or cap to MaxInt64) rather than allowing a negative retentionWindow; then
compute retentionWindow as a signed int64 (e.g., int64(params.PruneAfter) *
int64(params.EpochLength)) and only compute minHeight := currentHeight -
retentionWindow when retentionWindow is non-negative to avoid flipping the
subtraction. Include references to retentionWindow, minHeight,
params.PruneAfter, params.EpochLength, currentHeight and UpdateParams in your
changes.
---
Nitpick comments:
In `@modules/network/keeper/msg_server_test.go`:
- Around line 260-267: GetLastValidators iterates the map m.activeSet which
yields non-deterministic ordering; change the implementation to collect bonded
validators into a slice and then sort that slice deterministically (same
approach used by GetAllValidators) before returning. Specifically, inside
MockStakingKeeper.GetLastValidators gather validators where IsBonded() is true
into a local slice and then sort the slice by a stable key such as
Validator.OperatorAddress (or the same field used by GetAllValidators) using
sort.Slice to ensure consistent order across runs.
- Around line 127-182: Add a regression test case to TestAttestHeightBounds that
exercises the overflow path in the retentionWindow calculation by supplying
extreme values for pruneAfter and epochLength whose multiplication would
overflow uint64 (e.g., very large pruneAfter or epochLength) and assert the
handler returns the expected error (sdkerrors.ErrInvalidRequest); locate the
retention logic in the code symbol retentionWindow (or the function in
msg_server.go that computes pruneAfter * epochLength) and add a table entry
similar to other cases labeled like "pruneAfter*epochLength overflow rejected"
that sets blockHeight/attestH appropriately and expects an error to prevent
silent reversion.
In `@modules/network/keeper/msg_server.go`:
- Around line 33-52: Handler calls k.GetParams(ctx) twice (once for SignMode
check and again later); hoist a single params := k.GetParams(ctx) at the start
of the handler and replace both k.GetParams(ctx) usages with that local variable
(used for SignMode comparison and later param checks). Update references to
SignMode (params.SignMode) and any later uses of params so you do one store read
instead of two.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces important security validation for attestation height bounds, a great addition to prevent storage exhaustion attacks. However, a medium-severity integer overflow vulnerability was identified in the calculation of the retentionWindow. This could lead to a Denial of Service (DoS) for the Attest function if specific, large values are set for the PruneAfter and EpochLength parameters via governance. The accompanying tests are well-structured and cover the necessary boundary conditions effectively.
modules/network/keeper/msg_server.go
Outdated
| retentionWindow := int64(params.PruneAfter * params.EpochLength) | ||
| minHeight := currentHeight - retentionWindow |
There was a problem hiding this comment.
The calculation for retentionWindow is vulnerable to an integer overflow. Multiplying two uint64 values (params.PruneAfter and params.EpochLength) and casting the result to int64 can lead to an overflow if the product exceeds math.MaxInt64. This will cause the value to wrap around, potentially becoming negative, or making minHeight an extremely large positive number. This ultimately causes height validation to fail for legitimate attestations, leading to a Denial of Service (DoS). It's crucial to validate that the multiplication will not overflow before performing the cast to int64. Additionally, consider the case where the uint64 multiplication itself could overflow if parameters are extremely large, which might require parameter validation.
pruneProduct := uint64(params.PruneAfter) * params.EpochLength if pruneProduct > 9223372036854775807 { // math.MaxInt64
return nil, sdkerr.Wrap(sdkerrors.ErrInvalidRequest, "pruneAfter and epochLength combination would cause an overflow")
}
retentionWindow := int64(pruneProduct)
minHeight := currentHeight - retentionWindow
Cast PruneAfter and EpochLength to int64 before multiplication to avoid uint64 overflow followed by negative int64 wrapping. If the product overflows int64, cap at MaxInt64 to keep the lower bound permissive.
alpe
changed the title
The lower-bound check rejected stale attestations that the integration test legitimately submits (backward-attesting from height 1). Stale attestations are harmless — they just waste gas and get pruned by the existing pruning logic. The real security fix is preventing future-height spam that could exhaust state storage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/network/keeper/msg_server_test.go (1)
228-316: Good boundary coverage; make the test resilient to DefaultParams drift + cover EpochLength>1 rounding.
TestAttestHeightBounds(Line 228-316) hits the important boundaries (current/next accepted; future rejected; below-min rejected; early-chain floor=1). Two tweaks would make it sturdier:
- Don’t rely on comments/assumptions about
types.DefaultParams()values (Line 230-232). Explicitly setparams.EpochLengthandparams.PruneAfterin the test so it won’t break if defaults change.- Add at least one case where
EpochLength > 1andblockHeightis not an epoch boundary to validate the intended rounding behavior.Proposed test hardening (pin params explicitly)
- require.NoError(t, keeper.SetParams(ctx, types.DefaultParams())) + params := types.DefaultParams() + params.EpochLength = 1 + params.PruneAfter = 7 + require.NoError(t, keeper.SetParams(ctx, params))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server_test.go` around lines 228 - 316, TestAttestHeightBounds relies on assumed DefaultParams values; instead create a params variable from types.DefaultParams(), set params.EpochLength and params.PruneAfter explicitly and call keeper.SetParams(ctx, params) so the test won't break if defaults change. Also add a spec case where you set params.EpochLength > 1 and pick a blockHeight that is not an epoch boundary (e.g., EpochLength=3 and blockHeight=10) to assert rounding behavior for minHeight/acceptance, then run the same attestation checks against msgServer.Attest to validate the expected accept/reject outcome. Ensure references to SetParams, EpochLength, PruneAfter, TestAttestHeightBounds, and msgServer.Attest are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 62-76: Replace the manual epoch computation in the minHeight
calculation with GetCurrentEpoch(ctx) to ensure consistency with PruneOldBitmaps
and avoid divergence: call GetCurrentEpoch(ctx) (used elsewhere in this file) to
compute currentEpoch and derive minHeight from params.EpochLength and
params.PruneAfter, then keep the existing msg.Height < minHeight validation;
also extend TestAttestHeightBounds to include a case where params.EpochLength >
1 and blockHeight is not on an epoch boundary to assert the expected
rounding/epoch behavior remains correct.
---
Nitpick comments:
In `@modules/network/keeper/msg_server_test.go`:
- Around line 228-316: TestAttestHeightBounds relies on assumed DefaultParams
values; instead create a params variable from types.DefaultParams(), set
params.EpochLength and params.PruneAfter explicitly and call
keeper.SetParams(ctx, params) so the test won't break if defaults change. Also
add a spec case where you set params.EpochLength > 1 and pick a blockHeight that
is not an epoch boundary (e.g., EpochLength=3 and blockHeight=10) to assert
rounding behavior for minHeight/acceptance, then run the same attestation checks
against msgServer.Attest to validate the expected accept/reject outcome. Ensure
references to SetParams, EpochLength, PruneAfter, TestAttestHeightBounds, and
msgServer.Attest are updated accordingly.
| // Enforce attestation height lower bound: reject heights that fall below | ||
| // the PruneAfter retention window. Attesting pruned/about-to-be-pruned | ||
| // heights wastes storage and serves no purpose. This uses the same epoch | ||
| // calculation as PruneOldBitmaps so the two stay aligned. | ||
| params := k.GetParams(ctx) | ||
| minHeight := int64(1) | ||
| if params.PruneAfter > 0 && params.EpochLength > 0 { | ||
| currentEpoch := uint64(currentHeight) / params.EpochLength | ||
| if currentEpoch > params.PruneAfter { | ||
| minHeight = int64((currentEpoch - params.PruneAfter) * params.EpochLength) | ||
| } | ||
| } | ||
| if msg.Height < minHeight { | ||
| return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "attestation height %d is below retention window (min %d)", msg.Height, minHeight) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify epoch & pruning math matches the new minHeight calculation.
set -euo pipefail
# Find the pruning implementation and epoch helpers.
rg -n --type go "func \\(k .*\\) PruneOldBitmaps" modules/network
rg -n --type go "func \\(k .*\\) GetCurrentEpoch" modules/network
# Show the relevant code blocks for manual comparison.
rg -n --type go -C 5 "PruneOldBitmaps" modules/network/keeper/
Repository: evstack/ev-abci
Length of output: 2761
🏁 Script executed:
#!/bin/bash
# Get GetCurrentEpoch implementation and msg_server.go context around line 142
# Get GetCurrentEpoch function
rg -n --type go -A 10 "func \\(k Keeper\\) GetCurrentEpoch" modules/network/keeper/keeper.go
# Show msg_server.go around line 142
head -150 modules/network/keeper/msg_server.go | tail -30
# Get full PruneOldBitmaps implementation
rg -n --type go -A 15 "func \\(k Keeper\\) PruneOldBitmaps" modules/network/keeper/keeper.go
Repository: evstack/ev-abci
Length of output: 2102
🏁 Script executed:
#!/bin/bash
# Check the function signature and understand what currentHeight parameter is
# Get the full AddAttestation function signature and beginning
head -100 modules/network/keeper/msg_server.go | tail -50
# Also check if there are any tests for this validation logic
find modules/network -name "*test*.go" -type f | head -5
rg -n "TestAddAttestation|EpochLength.*>" modules/network --type go
Repository: evstack/ev-abci
Length of output: 2487
🏁 Script executed:
#!/bin/bash
# Check the msg_server_test.go file for existing tests
# Count lines and check structure
wc -l modules/network/keeper/msg_server_test.go
# Look for TestAddAttestation and test cases
rg -n "func Test|EpochLength|PruneAfter|minHeight" modules/network/keeper/msg_server_test.go -A 2
Repository: evstack/ev-abci
Length of output: 1109
🏁 Script executed:
#!/bin/bash
# Get the full TestAttestHeightBounds test to see all test cases
sed -n '228,310p' modules/network/keeper/msg_server_test.go
Repository: evstack/ev-abci
Length of output: 2573
🏁 Script executed:
#!/bin/bash
# Verify the GetCurrentEpoch usage at line 142 and surrounding context
sed -n '138,148p' modules/network/keeper/msg_server.go
# Also check if there are any edge case tests or tests with EpochLength > 1
rg -n "EpochLength.*2|EpochLength.*3|EpochLength.*[0-9]" modules/network/keeper/msg_server_test.go
Repository: evstack/ev-abci
Length of output: 365
Use GetCurrentEpoch(ctx) for consistency and to reduce divergence risk.
The epoch calculation in minHeight (currently uint64(currentHeight) / params.EpochLength) matches the formula in PruneOldBitmaps, but since GetCurrentEpoch(ctx) already encapsulates this logic and is used elsewhere in the function (line 142), extracting currentHeight into GetCurrentEpoch(ctx) eliminates duplication and reduces the risk of future divergence if the epoch formula changes.
Additionally, test coverage for TestAttestHeightBounds uses only DefaultParams with EpochLength=1, which doesn't verify rounding semantics with larger epoch lengths. Add a test case with EpochLength > 1 and a non-boundary blockHeight to lock in the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/network/keeper/msg_server.go` around lines 62 - 76, Replace the
manual epoch computation in the minHeight calculation with GetCurrentEpoch(ctx)
to ensure consistency with PruneOldBitmaps and avoid divergence: call
GetCurrentEpoch(ctx) (used elsewhere in this file) to compute currentEpoch and
derive minHeight from params.EpochLength and params.PruneAfter, then keep the
existing msg.Height < minHeight validation; also extend TestAttestHeightBounds
to include a case where params.EpochLength > 1 and blockHeight is not on an
epoch boundary to assert the expected rounding/epoch behavior remains correct.
Replace the inline epoch computation (uint64(currentHeight)/params.EpochLength) with k.GetCurrentEpoch(ctx) so the Attest lower-bound and PruneOldBitmaps use the same formula and cannot silently diverge. Extend TestAttestHeightBounds with two sub-cases using EpochLength=10 and a blockHeight (35) that is not on an epoch boundary, asserting that the floor-to-epoch-start rounding is correct: attestH=0 rejected (below minHeight=10), attestH=10 accepted (at minHeight).
…check" This reverts commit abae484.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/network/keeper/msg_server_test.go (2)
379-386: Remove the speculative comment;IsBonded()is well-defined.
stakingtypes.Validator.IsBonded()is not ambiguous — it checksv.Status == BondedStatus. The "Assuming…" hedge creates noise and could mislead future readers into thinking the filter criterion is uncertain.♻️ Proposed cleanup
func (m MockStakingKeeper) GetLastValidators(ctx context.Context) (validators []stakingtypes.Validator, err error) { for _, validator := range m.activeSet { - if validator.IsBonded() { // Assuming IsBonded() identifies if a validator is in the last validators + if validator.IsBonded() { validators = append(validators, validator) } } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server_test.go` around lines 379 - 386, Remove the speculative inline comment in the GetLastValidators method that reads "Assuming IsBonded() identifies if a validator is in the last validators"; IsBonded() is well-defined. Edit the function MockStakingKeeper.GetLastValidators to delete that hedge comment and leave the loop/filter logic using validator.IsBonded() unchanged so the intent remains clear and concise.
271-275: Consider adding anattestH=0sub-case to the early-chain scenario.The "early chain no stale rejection" comment states
minHeight=1, implying genesis (height 0) is below the floor. But the test only probesattestH=1. If the production code's clamp yieldsminHeight=0instead of1, genesis-height attestations would be silently accepted, and this test would give false confidence. Adding an explicitattestH=0sub-case — with the expectation derived from the actual clamp — would close that gap.💡 Suggested additional test case
"early chain no stale rejection": { // blockHeight=5, currentEpoch=5, PruneAfter=7 → currentEpoch <= PruneAfter → minHeight=1 blockHeight: 5, attestH: 1, }, +"early chain genesis height": { + // attestH=0 is below minHeight=1 on early chain; explicitly verify it is rejected + blockHeight: 5, + attestH: 0, + expErr: sdkerrors.ErrInvalidRequest, +},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server_test.go` around lines 271 - 275, Add a sub-case to the "early chain no stale rejection" test case that sets attestH = 0 (alongside the existing blockHeight: 5) and assert the outcome according to the clamp logic used in production (i.e., compare against the computed minHeight/PruneAfter behavior used by the code under test). Locate the test map entry keyed "early chain no stale rejection" in msg_server_test.go and add the new sub-case using the same fields (blockHeight, attestH) and the existing assertion pattern so the test explicitly verifies whether a genesis-height (0) attestation is accepted or rejected per the clamp logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@modules/network/keeper/msg_server_test.go`:
- Around line 379-386: Remove the speculative inline comment in the
GetLastValidators method that reads "Assuming IsBonded() identifies if a
validator is in the last validators"; IsBonded() is well-defined. Edit the
function MockStakingKeeper.GetLastValidators to delete that hedge comment and
leave the loop/filter logic using validator.IsBonded() unchanged so the intent
remains clear and concise.
- Around line 271-275: Add a sub-case to the "early chain no stale rejection"
test case that sets attestH = 0 (alongside the existing blockHeight: 5) and
assert the outcome according to the clamp logic used in production (i.e.,
compare against the computed minHeight/PruneAfter behavior used by the code
under test). Locate the test map entry keyed "early chain no stale rejection" in
msg_server_test.go and add the new sub-case using the same fields (blockHeight,
attestH) and the existing assertion pattern so the test explicitly verifies
whether a genesis-height (0) attestation is accepted or rejected per the clamp
logic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/network/keeper/msg_server.gomodules/network/keeper/msg_server_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/network/keeper/msg_server.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
modules/network/keeper/msg_server_test.go (1)
228-316: Good coverage of core boundary conditions; consider adding anEpochLength > 1case.The test cases correctly exercise all the key scenarios (future, two-ahead, current, next, stale, below/at retention, early chain), and the expected values check out against the implementation math.
SetParamsreturn value is now properly checked — nice.All cases use
DefaultParamswithEpochLength=1, which means the epoch-floor rounding (uint64(currentHeight) / params.EpochLength) is a no-op. Adding a case with e.g.EpochLength=10and a non-alignedblockHeight(like 95) would lock in the integer-division rounding behavior:currentEpoch = 95/10 = 9,minHeight = (9-7)*10 = 20.Example additional test case
+ "epoch rounding with EpochLength > 1": { + // EpochLength=10, PruneAfter=7, blockHeight=95 + // currentEpoch = 95/10 = 9, minHeight = (9-7)*10 = 20 + // attestH=20 should be accepted (at minHeight) + blockHeight: 95, + attestH: 20, + },This would require overriding
DefaultParamsin the test body with customEpochLength. You could add aparamsfield to the test spec (falling back toDefaultParams()when nil) to support this without restructuring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server_test.go` around lines 228 - 316, Add a spec that verifies epoch-floor rounding by testing with EpochLength > 1 (e.g. EpochLength=10 and blockHeight=95) in TestAttestHeightBounds: extend the specs map to include an optional params field (use types.DefaultParams() when nil) and call keeper.SetParams(ctx, params) instead of always DefaultParams(); add a case where currentEpoch = uint64(blockHeight)/params.EpochLength yields a non-aligned epoch (blockHeight=95 → currentEpoch=9) and assert minHeight calculation and accept/reject behavior matches expectations (e.g. attestH values at, below and above the computed minHeight/currentEpoch boundaries). Ensure you reference and use keeper.SetParams, types.DefaultParams, and the TestAttestHeightBounds/specs logic when adding the new case.modules/network/keeper/msg_server.go (1)
62-76: Lower bound logic is correct and overflow-safe; minor duplication withGetCurrentEpoch.The epoch formula here (
uint64(currentHeight) / params.EpochLength) is inherently bounded bycurrentHeight, so theint64()cast on line 71 cannot overflow — good. ThecurrentEpoch > params.PruneAfterguard on line 70 prevents uint64 underflow in the subtraction.Two observations:
Duplicate
GetParamscall:k.GetParams(ctx)is already called implicitly on line 37 (k.GetParams(ctx).SignMode). Consider hoistingparamsto a single call at the top ofAttestto avoid a redundant store read.Epoch formula duplication with
GetCurrentEpoch(ctx): Line 69 duplicates the formula thatGetCurrentEpoch(used on line 142) encapsulates. The commit history shows this was an intentional revert, so flagging for awareness only — if the epoch formula ever changes, these two sites could diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server.go` around lines 62 - 76, The code repeats a params lookup and duplicates epoch-calculation logic; inside the Attest handler hoist a single call to k.GetParams(ctx) into the function scope (reuse the returned params instead of calling k.GetParams(ctx).SignMode elsewhere) and replace the manual epoch computation (uint64(currentHeight) / params.EpochLength) with a call to k.GetCurrentEpoch(ctx) (or a single helper) to compute currentEpoch, then use that value to derive minHeight and keep the existing guards (params.PruneAfter/params.EpochLength) intact so the logic and overflow safety remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@modules/network/keeper/msg_server_test.go`:
- Around line 228-316: Add a spec that verifies epoch-floor rounding by testing
with EpochLength > 1 (e.g. EpochLength=10 and blockHeight=95) in
TestAttestHeightBounds: extend the specs map to include an optional params field
(use types.DefaultParams() when nil) and call keeper.SetParams(ctx, params)
instead of always DefaultParams(); add a case where currentEpoch =
uint64(blockHeight)/params.EpochLength yields a non-aligned epoch
(blockHeight=95 → currentEpoch=9) and assert minHeight calculation and
accept/reject behavior matches expectations (e.g. attestH values at, below and
above the computed minHeight/currentEpoch boundaries). Ensure you reference and
use keeper.SetParams, types.DefaultParams, and the TestAttestHeightBounds/specs
logic when adding the new case.
In `@modules/network/keeper/msg_server.go`:
- Around line 62-76: The code repeats a params lookup and duplicates
epoch-calculation logic; inside the Attest handler hoist a single call to
k.GetParams(ctx) into the function scope (reuse the returned params instead of
calling k.GetParams(ctx).SignMode elsewhere) and replace the manual epoch
computation (uint64(currentHeight) / params.EpochLength) with a call to
k.GetCurrentEpoch(ctx) (or a single helper) to compute currentEpoch, then use
that value to derive minHeight and keep the existing guards
(params.PruneAfter/params.EpochLength) intact so the logic and overflow safety
remain unchanged.
| DefaultQuorumFraction = math.LegacyNewDecWithPrec(667, 3) // 2/3 | ||
| DefaultMinParticipation = math.LegacyNewDecWithPrec(5, 1) // 1/2 | ||
| DefaultPruneAfter = uint64(7) | ||
| DefaultPruneAfter = uint64(15) // also used as number of blocks for attestations to land |
There was a problem hiding this comment.
I had to increase the window so that the integration tests gets their messages through
1 participant
Summary
The
Attesthandler previously accepted attestations for any height — including arbitrarily far-future blocks (MaxInt64) and ancient blocks. This enabled permissionless storage exhaustion.Summary by CodeRabbit
Bug Fixes
Tests
Chores