feat: fix IBC #2301
feat: fix IBC #2301
Conversation
WalkthroughThis update introduces significant refactoring and extensibility to the block manager and execution layers. It adds dependency injection for cryptographic hashing and signature payload generation, updates the executor interface to accept metadata, and propagates these changes throughout the codebase and tests. Numerous mocks, tests, and helper functions are revised for compatibility, and new utilities for hashing and signature payloads are introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant BlockManager
participant Executor
participant Hasher
participant SignatureProvider
BlockManager->>Hasher: Compute ValidatorHash/CommitHash
BlockManager->>SignatureProvider: Get signature payload for header
BlockManager->>BlockManager: Sign header and validate with payload provider
BlockManager->>Executor: ExecuteTxs(ctx, txs, height, ts, prevRoot, metadata)
Executor-->>BlockManager: updatedStateRoot, maxBytes, error
BlockManager->>BlockManager: Apply block, update state, broadcast
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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (13)
✨ 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 (
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
|
could you add a description to the pr with reasoning to the design? |
|
![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
♻️ Duplicate comments (3)
block/manager.go (3)
851-861: Inconsistent error handling for missing validatorHasher.The code logs and continues with a zero hash when
validatorHasheris missing, while other providers (likesignaturePayloadProvider) return errors. This inconsistency could lead to subtle bugs.
869-869: Avoid hardcoding block version.The block version should not be hardcoded. Consider making this configurable or deriving it from the state/genesis configuration.
907-912: Add nil check for commitHashProvider.The code assumes
commitHashProvideris always available but doesn't validate this assumption, which could lead to a panic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/evm/based/go.mod(1 hunks)apps/evm/single/go.mod(1 hunks)block/manager.go(19 hunks)block/manager_test.go(5 hunks)block/retriever.go(1 hunks)block/retriever_test.go(1 hunks)core/da/dummy_test.go(1 hunks)core/execution/dummy_test.go(4 hunks)go.mod(1 hunks)node/full.go(4 hunks)pkg/cmd/run_node.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- go.mod
- apps/evm/based/go.mod
- pkg/cmd/run_node.go
- apps/evm/single/go.mod
- core/execution/dummy_test.go
- core/da/dummy_test.go
- block/retriever_test.go
- block/manager_test.go
- node/full.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
block/manager.go (9)
types/hashing.go (2)
ValidatorHasher(64-64)CommitHashProvider(78-78)types/signer.go (1)
SignaturePayloadProvider(9-9)core/sequencer/sequencing.go (1)
Batch(33-35)types/header.go (2)
Hash(14-14)Header(35-62)types/signed_header.go (1)
SignedHeader(22-27)types/block.go (3)
Data(33-36)Signature(46-46)Version(16-19)block/errors.go (1)
ErrNoBatch(16-16)types/utils.go (1)
GetSignature(293-299)types/metadata_keys.go (1)
HeaderHashKey(5-5)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test / Run Unit Tests
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (8)
block/retriever.go (1)
121-121: LGTM! Signature payload provider integration is correct.The change correctly passes the injected
signaturePayloadProvidertoValidateBasic, ensuring consistent signature validation with the new pluggable cryptographic provider design.block/manager.go (7)
108-110: LGTM! Dependency injection fields are properly defined.The new cryptographic provider fields enable flexible injection of hashing and signature logic, improving modularity and testability.
175-197: LGTM! Clean functional options pattern implementation.The
ManagerOptionpattern provides a clean and extensible way to configure the cryptographic providers during Manager construction.
1010-1015: LGTM! Proper error handling for missing signaturePayloadProvider.The explicit nil check and clear error message provide good error handling when the required provider is not configured.
556-592: LGTM! Well-structured block data retrieval logic.The extracted
getBlockDatamethod improves code organization and readability by separating concerns for pending blocks and batch retrieval.
594-692: LGTM! Improved block publishing flow with better separation of concerns.The refactored
publishBlockInternalmethod with helper functions improves readability and maintainability while preserving the core logic.
923-927: LGTM! Metadata integration for ExecuteTxs.The addition of metadata containing the header hash enables the execution layer to access block-specific information during transaction processing, which is essential for IBC functionality.
792-792: LGTM! Consistent signature payload provider usage.The updated
ValidateBasiccalls correctly pass thesignaturePayloadProvider, ensuring consistent signature validation across the codebase.Also applies to: 639-639
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
Requesting chnages to avoid the merge. I think we will need some changes with the attester system
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
execution/evm/execution_test.go (1)
121-122: Test coverage for metadata functionality could be enhanced.The metadata parameter is initialized as empty but not tested for any specific behavior. Consider adding test assertions to verify metadata handling if it's critical for IBC functionality.
metadata := make(map[string]interface{}) newStateRoot, maxBytes, err := executionClient.ExecuteTxs(ctx, payload, blockHeight, time.Now(), prevStateRoot, metadata) +// Consider adding assertions to verify metadata population if relevant +// require.Contains(tt, metadata, "expectedKey", "metadata should contain expected data")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/testapp/cmd/run.go(2 hunks)block/manager.go(19 hunks)block/retriever_test.go(1 hunks)execution/evm/execution.go(1 hunks)execution/evm/execution_test.go(2 hunks)pkg/cmd/run_node.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/cmd/run_node.go
- execution/evm/execution.go
- block/retriever_test.go
- block/manager.go
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: lint / golangci-lint
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
apps/testapp/cmd/run.go (2)
13-13: LGTM: Imports added for new functionality.The new imports for
blockandtypespackages are necessary to support the new provider options being passed toStartNode.Also applies to: 21-21
111-122: LGTM: Default providers properly configured for testapp.The addition of
WithSignaturePayloadProviderandWithCommitHashProvideroptions with default implementations aligns well with the PR objectives to support IBC functionality through provider injection. Using default implementations is appropriate for a test application.execution/evm/execution_test.go (1)
182-183: Consistent metadata handling across test phases.Good consistency with the build chain phase. The empty metadata approach is appropriate for integration testing focused on execution flow rather than metadata-specific behavior.
alpe
left a comment
alpe
left a comment
There was a problem hiding this comment.
quick feedback. I have not read all the code yet.
| return fmt.Errorf("error while getting previous block data: %w", err) | ||
| } | ||
|
|
||
| if err = m.store.SaveBlockData(ctx, header, data, &signature); err != nil { |
There was a problem hiding this comment.
I could not see where you store the block before applyBlock is called.
When it fails for whatever reason, the data is lost and a new block is created on restart rather than loaded from store as before.
|
|
||
| // WithValidatorHasher sets the validator hasher provider | ||
| func WithValidatorHasher(hasher types.ValidatorHasher) ManagerOption { | ||
| return func(m *Manager) { |
There was a problem hiding this comment.
personal preference: this and the other Options: prevent nil values -> fail fast
block/manager.go
Outdated
|
|
||
| // previousBlockData holds information about the previous block, | ||
| // necessary for creating the next block. | ||
| type previousBlockData struct { |
There was a problem hiding this comment.
I understand that this represents historic block data but the term "previous" feels a bit too technical to the process. I know naming is hard but maybe you can find a term that is more focused on what this data represents or how it is used?
| // - blockHeight: Height of block being created (must be > 0) | ||
| // - timestamp: Block creation time in UTC | ||
| // - prevStateRoot: Previous block's state root hash | ||
| // - metadata: Arbitrary data passed from the block manager (e.g., current header hash) |
There was a problem hiding this comment.
nit: Arbitrary data sounds very weak. When used, are there any guarantees what data is available?
| } | ||
|
|
||
| // SetHeaderHasher allows different implementations to override the default header hasher | ||
| func SetHeaderHasher(hasher HeaderHasher) { |
There was a problem hiding this comment.
personal preference: I am with Julien on this global. The default hasher is stateless. Can you evaluate why this is needed? I have not found a call to SetHeaderHasher in this project. Is this an extension point? Some context would be great in Godoc as well
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
block/sync.go (1)
119-123: Consistent error handling in data event processing.The error handling here matches the header event processing, maintaining consistency. The same architectural considerations about error classification apply here as well.
Consider implementing a more sophisticated error handling strategy that:
- Categorizes errors by type (recoverable vs. non-recoverable)
- Implements exponential backoff for retryable errors
- Includes circuit breaker patterns for persistent failures
// Try to sync the next block, as this data event might have completed a block if err := m.trySyncNextBlock(ctx, dataEvent.DAHeight); err != nil { m.logger.Error("error while trying to sync next block", "error", err) + // Consider error classification here + if isRecoverableError(err) { + // Implement retry logic with backoff + } errCh <- err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
block/sync.go(2 hunks)types/utils.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- types/utils.go
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Run EVM Execution Tests
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Analyze (go)
- GitHub Check: Summary
| // Try to sync the next block, in case the corresponding data was already present | ||
| if err := m.trySyncNextBlock(ctx, daHeight); err != nil { | ||
| m.logger.Error("error while trying to sync next block", "error", err) | ||
| errCh <- err | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Improved error resilience in header event processing.
The error handling change allows the sync loop to continue processing after trySyncNextBlock failures, which improves system resilience. However, consider whether all error types should be handled uniformly.
Some errors from trySyncNextBlock (like validation failures) might indicate corrupted data that could repeatedly fail, while others (like temporary network issues) might be recoverable. Consider implementing error classification to distinguish between recoverable and non-recoverable errors.
🏁 Script executed:
#!/bin/bash
# Verify what types of errors trySyncNextBlock can return
ast-grep --pattern 'func (m *Manager) trySyncNextBlock($$$) error {
$$$
}'Length of output: 2786
Error Classification for trySyncNextBlock Failures
trySyncNextBlock currently returns the following error types:
ctx.Err()on context cancellation- Storage errors from
m.store.Heightandm.store.SetHeight nilwhen header or data aren’t yet in the cache- Wrapped
fmt.Errorferrors for:- validation (
failed to validate block) - application (
failed to apply block) - state save (
failed to save updated state) - block save (
failed to save block)
- validation (
Handling all of these uniformly can lead to infinite retries on non-recoverable failures (e.g. persistent validation errors). To improve robustness, introduce error classification:
- Define sentinel error variables or custom error types, for example:
var ( ErrBlockValidation = errors.New("block validation failed") ErrBlockApplication = errors.New("block application failed") )
- Wrap errors in
trySyncNextBlockwith these sentinels:if err := m.Validate(...); err != nil { return fmt.Errorf("%w: %v", ErrBlockValidation, err) }
- In the sync loop (lines 62–66 and 119–123), use
errors.Isorerrors.Asto distinguish:- Non-recoverable (e.g.
ErrBlockValidation): log and skip or terminate - Recoverable (e.g. transient storage errors): log and continue
- Non-recoverable (e.g.
This ensures permanent block errors don’t cause endless retries, while recoverable errors still allow the loop to progress.
🤖 Prompt for AI Agents
In block/sync.go around lines 62 to 66, improve error handling for
trySyncNextBlock by classifying errors into recoverable and non-recoverable
types. Define sentinel error variables for validation and application failures,
wrap corresponding errors in trySyncNextBlock with these sentinels, and then
update the sync loop to check errors using errors.Is or errors.As. For
non-recoverable errors like validation failures, log and skip or terminate
processing to avoid infinite retries; for recoverable errors, log and continue
the loop to maintain resilience.
|
Superseded by #2346 |
Superseed #2301 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for custom signature verification logic on block headers, allowing advanced users to specify how header signatures are verified. - Enhanced context handling to provide block header information during transaction execution. - **Bug Fixes** - Improved context handling to make block header information available during transaction execution. - **Chores** - Updated the initial block version to 11 for improved compatibility with external blockchains. - Minor test and code cleanups with no impact on functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Superseed #2301 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for custom signature verification logic on block headers, allowing advanced users to specify how header signatures are verified. - Enhanced context handling to provide block header information during transaction execution. - **Bug Fixes** - Improved context handling to make block header information available during transaction execution. - **Chores** - Updated the initial block version to 11 for improved compatibility with external blockchains. - Minor test and code cleanups with no impact on functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Superseed #2301 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for custom signature verification logic on block headers, allowing advanced users to specify how header signatures are verified. - Enhanced context handling to provide block header information during transaction execution. - **Bug Fixes** - Improved context handling to make block header information available during transaction execution. - **Chores** - Updated the initial block version to 11 for improved compatibility with external blockchains. - Minor test and code cleanups with no impact on functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Overview
Fixing IBC on Rollkit Post-Refactor
Following a refactor that extracted execution-specific code, Rollkit needs to operate without direct dependencies on underlying execution layers (e.g., CometBFT for ABCI). While this improves the design, it introduces challenges when Rollkit needs data or hashes whose generation logic is specific to the execution layer.
To address this, Rollkit relies on injected functions provided by the execution layer for implementation-specific operations.
Implementation-Specific Logic and Injected Providers
The following fields or computations require logic from the specific execution layer. This is managed by injecting provider functions:
Header Hash
The calculation of a block header's hash is implementation-dependent. Rollkit's Header type doesn't have a Hash() method (though it contains lastHeaderHash). To obtain the header hash without Rollkit depending directly on the execution layer's hashing logic, a HeaderHasher function is injected:
This function is supplied by the execution layer and used by the manager when the header hash is needed.
Validator Hash
The SignedHeader.ValidatorHash is also specific to the execution layer. For instance, CometBFT generates this hash using its ValidatorSet primitive, which is unknown to Rollkit. An injected ValidatorHasher provides this:
SignaturePayloadProvider
The precise data payload that a private key signs for a commit can vary by execution layer. To handle this, a SignaturePayloadProvider is injected:
CommitHashProvider
While the Rollkit Header includes a LastCommitHash, Rollkit itself doesn't know how to generate this hash, as the method is implementation-specific. A CommitHashProvider is injected for this purpose:
Header Hash on ExecuteTxs
The original signature for ExecuteTxs in the execution layer interface did not provide a way to access the Header.Hash during transaction processing:
Given that the header hash calculation is implementation-dependent, and the Header object itself was neither passed as an argument to ExecuteTxs nor finalized until after this function completed, retrieving the hash at this stage was not possible.
To address this, a metadata map[string]interface{} parameter was added to the ExecuteTxs signature. This serves as a mechanism for the execution layer to return necessary data, such as components required for the header hash or the hash itself, back to Rollkit:
While using a generic map means Rollkit might need to be aware of specific keys to retrieve arbitrary data (which is not strictly aligned with the Open/Closed Principle of SOLID), this approach was chosen as a pragmatic compromise to avoid over-engineering the solution solely for obtaining this field. A key benefit of this approach is the ability to return additional data fields in the future without changing the execution.ExecuteTxs interface signature, thereby avoiding breaking changes for other implementations.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores