chore: Use stored block data instead #90
Conversation
WalkthroughThe changes refactor ABCI conversion utilities by moving them from Changes
Sequence Diagram(s)sequenceDiagram
participant RPC as RPC Layer
participant Common as Common Convert Utilities
participant RollkitStore as Rollkit Store
participant Merkle as Merkle Utilities
participant ABCI as ABCI Types
RPC->>Common: ToABCIBlockMeta(header, data)
Common->>Common: ToABCIBlock(header, data)
Common->>Common: ToABCIHeader(header)
Common->>ABCI: Construct ABCI BlockMeta
Common-->>RPC: Return ABCI BlockMeta
RPC->>RollkitStore: GetBlockData(height)
RollkitStore-->>RPC: Return block data with txs
RPC->>Merkle: Compute Merkle root and proof for tx
Merkle-->>RPC: Return TxProof
RPC-->>Client: Return Tx with proof (if requested)
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| if len(invalidHexStr) < NodeIDByteLength*2 { | ||
| invalidHexStr = strings.Repeat("x", NodeIDByteLength*2) // Make it long enough but still invalid hex | ||
| } | ||
| require.GreaterOrEqual(t, len(invalidHexStr), NodeIDByteLength*2) |
There was a problem hiding this comment.
more readable and fails fast when const is changed
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/common/convert.go (5)
14-16: Documentation mismatch with implementation.The function comment states that the caller should fill fields like ChainID, but the implementation already sets ChainID at line 38 from
header.ChainID(). Please update the documentation to reflect the actual behavior.// ToABCIHeader converts Rollkit header to Header format defined in ABCI. -// Caller should fill all the fields that are not available in Rollkit header (like ChainID). +// The function attempts to fill all fields from the Rollkit header.
39-40: Consider documenting why ValidatorsHash and NextValidatorsHash use the same value.Both fields are set to the same value (
header.ValidatorHash). While this might be intentional for your use case, it's worth documenting why, especially since in typical blockchain implementations these could be different when validator sets change.
52-55: Verify the error handling for missing proposer address.Good defensive programming by checking for an empty proposer address. Consider adding a more specific error message that includes context about which block height is affected to aid in debugging.
if len(header.ProposerAddress) == 0 { - return nil, errors.New("proposer address is not set") + return nil, fmt.Errorf("proposer address is not set for block at height %d", header.Height()) }
95-112: Documentation doesn't match implementation.The function comment states that the caller needs to fill fields like ValidatorAddress and Timestamp, but the implementation already sets these fields. Please update the documentation to accurately reflect what the function does vs. what the caller needs to provide.
// ToABCICommit returns a commit format defined by ABCI. -// Other fields (especially ValidatorAddress and Timestamp of Signature) have to be filled by caller. +// The function sets ValidatorAddress and Timestamp based on the provided parameters.
46-93: Consider adding nil checks for pointer parameters.The
ToABCIBlockandToABCIBlockMetafunctions take pointer parameters (headeranddata) but don't check if they're nil. Consider adding defensive nil checks to avoid potential panics.func ToABCIBlock(header *rlktypes.SignedHeader, data *rlktypes.Data) (*cmttypes.Block, error) { + if header == nil { + return nil, errors.New("header cannot be nil") + } + if data == nil { + return nil, errors.New("data cannot be nil") + } abciHeader, err := ToABCIHeader(&header.Header) // Rest of the function... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/adapter/adapter_test.go(4 hunks)pkg/common/convert.go(1 hunks)pkg/rpc/core/blocks.go(7 hunks)pkg/rpc/core/utils.go(2 hunks)pkg/rpc/core/utils_test.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/rpc/core/utils_test.go (1)
pkg/rpc/core/utils.go (2)
NodeIDByteLength(14-14)TruncateNodeID(97-106)
pkg/rpc/core/blocks.go (1)
pkg/common/convert.go (3)
ToABCIBlock(46-77)ToABCICommit(97-112)ToABCIBlockMeta(80-93)
pkg/rpc/core/utils.go (1)
pkg/common/convert.go (1)
ToABCIBlockMeta(80-93)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test with Rollkit Chain
🔇 Additional comments (24)
pkg/rpc/core/utils_test.go (5)
11-11: LGTM: Adding require package for stronger test assertionsThe addition of the require package is appropriate as it allows failing fast when assertions don't pass.
25-25: Stronger assertion improves test reliabilityChanging from
assert.NoErrortorequire.NoErrorensures the test fails immediately if this critical check doesn't pass, preventing misleading follow-up errors.
41-41: More readable assertion for input validationUsing
require.GreaterOrEqualdirectly is cleaner than the previous conditional approach, making the minimum length requirement explicit and failing fast when the test constant is changed.
43-43: Consistent use of require for error checkingGood change to use
require.Errorfor consistency with other assertions that use the require package.
57-57: Improved assertion for exact length test caseUsing
require.NoErroris appropriate here as subsequent assertions depend on this check passing.pkg/rpc/core/blocks.go (8)
17-18: LGTM: Using the common package for ABCI type conversionGood refactoring to use the common package for ABCI type conversions, which helps prevent cyclic dependencies.
74-74: Proper update to use common package conversion functionCorrectly updated to use
common.ToABCIBlockinstead of the local implementation.
119-119: Consistent conversion with common packageThe conversion to ABCI block now uses the centralized implementation from the common package.
143-143: Using common conversion for block by hashUpdated
BlockByHashto use the common implementation for proper consistency.
176-176: Consistent commit conversion using common packageThe commit conversion is now using the centralized implementation from the common package.
178-178: Block conversion for commit also updatedBlock conversion for commit response now uses the common implementation.
243-243: Updated header by hash to use common conversionThe block meta conversion in
HeaderByHashnow correctly uses the common implementation.
285-285: Blockchain info now uses common conversionThe block meta conversion in
BlockchainInfonow uses the common implementation, maintaining consistency across all conversion points.pkg/rpc/core/utils.go (2)
11-11: LGTM: Added common package importCorrectly added import for the common package where conversion functions have been moved.
55-55: Updated to use common package implementationGood update to use
common.ToABCIBlockMetafrom the common package instead of the local implementation, centralizing conversion logic.pkg/adapter/adapter_test.go (6)
23-23: LGTM: Added rollkit types import for header fixtureThis import is needed for the new header fixture helper function.
77-77: Improved test readability with mockApp helperUsing the new
mockApphelper function makes the test setup cleaner and more maintainable.
80-84: Simplified adapter setup for testingThe adapter setup is now more concise and clearer, with direct field assignments after initialization.
85-86: Added block data setup for testsGood addition of saving block data using the
headerFixture, making the test more realistic.
134-150: Well-structured mockApp helper functionThe new helper function provides a clean way to create a mock ABCI app with default implementations that can be customized through the mutator functions. This improves code reuse and test readability.
179-187: Useful headerFixture for creating test headersThe new helper function creates a consistent test header fixture, improving code reuse and readability. Good use of the types imported from rollkit.
pkg/common/convert.go (3)
44-46: Good implementation with proper validation.The function correctly converts Rollkit block components to ABCI block format and includes validation to ensure the returned block meets requirements.
79-93: Well-structured with good error handling.The
ToABCIBlockMetafunction properly reuses theToABCIBlockfunction and includes appropriate error handling. The implementation is clean and efficient.
3-12: Clean import organization.The imports are well-organized with appropriate grouping between standard library packages, external dependencies, and internal packages. This follows Go best practices.
| // This assumes that we have only one signature | ||
| if len(abciCommit.Signatures) == 1 { | ||
| abciCommit.Signatures[0].ValidatorAddress = header.ProposerAddress | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Hardcoded assumption about signature count.
The code assumes there will only be one signature, which is currently correct but may not remain true if requirements change. Consider making this more robust by iterating through signatures or documenting this assumption clearly.
🏁 Script executed:
#!/bin/bash
# Check if there are any Rollkit files that handle multiple signatures
rg -A 2 -B 2 "[]signature" --type go -g "*.go" Length of output: 122
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of the Signatures field across Go files
rg -n "Signatures" -g "*.go"Length of output: 518
Avoid hardcoding single‐signature assumption
The code currently assumes there will only ever be one signature in Commit.Signatures, which could break if multi‐signature support is added. This pattern appears in:
-
pkg/common/convert.go
- Lines 60–62:
// This assumes that we have only one signature if len(abciCommit.Signatures) == 1 { abciCommit.Signatures[0].ValidatorAddress = header.ProposerAddress }
- Line 105:
Signatures: []cmttypes.CommitSig{{ /* … */ }},
- Lines 60–62:
-
pkg/rpc/utils.go
- Lines 91–92:
if len(abciCommit.Signatures) == 1 { abciCommit.Signatures[0].ValidatorAddress = header.ProposerAddress }
- Line 136:
Signatures: make([]cmttypes.CommitSig, 1),
- Line 144:
tmCommit.Signatures[0] = commitSig
- Lines 91–92:
Rather than special-casing a single signature, consider uniformly handling all signatures. For example, replace the one-off check/assignment with:
- if len(abciCommit.Signatures) == 1 {
- abciCommit.Signatures[0].ValidatorAddress = header.ProposerAddress
- }
+ for i := range abciCommit.Signatures {
+ abciCommit.Signatures[i].ValidatorAddress = header.ProposerAddress
+ }And update the static slice constructions to collect or initialize all signatures dynamically. If single-signature is an explicit protocol limitation, add a clear comment/TODO in one shared helper to document this behavior.
| func proofTXExists(txs []rlktypes.Tx, i uint32) cmttypes.TxProof { | ||
| hl := hashList(txs) | ||
| root, proofs := merkle.ProofsFromByteSlices(hl) | ||
|
|
||
| return cmttypes.TxProof{ | ||
| RootHash: root, | ||
| Data: cmttypes.Tx(txs[i]), | ||
| Proof: *proofs[i], | ||
| } |
There was a problem hiding this comment.
The proofTXExists function needs a bounds check before accessing txs[i] to prevent a panic when the transaction index is out of bounds. If a client requests a proof for a non-existent transaction index, the server would crash. Consider adding validation like:
if int(i) >= len(txs) {
// Return an error or handle the out-of-bounds case appropriately
}This would make the function more robust against invalid input parameters.
| func proofTXExists(txs []rlktypes.Tx, i uint32) cmttypes.TxProof { | |
| hl := hashList(txs) | |
| root, proofs := merkle.ProofsFromByteSlices(hl) | |
| return cmttypes.TxProof{ | |
| RootHash: root, | |
| Data: cmttypes.Tx(txs[i]), | |
| Proof: *proofs[i], | |
| } | |
| func proofTXExists(txs []rlktypes.Tx, i uint32) (cmttypes.TxProof, error) { | |
| if int(i) >= len(txs) { | |
| return cmttypes.TxProof{}, fmt.Errorf("transaction index %d out of bounds (len=%d)", i, len(txs)) | |
| } | |
| hl := hashList(txs) | |
| root, proofs := merkle.ProofsFromByteSlices(hl) | |
| return cmttypes.TxProof{ | |
| RootHash: root, | |
| Data: cmttypes.Tx(txs[i]), | |
| Proof: *proofs[i], | |
| }, nil | |
| } | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pkg/rpc/core/tx.go (1)
108-113: Avoid repeatedGetBlockDatacalls for the same block
TxSearchinvokesbuildProoffor every tx, which in turn hits the Rollkit store every time.
When a result set contains multiple transactions from the same height (common case), this becomes anO(n²)pattern on the store.Consider memoising proofs or at least the
DatabyblockHeightwithin the loop:dataCache := map[int64]*rlktypes.Data{} ... if prove { d, ok := dataCache[r.Height] if !ok { _, d, err = env.Adapter.RollkitStore.GetBlockData(ctx.Context(), uint64(r.Height)) if err != nil { ... } dataCache[r.Height] = d } proof = proofTXExists(d.Txs, r.Index) }This keeps complexity
O(n)and drastically cuts I/O.pkg/rpc/core/tx_test.go (2)
74-88:assert.Empty(result.Proof.Proof)may give false positives
merkle.Proof{}is notnil;assert.Emptyonly checks length/zero-value on certain types.
Useassert.Zero(result.Proof)or explicitly compare to the zero struct to avoid silent failures.
118-120: Stale TODO commentThe proof logic is now implemented and tested—this TODO is obsolete and could confuse future readers. Please remove it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/rpc/core/tx.go(4 hunks)pkg/rpc/core/tx_test.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/rpc/core/tx_test.go (4)
pkg/rpc/core/mocks_test.go (1)
MockRollkitStore(113-115)pkg/rpc/core/env.go (1)
Environment(27-33)pkg/adapter/adapter.go (1)
Adapter(53-69)pkg/rpc/core/tx.go (2)
Tx(22-49)TxSearch(54-125)
pkg/rpc/core/tx.go (1)
pkg/adapter/adapter.go (1)
Adapter(53-69)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test / Run Unit Tests
- GitHub Check: lint / golangci-lint
- GitHub Check: Test with Rollkit Chain
🔇 Additional comments (1)
pkg/rpc/core/tx_test.go (1)
233-278: Test duplicates store calls; consider caching proof generationThe
with proofssub-test expects three separateGetBlockDatacalls, even two for the same height.
This mirrors the current implementation, but it couples the test to an inefficient API usage pattern.
Refactoring production code to cache data (see previous comment) will break this test.Instead, set expectations per height, e.g.:
mockStore. On("GetBlockData", mock.Anything, uint64(10)). Return(nil, dataH10, nil).Twice() // if you really need two callsor verify via
AssertNumberOfCallsafter the fact.
This keeps the test robust against internal optimisations.
| func buildProof(ctx *rpctypes.Context, blockHeight int64, txIndex uint32) (cmttypes.TxProof, error) { | ||
| _, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), uint64(blockHeight)) | ||
| if err != nil { | ||
| return cmttypes.TxProof{}, fmt.Errorf("failed to get block data: %w", err) | ||
| } | ||
| return proofTXExists(data.Txs, txIndex), nil | ||
| } |
There was a problem hiding this comment.
Validate fetched block data and index bounds to avoid panics
buildProof blindly assumes that
- the Rollkit store returns non-nil
Data, Data.Txsis non-empty, andtxIndexis within bounds.
Any of these assumptions being violated will lead to a runtime panic inside proofTXExists (slice index out of range). Please guard against these cases and surface a descriptive error instead.
func buildProof(ctx *rpctypes.Context, blockHeight int64, txIndex uint32) (cmttypes.TxProof, error) {
- _, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), uint64(blockHeight))
- if err != nil {
- return cmttypes.TxProof{}, fmt.Errorf("failed to get block data: %w", err)
- }
- return proofTXExists(data.Txs, txIndex), nil
+ _, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), uint64(blockHeight))
+ if err != nil {
+ return cmttypes.TxProof{}, fmt.Errorf("failed to get block data: %w", err)
+ }
+ if data == nil {
+ return cmttypes.TxProof{}, fmt.Errorf("block %d has no data section", blockHeight)
+ }
+ if txIndex >= uint32(len(data.Txs)) {
+ return cmttypes.TxProof{}, fmt.Errorf(
+ "tx index %d out of bounds (block contains %d txs)", txIndex, len(data.Txs),
+ )
+ }
+ return proofTXExists(data.Txs, txIndex), 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.
| func buildProof(ctx *rpctypes.Context, blockHeight int64, txIndex uint32) (cmttypes.TxProof, error) { | |
| _, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), uint64(blockHeight)) | |
| if err != nil { | |
| return cmttypes.TxProof{}, fmt.Errorf("failed to get block data: %w", err) | |
| } | |
| return proofTXExists(data.Txs, txIndex), nil | |
| } | |
| func buildProof(ctx *rpctypes.Context, blockHeight int64, txIndex uint32) (cmttypes.TxProof, error) { | |
| _, data, err := env.Adapter.RollkitStore.GetBlockData(ctx.Context(), uint64(blockHeight)) | |
| if err != nil { | |
| return cmttypes.TxProof{}, fmt.Errorf("failed to get block data: %w", err) | |
| } | |
| if data == nil { | |
| return cmttypes.TxProof{}, fmt.Errorf("block %d has no data section", blockHeight) | |
| } | |
| if txIndex >= uint32(len(data.Txs)) { | |
| return cmttypes.TxProof{}, fmt.Errorf( | |
| "tx index %d out of bounds (block contains %d txs)", txIndex, len(data.Txs), | |
| ) | |
| } | |
| return proofTXExists(data.Txs, txIndex), nil | |
| } |
| func proofTXExists(txs []rlktypes.Tx, i uint32) cmttypes.TxProof { | ||
| hl := hashList(txs) | ||
| root, proofs := merkle.ProofsFromByteSlices(hl) | ||
|
|
||
| return cmttypes.TxProof{ | ||
| RootHash: root, | ||
| Data: cmttypes.Tx(txs[i]), | ||
| Proof: *proofs[i], | ||
| } | ||
| } |
There was a problem hiding this comment.
proofTXExists still needs an internal bounds check
Even after external validation, this helper should remain defensive to prevent misuse from other call-sites that might appear in the future.
func proofTXExists(txs []rlktypes.Tx, i uint32) cmttypes.TxProof {
+ if int(i) >= len(txs) {
+ return cmttypes.TxProof{} // defensive default
+ }
hl := hashList(txs)
root, proofs := merkle.ProofsFromByteSlices(hl)
return cmttypes.TxProof{
RootHash: root,
Data: cmttypes.Tx(txs[i]),
Proof: *proofs[i],
}
}📝 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.
| func proofTXExists(txs []rlktypes.Tx, i uint32) cmttypes.TxProof { | |
| hl := hashList(txs) | |
| root, proofs := merkle.ProofsFromByteSlices(hl) | |
| return cmttypes.TxProof{ | |
| RootHash: root, | |
| Data: cmttypes.Tx(txs[i]), | |
| Proof: *proofs[i], | |
| } | |
| } | |
| func proofTXExists(txs []rlktypes.Tx, i uint32) cmttypes.TxProof { | |
| if int(i) >= len(txs) { | |
| return cmttypes.TxProof{} // defensive default | |
| } | |
| hl := hashList(txs) | |
| root, proofs := merkle.ProofsFromByteSlices(hl) | |
| return cmttypes.TxProof{ | |
| RootHash: root, | |
| Data: cmttypes.Tx(txs[i]), | |
| Proof: *proofs[i], | |
| } | |
| } |
| t.Run("Success with proofs", func(t *testing.T) { | ||
| mockTxIndexer.On("Get", sampleHash).Return(sampleTxResult, nil).Once() | ||
| mockStore.On("GetBlockData", mock.Anything, uint64(sampleHeight)).Return(nil, | ||
| &rktypes.Data{Txs: rktypes.Txs{[]byte{0}, []byte{1}}}, nil).Once() | ||
| result, err := Tx(ctx, sampleHash, true) | ||
|
|
||
| result, err := Tx(ctx, sampleHash, false) // prove = false | ||
| require.NoError(err) | ||
| require.NotNil(result) | ||
| assert.Equal(sampleHash, []byte(result.Hash)) | ||
| assert.Equal(sampleHeight, result.Height) | ||
| assert.Equal(sampleIndex, result.Index) | ||
| assert.Equal(sampleResult, result.TxResult) | ||
| assert.Equal(sampleTx, result.Tx) | ||
| assert.Equal(int64(2), result.Proof.Proof.Total) | ||
| assert.Equal(int64(1), result.Proof.Proof.Index) | ||
| assert.NotEmpty(result.Proof.Proof.LeafHash) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mocked block data inconsistent for the same height
Two different GetBlockData expectations are set for height 10 but they return different transactions sets (len=2 vs len=3).
A real block cannot change between calls; this hides potential bugs and weakens the proof verification.
Align both expectations to return identical data or cache the response once.
| mockTxIndexer := new(MockTxIndexer) | ||
| mockStore := new(MockRollkitStore) | ||
|
|
||
| env = &Environment{ | ||
| TxIndexer: mockTxIndexer, | ||
| Logger: cmtlog.NewNopLogger(), | ||
| Adapter: &adapter.Adapter{}, // Minimal adapter needed? Add mocks if GetBlockData is used (for prove=true) | ||
| Adapter: &adapter.Adapter{ | ||
| RollkitStore: mockStore, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Global env mutation can introduce test races
All sub-tests mutate the package-level env singleton.
If other test files in the same package run in parallel (t.Parallel()), this leads to data races and flaky tests.
Prefer creating an Environment per test and injecting it through parameters, or guard access with a mutex.
After #88 to avoid merge conflicts in their branch
Overview
Summary by CodeRabbit