feat: optimize batch data size by emptying IBC update client msg data #65
Conversation
WalkthroughThis pull request introduces several enhancements across multiple files in the project, focusing on improving configuration management, Inter-Blockchain Communication (IBC) support, and testing capabilities. The changes primarily involve adding chain ID support, extending codec and node configurations, and introducing new methods for block and client querying. The modifications span across configuration types, test files, and provider implementations, suggesting a comprehensive update to the system's interoperability and configuration handling. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Node as NodeConfig
participant Host as BaseHost
participant BatchSubmitter as BatchSubmitter
Config->>Node: Set ChainID
Node-->>Config: Validate ChainID
Host->>Host: Implement ChainId() method
BatchSubmitter->>BatchSubmitter: Process block data
BatchSubmitter->>BatchSubmitter: Empty oracle/client data
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
executor/batchsubmitter/data_conversion.go (3)
35-38: Consider logging unregistered transactions for better traceability.In the
emptyOracleDatafunction, when a transaction cannot be decoded:Ignoring the error might make it difficult to debug issues related to unregistered transactions. Consider logging the error to help with future troubleshooting.
132-140: Optimizeslices.IndexFuncusage by simplifying the predicate function.The predicate in
slices.IndexFunccan be simplified for readability:Consider returning the boolean expression directly:
- if signature.ValidatorAddress != nil && - bytes.Equal(blockSig.ValidatorAddress.Bytes(), signature.ValidatorAddress) && - blockSig.Timestamp.Equal(signature.Timestamp) && - bytes.Equal(blockSig.Signature, signature.Signature) && - uint8(blockSig.BlockIDFlag) == uint8(signature.BlockIdFlag) { - return true - } - return false + return signature.ValidatorAddress != nil && + bytes.Equal(blockSig.ValidatorAddress.Bytes(), signature.ValidatorAddress) && + blockSig.Timestamp.Equal(signature.Timestamp) && + bytes.Equal(blockSig.Signature, signature.Signature) && + uint8(blockSig.BlockIDFlag) == uint8(signature.BlockIdFlag)
147-150: Simplify signature index calculation and add bounds check.When constructing
newSig, ensure thatblockSigIndexis within valid range and consider using built-in functions for byte conversion:Refactor to:
+ if blockSigIndex < 0 || blockSigIndex > 0xFFFF { + return nil, errors.New("blockSigIndex out of uint16 range") + } + newSig := make([]byte, 2) + binary.LittleEndian.PutUint16(newSig, uint16(blockSigIndex))e2e/batch_reconstruction_test.go (2)
Line range hint
158-181: Handle potential mismatch between reconstructed and expected blocks.In the loop comparing reconstructed blocks with expected ones:
...If there is a mismatch, it might be challenging to diagnose the cause. Consider adding detailed logging or assertions that compare individual fields for easier debugging.
321-405: Improve error handling and code reuse infillDatafunction.The
fillDatafunction contains repetitive error handling and similar logic for different message types:
- Similar error checks after
UnmarshalToandUnmarshal.- Repetitive marshaling steps for updated messages.
Consider refactoring common logic into helper functions to improve maintainability.
executor/batchsubmitter/common_test.go (2)
41-48: Add validation for negative height in QueryBlock.The
QueryBlockmethod should validate that the height parameter is not negative before accessing the blocks map.Apply this diff to add height validation:
func (m *mockHost) QueryBlock(ctx context.Context, height int64) (*coretypes.ResultBlock, error) { + if height < 0 { + return nil, errors.New("height must be non-negative") + } if block, ok := m.blocks[height]; ok { return &coretypes.ResultBlock{ Block: block, }, nil } return nil, errors.New("block not found") }
50-52: Add validation for SetBlock method.The
SetBlockmethod should validate its parameters to prevent setting invalid blocks.Apply this diff to add parameter validation:
func (m *mockHost) SetBlock(height int64, block *cmttypes.Block) { + if height < 0 { + panic("height must be non-negative") + } + if block == nil { + panic("block cannot be nil") + } m.blocks[height] = block }e2e/reconnect_node_test.go (1)
Line range hint
61-73: Consider making sleep durations configurable.The hardcoded sleep durations could be made configurable to make the test more flexible and maintainable.
Consider defining these as constants at the package level:
+const ( + // Time to wait after pausing a chain + chainPauseWaitTime = 10 * time.Second + // Time to wait after resuming a chain + chainResumeWaitTime = 20 * time.Second +) + func TestReconnectNodes(t *testing.T) { // ... - time.Sleep(10 * time.Second) + time.Sleep(chainPauseWaitTime) // ... - time.Sleep(20 * time.Second) + time.Sleep(chainResumeWaitTime)provider/host/host.go (1)
119-121: Consider renaming method to follow Go naming conventions.The method name
ChainIdshould beChainIDto follow Go's naming conventions for acronyms (ID should be capitalized). This would improve consistency with Go's standard library and common practices.-func (b BaseHost) ChainId() string { +func (b BaseHost) ChainID() string { return b.cfg.ChainID }executor/batchsubmitter/data_conversion_test.go (3)
39-50: Consider adding error handling test cases forcreateAuthzMsg.The helper function
createAuthzMsgcould benefit from test cases that verify error handling whenNewAnyWithValuefails.createAuthzMsg := func(t *testing.T, sender string, msgs []sdk.Msg) *authz.MsgExec { + t.Helper() msgsAny := make([]*cdctypes.Any, 0) for _, msg := range msgs { any, err := cdctypes.NewAnyWithValue(msg) - require.NoError(t, err) + require.NoError(t, err, "failed to create Any value: %v", err) msgsAny = append(msgsAny, any) } return &authz.MsgExec{ Grantee: sender, Msgs: msgsAny, } }
189-193: Add validation for empty clientID increateUpdateClientMsg.The helper function
createUpdateClientMsgshould validate theclientIDparameter to ensure it's not empty.createUpdateClientMsg := func(t *testing.T, clientID string, header *ibctmlightclients.Header, sender string) *ibcclienttypes.MsgUpdateClient { + t.Helper() + require.NotEmpty(t, clientID, "clientID cannot be empty") msg, err := ibcclienttypes.NewMsgUpdateClient(clientID, header, sender) require.NoError(t, err) return msg }
582-583: Consider adding timeout to the context.The context creation for
emptyUpdateClientDatashould include a timeout to prevent potential test hangs.-ctx := types.NewContext(context.TODO(), zap.NewNop(), "").WithPollingInterval(time.Millisecond) +ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) +defer cancel() +ctx = types.NewContext(ctx, zap.NewNop(), "").WithPollingInterval(time.Millisecond)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
challenger/types/config.go(2 hunks)e2e/batch_reconstruction_test.go(9 hunks)e2e/docker.go(1 hunks)e2e/go.mod(1 hunks)e2e/helper.go(8 hunks)e2e/multiple_txs_test.go(1 hunks)e2e/reconnect_node_test.go(1 hunks)executor/batchsubmitter/batch_submitter.go(2 hunks)executor/batchsubmitter/common_test.go(3 hunks)executor/batchsubmitter/data_conversion.go(1 hunks)executor/batchsubmitter/data_conversion_test.go(1 hunks)executor/batchsubmitter/handler.go(1 hunks)executor/batchsubmitter/oracle.go(0 hunks)executor/batchsubmitter/oracle_test.go(0 hunks)executor/types/config.go(3 hunks)go.mod(1 hunks)node/types/config.go(2 hunks)provider/child/child.go(2 hunks)provider/host/host.go(1 hunks)provider/host/query.go(2 hunks)
💤 Files with no reviewable changes (2)
- executor/batchsubmitter/oracle_test.go
- executor/batchsubmitter/oracle.go
🔇 Additional comments (26)
e2e/batch_reconstruction_test.go (1)
425-442: Ensure pagination handles all validators without infinite loops.In
getAllValidators, the loop continues until all validators are fetched:...Ensure that
result.Totalaccurately reflects the total number of validators to prevent potential infinite loops ifresult.Totalis inaccurate.✅ Verification successful
Pagination implementation is safe and follows Cosmos SDK standards
The pagination implementation is using the standard Cosmos SDK client, which provides reliable validator counts and proper pagination handling. The loop termination condition is safe as
result.Totalis computed server-side by the Cosmos SDK node.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for Validators endpoint implementation and related tests rg -g '!{vendor,node_modules}/**' -A 5 'func.*Validators.*context.*height.*page.*perPage' # Search for test files containing validator pagination tests rg -g '**/*test.go' -A 5 'TestValidators.*pagination' # Look for similar pagination patterns rg -g '!{vendor,node_modules}/**' 'result.Total.*len\(' -A 3 -B 3Length of output: 215
Script:
#!/bin/bash # Search for validator-related imports and interfaces rg -g '*.go' -A 3 'import.*validator|interface.*Validator' # Look for validator-related test files fd -e go -x grep -l "validator" {} # Search for client definitions and usage rg -g '*.go' -A 5 'type.*Client.*interface|func.*GetFullNode'Length of output: 1509
node/types/config.go (1)
35-38: Validation for emptyChainIDis appropriate.Adding a check for an empty
ChainIDensures that configurations are complete and prevents potential runtime issues.challenger/types/config.go (1)
116-116: LGTM!The addition of ChainID to both L1NodeConfig and L2NodeConfig is consistent with the NodeConfig struct definition and its validation.
Also applies to: 126-126
e2e/reconnect_node_test.go (1)
57-57: LGTM!The addition of
ibc.CosmosRlyparameter toSetupTestis correct and aligns with the PR's objective of handling IBC functionality.executor/batchsubmitter/batch_submitter.go (1)
13-13: LGTM! Interface changes are well-defined.The additions to the
hostNodeinterface enhance its capabilities by adding chain ID support and block querying functionality. The changes are minimal and focused.Also applies to: 24-26
provider/host/query.go (1)
10-10: LGTM! Clean implementation of block querying.The
QueryBlockimplementation is straightforward and properly delegates to the underlying RPC client while maintaining context propagation.Also applies to: 200-202
e2e/multiple_txs_test.go (2)
62-62: LGTM! Test setup properly configured with IBC relayer.The addition of
ibc.CosmosRlyparameter toSetupTestensures proper configuration of the IBC relayer implementation for the test scenario.
Line range hint
1-1: Verify PR title accuracy.The PR title suggests optimizing batch data size by emptying IBC update client message data, but the changes primarily focus on adding chain ID support and block querying capabilities. Please clarify if there are additional changes planned or if the PR title should be updated to reflect the current changes.
executor/types/config.go (3)
206-206: LGTM!The
ChainIDfield is correctly set from the L1Node configuration.
227-227: LGTM!The
ChainIDfield is correctly set from the L2Node configuration.
248-248: LGTM!The
ChainIDfield is correctly set from the DANode configuration.provider/child/child.go (2)
18-19: LGTM!The IBC-related imports are correctly added with specific versioning.
97-98: LGTM!The IBC interfaces are properly registered with the codec, enabling proper serialization/deserialization of IBC-related types.
e2e/docker.go (1)
497-497: LGTM!The change improves container name handling by using
dockerutil.CondenseHostNameto ensure valid Docker container names.e2e/helper.go (8)
16-17: LGTM!The IBC-related imports are correctly added with specific versioning.
123-124: LGTM!The IBC interfaces are properly registered with the codec for test environment.
137-137: LGTM!The addition of the
relayerImplparameter enhances test flexibility by allowing different IBC relayer implementations.
391-391: LGTM!The relayer is correctly instantiated using the provided implementation.
402-404: LGTM!The IBC client creation options are properly configured with the correct trusting period from the chain configuration.
568-568: LGTM!The RPC address is correctly constructed using the proper host name.
577-577: LGTM!The RPC address is correctly constructed using the proper host name.
586-586: LGTM!The RPC address is correctly constructed using the proper host name.
executor/batchsubmitter/data_conversion_test.go (2)
28-38: LGTM! Test setup is well-structured.The test setup properly initializes the required components (memory DB, codec, test node) and creates a BatchSubmitter instance.
52-142: Test cases cover essential scenarios.The test cases for
emptyOracleDatacover important scenarios:
- Single oracle transaction
- Multiple messages in a transaction
- Mixed transaction types
- Empty transaction list
- Authz transactions
go.mod (1)
16-16: Verify IBC version compatibility.The direct dependency on
github.com/cosmos/ibc-go/v8 v8.5.0has been added. Please ensure this version is compatible with the project's Go version (1.22.5) and other dependencies.✅ Verification successful
IBC v8.5.0 dependency is compatible
The project's dependencies are fully compatible with ibc-go v8.5.0:
- Using compatible Cosmos SDK version (v0.50.10)
- Matching ics23 version (v0.11.0)
- No usage of the breaking changes in connection handshake functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for compatibility issues with IBC v8.5.0 # Check if there are any known issues or breaking changes gh api graphql -f query=' { repository(owner: "cosmos", name: "ibc-go") { release(tagName: "v8.5.0") { description url } issues(first: 5, states: OPEN, labels: ["breaking-change"]) { nodes { title url labels(first: 5) { nodes { name } } } } } }'Length of output: 3378
Script:
#!/bin/bash # Check dependency versions and usage echo "=== Checking go.mod for dependency versions ===" cat go.mod echo -e "\n=== Checking for connection handshake usage ===" rg -i "ConnectionOpenInit|ConnectionOpenTry|ConnectionOpenAck|ConnectionOpenConfirm" -A 2Length of output: 9182
e2e/go.mod (1)
104-104: LGTM! Dependency change is consistent.The addition of the direct dependency on
github.com/cosmos/ibc-go/v8 v8.5.0aligns with the changes in the main go.mod file and supports the IBC functionality in e2e tests.
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Release Notes
New Features
Improvements
Testing
Dependency Updates
These changes enhance the system's interoperability, configuration management, and testing capabilities.