fix: align attester light client RPC data#409
Open
randygrok wants to merge 7 commits into
Open
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:
📝 WalkthroughWalkthroughAdds an ChangesAttester Light Validator Set
Sequence Diagram(s)sequenceDiagram
participant Client
participant CommitRPC as Commit RPC
participant getCommitForHeight
participant getAttesterValidatorSet
participant App as env.Adapter.App.Query
participant Store as Rollkit Store
Client->>CommitRPC: Commit(height)
CommitRPC->>Store: getBlockMeta(height) via buildABCIBlock
CommitRPC->>getCommitForHeight: height, &blockMeta.BlockID
getCommitForHeight->>getAttesterValidatorSet: ctx
getAttesterValidatorSet->>App: Query /evabci.network.v1.Query/Attesters
App-->>getAttesterValidatorSet: QueryAttestersResponse
getAttesterValidatorSet-->>getCommitForHeight: cmttypes.ValidatorSet
getCommitForHeight->>Store: get attester votes for height
loop validate each vote BlockID
getCommitForHeight-->>getCommitForHeight: store in signaturesByAddress
end
getCommitForHeight-->>getCommitForHeight: build CommitSig list in ValidatorSet order
getCommitForHeight-->>CommitRPC: cmttypes.Commit{blockID, signatures}
CommitRPC-->>Client: ResultCommit{SignedHeader}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
tests/integration/gm_gaia_health_test.go (1)
168-181: 💤 Low valueRedundant assertion can be removed.
The assertion at line 170 duplicates the check already performed inside
newDistinctAttesterPrivValidatorFilesat line 248. Consider removing one to avoid redundancy.♻️ Suggested simplification
attesterPrivValidatorKeyJSON, attesterPrivValidatorStateJSON := newDistinctAttesterPrivValidatorFiles(s.T(), sequencerPrivValidatorKeyJSON) - require.False(s.T(), bytes.Equal(sequencerPrivValidatorKeyJSON, attesterPrivValidatorKeyJSON), "attester consensus key must differ from sequencer consensus key") require.NoError(s.T(), attesterNode.WriteFile(🤖 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 168 - 181, The require.False assertion that verifies the attester consensus key differs from the sequencer consensus key is redundant because the same validation is already performed inside the newDistinctAttesterPrivValidatorFiles function. Remove the redundant assertion statement to eliminate code duplication and improve clarity, keeping only the call to newDistinctAttesterPrivValidatorFiles which already ensures the keys are distinct.pkg/rpc/core/attester_light_test.go (1)
16-63: 💤 Low valueConsider adding a success case and using
require.ErrorContains.The test comprehensively covers error paths but lacks a success case where a valid
RegisteredAttester(matching pubkey and validator address) is converted successfully. Also, testify providesrequire.ErrorContainswhich is more idiomatic thanstrings.Contains.♻️ Suggested improvements
- require.True(t, strings.Contains(err.Error(), test.errContains), err.Error()) + require.ErrorContains(t, err, test.errContains)Consider adding a success case:
"valid attester": { attester: &networktypes.RegisteredAttester{ ValidatorAddress: sdk.ValAddress(validPubKey.Address()).String(), AttesterInfo: &networktypes.AttesterInfo{ Pubkey: newCoreAttesterPubKeyAny(t, validPubKey), }, }, errContains: "", // expect no error },🤖 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/attester_light_test.go` around lines 16 - 63, The test function TestCometValidatorFromAttesterValidation lacks a success case and uses a non-idiomatic error assertion. Add a new test case in the tests map (e.g., "valid attester") that creates a RegisteredAttester with a matching ValidatorAddress and pubkey (using the validPubKeyAny variable and deriving its corresponding validator address via sdk.ValAddress), setting errContains to an empty string to expect successful conversion without error. Additionally, replace the strings.Contains assertion on the error with require.ErrorContains, which is the idiomatic testify approach for checking error messages.pkg/rpc/core/status.go (1)
102-110: 💤 Low valueEdge case: empty validator set with no error may silently use genesis validator.
The condition on line 107 checks
err == nil && len(attesterValidatorSet.Validators) > 0, which means ifgetAttesterValidatorSetreturns a valid but empty validator set, the code silently falls back to the genesis validator. This may be intentional (consistent witherrNoRegisteredAttestershandling), but consider whether an empty validator set (different from "no attesters registered" error) should be logged or handled distinctly.🤖 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/status.go` around lines 102 - 110, When getAttesterValidatorSet returns successfully (err is nil) but with an empty validator set, the code silently falls through and uses the genesis validator without any indication this happened. Add explicit logging when this edge case occurs (err is nil and len(attesterValidatorSet.Validators) is 0) to distinguish it from the errNoRegisteredAttesters case, making it clear in the logs that an empty attester validator set was received and the fallback to genesis validator is being used.
🤖 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/grpc_query_test.go`:
- Around line 47-52: The test fixture is creating pubKeyAny and consensusAddress
from different random ed25519 keypairs, which creates internally inconsistent
attester data. Refactor the test to generate a single keypair once using
ed25519.GenPrivKey(), then derive both the pubKeyAny (from that keypair's public
key) and consensusAddress (from that same keypair's public key address) from
this single keypair. This ensures the attesterInfo structure has a consistent
relationship between its Pubkey and the validator address, making the test data
more realistic and the test more valuable as a contract test.
In `@server/attester_cmd_test.go`:
- Around line 61-87: In server/attester_cmd_test.go, fatal assertions are being
made from within httptest handler goroutines, which prevents proper test failure
semantics. At lines 63-64 in the "height 1 reads block ID from RPC" test (the
anchor), capture the HTTP request path and height query parameter values into
variables within the handler instead of calling require.Equal there, then assert
those captured values after the getOriginalBlockID function returns in the main
test goroutine. Apply the same fix at lines 108-109 in the sibling test (which
appears to test "height 7"), capturing the path and height query parameter and
asserting them after the getEvolveBlock function returns.
In `@server/attester_cmd.go`:
- Around line 770-780: Add a height validation check after extracting the header
from the blockResponse. After the evolveHeaderFromResponse call succeeds,
compare the height field from the returned header with the caller-supplied
height parameter. If they do not match, return an error to reject the response
and avoid attesting to the wrong block. This validation should occur before the
blockIDFromResponse call and before the final return statement to ensure the
block height is verified before any further processing or signing occurs.
- Around line 757-768: The code is missing an HTTP status code check before
attempting to decode the JSON response. After receiving the response from
httpClient.Do(req), add a validation check for resp.StatusCode before the JSON
decoder call. If the status code indicates a non-success response (e.g., >= 400
or outside the 2xx range), return an error that includes the status code and
relevant response details instead of falling through to the JSON decoding. This
will prevent misleading attestation errors that occur when RPC failures are
masked by JSON decode errors.
- Around line 742-759: The getEvolveBlock function is hardcoding the http scheme
when constructing the block RPC URL on line 752, which breaks https endpoints.
After parsing the node URL with url.Parse to get the parsed variable, extract
the scheme from parsed.Scheme and conditionally determine which scheme to use:
if the scheme is tcp, map it to http, otherwise preserve the original scheme
(http or https). Then use this determined scheme in the fmt.Sprintf call when
building the block request URL instead of always hardcoding http.
---
Nitpick comments:
In `@pkg/rpc/core/attester_light_test.go`:
- Around line 16-63: The test function TestCometValidatorFromAttesterValidation
lacks a success case and uses a non-idiomatic error assertion. Add a new test
case in the tests map (e.g., "valid attester") that creates a RegisteredAttester
with a matching ValidatorAddress and pubkey (using the validPubKeyAny variable
and deriving its corresponding validator address via sdk.ValAddress), setting
errContains to an empty string to expect successful conversion without error.
Additionally, replace the strings.Contains assertion on the error with
require.ErrorContains, which is the idiomatic testify approach for checking
error messages.
In `@pkg/rpc/core/status.go`:
- Around line 102-110: When getAttesterValidatorSet returns successfully (err is
nil) but with an empty validator set, the code silently falls through and uses
the genesis validator without any indication this happened. Add explicit logging
when this edge case occurs (err is nil and len(attesterValidatorSet.Validators)
is 0) to distinguish it from the errNoRegisteredAttesters case, making it clear
in the logs that an empty attester validator set was received and the fallback
to genesis validator is being used.
In `@tests/integration/gm_gaia_health_test.go`:
- Around line 168-181: The require.False assertion that verifies the attester
consensus key differs from the sequencer consensus key is redundant because the
same validation is already performed inside the
newDistinctAttesterPrivValidatorFiles function. Remove the redundant assertion
statement to eliminate code duplication and improve clarity, keeping only the
call to newDistinctAttesterPrivValidatorFiles which already ensures the keys are
distinct.
🪄 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: 54fce716-34e2-48b2-add7-cddb9bb880c1
⛔ 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 (13)
modules/network/keeper/grpc_query.gomodules/network/keeper/grpc_query_test.gomodules/proto/evabci/network/v1/query.protopkg/rpc/core/attester_light.gopkg/rpc/core/attester_light_test.gopkg/rpc/core/blocks.gopkg/rpc/core/blocks_test.gopkg/rpc/core/consensus.gopkg/rpc/core/status.gopkg/rpc/core/utils.goserver/attester_cmd.goserver/attester_cmd_test.gotests/integration/gm_gaia_health_test.go
Comment on lines
+47
to
+52
| pubKeyAny := newAttesterPubKeyAny(t) | ||
| consensusAddress := sdk.ValAddress(ed25519.GenPrivKey().PubKey().Address()).String() | ||
| attesterInfo := &types.AttesterInfo{ | ||
| Authority: sdk.AccAddress("operator").String(), | ||
| Pubkey: pubKeyAny, | ||
| JoinedHeight: ctx.BlockHeight(), |
Contributor
There was a problem hiding this comment.
Use one keypair for both validator_address and attester_info.pubkey in the success fixture.
At Line 47-52, the test builds pubKeyAny and consensusAddress from different random keys. That creates internally inconsistent attester data and reduces the value of this contract test.
Proposed fix
- pubKeyAny := newAttesterPubKeyAny(t)
- consensusAddress := sdk.ValAddress(ed25519.GenPrivKey().PubKey().Address()).String()
+ priv := ed25519.GenPrivKey()
+ sdkPubKey, err := cryptocodec.FromCmtPubKeyInterface(priv.PubKey())
+ require.NoError(t, err)
+ pubKeyAny, err := codectypes.NewAnyWithValue(sdkPubKey)
+ require.NoError(t, err)
+ consensusAddress := sdk.ValAddress(priv.PubKey().Address()).String()🤖 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/grpc_query_test.go` around lines 47 - 52, The test
fixture is creating pubKeyAny and consensusAddress from different random ed25519
keypairs, which creates internally inconsistent attester data. Refactor the test
to generate a single keypair once using ed25519.GenPrivKey(), then derive both
the pubKeyAny (from that keypair's public key) and consensusAddress (from that
same keypair's public key address) from this single keypair. This ensures the
attesterInfo structure has a consistent relationship between its Pubkey and the
validator address, making the test data more realistic and the test more
valuable as a contract test.
Comment on lines
+61
to
+87
| t.Run("height 1 reads block ID from RPC", func(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| require.Equal(t, "/block", r.URL.Path) | ||
| require.Equal(t, "1", r.URL.Query().Get("height")) | ||
| _, _ = w.Write([]byte(`{ | ||
| "result": { | ||
| "block_id": { | ||
| "hash": "1111111111111111111111111111111111111111111111111111111111111111", | ||
| "parts": { | ||
| "hash": "2222222222222222222222222222222222222222222222222222222222222222", | ||
| "total": 3 | ||
| } | ||
| }, | ||
| "block": { | ||
| "header": { | ||
| "height": "1", | ||
| "time": "2026-06-16T00:00:00Z", | ||
| "chain_id": "test" | ||
| } | ||
| } | ||
| } | ||
| }`)) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| blockID, err := getOriginalBlockID(context.Background(), server.URL, 1) | ||
| require.NoError(t, err) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify testify require calls inside HTTP handler closures.
rg -n -C3 'httptest\.NewServer|require\.' --type goRepository: evstack/ev-abci
Length of output: 50372
🏁 Script executed:
head -160 server/attester_cmd_test.go | tail -110Repository: evstack/ev-abci
Length of output: 3657
Move fatal assertions out of httptest handler goroutines. Both handlers call require.Equal from the server goroutine; require uses fatal test failure semantics, so these checks should be captured and asserted back in the test goroutine.
server/attester_cmd_test.go#L63-L64: capture the/blockpath andheight=1query in variables, then assert them aftergetOriginalBlockIDreturns.server/attester_cmd_test.go#L108-L109: capture the/blockpath andheight=7query in variables, then assert them aftergetEvolveBlockreturns.
📍 Affects 1 file
server/attester_cmd_test.go#L61-L87(this comment)server/attester_cmd_test.go#L104-L143
🤖 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_test.go` around lines 61 - 87, In
server/attester_cmd_test.go, fatal assertions are being made from within
httptest handler goroutines, which prevents proper test failure semantics. At
lines 63-64 in the "height 1 reads block ID from RPC" test (the anchor), capture
the HTTP request path and height query parameter values into variables within
the handler instead of calling require.Equal there, then assert those captured
values after the getOriginalBlockID function returns in the main test goroutine.
Apply the same fix at lines 108-109 in the sibling test (which appears to test
"height 7"), capturing the path and height query parameter and asserting them
after the getEvolveBlock function returns.
Comment on lines
+742
to
+759
| func getEvolveBlock(ctx context.Context, node string, height int64) (*evolvetypes.Header, cmtproto.BlockID, error) { | ||
| parsed, err := url.Parse(node) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("parse node URL: %w", err) | ||
| return nil, cmtproto.BlockID{}, fmt.Errorf("parse node URL: %w", err) | ||
| } | ||
|
|
||
| httpClient := &http.Client{ | ||
| Timeout: 10 * time.Second, | ||
| } | ||
|
|
||
| resp, err := httpClient.Get(fmt.Sprintf("http://%s/block?height=%d", parsed.Host, height)) | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("http://%s/block?height=%d", parsed.Host, height), nil) | ||
| if err != nil { | ||
| return nil, cmtproto.BlockID{}, fmt.Errorf("creating block request: %w", err) | ||
| } | ||
|
|
||
| resp, err := httpClient.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("querying block: %w", err) | ||
| return nil, cmtproto.BlockID{}, fmt.Errorf("querying block: %w", err) |
Contributor
There was a problem hiding this comment.
Preserve HTTPS when building the block RPC URL.
Line 752 always rewrites the configured node to http://..., so an https:// RPC endpoint is silently downgraded or broken. Map tcp to http, but preserve explicit http/https.
Suggested fix
func getEvolveBlock(ctx context.Context, node string, height int64) (*evolvetypes.Header, cmtproto.BlockID, error) {
parsed, err := url.Parse(node)
if err != nil {
return nil, cmtproto.BlockID{}, fmt.Errorf("parse node URL: %w", err)
}
+ scheme := parsed.Scheme
+ switch scheme {
+ case "", "tcp":
+ scheme = "http"
+ case "http", "https":
+ default:
+ return nil, cmtproto.BlockID{}, fmt.Errorf("unsupported node URL scheme %q", parsed.Scheme)
+ }
httpClient := &http.Client{
Timeout: 10 * time.Second,
}
- req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("http://%s/block?height=%d", parsed.Host, height), nil)
+ blockURL := url.URL{Scheme: scheme, Host: parsed.Host, Path: "/block"}
+ query := blockURL.Query()
+ query.Set("height", strconv.FormatInt(height, 10))
+ blockURL.RawQuery = query.Encode()
+
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, blockURL.String(), nil)📝 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
| func getEvolveBlock(ctx context.Context, node string, height int64) (*evolvetypes.Header, cmtproto.BlockID, error) { | |
| parsed, err := url.Parse(node) | |
| if err != nil { | |
| return nil, fmt.Errorf("parse node URL: %w", err) | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("parse node URL: %w", err) | |
| } | |
| httpClient := &http.Client{ | |
| Timeout: 10 * time.Second, | |
| } | |
| resp, err := httpClient.Get(fmt.Sprintf("http://%s/block?height=%d", parsed.Host, height)) | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("http://%s/block?height=%d", parsed.Host, height), nil) | |
| if err != nil { | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("creating block request: %w", err) | |
| } | |
| resp, err := httpClient.Do(req) | |
| if err != nil { | |
| return nil, fmt.Errorf("querying block: %w", err) | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("querying block: %w", err) | |
| func getEvolveBlock(ctx context.Context, node string, height int64) (*evolvetypes.Header, cmtproto.BlockID, error) { | |
| parsed, err := url.Parse(node) | |
| if err != nil { | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("parse node URL: %w", err) | |
| } | |
| scheme := parsed.Scheme | |
| switch scheme { | |
| case "", "tcp": | |
| scheme = "http" | |
| case "http", "https": | |
| default: | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("unsupported node URL scheme %q", parsed.Scheme) | |
| } | |
| httpClient := &http.Client{ | |
| Timeout: 10 * time.Second, | |
| } | |
| blockURL := url.URL{Scheme: scheme, Host: parsed.Host, Path: "/block"} | |
| query := blockURL.Query() | |
| query.Set("height", strconv.FormatInt(height, 10)) | |
| blockURL.RawQuery = query.Encode() | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, blockURL.String(), nil) | |
| if err != nil { | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("creating block request: %w", err) | |
| } | |
| resp, err := httpClient.Do(req) | |
| if err != nil { | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("querying block: %w", err) |
🤖 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 742 - 759, The getEvolveBlock function
is hardcoding the http scheme when constructing the block RPC URL on line 752,
which breaks https endpoints. After parsing the node URL with url.Parse to get
the parsed variable, extract the scheme from parsed.Scheme and conditionally
determine which scheme to use: if the scheme is tcp, map it to http, otherwise
preserve the original scheme (http or https). Then use this determined scheme in
the fmt.Sprintf call when building the block request URL instead of always
hardcoding http.
Comment on lines
+757
to
768
| resp, err := httpClient.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("querying block: %w", err) | ||
| return nil, cmtproto.BlockID{}, fmt.Errorf("querying block: %w", err) | ||
| } | ||
| defer func() { | ||
| _ = resp.Body.Close() | ||
| }() | ||
|
|
||
| var blockResponse struct { | ||
| Result struct { | ||
| Block struct { | ||
| Header struct { | ||
| Version struct { | ||
| Block string `json:"block"` | ||
| App string `json:"app"` | ||
| } `json:"version"` | ||
| Height string `json:"height"` | ||
| Time string `json:"time"` | ||
| LastBlockID struct { | ||
| Hash string `json:"hash"` | ||
| } `json:"last_block_id"` | ||
| LastCommitHash string `json:"last_commit_hash"` | ||
| DataHash string `json:"data_hash"` | ||
| ValidatorsHash string `json:"validators_hash"` | ||
| NextValidatorsHash string `json:"next_validators_hash"` | ||
| ConsensusHash string `json:"consensus_hash"` | ||
| AppHash string `json:"app_hash"` | ||
| LastResultsHash string `json:"last_results_hash"` | ||
| EvidenceHash string `json:"evidence_hash"` | ||
| ProposerAddress string `json:"proposer_address"` | ||
| ChainID string `json:"chain_id"` | ||
| } `json:"header"` | ||
| } `json:"block"` | ||
| } `json:"result"` | ||
| var blockResponse evolveBlockResponse | ||
| if err := json.NewDecoder(resp.Body).Decode(&blockResponse); err != nil { | ||
| return nil, cmtproto.BlockID{}, fmt.Errorf("decoding response: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Check the HTTP status before decoding the block response.
A non-2xx RPC response currently falls through to JSON/header parsing, which hides the actual RPC failure and can produce misleading attestation errors.
Suggested fix
resp, err := httpClient.Do(req)
if err != nil {
return nil, cmtproto.BlockID{}, fmt.Errorf("querying block: %w", err)
}
defer func() {
_ = resp.Body.Close()
}()
+ if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 4096))
+ return nil, cmtproto.BlockID{}, fmt.Errorf(
+ "querying block: unexpected status %d: %s",
+ resp.StatusCode,
+ strings.TrimSpace(string(body)),
+ )
+ }
var blockResponse evolveBlockResponse📝 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
| resp, err := httpClient.Do(req) | |
| if err != nil { | |
| return nil, fmt.Errorf("querying block: %w", err) | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("querying block: %w", err) | |
| } | |
| defer func() { | |
| _ = resp.Body.Close() | |
| }() | |
| var blockResponse struct { | |
| Result struct { | |
| Block struct { | |
| Header struct { | |
| Version struct { | |
| Block string `json:"block"` | |
| App string `json:"app"` | |
| } `json:"version"` | |
| Height string `json:"height"` | |
| Time string `json:"time"` | |
| LastBlockID struct { | |
| Hash string `json:"hash"` | |
| } `json:"last_block_id"` | |
| LastCommitHash string `json:"last_commit_hash"` | |
| DataHash string `json:"data_hash"` | |
| ValidatorsHash string `json:"validators_hash"` | |
| NextValidatorsHash string `json:"next_validators_hash"` | |
| ConsensusHash string `json:"consensus_hash"` | |
| AppHash string `json:"app_hash"` | |
| LastResultsHash string `json:"last_results_hash"` | |
| EvidenceHash string `json:"evidence_hash"` | |
| ProposerAddress string `json:"proposer_address"` | |
| ChainID string `json:"chain_id"` | |
| } `json:"header"` | |
| } `json:"block"` | |
| } `json:"result"` | |
| var blockResponse evolveBlockResponse | |
| if err := json.NewDecoder(resp.Body).Decode(&blockResponse); err != nil { | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("decoding response: %w", err) | |
| } | |
| resp, err := httpClient.Do(req) | |
| if err != nil { | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("querying block: %w", err) | |
| } | |
| defer func() { | |
| _ = resp.Body.Close() | |
| }() | |
| if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { | |
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) | |
| return nil, cmtproto.BlockID{}, fmt.Errorf( | |
| "querying block: unexpected status %d: %s", | |
| resp.StatusCode, | |
| strings.TrimSpace(string(body)), | |
| ) | |
| } | |
| var blockResponse evolveBlockResponse | |
| if err := json.NewDecoder(resp.Body).Decode(&blockResponse); err != nil { | |
| return nil, cmtproto.BlockID{}, fmt.Errorf("decoding response: %w", err) | |
| } |
🤖 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 757 - 768, The code is missing an HTTP
status code check before attempting to decode the JSON response. After receiving
the response from httpClient.Do(req), add a validation check for resp.StatusCode
before the JSON decoder call. If the status code indicates a non-success
response (e.g., >= 400 or outside the 2xx range), return an error that includes
the status code and relevant response details instead of falling through to the
JSON decoding. This will prevent misleading attestation errors that occur when
RPC failures are masked by JSON decode errors.
Comment on lines
+770
to
+780
| header, err := evolveHeaderFromResponse(blockResponse) | ||
| if err != nil { | ||
| return nil, cmtproto.BlockID{}, err | ||
| } | ||
|
|
||
| blockID, err := blockIDFromResponse(blockResponse) | ||
| if err != nil { | ||
| return nil, cmtproto.BlockID{}, err | ||
| } | ||
|
|
||
| return header, blockID, nil |
Contributor
There was a problem hiding this comment.
Verify the returned block height before signing its BlockID.
submitAttestation signs using the caller-supplied height, but the blockID and timestamp come from this response. Reject responses whose header height does not match the requested height to avoid attesting the wrong block.
Suggested fix
header, err := evolveHeaderFromResponse(blockResponse)
if err != nil {
return nil, cmtproto.BlockID{}, err
}
+ if height < 0 || header.Height() != uint64(height) {
+ return nil, cmtproto.BlockID{}, fmt.Errorf(
+ "block response height mismatch: requested %d, got %d",
+ height,
+ header.Height(),
+ )
+ }
blockID, err := blockIDFromResponse(blockResponse)📝 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
| header, err := evolveHeaderFromResponse(blockResponse) | |
| if err != nil { | |
| return nil, cmtproto.BlockID{}, err | |
| } | |
| blockID, err := blockIDFromResponse(blockResponse) | |
| if err != nil { | |
| return nil, cmtproto.BlockID{}, err | |
| } | |
| return header, blockID, nil | |
| header, err := evolveHeaderFromResponse(blockResponse) | |
| if err != nil { | |
| return nil, cmtproto.BlockID{}, err | |
| } | |
| if height < 0 || header.Height() != uint64(height) { | |
| return nil, cmtproto.BlockID{}, fmt.Errorf( | |
| "block response height mismatch: requested %d, got %d", | |
| height, | |
| header.Height(), | |
| ) | |
| } | |
| blockID, err := blockIDFromResponse(blockResponse) | |
| if err != nil { | |
| return nil, cmtproto.BlockID{}, err | |
| } | |
| return header, blockID, 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 770 - 780, Add a height validation check
after extracting the header from the blockResponse. After the
evolveHeaderFromResponse call succeeds, compare the height field from the
returned header with the caller-supplied height parameter. If they do not match,
return an error to reject the response and avoid attesting to the wrong block.
This validation should occur before the blockIDFromResponse call and before the
final return statement to ensure the block height is verified before any further
processing or signing occurs.
randygrok
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/rpc/core/blocks_test.go`:
- Around line 635-643: The ABCI query matchers in the test are too permissive,
matching only on req.Path without validating the request payload, which allows
tests to pass even with incorrect request data. Enhance the MatchedBy matcher
function in both affected locations to validate not only that req.Path equals
the expected query path, but also that the request data/payload matches the
expected attester information being queried. This applies to the mock setup at
the anchor location (lines 640-642) and the sibling location at lines 656-663,
ensuring both validate the complete request structure rather than just the 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: 7b02e9d2-bd18-4ac4-b5d5-2a21a635874e
📒 Files selected for processing (2)
modules/network/keeper/msg_server_test.gopkg/rpc/core/blocks_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/attester_cmd.go (1)
700-706:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSilently ignoring hex decode errors for critical header fields.
Lines 700-706 discard errors from
hex.DecodeString. If any hash field contains malformed hex, the header will be constructed with empty/nil byte slices, potentially causing downstream issues or silent data corruption.Consider validating these fields:
- lastHeaderHash, _ := hex.DecodeString(header.LastBlockID.Hash) - dataHash, _ := hex.DecodeString(header.DataHash) - validatorsHash, _ := hex.DecodeString(header.ValidatorsHash) - appHash, _ := hex.DecodeString(header.AppHash) - proposerAddress, _ := hex.DecodeString(header.ProposerAddress) - - appVersion, _ := strconv.ParseUint(header.Version.App, 10, 64) + lastHeaderHash, err := hex.DecodeString(header.LastBlockID.Hash) + if err != nil { + return nil, fmt.Errorf("decoding last block hash: %w", err) + } + dataHash, err := hex.DecodeString(header.DataHash) + if err != nil { + return nil, fmt.Errorf("decoding data hash: %w", err) + } + validatorsHash, err := hex.DecodeString(header.ValidatorsHash) + if err != nil { + return nil, fmt.Errorf("decoding validators hash: %w", err) + } + appHash, err := hex.DecodeString(header.AppHash) + if err != nil { + return nil, fmt.Errorf("decoding app hash: %w", err) + } + proposerAddress, err := hex.DecodeString(header.ProposerAddress) + if err != nil { + return nil, fmt.Errorf("decoding proposer address: %w", err) + } + + appVersion, err := strconv.ParseUint(header.Version.App, 10, 64) + if err != nil { + return nil, fmt.Errorf("parsing app version: %w", err) + }🤖 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 700 - 706, The code is silently ignoring errors returned by hex.DecodeString and strconv.ParseUint operations on critical header fields (LastBlockID.Hash, DataHash, ValidatorsHash, AppHash, ProposerAddress, and Version.App). Instead of using the blank identifier _ to discard these errors, capture the error return values from each hex.DecodeString call and the strconv.ParseUint call, then add error handling to validate that all these operations succeeded before proceeding with the header construction. Return an error or log and exit early if any of these critical fields fail to decode or parse.
🧹 Nitpick comments (1)
server/attester_cmd.go (1)
560-585: 💤 Low valueInconsistent validator address derivation.
Line 566 derives the address via
pv.Key.PrivKey.PubKey().Address()while line 579 usespv.Key.Addressdirectly. Both should yield the same value, but using the canonicalpv.Key.Addressconsistently would be clearer.vote := cmtproto.Vote{ Type: cmtproto.PrecommitType, Height: height, BlockID: blockID, Round: 0, Timestamp: header.Time(), - ValidatorAddress: pv.Key.PrivKey.PubKey().Address(), + ValidatorAddress: pv.Key.Address, ValidatorIndex: 0, }🤖 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 560 - 585, The validator address is being derived inconsistently between two vote structures. In the initial vote struct creation, ValidatorAddress is set using pv.Key.PrivKey.PubKey().Address(), while in the attesterVote struct it uses pv.Key.Address directly. Replace the ValidatorAddress assignment in the first vote struct to use pv.Key.Address (the canonical approach) to ensure consistency and clarity throughout the function.
🤖 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 `@pkg/rpc/core/utils.go`:
- Around line 27-31: The nil check on env at line 27 only protects the if branch
for AttesterMode, leaving the else branch vulnerable to a nil pointer
dereference when accessing env.Adapter.RollkitStore.Height(). Restructure the
conditional logic to first check if env is not nil, then check the AttesterMode
condition within that guard, or add an explicit nil check for env in the else
branch before attempting to access env.Adapter. Ensure both code paths that
access env members have proper nil protection.
In `@server/attester_cmd.go`:
- Around line 300-331: The queryLatestProductionHeight function does not check
the HTTP response status code before attempting to decode the JSON response
body. After receiving the response from httpClient.Do(req), add a validation
check to ensure the status code is in the 2xx range (such as checking if
resp.StatusCode is between 200 and 299). If the status code indicates an error,
return an appropriate error message that includes the actual status code and
response body context rather than proceeding to decode invalid JSON, which would
produce misleading error messages.
In `@tests/integration/docker/patches/app-wiring/patch-app-wiring.sh`:
- Around line 216-224: The grep patterns checking for NetworkKeeper presence are
too strict with fixed string matching, causing false negatives when app.go has
different whitespace formatting (tabs vs spaces). Replace the exact string
patterns "NetworkKeeper networkkeeper.Keeper" and "&app.NetworkKeeper" with
regex patterns using [[:space:]]+ to match any amount of whitespace between
tokens. Update both the grep commands on lines 216 and 220 to use grep -qE
(extended regex) with patterns like
"NetworkKeeper[[:space:]]+networkkeeper\.Keeper" and "&app\.NetworkKeeper"
respectively, ensuring the dot in the namespace is escaped as a literal
character in the regex.
---
Outside diff comments:
In `@server/attester_cmd.go`:
- Around line 700-706: The code is silently ignoring errors returned by
hex.DecodeString and strconv.ParseUint operations on critical header fields
(LastBlockID.Hash, DataHash, ValidatorsHash, AppHash, ProposerAddress, and
Version.App). Instead of using the blank identifier _ to discard these errors,
capture the error return values from each hex.DecodeString call and the
strconv.ParseUint call, then add error handling to validate that all these
operations succeeded before proceeding with the header construction. Return an
error or log and exit early if any of these critical fields fail to decode or
parse.
---
Nitpick comments:
In `@server/attester_cmd.go`:
- Around line 560-585: The validator address is being derived inconsistently
between two vote structures. In the initial vote struct creation,
ValidatorAddress is set using pv.Key.PrivKey.PubKey().Address(), while in the
attesterVote struct it uses pv.Key.Address directly. Replace the
ValidatorAddress assignment in the first vote struct to use pv.Key.Address (the
canonical approach) to ensure consistency and clarity throughout the function.
🪄 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: 4240c51d-3828-44e8-9584-39b24f8d7b41
📒 Files selected for processing (10)
modules/network/keeper/keeper.gomodules/network/keeper/msg_server_test.gopkg/rpc/core/blocks_test.gopkg/rpc/core/status.gopkg/rpc/core/utils.gopkg/rpc/core/utils_test.goserver/attester_cmd.goserver/attester_cmd_test.gotests/integration/docker/patches/app-wiring/patch-app-wiring.shtests/integration/gm_gaia_health_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- modules/network/keeper/msg_server_test.go
- pkg/rpc/core/status.go
- server/attester_cmd_test.go
- pkg/rpc/core/blocks_test.go
- tests/integration/gm_gaia_health_test.go
Comment on lines
+27
to
+31
| if env != nil && env.AttesterMode { | ||
| heightValue, err = getLastAttestedHeightFromContext(ctx) | ||
| } else { | ||
| heightValue, err = env.Adapter.RollkitStore.Height(ctx) | ||
| } |
Contributor
There was a problem hiding this comment.
Potential nil pointer dereference in the non-attester branch.
The nil check on env (line 27) only guards the AttesterMode condition. If env is nil and height == nil || *height < 0, line 30 will dereference env.Adapter, causing a panic.
Consider extending the nil guard:
if height == nil || *height < 0 {
- if env != nil && env.AttesterMode {
+ if env == nil {
+ return 0, fmt.Errorf("environment not initialized")
+ }
+ if env.AttesterMode {
heightValue, err = getLastAttestedHeightFromContext(ctx)
} else {
heightValue, err = env.Adapter.RollkitStore.Height(ctx)
}📝 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
| if env != nil && env.AttesterMode { | |
| heightValue, err = getLastAttestedHeightFromContext(ctx) | |
| } else { | |
| heightValue, err = env.Adapter.RollkitStore.Height(ctx) | |
| } | |
| if env == nil { | |
| return 0, fmt.Errorf("environment not initialized") | |
| } | |
| if env.AttesterMode { | |
| heightValue, err = getLastAttestedHeightFromContext(ctx) | |
| } else { | |
| heightValue, err = env.Adapter.RollkitStore.Height(ctx) | |
| } |
🤖 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/utils.go` around lines 27 - 31, The nil check on env at line 27
only protects the if branch for AttesterMode, leaving the else branch vulnerable
to a nil pointer dereference when accessing env.Adapter.RollkitStore.Height().
Restructure the conditional logic to first check if env is not nil, then check
the AttesterMode condition within that guard, or add an explicit nil check for
env in the else branch before attempting to access env.Adapter. Ensure both code
paths that access env members have proper nil protection.
Comment on lines
+300
to
+331
| func queryLatestProductionHeight(ctx context.Context, httpClient *http.Client, rpcHost string) (int64, error) { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("http://%s/blockchain", rpcHost), nil) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("creating blockchain request: %w", err) | ||
| } | ||
|
|
||
| resp, err := httpClient.Do(req) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("querying blockchain: %w", err) | ||
| } | ||
| defer func() { | ||
| _ = resp.Body.Close() | ||
| }() | ||
|
|
||
| var blockchainResponse struct { | ||
| Result struct { | ||
| LastHeight string `json:"last_height"` | ||
| } `json:"result"` | ||
| } | ||
| if err := json.NewDecoder(resp.Body).Decode(&blockchainResponse); err != nil { | ||
| return 0, fmt.Errorf("decoding blockchain response: %w", err) | ||
| } | ||
| if blockchainResponse.Result.LastHeight == "" { | ||
| return 0, nil | ||
| } | ||
|
|
||
| height, err := strconv.ParseInt(blockchainResponse.Result.LastHeight, 10, 64) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("parsing latest block height: %w", err) | ||
| } | ||
| return height, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Missing HTTP status check before decoding response.
Similar to the issue flagged in past reviews for getEvolveBlock, this function doesn't verify the HTTP status code before attempting to decode JSON. A non-2xx response will produce misleading errors.
Suggested fix
resp, err := httpClient.Do(req)
if err != nil {
return 0, fmt.Errorf("querying blockchain: %w", err)
}
defer func() {
_ = resp.Body.Close()
}()
+ if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
+ return 0, fmt.Errorf("querying blockchain: unexpected status %d", resp.StatusCode)
+ }
var blockchainResponse struct {🤖 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 300 - 331, The
queryLatestProductionHeight function does not check the HTTP response status
code before attempting to decode the JSON response body. After receiving the
response from httpClient.Do(req), add a validation check to ensure the status
code is in the 2xx range (such as checking if resp.StatusCode is between 200 and
299). If the status code indicates an error, return an appropriate error message
that includes the actual status code and response body context rather than
proceeding to decode invalid JSON, which would produce misleading error
messages.
Comment on lines
+216
to
+224
| if ! grep -q "NetworkKeeper networkkeeper.Keeper" "$APP_GO"; then | ||
| echo "[patch-app-wiring] ERROR: NetworkKeeper field missing in app.go!" >&2 | ||
| errors=$((errors + 1)) | ||
| fi | ||
|
|
||
| if ! grep -q "&app.NetworkKeeper" "$APP_GO"; then | ||
| echo "[patch-app-wiring] ERROR: NetworkKeeper not injected in app.go!" >&2 | ||
| errors=$((errors + 1)) | ||
| fi |
Contributor
There was a problem hiding this comment.
Make NetworkKeeper presence checks whitespace-tolerant.
The fixed-string grep ("NetworkKeeper networkkeeper.Keeper") is formatting-sensitive; tabs or aligned spacing in app.go can trigger false negatives, which then causes duplicate insertion (Line 162 path) or false validation failures here. Use a regex like NetworkKeeper[[:space:]]+networkkeeper\.Keeper for both insertion guard and final validation.
Suggested patch
-if ! grep -q "NetworkKeeper networkkeeper.Keeper" "$APP_GO"; then
+if ! grep -Eq "NetworkKeeper[[:space:]]+networkkeeper\.Keeper" "$APP_GO"; then
echo "[patch-app-wiring] Adding NetworkKeeper field"
...
fi
@@
-if ! grep -q "NetworkKeeper networkkeeper.Keeper" "$APP_GO"; then
+if ! grep -Eq "NetworkKeeper[[:space:]]+networkkeeper\.Keeper" "$APP_GO"; then
echo "[patch-app-wiring] ERROR: NetworkKeeper field missing in app.go!" >&2
errors=$((errors + 1))
fi📝 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
| if ! grep -q "NetworkKeeper networkkeeper.Keeper" "$APP_GO"; then | |
| echo "[patch-app-wiring] ERROR: NetworkKeeper field missing in app.go!" >&2 | |
| errors=$((errors + 1)) | |
| fi | |
| if ! grep -q "&app.NetworkKeeper" "$APP_GO"; then | |
| echo "[patch-app-wiring] ERROR: NetworkKeeper not injected in app.go!" >&2 | |
| errors=$((errors + 1)) | |
| fi | |
| if ! grep -Eq "NetworkKeeper[[:space:]]+networkkeeper\.Keeper" "$APP_GO"; then | |
| echo "[patch-app-wiring] ERROR: NetworkKeeper field missing in app.go!" >&2 | |
| errors=$((errors + 1)) | |
| fi | |
| if ! grep -q "&app.NetworkKeeper" "$APP_GO"; then | |
| echo "[patch-app-wiring] ERROR: NetworkKeeper not injected in app.go!" >&2 | |
| errors=$((errors + 1)) | |
| fi |
🤖 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/docker/patches/app-wiring/patch-app-wiring.sh` around lines
216 - 224, The grep patterns checking for NetworkKeeper presence are too strict
with fixed string matching, causing false negatives when app.go has different
whitespace formatting (tabs vs spaces). Replace the exact string patterns
"NetworkKeeper networkkeeper.Keeper" and "&app.NetworkKeeper" with regex
patterns using [[:space:]]+ to match any amount of whitespace between tokens.
Update both the grep commands on lines 216 and 220 to use grep -qE (extended
regex) with patterns like "NetworkKeeper[[:space:]]+networkkeeper\.Keeper" and
"&app\.NetworkKeeper" respectively, ensuring the dot in the namespace is escaped
as a literal character in the regex.
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
1 participant
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.
Adds a Query/Attesters endpoint and uses registered attester consensus pubkeys to build CometBFT validator sets for attester-mode light-client RPC responses.
Updates block/header RPC conversion paths to apply the attester validator set consistently and verifies commits against attester-signed BlockIDs.
Refactors the attester command to fetch block header and BlockID from a single RPC response while preserving existing wrappers, and keeps the integration attester consensus key distinct from the operator key.
Tests:
go test ./pkg/rpc/core ./server ./modules/network/keeper -count=1andcd tests/integration && go test ./... -run TestNewDistinctAttesterPrivValidatorFilesUsesDifferentKey -count=1.Closes #271
Summary by CodeRabbit
Release Notes
New Features
AttestersgRPC/API query endpoint to list registered attesters and metadata.Bug Fixes
Tests