feat(network): load fixed attester set from genesis (Part 1)#394
Open
feat(network): load fixed attester set from genesis (Part 1)#394
Conversation
Contributor
|
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 (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGenesis now builds the validator set deterministically from Changes
Sequence DiagramsequenceDiagram
participant Genesis as Genesis
participant Keeper as Keeper
participant Store as KVStore
Note over Genesis,Keeper: Genesis initialization with AttesterInfos
Genesis->>Keeper: InitGenesis(state.attester_infos)
Keeper->>Keeper: Decode/unpack AttesterInfo.Pubkey (Any)
Keeper->>Keeper: Verify bech32 ConsensusAddress == pk.Address()
Keeper->>Keeper: Normalize ConsensusAddress string
Keeper->>Keeper: Sort attesters by pubkey address bytes
Keeper->>Keeper: Assign validator indices (0..N-1) and set power=1
Keeper->>Store: Persist AttesterInfo keyed by consensus_address
Keeper->>Store: Persist ValidatorIndex & ValidatorPower entries
Note over Genesis,Keeper: Genesis export
Genesis->>Keeper: ExportGenesis()
Keeper->>Store: Read all AttesterInfo entries
Keeper->>Keeper: Sort by pubkey address bytes
Keeper-->>Genesis: Return GenesisState with attester_infos (validator_indices nil)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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
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
2 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
Breaking Changes
New Features
Bug Fixes
Tests