modules/network/keeper/msg_server.go (1)

33-52: GetParams is called twice within the same handler.

k.GetParams(ctx) is invoked at line 33 (for SignMode) and again at line 52. Hoisting a single params variable 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 in GetLastValidators.

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 for PruneAfter ×ばつ EpochLength overflow in retentionWindow.

The table has no case with extreme PruneAfter/EpochLength values that would trigger the overflow described in msg_server.go line 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.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

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 set params.EpochLength and params.PruneAfter in the test so it won’t break if defaults change.
• Add at least one case where EpochLength > 1 and blockHeight is 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.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

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 checks v.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 an attestH=0 sub-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 probes attestH=1. If the production code's clamp yields minHeight=0 instead of 1, genesis-height attestations would be silently accepted, and this test would give false confidence. Adding an explicit attestH=0 sub-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.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

modules/network/keeper/msg_server_test.go (1)

228-316: Good coverage of core boundary conditions; consider adding an EpochLength > 1 case.

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. SetParams return value is now properly checked — nice.

All cases use DefaultParams with EpochLength=1, which means the epoch-floor rounding (uint64(currentHeight) / params.EpochLength) is a no-op. Adding a case with e.g. EpochLength=10 and a non-aligned blockHeight (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 DefaultParams in the test body with custom EpochLength. You could add a params field to the test spec (falling back to DefaultParams() 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 with GetCurrentEpoch.

The epoch formula here (uint64(currentHeight) / params.EpochLength) is inherently bounded by currentHeight, so the int64() cast on line 71 cannot overflow — good. The currentEpoch > params.PruneAfter guard on line 70 prevents uint64 underflow in the subtraction.

Two observations:

1. Duplicate GetParams call: k.GetParams(ctx) is already called implicitly on line 37 (k.GetParams(ctx).SignMode). Consider hoisting params to a single call at the top of Attest to avoid a redundant store read.

2. Epoch formula duplication with GetCurrentEpoch(ctx): Line 69 duplicates the formula that GetCurrentEpoch (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.

Configuration used: defaults

Review profile: CHILL

Plan: Pro