feat(network): load fixed attester set from genesis (Part 1)#394
Open
randygrok wants to merge 10 commits into
Open
feat(network): load fixed attester set from genesis (Part 1)#394randygrok wants to merge 10 commits into
randygrok wants to merge 10 commits into
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughGenesis now stores attester metadata, runtime validator-index rebuilding is removed, attestation and commit reconstruction use fixed attester sets, and startup wires block ID access into the network keeper. Tests and integration flows were updated for the new attester registration and verification paths. ChangesNetwork attester runtime and verification
Sequence Diagram(s)sequenceDiagram
participant Start as server/start.go
participant Keeper as modules/network/keeper
participant MsgServer as msgServer.Attest
participant Query as AttesterSet / AttesterSignatures
Start->>Keeper: SetBlockIDProvider(executor.Store)
MsgServer->>Keeper: store bitmap and signature
Query->>Keeper: fetch ordered attester data
Keeper-->>Query: sorted entries and signatures
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes 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 |
randygrok
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
modules/network/keeper/bitmap_test.go (1)
229-242: Consider pinning negative-bound behavior inCountInRangetests.Optional: add negative
start/endcases so current bound-handling semantics stay explicit over time.Suggested test-table additions
specs := map[string]struct { start, end int expCount int }{ "full range": {start: 0, end: 16, expCount: 5}, + "negative start includes valid lower bits": {start: -5, end: 2, expCount: 1}, + "negative end is empty range": {start: 0, end: -1, expCount: 0}, "first byte only": {start: 0, end: 8, expCount: 3}, "second byte only": {start: 8, end: 16, expCount: 2},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/bitmap_test.go` around lines 229 - 242, The test table for CountInRange in bitmap_test.go omits negative start/end cases which can allow boundary-handling semantics to drift; add entries to the specs map that use negative start and/or end (e.g., start: -5 end: 2 and start: -10 end: -1) with explicit expCount values that match the current CountInRange behavior so the intended semantics are pinned; update the test cases in the specs map near the existing entries and ensure they reference the same CountInRange call used by the test harness so future changes to bounds handling will fail the tests if behavior changes.modules/network/keeper/genesis_test.go (1)
100-103: Use a valid-but-different consensus address to harden mismatch-path coverage.Line 102 uses an arbitrary byte slice cast to
ConsAddress. Using a second real pubkey-derived consensus address would make this test more explicitly target pubkey/address mismatch instead of relying on address-shape behavior.Proposed test hardening
pk := cmted25519.GenPrivKey().PubKey().(cmted25519.PubKey) info := mustAnyPubKey(t, pk) -info.ConsensusAddress = sdk.ConsAddress([]byte("not-matching-20-bytes")).String() +otherPk := cmted25519.GenPrivKey().PubKey().(cmted25519.PubKey) +info.ConsensusAddress = sdk.ConsAddress(otherPk.Address()).String()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/genesis_test.go` around lines 100 - 103, The test currently assigns a bogus byte slice to info.ConsensusAddress; instead derive a real-but-different consensus address from a second ed25519 pubkey so the test exercises a true pubkey/address mismatch. Generate a second key (e.g., via cmted25519.GenPrivKey().PubKey()), derive its consensus address (sdk.ConsAddress(secondPubKey.Address()).String()), and assign that to info.ConsensusAddress while keeping the original pk and mustAnyPubKey usage, ensuring the new consensus address is different from pk's address to harden the mismatch path.
🤖 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/genesis.go`:
- Around line 58-66: The loop over attesters casts idx to uint16 when calling
SetValidatorIndex, which can overflow if len(attesters) > 65535; before calling
SetValidatorIndex (in the for idx, info := range attesters loop) add a guard
that checks if idx exceeds math.MaxUint16 (or 65535) and return a clear error
(e.g., "too many attesters: exceeds uint16 limit") to prevent index wrapping;
ensure this validation happens prior to SetValidatorIndex invocation so
SetValidatorIndex is only called with a safe uint16 cast.
- Around line 25-49: The loop over the attesters slice must detect and reject
duplicate consensus addresses after you normalize the bech32 prefix: after
computing derived := sdk.ConsAddress(pk.Address()).String() (in the attesters
processing loop), keep a map (e.g. map[string]int) keyed by the normalized
derived string (or the raw 20-byte address pk.Address()) and if the derived
address is already present return an error indicating the duplicate attester
(include both indices and the conflicting ConsensusAddress) instead of allowing
the later SetAttesterInfo / SetValidatorIndex writes to silently overwrite the
earlier entry; ensure you reference the same symbols used in the loop
(attesters, info.ConsensusAddress, pk.Address(), derived) when implementing the
check and error.
In `@modules/proto/evabci/network/v1/attester.proto`:
- Around line 25-28: The schema comment for the field consensus_address
incorrectly hard-codes the bech32 prefix; update the comment on the
consensus_address string field so it does not specify "cosmosvalcons1..." and
instead states it is a bech32 consensus address (derived from pubkey) that may
use any valid chain-specific consensus-address prefix; mention that InitGenesis
will accept and normalize any valid bech32 consensus-address prefix so the
stored consensus_address matches the keeper's collection key.
---
Nitpick comments:
In `@modules/network/keeper/bitmap_test.go`:
- Around line 229-242: The test table for CountInRange in bitmap_test.go omits
negative start/end cases which can allow boundary-handling semantics to drift;
add entries to the specs map that use negative start and/or end (e.g., start: -5
end: 2 and start: -10 end: -1) with explicit expCount values that match the
current CountInRange behavior so the intended semantics are pinned; update the
test cases in the specs map near the existing entries and ensure they reference
the same CountInRange call used by the test harness so future changes to bounds
handling will fail the tests if behavior changes.
In `@modules/network/keeper/genesis_test.go`:
- Around line 100-103: The test currently assigns a bogus byte slice to
info.ConsensusAddress; instead derive a real-but-different consensus address
from a second ed25519 pubkey so the test exercises a true pubkey/address
mismatch. Generate a second key (e.g., via cmted25519.GenPrivKey().PubKey()),
derive its consensus address (sdk.ConsAddress(secondPubKey.Address()).String()),
and assign that to info.ConsensusAddress while keeping the original pk and
mustAnyPubKey usage, ensuring the new consensus address is different from pk's
address to harden the mismatch path.
🪄 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: ea747e09-6937-403d-bcbc-30eef5feab0b
⛔ Files ignored due to path filters (2)
modules/network/types/attester.pb.gois excluded by!**/*.pb.gomodules/network/types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
modules/network/genesis.gomodules/network/keeper/abci.gomodules/network/keeper/bitmap_test.gomodules/network/keeper/genesis_test.gomodules/network/keeper/keeper.gomodules/network/keeper/msg_server.gomodules/network/keeper/msg_server_test.gomodules/network/types/attester.gomodules/network/types/genesis.gomodules/proto/evabci/network/v1/attester.protomodules/proto/evabci/network/v1/genesis.proto
💤 Files with no reviewable changes (1)
- modules/network/keeper/keeper.go
Comment on lines
+25
to
+49
| for i := range attesters { | ||
| info := attesters[i] | ||
| pk, err := info.GetPubKey() | ||
| if err != nil { | ||
| return fmt.Errorf("attester %d: %w", i, err) | ||
| } | ||
| // Also add to attester set | ||
| if err := k.SetAttesterSetMember(ctx, vi.Address); err != nil { | ||
| return err | ||
| // Validate pubkey ↔ consensus_address match at the raw-bytes level so | ||
| // the check is independent of bech32 prefix (e.g. "cosmosvalcons" vs | ||
| // "celestiavalcons"). Whatever prefix was used in genesis, the 20-byte | ||
| // payload must equal the pubkey's Address(). | ||
| _, rawAddr, decErr := bech32.DecodeAndConvert(info.ConsensusAddress) | ||
| if decErr != nil { | ||
| return fmt.Errorf("attester %d: decode consensus_address %q: %w", i, info.ConsensusAddress, decErr) | ||
| } | ||
| if !bytes.Equal(rawAddr, pk.Address()) { | ||
| return fmt.Errorf("attester %d: pubkey address mismatch (derived bytes %x, stated bytes %x)", | ||
| i, pk.Address(), rawAddr) | ||
| } | ||
| // Re-encode consensus_address with the runtime SDK config so the | ||
| // stored value matches what ConsAddress().String() produces elsewhere | ||
| // in the module at runtime. | ||
| derived := sdk.ConsAddress(pk.Address()).String() | ||
| info.ConsensusAddress = derived | ||
| attesters[i] = info | ||
| } |
Contributor
There was a problem hiding this comment.
Reject duplicate attesters after consensus-address normalization.
After Line 46 rewrites the address to the runtime bech32 prefix, two genesis entries that decode to the same 20-byte consensus address will collapse onto the same keeper key. The later SetAttesterInfo / SetValidatorIndex writes then overwrite the earlier one instead of failing, which silently corrupts the fixed attester set.
🛡️ Suggested guard
attesters := make([]types.AttesterInfo, len(genState.AttesterInfos))
copy(attesters, genState.AttesterInfos)
+ seenConsensus := make(map[string]struct{}, len(attesters))
for i := range attesters {
info := attesters[i]
pk, err := info.GetPubKey()
if err != nil {
@@
// Re-encode consensus_address with the runtime SDK config so the
// stored value matches what ConsAddress().String() produces elsewhere
// in the module at runtime.
derived := sdk.ConsAddress(pk.Address()).String()
+ if _, exists := seenConsensus[derived]; exists {
+ return fmt.Errorf("attester %d: duplicate consensus_address %q", i, derived)
+ }
+ seenConsensus[derived] = struct{}{}
info.ConsensusAddress = derived
attesters[i] = info
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/network/genesis.go` around lines 25 - 49, The loop over the attesters
slice must detect and reject duplicate consensus addresses after you normalize
the bech32 prefix: after computing derived :=
sdk.ConsAddress(pk.Address()).String() (in the attesters processing loop), keep
a map (e.g. map[string]int) keyed by the normalized derived string (or the raw
20-byte address pk.Address()) and if the derived address is already present
return an error indicating the duplicate attester (include both indices and the
conflicting ConsensusAddress) instead of allowing the later SetAttesterInfo /
SetValidatorIndex writes to silently overwrite the earlier entry; ensure you
reference the same symbols used in the loop (attesters, info.ConsensusAddress,
pk.Address(), derived) when implementing the check and error.
Comment on lines
+58
to
+66
| for idx, info := range attesters { | ||
| if err := k.SetAttesterInfo(ctx, info.ConsensusAddress, &info); err != nil { | ||
| return fmt.Errorf("set attester info: %w", err) | ||
| } | ||
| if err := k.SetAttesterSetMember(ctx, info.ConsensusAddress); err != nil { | ||
| return fmt.Errorf("set attester set member: %w", err) | ||
| } | ||
| if err := k.SetValidatorIndex(ctx, info.ConsensusAddress, uint16(idx), 1); err != nil { | ||
| return fmt.Errorf("set validator index: %w", err) |
Contributor
There was a problem hiding this comment.
Guard the uint16 validator-index cast.
Line 65 narrows idx to uint16 without checking the attester count first. A malformed genesis with too many attesters will wrap indices and make different attesters share the same bitmap position.
🚧 Suggested bound check
import (
"bytes"
"fmt"
+ "math"
"sort"
@@
+ if len(attesters) > math.MaxUint16 {
+ return fmt.Errorf("too many attesters: %d", len(attesters))
+ }
+
for idx, info := range attesters {
if err := k.SetAttesterInfo(ctx, info.ConsensusAddress, &info); err != nil {
return fmt.Errorf("set attester info: %w", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/network/genesis.go` around lines 58 - 66, The loop over attesters
casts idx to uint16 when calling SetValidatorIndex, which can overflow if
len(attesters) > 65535; before calling SetValidatorIndex (in the for idx, info
:= range attesters loop) add a guard that checks if idx exceeds math.MaxUint16
(or 65535) and return a clear error (e.g., "too many attesters: exceeds uint16
limit") to prevent index wrapping; ensure this validation happens prior to
SetValidatorIndex invocation so SetValidatorIndex is only called with a safe
uint16 cast.
Comment on lines
+25
to
+28
| // consensus_address is the bech32 cosmosvalcons1... derived from pubkey. | ||
| // Redundant with pubkey but persisted so the keeper's collections key | ||
| // (consensus address) matches the stored struct. | ||
| string consensus_address = 4 [(cosmos_proto.scalar) = "cosmos.AddressString"]; |
Contributor
There was a problem hiding this comment.
Don’t hard-code the valcons prefix in the schema comment.
InitGenesis accepts any valid bech32 consensus-address prefix and normalizes it at import time, so documenting this field as always cosmosvalcons1... is misleading for chains using a different runtime prefix.
✏️ Suggested wording
- // consensus_address is the bech32 cosmosvalcons1... derived from pubkey.
+ // consensus_address is the bech32 consensus-address string derived from pubkey.
+ // The exact prefix is chain-config dependent and is normalized during genesis import.📝 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
| // consensus_address is the bech32 cosmosvalcons1... derived from pubkey. | |
| // Redundant with pubkey but persisted so the keeper's collections key | |
| // (consensus address) matches the stored struct. | |
| string consensus_address = 4 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | |
| // consensus_address is the bech32 consensus-address string derived from pubkey. | |
| // The exact prefix is chain-config dependent and is normalized during genesis import. | |
| // Redundant with pubkey but persisted so the keeper's collections key | |
| // (consensus address) matches the stored struct. | |
| string consensus_address = 4 [(cosmos_proto.scalar) = "cosmos.AddressString"]; |
🧰 Tools
🪛 Buf (1.68.4)
[error] 28-28: cannot find cosmos_proto.scalar in this scope
(COMPILE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/proto/evabci/network/v1/attester.proto` around lines 25 - 28, The
schema comment for the field consensus_address incorrectly hard-codes the bech32
prefix; update the comment on the consensus_address string field so it does not
specify "cosmosvalcons1..." and instead states it is a bech32 consensus address
(derived from pubkey) that may use any valid chain-specific consensus-address
prefix; mention that InitGenesis will accept and normalize any valid bech32
consensus-address prefix so the stored consensus_address matches the keeper's
collection key.
tac0turtle
approved these changes
Apr 29, 2026
julienrbrt
reviewed
May 12, 2026
| if err := k.BuildValidatorIndexMap(ctx); err != nil { | ||
| return fmt.Errorf("rebuilding validator index map at epoch %d: %w", epoch, err) | ||
| } | ||
| // Validator indices are established at genesis and never mutate at runtime |
| @@ -161,86 +159,14 @@ func (k msgServer) Attest(goCtx context.Context, msg *types.MsgAttest) (*types.M | |||
|
|
|||
| // JoinAttesterSet handles MsgJoinAttesterSet | |||
| func (k msgServer) JoinAttesterSet(goCtx context.Context, msg *types.MsgJoinAttesterSet) (*types.MsgJoinAttesterSetResponse, error) { | |||
| } | ||
|
|
||
| // LeaveAttesterSet handles MsgLeaveAttesterSet | ||
| func (k msgServer) LeaveAttesterSet(goCtx context.Context, msg *types.MsgLeaveAttesterSet) (*types.MsgLeaveAttesterSetResponse, error) { |
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 (2)
modules/network/keeper/msg_server.go (1)
41-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate attestation height before calling
verifyVote.Line 42 reaches
provider.GetBlockID(ctx, uint64(msgHeight))before the height bounds at Line 52. A negative or out-of-window height can be converted to a hugeuint64and hit the block store before being rejected.🐛 Proposed fix
- // Verify the vote: decode, internal checks, signature check. - if _, err := k.verifyVote(ctx, msg.ConsensusAddress, msg.Vote, msg.Height); err != nil { - return nil, err - } - - index, found := k.GetValidatorIndex(ctx, msg.ConsensusAddress) - if !found { - return nil, sdkerr.Wrapf(sdkerrors.ErrNotFound, "validator index not found for %s", msg.ConsensusAddress) - } - // Height bounds currentHeight := ctx.BlockHeight() maxFutureHeight := currentHeight + 1 if msg.Height > maxFutureHeight { @@ if msg.Height < minHeight { return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "attestation height %d is below retention window (min %d)", msg.Height, minHeight) } + + // Verify the vote: decode, internal checks, signature check. + if _, err := k.verifyVote(ctx, msg.ConsensusAddress, msg.Vote, msg.Height); err != nil { + return nil, err + } + + index, found := k.GetValidatorIndex(ctx, msg.ConsensusAddress) + if !found { + return nil, sdkerr.Wrapf(sdkerrors.ErrNotFound, "validator index not found for %s", msg.ConsensusAddress) + }Also applies to: 223-223
🤖 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 41 - 52, The attestation height validation in the msg server happens too late, allowing verifyVote to call GetBlockID with an unsafe uint64-converted height first. Move the height bounds check in the message handler before calling verifyVote, using the existing currentHeight logic and msg.Height to reject negative or out-of-window values early. Apply the same ordering fix anywhere the same verifyVote flow is used so provider.GetBlockID is never reached with an invalid height.modules/network/keeper/msg_server_test.go (1)
341-380: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix contradictory
minHeightcomments; boundary case doesn't test the real boundary.The header comment computes
minHeight=(100-7)*1=93, but the"early chain no stale rejection"case (blockHeight=16, attestH=1 accepted) only passes whenPruneAfter=15, which makes the realminHeightat height 100 equal to(100-15)*1=85. The"below retention window rejected"case correctly notes// minHeight = 85, but the header (Line 343) and"at retention boundary accepted"(Line 379,attestH=93 // exactly minHeight) are wrong. As a result the accept-side boundary (85) is never exercised.📝 Proposed comment/value fix
- // With DefaultParams: EpochLength=1, PruneAfter=15 - // At blockHeight=100: currentEpoch=100, minHeight=(100-7)*1=93 + // With DefaultParams: EpochLength=1, PruneAfter=15 + // At blockHeight=100: currentEpoch=100, minHeight=(100-15)*1=85"at retention boundary accepted": { blockHeight: 100, - attestH: 93, // exactly minHeight + attestH: 85, // exactly minHeight },🤖 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_test.go` around lines 341 - 380, The height-bound test comments in TestAttestHeightBounds are inconsistent with the actual PruneAfter-based minimum height, and the boundary case is not testing the true cutoff. Update the header note and the "at retention boundary accepted" case to use the real minHeight derived from DefaultParams/PruneAfter=15 (so the boundary is 85 at blockHeight=100), and add or adjust a case so the accepted boundary is exercised at that exact height. Keep the other cases in TestAttestHeightBounds aligned with the same minHeight calculation.
🤖 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/keeper.go`:
- Around line 257-263: The quorum check in Keeper.CheckQuorum validates
params.QuorumFraction but then ignores it by hard-coding the 2/3 threshold.
Update CheckQuorum to parse and use the configured quorum fraction from
params.QuorumFraction when comparing votedPower against totalPower, and keep the
existing validation/error handling in place so the comparison reflects
governance/config changes.
In `@server/start.go`:
- Around line 450-454: The startup wiring in the `networkKeeperBlockIDWirer`
check is too lenient when `FlagAttesterMode` is enabled: if `app` cannot be
wired with `SetNetworkKeeperBlockIDProvider`, `MsgAttest` will later reject
votes because the provider stays nil. Update the logic in `start.go` so that
this path returns an error when attester mode is on, and keep the existing
`sdkLogger.Warn` fallback only for non-attester mode. Use the existing `app`
type assertion and `executor.Store` wiring point to locate the change.
---
Outside diff comments:
In `@modules/network/keeper/msg_server_test.go`:
- Around line 341-380: The height-bound test comments in TestAttestHeightBounds
are inconsistent with the actual PruneAfter-based minimum height, and the
boundary case is not testing the true cutoff. Update the header note and the "at
retention boundary accepted" case to use the real minHeight derived from
DefaultParams/PruneAfter=15 (so the boundary is 85 at blockHeight=100), and add
or adjust a case so the accepted boundary is exercised at that exact height.
Keep the other cases in TestAttestHeightBounds aligned with the same minHeight
calculation.
In `@modules/network/keeper/msg_server.go`:
- Around line 41-52: The attestation height validation in the msg server happens
too late, allowing verifyVote to call GetBlockID with an unsafe uint64-converted
height first. Move the height bounds check in the message handler before calling
verifyVote, using the existing currentHeight logic and msg.Height to reject
negative or out-of-window values early. Apply the same ordering fix anywhere the
same verifyVote flow is used so provider.GetBlockID is never reached with an
invalid height.
🪄 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: a56c40a6-0890-4b93-bb48-72d6c7ec3617
📒 Files selected for processing (7)
modules/network/keeper/attester_ibc_test.gomodules/network/keeper/keeper.gomodules/network/keeper/msg_server.gomodules/network/keeper/msg_server_test.gomodules/network/keeper/testhelpers_test.gomodules/network/types/expected_keepers.goserver/start.go
✅ Files skipped from review due to trivial changes (1)
- modules/network/keeper/testhelpers_test.go
Comment on lines
257
to
+263
| func (k Keeper) CheckQuorum(ctx sdk.Context, votedPower, totalPower uint64) (bool, error) { | ||
| params := k.GetParams(ctx) | ||
| quorumFrac, err := math.LegacyNewDecFromStr(params.QuorumFraction) | ||
| if err != nil { | ||
| if _, err := math.LegacyNewDecFromStr(params.QuorumFraction); err != nil { | ||
| return false, fmt.Errorf("invalid quorum fraction: %w", err) | ||
| } | ||
|
|
||
| requiredPower := math.LegacyNewDec(int64(totalPower)).Mul(quorumFrac).TruncateInt().Uint64() | ||
| return votedPower >= requiredPower, nil | ||
| return votedPower*3 > totalPower*2, nil |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the parsed quorum fraction instead of hard-coding 2/3.
Line 259 validates params.QuorumFraction, but Line 263 ignores it. Any governance/config change to the quorum fraction will be accepted yet still evaluated as > 2/3.
🐛 Suggested direction
- if _, err := math.LegacyNewDecFromStr(params.QuorumFraction); err != nil {
+ quorumFraction, err := math.LegacyNewDecFromStr(params.QuorumFraction)
+ if err != nil {
return false, fmt.Errorf("invalid quorum fraction: %w", err)
}
- return votedPower*3 > totalPower*2, nil
+ // Compare votedPower / totalPower > quorumFraction.
+ // If quorum is intended to be fixed at >2/3, reject any other param value
+ // during params validation instead of silently ignoring it here.🤖 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/keeper.go` around lines 257 - 263, The quorum check in
Keeper.CheckQuorum validates params.QuorumFraction but then ignores it by
hard-coding the 2/3 threshold. Update CheckQuorum to parse and use the
configured quorum fraction from params.QuorumFraction when comparing votedPower
against totalPower, and keep the existing validation/error handling in place so
the comparison reflects governance/config changes.
* feat(network): verify attester votes * feat(rpc): reconstruct attester commits deterministically * feat(attester): attest from fixed genesis set
* feat(network): verify attester votes * feat(rpc): reconstruct attester commits deterministically * feat(attester): attest from fixed genesis set * test(integration): cover attested IBC commits
* feat(network): verify attester votes * feat(rpc): reconstruct attester commits deterministically * feat(attester): attest from fixed genesis set * test(integration): cover attested IBC commits * fix: preserve attester query errors and vote address * test: add multi-attester docker e2e
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
pkg/rpc/core/blocks.go (1)
470-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the test-only env swap out of the production API.
GetCommitForHeightForTestis in a non-test file and mutates the package-globalenv, so it becomes a public runtime entry point even though the comment says it is test-only. Moving this wrapper to anexport_test.gohook would keep it available tocore_testwithout widening the production surface with a stateful helper.🤖 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 `@pkg/rpc/core/blocks.go` around lines 470 - 476, Move the test-only env swap helper out of the production API by removing GetCommitForHeightForTest from the non-test source and exposing it only through an export_test.go hook for core_test. Keep the underlying getCommitForHeight path unchanged, but ensure the wrapper that temporarily swaps the package-global env is only compiled for tests and not available as a runtime entry point.
🤖 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 @.github/workflows/migration_test.yml:
- Around line 59-61: The workflow’s checkout steps are leaving persisted git
credentials enabled, which is unnecessary for these jobs and increases leak
risk. Update each of the four actions/checkout usages in this workflow to
disable credential persistence by setting the appropriate checkout option in the
existing with block, keeping fetch-depth as-is. Use the actions/checkout steps
in the migration test jobs as the target for the change and apply it
consistently to all four checkouts.
In `@pkg/rpc/core/consensus_test.go`:
- Around line 291-318: The test is mutating the package-global env and leaving
attester mode plus the mocked ABCI app in place for later tests. In the
consensus_test.go setup around env = &Environment{...}, save the previous env
before overwriting it and restore it with t.Cleanup so the global state is
always reset after this test. Use the existing env variable and Environment
initialization in this test, following the cleanup pattern already used in
pkg/rpc/core/blocks_test.go.
In `@server/attester_cmd.go`:
- Around line 508-529: The block-height fetch in attester block querying is
swallowing JSON-RPC error responses by returning height 0 with no error. Update
the local response struct in the block lookup path to include an Error field
alongside Result, and in the JSON decode flow after httpClient.Get and
json.NewDecoder(resp.Body).Decode, detect a populated RPC error and return it
instead of treating missing block.header.height as success. Keep the check in
the same block-query logic so parsed.Host and the existing height parsing
behavior remain intact.
- Around line 158-160: `buildAttesterVoteBytes` is hardcoding the attester index
to zero, so signed vote bytes are wrong for non-zero attesters. Update
`assertRegistered` to return `(int32, error)` and return the matched `e.Index`,
then change `pullBlocksAndAttest` to capture that index and pass it into
`buildAttesterVoteBytes`. Also update `buildAttesterVoteBytes` to accept
`validatorIndex int32` and use it when constructing the `Vote` so the signed
bytes reflect the correct attester identity.
In `@tests/integration/gm_gaia_health_test.go`:
- Around line 62-65: Set the GM consensus Bech32 prefix before calling
generateAttesterIdentities in gm_gaia_health_test.go, since that helper
serializes sdk.ConsAddress values before any consensus prefix is configured.
Update the test setup around getGmChain and generateAttesterIdentities to
initialize the full Bech32 config (not just the account prefix) so genesis
attester_infos use celestiavalcons instead of the default cosmosvalcons.
---
Nitpick comments:
In `@pkg/rpc/core/blocks.go`:
- Around line 470-476: Move the test-only env swap helper out of the production
API by removing GetCommitForHeightForTest from the non-test source and exposing
it only through an export_test.go hook for core_test. Keep the underlying
getCommitForHeight path unchanged, but ensure the wrapper that temporarily swaps
the package-global env is only compiled for tests and not available as a runtime
entry point.
🪄 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: 95e3e525-cb64-4619-a4a5-85abcd71b792
⛔ Files ignored due to path filters (2)
modules/network/types/query.pb.gois excluded by!**/*.pb.gomodules/network/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (15)
.github/workflows/migration_test.ymlmodules/network/keeper/grpc_query.gomodules/proto/evabci/network/v1/query.protopkg/adapter/providers_test.gopkg/rpc/core/blocks.gopkg/rpc/core/blocks_test.gopkg/rpc/core/commit_reconstruction_test.gopkg/rpc/core/consensus.gopkg/rpc/core/consensus_test.goserver/attester_cmd.goserver/attester_cmd_test.gotests/integration/docker/Dockerfile.gmtests/integration/docker/patches/app-wiring/patch-app-wiring.shtests/integration/gm_gaia_health_multi_attester_test.gotests/integration/gm_gaia_health_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/adapter/providers_test.go
Comment on lines
59
to
+61
| - uses: actions/checkout@v5 | ||
| with: | ||
| fetch-depth: 0 | ||
| fetch-depth: 1 |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read-only verification: check whether any Dockerfile or build config copies the repo
# workspace in a way that could include .git / checkout credentials.
fd -a 'Dockerfile*' .
fd -a '.dockerignore' .
rg -n --hidden \
-g 'Dockerfile*' \
-g '.dockerignore' \
-e 'COPY\s+\.\s' \
-e 'ADD\s+\.\s' \
-e '\.git' \
-e 'persist-credentials' \
-e 'actions/checkout' \
.Repository: evstack/ev-abci
Length of output: 385
Disable persisted checkout credentials in these jobs.
actions/checkout keeps the token in git config by default. These jobs do not need repo credentials after checkout, and leaving them enabled raises avoidable leak risk if the workspace is copied into a Docker context or cache, as this workspace is included in the Docker images built by ./Dockerfile and ./tests/integration/docker/Dockerfile.gm.
🔒 Suggested fix
- uses: actions/checkout@v5
with:
+ persist-credentials: false
fetch-depth: 1Apply the same change to all four checkout steps.
📝 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
| - uses: actions/checkout@v5 | |
| with: | |
| fetch-depth: 0 | |
| fetch-depth: 1 | |
| - uses: actions/checkout@v5 | |
| with: | |
| persist-credentials: false | |
| fetch-depth: 1 |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 59-61: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 59-59: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/migration_test.yml around lines 59 - 61, The workflow’s
checkout steps are leaving persisted git credentials enabled, which is
unnecessary for these jobs and increases leak risk. Update each of the four
actions/checkout usages in this workflow to disable credential persistence by
setting the appropriate checkout option in the existing with block, keeping
fetch-depth as-is. Use the actions/checkout steps in the migration test jobs as
the target for the change and apply it consistently to all four checkouts.
Source: Linters/SAST tools
Comment on lines
+291
to
+318
| mApp := new(MockApp) | ||
| mApp.On("Query", testifymock.Anything, testifymock.MatchedBy(func(r *abci.RequestQuery) bool { | ||
| return r.Path == "/evabci.network.v1.Query/AttesterSet" | ||
| })).Return(&abci.ResponseQuery{Code: 0, Value: setRespBz}, nil) | ||
| mApp.On("Query", testifymock.Anything, testifymock.MatchedBy(func(r *abci.RequestQuery) bool { | ||
| return r.Path == "/evabci.network.v1.Query/AttesterSignatures" | ||
| })).Return(&abci.ResponseQuery{Code: 0, Value: sigRespBz}, nil).Maybe() | ||
| for consAddr, info := range attesterInfoByAddr { | ||
| infoRespBz, err := proto.Marshal(&networktypes.QueryAttesterInfoResponse{AttesterInfo: info}) | ||
| require.NoError(t, err) | ||
| consAddr := consAddr | ||
| mApp.On("Query", testifymock.Anything, testifymock.MatchedBy(func(r *abci.RequestQuery) bool { | ||
| if r.Path != "/evabci.network.v1.Query/AttesterInfo" { | ||
| return false | ||
| } | ||
| var req networktypes.QueryAttesterInfoRequest | ||
| if err := proto.Unmarshal(r.Data, &req); err != nil { | ||
| return false | ||
| } | ||
| return req.ValidatorAddress == consAddr | ||
| })).Return(&abci.ResponseQuery{Code: 0, Value: infoRespBz}, nil).Maybe() | ||
| } | ||
|
|
||
| env = &Environment{ | ||
| Adapter: &adapter.Adapter{App: mApp}, | ||
| AttesterMode: true, | ||
| Logger: cmtlog.NewNopLogger(), | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore env after this test.
This test overwrites the package-global env and never puts it back, so later pkg/rpc/core tests can inherit attester mode and the mocked ABCI app. Please save the previous value and restore it with t.Cleanup, like the new tests in pkg/rpc/core/blocks_test.go already do.
🤖 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 `@pkg/rpc/core/consensus_test.go` around lines 291 - 318, The test is mutating
the package-global env and leaving attester mode plus the mocked ABCI app in
place for later tests. In the consensus_test.go setup around env =
&Environment{...}, save the previous env before overwriting it and restore it
with t.Cleanup so the global state is always reset after this test. Use the
existing env variable and Environment initialization in this test, following the
cleanup pattern already used in pkg/rpc/core/blocks_test.go.
Comment on lines
+158
to
160
| for _, e := range resp.Entries { | ||
| if e.ConsensusAddress == consAddr { | ||
| return nil |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'ValidatorIndex:\s*0|buildAttesterVoteBytes\(|VerifyCommitLight|AttesterSet' server tests/integration pkg/rpc modules/networkRepository: evstack/ev-abci
Length of output: 50372
🏁 Script executed:
cat -n server/attester_cmd.go | sed -n '150,180p'Repository: evstack/ev-abci
Length of output: 1290
🏁 Script executed:
cat -n pkg/rpc/core/blocks.go | sed -n '1,80p'Repository: evstack/ev-abci
Length of output: 2812
🏁 Script executed:
rg -n -A 5 'func buildAttesterVoteBytes' pkg/rpc/core/blocks.goRepository: evstack/ev-abci
Length of output: 153
🏁 Script executed:
rg -n 'ValidatorIndex' pkg/rpc/core/blocks.go server/attester_cmd.goRepository: evstack/ev-abci
Length of output: 203
🏁 Script executed:
rg -n -B 10 -A 15 'func buildAttesterVoteBytes|buildAttesterVoteBytes(' server/Repository: evstack/ev-abci
Length of output: 306
🏁 Script executed:
cat -n server/attester_cmd.go | sed -n '460,500p'Repository: evstack/ev-abci
Length of output: 1618
🏁 Script executed:
cat -n server/attester_cmd.go | sed -n '166,180p'Repository: evstack/ev-abci
Length of output: 647
🏁 Script executed:
rg -n -B 2 -A 2 'buildAttesterVoteBytes(' server/attester_cmd.goRepository: evstack/ev-abci
Length of output: 250
🏁 Script executed:
cat -n server/attester_cmd.go | sed -n '467,495p'Repository: evstack/ev-abci
Length of output: 1103
🏁 Script executed:
rg -n 'buildAttesterVoteBytes' server/attester_cmd.goRepository: evstack/ev-abci
Length of output: 284
Propagate attester index into signed vote bytes.
buildAttesterVoteBytes hardcodes ValidatorIndex to 0. Multi-attester setups will generate invalid sign bytes for any attester with a non-zero index. Update assertRegistered to return the matched index and pass it through the call chain.
- Change
assertRegisteredto return(int32, error)and returnint32(e.Index)on match. - Update the call in
pullBlocksAndAttestto capture the index. - Update
buildAttesterVoteBytessignature to acceptvalidatorIndex int32and use it inVote.
Diffs
--- a/server/attester_cmd.go
+++ b/server/attester_cmd.go
@@ -147,9 +147,9 @@ func attesterCommand() *cobra.Command {
return cmd
}
-func assertRegistered(
+func assertRegistered(
ctx context.Context,
consensusPrivKey *pvm.FilePV,
clientCtx client.Context,
-) error {
+) (int32, error) {
consAddr := sdk.ConsAddress(consensusPrivKey.Key.PubKey.Address()).String()
queryClient := networktypes.NewQueryClient(clientCtx)
resp, err := queryClient.AttesterSet(ctx, &networktypes.QueryAttesterSetRequest{})
@@ -157,10 +157,10 @@ func assertRegistered(
return fmt.Errorf("query attester set: %w", err)
}
for _, e := range resp.Entries {
if e.ConsensusAddress == consAddr {
- return nil
+ return int32(e.Index), nil
}
}
- return fmt.Errorf("consensus address %s is not in the attester set; must be registered in genesis", consAddr)
+ return 0, fmt.Errorf("consensus address %s is not in the attester set; must be registered in genesis", consAddr)
}
func pullBlocksAndAttest(
@@ -171,7 +171,8 @@ func pullBlocksAndAttest(
consensusPrivKey *pvm.FilePV,
clientCtx client.Context,
) error {
- if err := assertRegistered(ctx, consensusPrivKey, clientCtx); err != nil {
+ validatorIndex, err := assertRegistered(ctx, consensusPrivKey, clientCtx)
+ if err != nil {
return err
}
@@ -445,7 +446,7 @@ func addBlockAndAttest(
return err
}
- voteBytes, err := buildAttesterVoteBytes(config.ChainID, height, blockID, header.Time(), pv)
+ voteBytes, err := buildAttesterVoteBytes(config.ChainID, height, blockID, header.Time(), pv, validatorIndex)
if err != nil {
return err
}
@@ -469,9 +470,10 @@ func addBlockAndAttest(
func buildAttesterVoteBytes(
chainID string,
height int64,
blockID cmtproto.BlockID,
timestamp time.Time,
pv *pvm.FilePV,
+ validatorIndex int32,
) ([]byte, error) {
validatorAddress := pv.Key.Address
vote := cmtproto.Vote{
@@ -479,7 +481,7 @@ func buildAttesterVoteBytes(
BlockID: blockID,
Timestamp: timestamp,
ValidatorAddress: validatorAddress,
- ValidatorIndex: 0,
+ ValidatorIndex: validatorIndex,
}
signBytes := cmttypes.VoteSignBytes(chainID, &vote)
sig, err := pv.Key.PrivKey.Sign(signBytes)🤖 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 `@server/attester_cmd.go` around lines 158 - 160, `buildAttesterVoteBytes` is
hardcoding the attester index to zero, so signed vote bytes are wrong for
non-zero attesters. Update `assertRegistered` to return `(int32, error)` and
return the matched `e.Index`, then change `pullBlocksAndAttest` to capture that
index and pass it into `buildAttesterVoteBytes`. Also update
`buildAttesterVoteBytes` to accept `validatorIndex int32` and use it when
constructing the `Vote` so the signed bytes reflect the correct attester
identity.
Comment on lines
+508
to
+529
| httpClient := &http.Client{Timeout: 10 * time.Second} | ||
| resp, err := httpClient.Get(fmt.Sprintf("http://%s/block", parsed.Host)) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("query block: %w", err) | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
|
|
||
| if config.Verbose { | ||
| fmt.Printf("Attestation submitted for block %d with hash: %s\n", height, txHash) | ||
| var blockResp struct { | ||
| Result struct { | ||
| Block struct { | ||
| Header struct { | ||
| Height string `json:"height"` | ||
| } `json:"header"` | ||
| } `json:"block"` | ||
| } `json:"result"` | ||
| } | ||
| return nil | ||
| if err := json.NewDecoder(resp.Body).Decode(&blockResp); err != nil { | ||
| return 0, fmt.Errorf("decode block: %w", err) | ||
| } | ||
| heightStr := blockResp.Result.Block.Header.Height | ||
| if heightStr == "" { | ||
| return 0, nil |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Surface JSON-RPC errors instead of treating them as height 0.
Any JSON response without result.block.header.height currently returns 0, nil, including JSON-RPC error bodies. That can make the attester wait forever while hiding node/RPC failures.
Suggested guard
if err := json.NewDecoder(resp.Body).Decode(&blockResp); err != nil {
return 0, fmt.Errorf("decode block: %w", err)
}
+ if blockResp.Error != nil {
+ return 0, fmt.Errorf("block RPC error: %s", blockResp.Error.Message)
+ }
heightStr := blockResp.Result.Block.Header.HeightAdd an Error field to the local response struct:
var blockResp struct {
+ Error *struct {
+ Code int `json:"code"`
+ Message string `json:"message"`
+ } `json:"error"`
Result struct {📝 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
| httpClient := &http.Client{Timeout: 10 * time.Second} | |
| resp, err := httpClient.Get(fmt.Sprintf("http://%s/block", parsed.Host)) | |
| if err != nil { | |
| return 0, fmt.Errorf("query block: %w", err) | |
| } | |
| defer func() { _ = resp.Body.Close() }() | |
| if config.Verbose { | |
| fmt.Printf("Attestation submitted for block %d with hash: %s\n", height, txHash) | |
| var blockResp struct { | |
| Result struct { | |
| Block struct { | |
| Header struct { | |
| Height string `json:"height"` | |
| } `json:"header"` | |
| } `json:"block"` | |
| } `json:"result"` | |
| } | |
| return nil | |
| if err := json.NewDecoder(resp.Body).Decode(&blockResp); err != nil { | |
| return 0, fmt.Errorf("decode block: %w", err) | |
| } | |
| heightStr := blockResp.Result.Block.Header.Height | |
| if heightStr == "" { | |
| return 0, nil | |
| httpClient := &http.Client{Timeout: 10 * time.Second} | |
| resp, err := httpClient.Get(fmt.Sprintf("http://%s/block", parsed.Host)) | |
| if err != nil { | |
| return 0, fmt.Errorf("query block: %w", err) | |
| } | |
| defer func() { _ = resp.Body.Close() }() | |
| var blockResp struct { | |
| Error *struct { | |
| Code int `json:"code"` | |
| Message string `json:"message"` | |
| } `json:"error"` | |
| Result struct { | |
| Block struct { | |
| Header struct { | |
| Height string `json:"height"` | |
| } `json:"header"` | |
| } `json:"block"` | |
| } `json:"result"` | |
| } | |
| if err := json.NewDecoder(resp.Body).Decode(&blockResp); err != nil { | |
| return 0, fmt.Errorf("decode block: %w", err) | |
| } | |
| if blockResp.Error != nil { | |
| return 0, fmt.Errorf("block RPC error: %s", blockResp.Error.Message) | |
| } | |
| heightStr := blockResp.Result.Block.Header.Height | |
| if heightStr == "" { | |
| return 0, nil |
🤖 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 `@server/attester_cmd.go` around lines 508 - 529, The block-height fetch in
attester block querying is swallowing JSON-RPC error responses by returning
height 0 with no error. Update the local response struct in the block lookup
path to include an Error field alongside Result, and in the JSON decode flow
after httpClient.Get and json.NewDecoder(resp.Body).Decode, detect a populated
RPC error and return it instead of treating missing block.header.height as
success. Keep the check in the same block-query logic so parsed.Host and the
existing height parsing behavior remain intact.
Comment on lines
+62
to
+65
| attesterIdentities, err := generateAttesterIdentities(dockerAttesterCount) | ||
| require.NoError(s.T(), err) | ||
|
|
||
| gmChain := s.getGmChain(ctx, attesterIdentities) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'SetBech32PrefixFor(Account|Validator|ConsensusNode)|ConsensusAddress|address-prefix celestia' tests/integration tests/integration/dockerRepository: evstack/ev-abci
Length of output: 8380
Set the GM consensus Bech32 prefix before generating attester identities
The chain is scaffolded with --address-prefix celestia, but generateAttesterIdentities (line 340 in gm_gaia_health_test.go) serializes consensus addresses using sdk.ConsAddress before any Bech32 configuration for the consensus prefix occurs. Other tests in the suite configure only the account prefix, leaving consensus nodes on the default SDK prefix. This causes a mismatch where genesis attester_infos contain addresses prefixed with cosmosvalcons instead of celestiavalcons.
Add the full Bech32 configuration before calling generateAttesterIdentities:
@@ -224,6 +224,11 @@ func (s *DockerIntegrationTestSuite) TestAttesterSystem() {
gmChain := s.getGmChain(ctx, attesterIdentities)
+ // Set GM specific Bech32 prefixes before generating identities to ensure correct address encoding
+ cfg := sdk.GetConfig()
+ cfg.SetBech32PrefixForAccount("celestia", "celestiapub")
+ cfg.SetBech32PrefixForValidator("celestiavaloper", "celestiavaloperpub")
+ cfg.SetBech32PrefixForConsensusNode("celestiavalcons", "celestiavalconspub")
+
attesterIdentities, err := generateAttesterIdentities(dockerAttesterCount)🤖 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 `@tests/integration/gm_gaia_health_test.go` around lines 62 - 65, Set the GM
consensus Bech32 prefix before calling generateAttesterIdentities in
gm_gaia_health_test.go, since that helper serializes sdk.ConsAddress values
before any consensus prefix is configured. Update the test setup around
getGmChain and generateAttesterIdentities to initialize the full Bech32 config
(not just the account prefix) so genesis attester_infos use celestiavalcons
instead of the default cosmosvalcons.
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
3 participants
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
This is Part 1 of the prod-readiness split. It introduces the genesis-backed fixed attester set as the foundation for the later vote verification, RPC reconstruction, attester CLI, and integration PRs.
Changes:
attester_infosto network genesis and generated protobuf types.MsgJoinAttesterSetandMsgLeaveAttesterSetchanges.Validation
go test ./modules/network/...Summary by CodeRabbit
validator_indicesinputs are ignored.