feat: rollapp consensus params #991

srene · 2024-07-30T15:18:46Z

PR Standards

Opening a pull request should be able to meet the following requirements

--

PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A

Close #989

<-- Briefly describe the content of this pull request -->

This PR refactors the rollapp params in dymint, according to the following ADR: https://www.notion.so/dymension/ADR-x-Rollapp-Consensus-Params-d60daee1f8cb42e38e43e1ef5289a220?pvs=4

The main modifications are the following:

New config checks:

max_batch_size: Add validation max_batch_size is smaller than max blob accepted for each DA. celestia is 500KB.
max_batch_skew : The max_batch_skew param is modified for max_block_skew, so the skew is measured in blocks and not in batches. the max_block_skew is configurable but with a limit set to 1 day of blocks at max block rate.
max_batch_time: Add validation is no more than 1 hour.
block_time : check config is between 200 ms and 6 seconds

Existing consensus params:

max_block_size : Add limit produced block max_block_size, which can never be greater than max_batch_size
update params on gov proposal

New consensus params:

rollapp version (commit): Add validation on startup for version specified in consensus param.
da_provider : get it from consensus (genesis) instead of configurable.

New endblocker response to update new consensus params on gov proposal.

For Author:

Targeted PR against correct branch
included the correct type prefix in the PR title
Linked to Github issue with discussion and accepted design
Targets only one github issue
Wrote unit and integration tests
All CI checks have passed
Added relevant godoc comments

For Reviewer:

confirmed the correct type prefix in the PR title
Reviewers assigned
confirmed all author checklist items have been addressed

After reviewer approval:

In case targets main branch, PR should be squashed and merged.
In case PR targets a release branch, PR should be rebased.

Summary by CodeRabbit

New Features
- Enhanced error handling for block application processes.
- Introduced RollappConsensusParams to improve consensus parameter management.
- Added a new GetMaxBlobSizeBytes method to the Data Availability Layer client.
- Streamlined initialization for clients and configuration management.
- Added new constants for block processing time settings.
- Removed deprecated fields and parameters from configuration structures.
- Improved the handling of block production and submission processes.
Bug Fixes
- Improved error reporting and handling across multiple components.
- Resolved issues with consensus parameter updates and state management.
Documentation
- Updated various files to reflect changes in configuration and consensus parameters.
Tests
- Enhanced testing framework with improved mock responses and restructuring of test cases.
Chores
- Updated module dependencies to reflect changes in underlying libraries.

block/manager.go

coderabbitai

Actionable comments posted: 4

Outside diff range, codebase verification and nitpick comments (5)

da/errors.go (1)
25-27: Ensure consistency with error naming conventions.

The new error variable ErrNonexistentDA is defined but not used anywhere in the codebase. Please verify if this error needs to be integrated into the relevant parts of the codebase or if it should be removed if it is not necessary.

ErrNonexistentDA is defined in da/errors.go but not used elsewhere.

Analysis chain

Ensure consistency with error naming conventions.

The new error variables ErrDANotMatching and ErrNonexistentDA are clear and descriptive. Ensure that these new error variables are used consistently throughout the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of new error variables in the codebase.

# Test: Search for the usage of `ErrDANotMatching` and `ErrNonexistentDA`. Expect: Consistent usage.
rg --type go 'ErrDANotMatching|ErrNonexistentDA'
Length of output: 304

Script:
#!/bin/bash
# Description: Verify if there are places where `ErrNonexistentDA` should be used.

# Test: Search for error handling related to non-existent DAs.
rg --type go 'nonexistent DA|non-existent DA|DA does not exist|DA not found|DA missing'
Length of output: 151
testutil/node.go (1)
33-40: Unknown Block field in RollappConsensusParams struct

The RollappConsensusParams struct defined in types/pb/dymint/state.pb.go does not contain a Block field. The struct only has a single field named Params. Please correct the struct literal in the code snippet to match the actual struct definition.
Location: types/pb/dymint/state.pb.go
Struct Definition:
type RollappConsensusParams struct {
    Params *Params `protobuf:"bytes,1,opt,name=params,proto3" json:"params,omitempty"`
}
Analysis chain

Verify: Ensure the Block field is correctly defined.

The static analysis tool flagged an unknown field Block in the struct literal of type RollappConsensusParams.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the definition of the `Block` field in the `RollappConsensusParams` struct.

# Test: Search for the definition of the `RollappConsensusParams` struct. Expect: The `Block` field should be correctly defined.
rg --type go -A 10 'type RollappConsensusParams struct'
Length of output: 890
Tools

golangci-lint

36-36: unknown field Block in struct literal of type "github.com/tendermint/tendermint/abci/types".RollappConsensusParams

(typecheck)
block/production_test.go (1)
197-214: Old GetManager function signature still in use

The following instances of GetManager still use the old signature and need to be updated:

block/submit_test.go: manager, err := testutil.GetManager(testutil.GetManagerConfig(), nil, 1, 1, 0, nil, nil)

block/submit_test.go: manager, err := testutil.GetManager(testutil.GetManagerConfig(), nil, 1, 1, 0, proxyApp, nil)

block/manager_test.go: manager, err := testutil.GetManager(testutil.GetManagerConfig(), nil, 1, 1, 0, nil, nil)

block/manager_test.go: manager, err := testutil.GetManager(testutil.GetManagerConfig(), nil, 1, 1, 0, proxyApp, nil)

block/manager_test.go: manager, err := testutil.GetManager(testutil.GetManagerConfig(), nil, 1, 1, 0, proxyApp, mockStore)

Please update these instances to match the new function signature.

Analysis chain

LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to GetManager match the new signature.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `GetManager` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'GetManager'
Length of output: 9435
go.mod (2)
283-283: Update references to the new dependency.

The old dependency github.com/libp2p/go-libp2p-pubsub is still being referenced in the following files:

p2p/gossip.go

p2p/client.go

Please update these references to the new dependency github.com/dymensionxyz/go-libp2p-pubsub to ensure compatibility and consistency in the codebase.

Analysis chain

Verify compatibility with the new dependency.

The dependency github.com/libp2p/go-libp2p-pubsub has been replaced with a version under the dymensionxyz namespace. Ensure that this change is intentional and verify its compatibility with the existing codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify compatibility of the new dependency `github.com/dymensionxyz/go-libp2p-pubsub`.

# Test: Search for any usage of the old dependency to ensure all references are updated.
rg --type go 'github.com/libp2p/go-libp2p-pubsub'
Length of output: 167

Script:
#!/bin/bash
# Description: Check the specific lines in `p2p/gossip.go` and `p2p/client.go` where the old dependency is referenced.

# Display the lines in `p2p/gossip.go` that reference the old dependency.
rg --type go --context 5 'github.com/libp2p/go-libp2p-pubsub' p2p/gossip.go

# Display the lines in `p2p/client.go` that reference the old dependency.
rg --type go --context 5 'github.com/libp2p/go-libp2p-pubsub' p2p/client.go
Length of output: 733
283-283: Update all references to the new dependency.

The dependency github.com/tendermint/tendermint is still being referenced in multiple files. Ensure that all instances are updated to use github.com/dymensionxyz/cometbft to maintain compatibility and consistency across the codebase.

types/state.go

types/tx.go

types/serialization.go

types/validation.go

types/serialization_test.go

types/evidence.go

types/conv.go

types/block_source.go

types/hashing.go

types/block.go

types/pb/dymint/state.pb.go

types/pb/dymint/dymint.pb.go

utils/event/funcs.go

types/pb/interchain_da/da.pb.go

settlement/settlement.go

testutil/types.go

store/store_test.go

testutil/rpc.go

store/storeIface.go

testutil/node.go

testutil/block.go

testutil/p2p.go

testutil/mocks.go

settlement/local/local_test.go

settlement/local/local.go

settlement/grpc/grpc.go

store/store.go

settlement/dymension/dymension_test.go

settlement/dymension/dymension.go

settlement/dymension/utils.go

settlement/dymension/cosmosclient.go

p2p/validator.go

p2p/gossiped_block.go

p2p/client_test.go

p2p/client.go

node/node_test.go

p2p/validator_test.go

mocks/github.com/dymensionxyz/dymint/da/mock_DataAvailabilityLayerClient.go

mocks/github.com/tendermint/tendermint/abci/types/mock_Application.go

mocks/github.com/dymensionxyz/dymint/store/mock_Store.go

mocks/github.com/dymensionxyz/dymint/settlement/dymension/mock_CosmosClient.go

node/mempool/mempool.go

mempool/tx.go

mempool/v1/tx.go

mempool/v1/mempool.go

mempool/v1/mempool_test.go

mocks/github.com/tendermint/tendermint/proxy/mock_AppConns.go

mocks/github.com/tendermint/tendermint/proxy/mock_AppConnConsensus.go

mempool/mempool.go

mempool/ids_test.go

mempool/mock/mempool.go

mempool/cache.go

mempool/clist/clist.go

rpc/server.go

node/node.go

indexers/blockindexer/null/null.go

indexers/txindex/null/null.go

indexers/blockindexer/block.go

indexers/txindex/kv/kv_test.go

indexers/blockindexer/kv/util.go

indexers/txindex/kv/kv_bench_test.go

indexers/blockindexer/kv/kv_test.go

indexers/txindex/kv/kv.go

indexers/blockindexer/kv/kv.go

mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go

indexers/txindex/indexer_service.go

indexers/txindex/indexer.go

indexers/blockindexer/query_range.go

indexers/txindex/indexer_service_test.go

rpc/json/service.go

rpc/json/handler.go

mempool/clist/clist_test.go

conv/crypto_test.go

conv/config_test.go

rpc/client/utils.go

rpc/client/client_test.go

rpc/client/client.go

conv/config.go

da/local/local.go

da/da_test.go

da/grpc/grpc.go

da/celestia/celestia_test.go

da/da.go

da/celestia/celestia.go

da/celestia/rpc_test.go

da/grpc/mockserv/mockserv.go

da/celestia/mock/server.go

da/avail/avail.go

da/avail/avail_test.go

config/toml.go

config/flags.go

config/config.go

block/submit_test.go

rpc/json/service_test.go

block/state.go

cmd/dymint/commands/root.go

cmd/dymint/commands/start.go

cmd/dymint/commands/show_sequencer.go

cmd/dymint/commands/init.go

rpc/json/ws_test.go

block/production_test.go

rpc/json/ws.go

rpc/middleware/client.go

rpc/middleware/status.go

rpc/middleware/registry.go

rpc/middleware/middleware_test.go

block/manager_test.go

block/manager.go

block/executor_test.go

block/produce.go

rpc/json/types.go

block/gossip.go

block/initchain.go

conv/crypto.go

cmd/dymint/commands/show_node_id.go

block/sync.go

block/executor.go

cmd/dymint/main.go

Analysis chain

Verify compatibility with the new dependency.

The dependency github.com/tendermint/tendermint has been replaced with github.com/dymensionxyz/cometbft with a version suffix -dym. Ensure that this change is intentional and verify its compatibility with the existing codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify compatibility of the new dependency `github.com/dymensionxyz/cometbft`.

# Test: Search for any usage of the old dependency to ensure all references are updated.
rg --type go 'github.com/tendermint/tendermint'
Length of output: 23223

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 402dd00 and c466f22.

Files ignored due to path filters (2)

go.sum is excluded by !**/*.sum
types/pb/dymint/state.pb.go is excluded by !**/*.pb.go

Files selected for processing (57)

block/block.go (4 hunks)
block/errors.go (1 hunks)
block/executor.go (3 hunks)
block/executor_test.go (5 hunks)
block/manager.go (4 hunks)
block/manager_test.go (17 hunks)
block/produce.go (4 hunks)
block/production_test.go (5 hunks)
block/retriever.go (1 hunks)
block/state.go (4 hunks)
block/submit.go (6 hunks)
block/submit_loop_test.go (8 hunks)
block/submit_test.go (8 hunks)
cmd/dymint/commands/init.go (1 hunks)
cmd/dymint/commands/root.go (1 hunks)
config/config.go (6 hunks)
config/config_test.go (5 hunks)
config/defaults.go (3 hunks)
config/flags.go (6 hunks)
config/toml.go (3 hunks)
da/avail/avail.go (1 hunks)
da/celestia/celestia.go (1 hunks)
da/celestia/config.go (1 hunks)
da/da.go (1 hunks)
da/errors.go (1 hunks)
da/grpc/grpc.go (1 hunks)
da/local/local.go (1 hunks)
go.mod (1 hunks)
mocks/github.com/dymensionxyz/dymension/v3/x/rollapp/types/mock_QueryClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymension/v3/x/sequencer/types/mock_QueryClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/da/avail/mock_SubstrateApiI.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/da/celestia/types/mock_CelestiaRPCClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/da/mock_DataAvailabilityLayerClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/settlement/dymension/mock_CosmosClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go (5 hunks)
mocks/github.com/dymensionxyz/dymint/store/mock_Store.go (3 hunks)
mocks/github.com/tendermint/tendermint/abci/types/mock_Application.go (1 hunks)
mocks/github.com/tendermint/tendermint/proxy/mock_AppConnConsensus.go (1 hunks)
mocks/github.com/tendermint/tendermint/proxy/mock_AppConns.go (1 hunks)
node/node.go (11 hunks)
node/node_test.go (4 hunks)
p2p/validator_test.go (3 hunks)
proto/types/dymint/state.proto (2 hunks)
proto/types/tendermint/abci/types.proto (2 hunks)
proto/types/tendermint/types/params.proto (1 hunks)
rpc/client/client_test.go (13 hunks)
rpc/json/service_test.go (4 hunks)
settlement/config.go (2 hunks)
settlement/dymension/dymension.go (7 hunks)
settlement/dymension/dymension_test.go (3 hunks)
settlement/grpc/grpc.go (2 hunks)
settlement/local/local.go (2 hunks)
settlement/local/local_test.go (4 hunks)
settlement/settlement.go (1 hunks)
testutil/block.go (5 hunks)
testutil/node.go (2 hunks)
testutil/rpc.go (1 hunks)

Files not processed due to max files limit (6)

testutil/types.go
types/metrics.go
types/serialization.go
types/serialization_test.go
types/state.go
version/version.go

Files skipped from review due to trivial changes (11)

config/flags.go
mocks/github.com/dymensionxyz/dymension/v3/x/rollapp/types/mock_QueryClient.go
mocks/github.com/dymensionxyz/dymension/v3/x/sequencer/types/mock_QueryClient.go
mocks/github.com/dymensionxyz/dymint/da/avail/mock_SubstrateApiI.go
mocks/github.com/dymensionxyz/dymint/da/celestia/types/mock_CelestiaRPCClient.go
mocks/github.com/dymensionxyz/dymint/da/mock_DataAvailabilityLayerClient.go
mocks/github.com/dymensionxyz/dymint/settlement/dymension/mock_CosmosClient.go
mocks/github.com/tendermint/tendermint/abci/types/mock_Application.go
mocks/github.com/tendermint/tendermint/proxy/mock_AppConnConsensus.go
mocks/github.com/tendermint/tendermint/proxy/mock_AppConns.go
settlement/config.go

Additional context used

golangci-lint

testutil/node.go

36-36: unknown field Block in struct literal of type "github.com/tendermint/tendermint/abci/types".RollappConsensusParams

(typecheck)

Additional comments not posted (135)

block/errors.go (1)
8-9: Ensure consistency with error naming conventions.

The new error variables ErrDAUpgrade and ErrVersionUpgrade are clear and descriptive. Ensure that these new error variables are used consistently throughout the codebase.

Verification successful

The new error variables ErrDAUpgrade and ErrVersionUpgrade are used consistently throughout the codebase.

Verified occurrences in:

block/block.go

block/produce.go

block/state.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of new error variables in the codebase.

# Test: Search for the usage of `ErrDAUpgrade` and `ErrVersionUpgrade`. Expect: Consistent usage.
rg --type go 'ErrDAUpgrade|ErrVersionUpgrade'
Length of output: 994

Script:
#!/bin/bash
# Description: Verify the usage of all error variables in the block package to ensure consistency.

# Test: Search for all error variables in the block package.
rg --type go 'errors.New' block/
Length of output: 366
testutil/rpc.go (1)

25-25: Verify the impact of GenerateGenesis(0) on node initialization.

The addition of GenerateGenesis(0) to the CreateNode function call changes the initial state of the node. Ensure that this change is intentional and does not introduce any issues in the testing context.

proto/types/dymint/state.proto (7)

32-32: Approved: Reserved field 12.

The consensus_params field is removed and replaced by a reserved declaration at position 12, indicating it's reserved for future use.

41-41: Approved: Added new consensus_params field.

The new field consensus_params of type RollappConsensusParams is added at position 18, indicating a shift towards a more specialized representation of consensus parameters.

45-46: Approved: Added new message RollappConsensusParams.

The new message RollappConsensusParams is introduced with a single field params, encapsulating consensus parameters.

46-46: Approved: Added new params field to RollappConsensusParams.

The new field params of type Params is added to the RollappConsensusParams message, introducing a nested structure for consensus parameters.

49-54: Approved: Added new message Params.

The new message Params is introduced with multiple fields: block_max_gas, block_max_size, da, and commit, encapsulating various consensus parameters.

50-50: Approved: Added new block_max_gas field to Params.

The new field block_max_gas of type int64 is added to the Params message, introducing a new parameter for the maximum gas per block.

51-54: Approved: Added new fields to Params.

The new fields block_max_size, da, and commit are added to the Params message, introducing new parameters for the maximum block size, data availability layer, and commit hash.

testutil/node.go (2)

24-24: Approved: Modified function signature.

The function signature of CreateNode is modified to include a new parameter genesis, enhancing its capability to initialize nodes with specific genesis configurations.

25-41: Approved: Enhanced mock application setup.

The function body is enhanced to improve the mock application setup, including specific mock behavior for the EndBlock method and a new proxy application connection.

Tools

golangci-lint

36-36: unknown field Block in struct literal of type "github.com/tendermint/tendermint/abci/types".RollappConsensusParams

(typecheck)

config/defaults.go (3)

19-19: Approved: Simplified default node configuration.

The DefaultNodeConfig variable is modified to use the simplified DefaultConfig function, streamlining the default node configuration initialization.

22-22: Approved: Simplified DefaultConfig function.

The DefaultConfig function is simplified to accept only the home parameter, removing the chainId parameter and streamlining the configuration setup.

29-29: Approved: Replaced MaxBatchSkew with MaxBlockSkew.

The MaxBatchSkew field is replaced with MaxBlockSkew in the BlockManagerConfig, shifting the focus from batch processing to block timing and potentially improving block submission handling.
cmd/dymint/commands/root.go (1)
58-58: LGTM! But verify the DefaultConfig function's signature.

The code changes are approved.

However, ensure that the DefaultConfig function's signature has been updated accordingly.
Verification successful

The DefaultConfig function's signature is correct.

The DefaultConfig function in config/defaults.go now takes only one parameter, confirming that the code change in cmd/dymint/commands/root.go is accurate.
config/defaults.go
func DefaultConfig(home string) *NodeConfig {
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the `DefaultConfig` function's signature.

# Test: Search for the `DefaultConfig` function definition. Expect: Only one parameter.
rg --type go 'func DefaultConfig' -A 5
Length of output: 395
da/celestia/config.go (1)

19-19: Approved: Addition of maxBlobSize constant.

The addition of the maxBlobSize constant enhances configuration management by providing a reference for the maximum allowable blob size.

cmd/dymint/commands/init.go (1)

64-64: Approved: Addition of AppState field.

The addition of the AppState field enhances the initialization process by providing default application state information, aligning with the new consensus parameters.

settlement/settlement.go (1)

62-62: Verify all implementations of ClientI interface accommodate the new parameter.

The Init method signature has been updated to include an additional parameter rollappId. Ensure that all implementations of the ClientI interface are updated accordingly.
proto/types/tendermint/types/params.proto (1)
83-86: Verify the integration of the new message type RollappConsensusParams.

The new message type RollappConsensusParams includes two string fields: da and version. Ensure that this new message type is integrated correctly in the codebase.

Verification successful

The new message type RollappConsensusParams is integrated correctly in the codebase.

The search results show multiple instances of RollappConsensusParams being used throughout the codebase, indicating that it is integrated and utilized as expected.

testutil/types.go

testutil/node.go

types/state.go

types/serialization_test.go

types/pb/dymint/state.pb.go

rpc/json/service_test.go

proto/types/dymint/state.proto

proto/types/tendermint/abci/types.proto

rpc/client/client_test.go

p2p/validator_test.go

block/manager_test.go

block/submit_test.go

block/production_test.go

block/executor_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the integration of the new message type `RollappConsensusParams`.

# Test: Search for the usage of the `RollappConsensusParams` message type. Expect: The new message type should be used appropriately in the codebase.
rg --type proto -A 5 $'RollappConsensusParams'
Length of output: 75

Script:
#!/bin/bash
# Description: Verify the integration of the new message type `RollappConsensusParams`.

# Test: Search for the usage of the `RollappConsensusParams` message type. Expect: The new message type should be used appropriately in the codebase.
rg -A 5 'RollappConsensusParams'
Length of output: 17245
node/node_test.go (2)
35-35: Verify the integration of the new parameter in TestStartup.

The function TestStartup has been updated to include a new parameter testutil.GenerateGenesis(0) in the call to testutil.CreateNode. Ensure that this new parameter is correctly integrated and does not introduce any issues.

Verification successful

Verify the integration of the new parameter in TestStartup.

The function TestStartup has been updated to include a new parameter testutil.GenerateGenesis(0) in the call to testutil.CreateNode. The new parameter is correctly integrated and used appropriately in the function.

node/node_test.go:

TestStartup function: testutil.CreateNode(false, nil, testutil.GenerateGenesis(0))
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the integration of the new parameter in `TestStartup`.

# Test: Search for the usage of the `testutil.GenerateGenesis(0)` parameter in the `TestStartup` function. Expect: The new parameter should be used appropriately in the function.
rg --type go -A 5 $'testutil.CreateNode(false, nil, testutil.GenerateGenesis(0))'
Length of output: 82

Script:
#!/bin/bash
# Find the definition of the `TestStartup` function and its usage of `testutil.GenerateGenesis(0)`
ast-grep --lang go --pattern $'func TestStartup($_, $_) {
  $$$
  testutil.CreateNode(false, nil, testutil.GenerateGenesis(0))
  $$$
}'
Length of output: 136

Script:
#!/bin/bash
# Find the definition of the `TestStartup` function
rg --type go -A 20 'func TestStartup'
Length of output: 896
77-81: Verify the impact of the changes in TestMempoolDirectly.

The function TestMempoolDirectly has been updated to remove the rollappID variable and simplify the SettlementConfig initialization. Ensure that these changes do not introduce any issues.
block/submit_loop_test.go (5)

16-16: LGTM!

Renaming batchSkew to blockSkew aligns with the shift in terminology from batch processing to block production.

26-26: LGTM!

Simplifying the testSubmitLoop function by removing parallel execution logic enhances readability and maintainability.

68-69: LGTM!

Adding to pendingBlocks and checking if it exceeds blockSkew ensures that block production does not get too far ahead.

85-86: LGTM!

Changing the calculation of absoluteMax to use a factor of 1 instead of 1.5 potentially leads to stricter timing constraints for successful submissions.

100-101: LGTM!

Using args.blockSkew in the block.SubmitLoopInner call aligns with the renaming of batchSkew to blockSkew.

settlement/local/local_test.go (4)

31-31: LGTM!

Adding the new parameter "rollappTest" to the Init method of the sllayer client affects the initialization context, aligning with the new configuration requirement.

54-54: LGTM!

Adding the new parameter "rollappTest" to the Init method of the sllayer client affects the initialization context, aligning with the new configuration requirement.

129-129: LGTM!

Adding the new parameter "rollappTest" to the Init method of the sllayer client affects the initialization context, aligning with the new configuration requirement.

154-154: LGTM!

Adding the new parameter "rollappTest" to the Init method of the sllayer client affects the initialization context, aligning with the new configuration requirement.
testutil/block.go (4)
151-151: LGTM!

The renaming of MaxBatchSkew to MaxBlockSkew improves clarity and aligns with the updated logic related to block management.

115-120: LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to GetManager match the new signature.

Verification successful

All function calls to GetManager match the new signature.

The code changes are consistent across the codebase, and no instances with the old signature were found.

GetManager calls in block/production_test.go

GetManager calls in block/manager_test.go

GetManager calls in block/submit_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `GetManager` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'GetManager'
Length of output: 9435
134-135: LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to initSettlementLayerMock match the new signature.

Verification successful

Function usage verified successfully.

All function calls to initSettlementLayerMock match the new signature.

testutil/block.go:

Line 134: Function definition with the new signature.

Line 112: Function call with the new signature.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `initSettlementLayerMock` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'initSettlementLayerMock'
Length of output: 765
Line range hint 36-69:
LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to GetManagerWithProposerKey match the new signature.

Verification successful

All usages of GetManagerWithProposerKey match the new function signature.

The function calls have been correctly updated to reflect the new signature.

testutil/block.go

block/submit_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `GetManagerWithProposerKey` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'GetManagerWithProposerKey'
Length of output: 1521

Script:
#!/bin/bash
# Comprehensive search for all usages of `GetManagerWithProposerKey` to confirm they match the new signature.
rg 'GetManagerWithProposerKey\(' --type go
Length of output: 644
block/retriever.go (1)

157-157: LGTM!

The update to the error message improves the clarity and specificity of the error handling.

config/toml.go (1)

73-73: LGTM!

The renaming of max_supported_batch_skew to max_supported_block_skew improves clarity and aligns with the updated logic related to block management.

p2p/validator_test.go (2)

138-143: Verify the initialization of state.ConsensusParams.

The initialization of state.ConsensusParams using dymint.RollappConsensusParams aligns with the new structure for managing consensus parameters. Ensure that the values for BlockMaxGas and BlockMaxSize are correctly set.

154-154: Verify the client.Init function call.

The client.Init function call now uses a string literal "rollappTest". Ensure that this change is intentional and aligns with the new initialization parameters.

block/state.go (5)

72-75: Verify the loading of consensus parameters.

The LoadConsensusFromAppState method is called to load consensus parameters from the application state. Ensure that the method correctly loads the parameters and handles any errors appropriately.

98-103: Verify the error handling in UpdateStateFromApp.

The method captures the error returned by m.Executor.UpdateStateAfterCommit. Ensure that the error handling is correctly implemented and the state is updated appropriately.

Line range hint 103-123:
Verify the state updates and validator settings in UpdateStateAfterInitChain.

The method updates the state based on the ResponseInitChain and sets the validators. Ensure that the state is correctly updated and the validators are set appropriately.

128-129: Verify the mempool configuration in UpdateMempoolAfterInitChain.

The method references the new structure of consensus parameters. Ensure that the mempool configuration aligns with the updated consensus parameter structure.

133-160: Verify the updates to consensus parameters and error handling in UpdateStateAfterCommit.

The method includes checks for updates to the consensus parameters based on the EndBlock response and returns an error if updates are made. Ensure that the consensus parameters are correctly updated and handle any errors appropriately.
block/block.go (4)
4-4: Import errors package.

The errors package is imported to enhance error handling.

87-90: Update state after commit.

The new error handling logic ensures that any errors during the state update are properly reported.

143-146: Enhanced error handling for critical failures.

The changes ensure that critical failures trigger a node restart, improving resilience.

103-103: LGTM! But verify the method usage in the codebase.

The code changes are approved.

However, ensure that all method calls to applyBlock handle the new error signature correctly.

Verification successful

The new error signature of applyBlock is handled correctly in all instances.

The error handling in the identified usages of applyBlock is consistent and thorough.

block/retriever.go

block/produce.go

block/block.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all method calls to `applyBlock` handle the new error signature correctly.

# Test: Search for the method usage. Expect: Proper error handling in all occurrences.
rg --type go -A 5 $'applyBlock'
Length of output: 2573

Script:
#!/bin/bash
# Description: List all occurrences of `applyBlock` in the codebase to verify error handling.

# Search for all usages of `applyBlock` in the codebase.
rg --type go 'applyBlock'
Length of output: 703
config/config_test.go (3)

23-27: Simplify configuration initialization.

The changes simplify the configuration initialization and enforce stricter checks on critical parameters.

35-35: Update block time assertion.

The block time assertion has been updated to reflect the new expected value.

Line range hint 67-111:
Enhance validation logic.

The new test cases ensure that the block time, batch submit max time, and max block skew adhere to specified constraints.

block/manager.go (3)

20-20: Update logger import.

The logger import has been updated to use a more specific logging library.

Line range hint 93-114:
Improve logging capabilities.

The changes improve logging capabilities and standardization across the codebase.

242-255: Validate rollapp parameters.

The new method ensures that the maximum blob size and block size adhere to specified constraints and checks the binary version.

config/config.go (7)

21-24: LGTM!

The new constants are well-defined and enhance the configurability and enforce constraints on block timing parameters.

59-60: LGTM!

The renaming of the MaxBatchSkew field to MaxBlockSkew aligns with the new terminology.

130-132: LGTM!

The validation logic for BlockTime ensures it is not less than MinBlockTime.

134-135: LGTM!

The validation logic for BlockTime ensures it is not greater than MaxBlockTime.

163-165: LGTM!

The validation logic for BatchSubmitMaxTime ensures it does not exceed MaxBatchSubmitMaxTime.

171-173: LGTM!

The validation logic for MaxBlockSkew ensures it is positive.

175-176: LGTM!

The validation logic for MaxBlockSkew ensures it does not exceed MaxBlockSkewSupported.

da/da.go (2)

219-221: LGTM!

The Synced method enhances the interface by providing a synchronization mechanism.

222-223: LGTM!

The GetMaxBlobSize method enhances the interface by providing the maximum allowed blob size in the DA.

settlement/local/local.go (1)

Line range hint 55-79:
LGTM!

The Init method now accepts a rollappId parameter, enhancing flexibility and clarity in client initialization.
block/submit.go (3)
39-40: LGTM! But verify the usage of atomic operations and new parameters.

The code changes are approved.

However, ensure that the atomic operations and new parameters are correctly used in the codebase.

Also applies to: 48-50, 59-63, 80-86, 103-116

Verification successful

Verify atomic operations and new parameters usage in SubmitLoopInner.

The pendingBlocks parameter and atomic operations are used correctly in the block/submit.go and block/submit_loop_test.go files. Here are the relevant lines:

block/submit.go

pendingBlocks := atomic.Uint64{}

pendingBlocks.Store(pendingSubmittedBlocks())

if maxBlockSkew <= pendingBlocks.Load()

pendingBlocks.Add(uint64(1))

types.RollappPendingSubmissionsSkewNumBlocks.Set(float64(pendingBlocks.Load()))

pendingBlocks.Store(pendingSubmittedBlocks())

block/submit_loop_test.go

pendingBlocks := atomic.Uint64{}

pendingBlocks.Add(1)

require.True(t, pendingBlocks.Load() <= args.blockSkew)

pendingBlocks.Store(0)

return pendingBlocks.Load()

These lines confirm that atomic operations and the pendingBlocks parameter are used correctly in the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of atomic operations and new parameters in `SubmitLoopInner`.

# Test: Search for the usage of atomic operations and new parameters. Expect: Correct usage in the codebase.
rg --type go 'atomic'
rg --type go 'pendingBlocks'
Length of output: 2950
235-237: LGTM! But verify the usage of the function.

The code changes are approved.

However, ensure that the GetUnsubmittedBlocks function is correctly used in the codebase.

Verification successful

The usage of the GetUnsubmittedBlocks function is correct.

The function is used appropriately within the SubmitLoop method and passed to SubmitLoopInner.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the `GetUnsubmittedBlocks` function.

# Test: Search for the usage of `GetUnsubmittedBlocks`. Expect: Correct usage in the codebase.
rg --type go 'GetUnsubmittedBlocks'
Length of output: 143

Script:
#!/bin/bash
# Extract lines around the usage of `GetUnsubmittedBlocks` in `block/submit.go` for detailed review.
rg --type go -A 10 -B 10 'GetUnsubmittedBlocks' block/submit.go
Length of output: 1274
28-29: LGTM! But verify the usage of new parameters.

The code changes are approved.

However, ensure that the maxBlockSkew and GetUnsubmittedBlocks parameters are correctly used in the codebase.

Verification successful

The new parameters maxBlockSkew and GetUnsubmittedBlocks are correctly used in the codebase.

maxBlockSkew is used to control the flow of block submission by checking against the number of pending blocks.

GetUnsubmittedBlocks is used to initialize and update the count of pending blocks.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of new parameters `maxBlockSkew` and `GetUnsubmittedBlocks`.

# Test: Search for the usage of `maxBlockSkew` and `GetUnsubmittedBlocks`. Expect: Correct usage in the codebase.
rg --type go 'maxBlockSkew'
rg --type go 'GetUnsubmittedBlocks'
Length of output: 336

Script:
#!/bin/bash
# Description: Extract the full context of the `SubmitLoop` function to verify the correct usage of `maxBlockSkew` and `GetUnsubmittedBlocks`.

# Test: Search for the `SubmitLoop` function definition and its full implementation.
rg --type go -A 20 'func SubmitLoop' block/submit.go
Length of output: 1063

Script:
#!/bin/bash
# Description: Extract the complete implementation of the `SubmitLoopInner` function to verify the correct usage of `maxBlockSkew` and `GetUnsubmittedBlocks`.

# Test: Search for the `SubmitLoopInner` function definition and its full implementation.
rg --type go -A 100 'func SubmitLoopInner' block/submit.go
Length of output: 3887
block/produce.go (3)
103-105: LGTM! But verify the handling of upgrade errors.

The code changes are approved.

However, ensure that the handling of ErrDAUpgrade and ErrVersionUpgrade errors is correctly implemented in the codebase.

Also applies to: 112-112

Verification successful

The handling of ErrDAUpgrade and ErrVersionUpgrade errors is correctly implemented.

The search results confirm that these errors are consistently checked and handled in the codebase, particularly in block/produce.go, block/state.go, and block/block.go.

block/produce.go

block/state.go

block/block.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the handling of `ErrDAUpgrade` and `ErrVersionUpgrade` errors.

# Test: Search for the handling of `ErrDAUpgrade` and `ErrVersionUpgrade` errors. Expect: Correct handling in the codebase.
rg --type go 'ErrDAUpgrade'
rg --type go 'ErrVersionUpgrade'
Length of output: 1432
59-64: LGTM! But verify the handling of upgrade errors.

The code changes are approved.

However, ensure that the handling of ErrDAUpgrade and ErrVersionUpgrade errors is correctly implemented in the codebase.

Also applies to: 86-86

Verification successful

The handling of ErrDAUpgrade and ErrVersionUpgrade errors is correctly implemented.

The codebase consistently checks for these errors and takes appropriate actions to ensure the node responds correctly to upgrade requirements.

block/errors.go: Defines the errors.

block/state.go: Uses the errors in error messages.

block/produce.go: Checks for the errors and panics if they occur.

block/block.go: Checks for the errors and takes appropriate actions.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the handling of `ErrDAUpgrade` and `ErrVersionUpgrade` errors.

# Test: Search for the handling of `ErrDAUpgrade` and `ErrVersionUpgrade` errors. Expect: Correct handling in the codebase.
rg --type go 'ErrDAUpgrade'
rg --type go 'ErrVersionUpgrade'
Length of output: 1432
189-190: LGTM! But verify the handling of upgrade errors.

The code changes are approved.

However, ensure that the handling of ErrDAUpgrade and ErrVersionUpgrade errors is correctly implemented in the codebase.

Also applies to: 198-199

Verification successful

The handling of ErrDAUpgrade and ErrVersionUpgrade is correctly implemented and consistent across the codebase.

The errors are appropriately checked and managed to ensure the node restarts with new configurations or a new binary version when necessary.

block/state.go

block/produce.go

block/block.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the handling of `ErrDAUpgrade` and `ErrVersionUpgrade` errors.

# Test: Search for the handling of `ErrDAUpgrade` and `ErrVersionUpgrade` errors. Expect: Correct handling in the codebase.
rg --type go 'ErrDAUpgrade'
rg --type go 'ErrVersionUpgrade'
Length of output: 1432

Script:
#!/bin/bash
# Description: Gather more context around the occurrences of `ErrDAUpgrade` and `ErrVersionUpgrade` in the relevant files.

# Get context around occurrences of `ErrDAUpgrade`
rg --type go 'ErrDAUpgrade' -A 5 -B 5

# Get context around occurrences of `ErrVersionUpgrade`
rg --type go 'ErrVersionUpgrade' -A 5 -B 5
Length of output: 7706
settlement/grpc/grpc.go (1)
51-51: LGTM! But verify the usage of the new parameter.

The code changes are approved.

However, ensure that the rollappId parameter is correctly used in the codebase.

Also applies to: 90-90

Verification successful

The rollappId parameter is correctly used in the codebase.

The parameter is consistently integrated into various methods and functions, ensuring its proper usage.

settlement/dymension/dymension.go

settlement/local/local.go

settlement/grpc/grpc.go

testutil/block.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the `rollappId` parameter.

# Test: Search for the usage of `rollappId`. Expect: Correct usage in the codebase.
rg --type go 'rollappId'
Length of output: 3146
block/production_test.go (2)
129-129: LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to GetManager match the new signature.

Verification successful

All instances of GetManager match the new function signature.

The verification confirms that all calls to GetManager in the codebase have been updated to match the new signature.

Instances found in testutil/block.go, block/manager_test.go, block/production_test.go, and block/submit_test.go are consistent with the new function signature.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `GetManager` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'GetManager'
Length of output: 9435
37-45: LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to GetManager match the new signature.

Verification successful

All function calls to GetManager match the new signature.

The code changes are verified, and all instances of GetManager in the codebase are consistent with the updated function signature.

block/production_test.go

block/submit_test.go

block/manager_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `GetManager` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'GetManager'
Length of output: 9435
block/executor.go (3)

Line range hint 49-84:
LGTM!

The variable renaming and consensus parameter updates improve readability and correctness.

92-95: LGTM!

The updates to consensus parameters improve the block creation logic.

161-162: LGTM!

The updates to consensus parameters improve the commit logic.

block/executor_test.go (3)

54-59: LGTM!

The updates to consensus parameters improve the test setup.

98-107: LGTM!

The updates to consensus parameters and the mock response improve the test setup.

161-169: LGTM!

The updates to consensus parameters improve the test setup.
node/node.go (8)
22-22: Import looks good.

The new import github.com/dymensionxyz/dymint/da/registry is necessary for the DAL client initialization logic.

122-122: Change looks good.

The change to the settlementlc.Init method to include the genesis.ChainID aligns with the new architecture.

163-163: Change looks good.

Including the logger in the blockManager initialization improves logging and debugging capabilities.

182-182: Change looks good.

The initialization of the Node struct now includes baseKV, Store, TxIndexer, IndexerService, and BlockIndexer, which are necessary for the new DAL client logic and overall architecture improvements.

217-217: Change looks good.

The change to the OnStart method to utilize the new DAClient property of the BlockManager streamlines client management and aligns with the new architecture.

249-249: Change looks good.

The change to the OnStop method to utilize the new DAClient property of the BlockManager streamlines client management and aligns with the new architecture.

351-369: Method implementation looks good.

The setDA method correctly initializes the DAL client in the BlockManager according to the DA type set in genesis or stored in state.

191-199: Changes look good.

The new method call node.setDA(dalcKV, logger) initializes the DAL client in the BlockManager according to the DA type set in genesis or stored in state. The ValidateRollappParams method validates the rollapp parameters.

However, ensure that the setDA method is correctly implemented.
Verification successful

The setDA method is correctly implemented.

The setDA method initializes the DA client in the BlockManager according to the DA type set in genesis or stored in state. The method is defined as follows:
// setDA initializes DA client in blockmanager according to DA type set in genesis or stored in state
func (n *Node) setDA(dalcKV store.KV, logger log.Logger) error {
	da_layer := n.BlockManager.State.ConsensusParams.Params.Da
	dalc := registry.GetClient(da_layer)
	if dalc == nil {
		return fmt.Errorf("get data availability client named '%s'", da_layer)
	}

	err := dalc.Init([]byte(n.conf.DAConfig), n.PubsubServer, dalcKV, logger.With("module", string(dalc.GetClientType())))
	if err != nil {
		return fmt.Errorf("data availability layer client initialization  %w", err)
	}
	n.BlockManager.DAClient = dalc
	retriever, ok := dalc.(da.BatchRetriever)
	if !ok {
		return fmt.Errorf("data availability layer client is not of type BatchRetriever")
	}
	n.BlockManager.Retriever = retriever
	return nil
}
The method retrieves the DA client based on the DA type.

It initializes the DA client with the necessary configurations.

It sets the DA client and retriever in the BlockManager.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the implementation of the `setDA` method.

# Test: Search for the `setDA` method implementation. Expect: Correct implementation of the method.
rg --type go -A 10 'func (n \*Node) setDA'
Length of output: 44

Script:
#!/bin/bash
# Description: Verify the implementation of the `setDA` method.

# Test: Search for the `setDA` method implementation with a more flexible pattern.
rg --type go -A 20 'func (n \*Node) setDA'

# Additionally, search for any method containing 'DA' in its name within the Node struct.
rg --type go -A 20 'func (n \*Node) .*DA'
Length of output: 88

Script:
#!/bin/bash
# Description: Verify the methods within the `Node` struct.

# Test: Search for all methods within the `Node` struct.
rg --type go -A 20 'func (n \*Node)'
Length of output: 38

Script:
#!/bin/bash
# Description: Verify the definition of the `Node` struct and its methods.

# Test: Search for the definition of the `Node` struct.
rg --type go -A 10 'type Node struct'

# Additionally, search for any methods defined for the `Node` struct.
rg --type go -A 20 'func (n \*Node)'
Length of output: 425

Script:
#!/bin/bash
# Description: Verify any methods related to `setDA` or DA initialization in the `node/node.go` file.

# Test: Search for any methods related to `setDA` or DA initialization in the `node/node.go` file.
rg --type go -A 20 'DA' node/node.go
Length of output: 2751
block/submit_test.go (7)

15-15: Import looks good.

The new import github.com/tendermint/tendermint/abci/types is necessary for handling the EndBlock method in the mocked application.

24-24: Import looks good.

The new import github.com/dymensionxyz/dymint/da is necessary for the new DAL client logic in the tests.

106-126: Changes look good.

The changes to the TestBatchSubmissionHappyFlow test to handle the EndBlock method and initialize DAClient and Retriever enhance the functionality of the test by accurately simulating the blockchain environment.

Line range hint 145-183: Changes look good.

The changes to the TestBatchSubmissionFailedSubmission test to handle the EndBlock method and initialize DAClient and Retriever enhance the functionality of the test by accurately simulating the blockchain environment.

Line range hint 215-244: Changes look good.

The changes to the TestSubmissionByTime test to handle the EndBlock method and initialize DAClient and Retriever enhance the functionality of the test by accurately simulating the blockchain environment.

294-316: Changes look good.

The changes to the TestSubmissionByBatchSize test to handle the EndBlock method and initialize DAClient and Retriever enhance the functionality of the test by accurately simulating the blockchain environment.

234-234: Change looks good.

The change in managerConfig to rename MaxBatchSkew to MaxBlockSkew reflects a more appropriate naming convention for the parameter's intended use.

rpc/json/service_test.go (4)

35-35: Import looks good.

The new import github.com/dymensionxyz/dymint/version is necessary for handling the Commit field in the EndBlock method response.

283-290: Changes look good.

The changes to the EndBlock method of the MockApplication to include RollappConsensusParamUpdates enhance the configuration and response handling within the application.

311-315: Changes look good.

The changes to the NodeConfig struct's initialization to remove DALayer and rename MaxBatchSkew to MaxBlockSkew indicate a shift in focus from batch processing to block management parameters.

334-334: Changes look good.

The changes to the GenesisDoc initialization to include an AppState field with rollapp parameters enhance the configuration and response handling within the application.

settlement/dymension/dymension_test.go (3)

63-63: Verify the context change for hubClient.Init.

The hubClient.Init method now uses the string literal "rollappTest" instead of pubsubServer. Ensure this change aligns with the intended test environment or scenario.

102-102: Verify the context change for hubClient.Init.

The hubClient.Init method now uses the string literal "rollappTest" instead of pubsubServer. Ensure this change aligns with the intended test environment or scenario.

247-247: Verify the context change for hubClient.Init.

The hubClient.Init method now uses the string literal "rollappTest" instead of pubsubServer. Ensure this change aligns with the intended test environment or scenario.
proto/types/tendermint/abci/types.proto (2)
235-235: LGTM! But verify the usage of the new field.

The addition of the rollapp_consensus_params_updates field to ResponseEndBlock looks good. Ensure that this new field is correctly utilized in the codebase.

378-382: LGTM! But verify the usage of the new message type.

The definition of the RollappConsensusParams message type looks good. Ensure that this new message type is correctly utilized in the codebase.

Verification successful

The new message type RollappConsensusParams is correctly utilized in the codebase.

The RollappConsensusParams message type is found in the following files:

proto/types/dymint/state.proto (lines 41, 45)

proto/types/tendermint/types/params.proto (line 83)

proto/types/tendermint/abci/types.proto (lines 235, 378)
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the new message type `RollappConsensusParams`.

# Test: Search for the new message type usage. Expect: Occurrences of `RollappConsensusParams`.
rg --type proto 'RollappConsensusParams'
Length of output: 70

Script:
#!/bin/bash
# Description: Verify the usage of the new message type `RollappConsensusParams`.

# Find all .proto files and search for the usage of `RollappConsensusParams`.
fd -e proto -x grep -Hn 'RollappConsensusParams' {}
Length of output: 505
go.mod (1)

283-283: Verify compatibility with the new dependency.

The dependency github.com/osmosis-labs/osmosis/v15 has been replaced with a version under the dymensionxyz namespace. Ensure that this change is intentional and verify its compatibility with the existing codebase.
mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go (4)
365-366: LGTM! But verify the usage of the new parameters.

The RunAndReturn method in MockClientI_Init_Call has been updated to reflect the new parameter names and types. Ensure that all references to this method are updated accordingly.

Line range hint 306-326: LGTM! But verify the usage of the new parameters.

The Init method has been updated to include a rollappId parameter instead of _a1, and _a2 instead of the previous _a1 for the *pubsub.Server type. Ensure that all references to this method are updated accordingly.

Verification successful

The Init method in MockClientI has been updated correctly with the new parameters.

The search results confirm that the Init method in MockClientI is using the new parameters config, rollappId, _a2, logger, and options consistently.

mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the updated `Init` method with the new parameters.

# Test: Search for the `Init` method usage. Expect: Only occurrences of the new signature.
rg --type go 'Init'
Length of output: 24066

Script:
#!/bin/bash
# Description: Verify the usage of the updated `Init` method in MockClientI with the new parameters.

# Test: Search for MockClientI Init method usage. Expect: Only occurrences of the new signature.
rg --type go 'MockClientI.*Init'
Length of output: 1502
338-345: LGTM! But verify the usage of the new parameters.

The Init method in MockClientI_Expecter has been updated to reflect the new parameter names and types. Ensure that all references to this method are updated accordingly.

Verification successful

Verified: The Init method in MockClientI_Expecter is correctly updated and referenced with the new parameters.

The updated Init method in MockClientI_Expecter has been verified to reflect the new parameter names and types.

No inconsistencies or additional occurrences were found.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the updated `Init` method in `MockClientI_Expecter` with the new parameters.

# Test: Search for the `Init` method usage. Expect: Only occurrences of the new signature.
rg --type go 'Init'
Length of output: 24066

Script:
#!/bin/bash
# Description: Verify the usage of the updated `Init` method in `MockClientI_Expecter` with the new parameters.

# Test: Search for the `Init` method usage specifically within `MockClientI_Expecter`. Expect: Only occurrences of the new signature.
rg --type go 'MockClientI_Expecter.*Init'
Length of output: 273
347-356: LGTM! But verify the usage of the new parameters.

The Run method in MockClientI_Init_Call has been updated to reflect the new parameter names and types. Ensure that all references to this method are updated accordingly.

Verification successful

LGTM! The Run method in MockClientI_Init_Call has been updated to reflect the new parameter names and types.

The search results confirm that there are no other usages of MockClientI_Init_Call outside of its definition. Therefore, the changes to the method's parameters do not impact other parts of the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the updated `Run` method in `MockClientI_Init_Call` with the new parameters.

# Test: Search for the `Run` method usage. Expect: Only occurrences of the new signature.
rg --type go 'Run'
Length of output: 118944

Script:
#!/bin/bash
# Description: Verify the usage of the updated `Run` method in `MockClientI_Init_Call` with the new parameters.

# Test: Search for the `MockClientI_Init_Call` class usage to verify the new `Run` method parameters.
rg --type go 'MockClientI_Init_Call'
Length of output: 1289
settlement/dymension/dymension.go (6)
51-51: LGTM!

The Client struct has been updated to include a new field rollappId. Ensure that this field is used consistently across the struct.

Line range hint 250-257: LGTM!

The method getStateInfo has been updated to use c.rollappId instead of c.config.RollappID. The changes are correctly implemented.

328-331: LGTM!

The method GetSequencers has been updated to use c.rollappId instead of c.config.RollappID. The changes are correctly implemented.

384-387: LGTM!

The method eventHandler has been updated to use c.rollappId instead of c.config.RollappID. The changes are correctly implemented.

435-438: LGTM!

The method convertBatchToMsgUpdateState has been updated to use c.rollappId instead of c.config.RollappID. The changes are correctly implemented.

Line range hint 72-109: LGTM! But verify the usage of the new parameter.

The Init method has been updated to accept a rollappId parameter and initialize the rollappId field. Ensure that all references to this method are updated accordingly.

Verification successful

LGTM! But verify the usage of the new parameter.

The Init method has been updated to accept a rollappId parameter and initialize the rollappId field. The search results show that all references to this method have been updated accordingly.

settlement/local/local.go

settlement/grpc/grpc.go

settlement/local/local_test.go

settlement/dymension/dymension_test.go

node/node.go

block/manager_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the updated `Init` method with the new `rollappId` parameter.

# Test: Search for the `Init` method usage. Expect: Only occurrences of the new signature.
rg --type go 'Init'
Length of output: 24066
block/manager_test.go (14)

41-43: LGTM!

The configuration structure update to use NodeConfig enhances organization and maintainability.

50-50: LGTM!

The use of genesis.ChainID in the settlementlc client initialization improves consistency and traceability.

134-134: LGTM!

The GetManager function call update to remove unnecessary parameters streamlines the initialization process.

121-128: LGTM!

The enhancement of the EndBlock method in the mocked application to include parameters for RollappConsensusParams allows for more realistic test scenarios.

180-180: LGTM!

The GetManager function call update to remove unnecessary parameters streamlines the initialization process.

214-214: LGTM!

The GetManager function call update to remove unnecessary parameters streamlines the initialization process.

200-207: LGTM!

The enhancement of the EndBlock method in the mocked application to include parameters for RollappConsensusParams allows for more realistic test scenarios.

244-244: LGTM!

The GetManager function call update to remove unnecessary parameters streamlines the initialization process.

280-280: LGTM!

The GetManager function call update to remove unnecessary parameters streamlines the initialization process.

337-344: LGTM!

The enhancement of the EndBlock method in the mocked application to include parameters for RollappConsensusParams allows for more realistic test scenarios.

381-381: LGTM!

The GetManager function call update to use managerConfig instead of testutil.GetManagerConfig() enhances configuration management.

365-372: LGTM!

The enhancement of the EndBlock method in the mocked application to include parameters for RollappConsensusParams allows for more realistic test scenarios.

459-459: LGTM!

The GetManager function call update to remove unnecessary parameters streamlines the initialization process.

443-450: LGTM!

The enhancement of the EndBlock method in the mocked application to include parameters for RollappConsensusParams allows for more realistic test scenarios.

da/celestia/celestia.go (1)

641-644: LGTM!

The new method GetMaxBlobSize enhances the functionality of the DataAvailabilityLayerClient by providing a way to retrieve the maximum batch size configured.

mocks/github.com/dymensionxyz/dymint/store/mock_Store.go (3)

29-43: LGTM!

The changes to the Close method enhance the mock's functionality by allowing it to simulate error scenarios more effectively.

63-64: LGTM!

The changes to the Return method ensure that the mock can properly simulate the behavior of the Close method.

68-68: LGTM!

The changes to the RunAndReturn method ensure that the mock can properly simulate the behavior of the Close method.

rpc/client/client_test.go (4)

86-87: Simplified genesis document creation.

The use of testutil.GenerateGenesis(1) improves consistency and maintainability.

109-111: Updated naming convention for MaxBlockSkew.

The parameter MaxBatchSkew has been renamed to MaxBlockSkew and set to 10, aligning with the updated naming conventions.

445-452: Enhanced EndBlock response in tests.

The EndBlock method in the mock application now returns a detailed abci.ResponseEndBlock with RollappConsensusParamUpdates, improving test realism.

691-730: Updated EndBlock response and genesis document creation.

The EndBlock method in the mock application now returns multiple abci.ResponseEndBlock with RollappConsensusParamUpdates, and the MaxBlockSkew parameter is updated. The use of testutil.GenerateGenesis improves maintainability.

Also applies to: 746-746, 759-759, 902-902, 916-916, 990-990, 1006-1006, 1009-1009, 1022-1022, 1031-1031

da/grpc/grpc.go

coderabbitai · 2024-08-06T13:09:59Z

da/local/local.go

+// GetMaxBlobSize returns the maximum allowed blob size in the DA, used to check the max batch size configured
+func (d *DataAvailabilityLayerClient) GetMaxBlobSize() uint32 {
+	return 0
+}


Implement the logic for GetMaxBlobSize or add a TODO comment.

The method currently returns a hardcoded value of 0, which seems to be a placeholder. Ensure to implement the actual logic or add a TODO comment indicating further development is needed.

func (d *DataAvailabilityLayerClient) GetMaxBlobSize() uint32 { - return 0 + // TODO: Implement the logic to return the actual maximum blob size. + return 0 // Placeholder value }

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

// GetMaxBlobSize returns the maximum allowed blob size in the DA, used to check the max batch size configured

func (d *DataAvailabilityLayerClient) GetMaxBlobSize() uint32 {

return 0

}

// GetMaxBlobSize returns the maximum allowed blob size in the DA, used to check the max batch size configured

func (d *DataAvailabilityLayerClient) GetMaxBlobSize() uint32 {

// TODO: Implement the logic to return the actual maximum blob size.

return 0 // Placeholder value

}

da/avail/avail.go

node/node.go

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c466f22 and 45058a1.

Files selected for processing (3)

block/manager_test.go (17 hunks)
block/production_test.go (5 hunks)
block/submit_test.go (8 hunks)

Files skipped from review as they are similar to previous changes (2)

block/manager_test.go
block/production_test.go

Additional comments not posted (10)

block/submit_test.go (10)

37-37: LGTM! Simplified function call.

The removal of unnecessary parameters in the GetManager function call improves readability and maintainability.

106-106: LGTM! Enhanced test accuracy with mocked EndBlock method.

The EndBlock method now returns a structured response containing RollappConsensusParamUpdates, which enhances the accuracy of the test.

113-120: LGTM! Simplified function call and improved test setup.

The GetManager function call is simplified by removing unnecessary parameters, and the test setup is improved with the structured response from the EndBlock method.

145-145: LGTM! Enhanced test accuracy with mocked EndBlock method.

The EndBlock method now returns a structured response containing RollappConsensusParamUpdates, which enhances the accuracy of the test.

147-154: LGTM! Simplified function call and improved test setup.

The GetManagerWithProposerKey function call is simplified by removing unnecessary parameters, and the test setup is improved with the structured response from the EndBlock method.

215-223: LGTM! Enhanced test accuracy with mocked EndBlock method.

The EndBlock method now returns a structured response containing RollappConsensusParamUpdates, which enhances the accuracy of the test.

234-234: LGTM! Updated parameter naming convention.

The MaxBlockSkew parameter is updated to reflect the new naming convention, improving clarity and consistency.

239-244: LGTM! Simplified function call and improved test setup.

The GetManager function call is simplified by removing unnecessary parameters, and the test setup is improved with the structured response from the EndBlock method.

294-303: LGTM! Enhanced test accuracy with mocked EndBlock method.

The EndBlock method now returns a structured response containing RollappConsensusParamUpdates, which enhances the accuracy of the test.

311-316: LGTM! Simplified function call and improved test setup.

The GetManager function call is simplified by removing unnecessary parameters, and the test setup is improved with the structured response from the EndBlock method.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 45058a1 and b027aaf.

Files ignored due to path filters (1)

go.sum is excluded by !**/*.sum

Files selected for processing (1)

go.mod (1 hunks)

Files skipped from review as they are similar to previous changes (1)

go.mod

omritoptix

My general comment is that I think the validation flow could be improved and simplified. What it comes to DA/version we have 3 types of variants which should check: 

What’s the actual version, da (from build and config)
What dymint has in state
The new state (by the end blocker)

If (1),(2) match (which we can validate on startup), than you only need to check (1)=(3) or (2)=(3)

Currently you are validating (1) == (2) on startup and than (2) == (3) on change. I think it’s redundant and complicates things. How?   cause if (2) != (3) now you need to first save the state and than handle the mismatch after the fact, which imo is unintuitive and creates a code smell (look here).

Imo, have a generic function that validates (1) == (2). This function should be Called on:

Startup
Anytime after state update.

If the function fails, than update your local version and than state update should work as usual.

block/block.go

block/state.go

block/errors.go

block/state.go

block/manager.go

node/node.go

da/celestia/config.go

da/celestia/celestia.go

block/manager.go

omritoptix · 2024-08-11T11:26:37Z

proto/types/dymint/state.proto

+}
+
+message RollappConsensusParams {
+  Params params = 1;


why do we need another Params message here vs just inlining the params ?

it helps reading params from genesis state

I think there should not be this wrapper

proto/types/tendermint/types/params.proto

proto/types/tendermint/abci/types.proto

types/serialization.go

node/node.go

config/toml.go

config/config.go

block/produce.go

coderabbitai

Actionable comments posted: 2

Outside diff range, codebase verification and nitpick comments (6)

types/state.go (1)
67-84: Ensure robust error handling in LoadConsensusFromAppState.

The method correctly unmarshals JSON and checks for rollapp_params. Consider logging errors for better traceability and ensuring that the method handles potential edge cases, such as malformed JSON or unexpected data types.
+	// Consider adding logging for errors to improve traceability.
	if err != nil {
		return fmt.Errorf("failed to unmarshal appState: %w", err)
	}
	if !ok {
		return fmt.Errorf("rollapp_params not defined in genesis")
	}
	err = json.Unmarshal(params, &s.ConsensusParams)
	if err != nil {
		return fmt.Errorf("failed to unmarshal rollapp_params: %w", err)
	}
da/da.go (2)
219-219: Add a comment for Synced() method.

Consider adding a comment to explain the purpose of the Synced() method, which is to signal when the DA light client has finished syncing. This will improve code readability and maintainability.
+	// Synced returns a channel that signals when the DA light client has finished syncing.
222-223: Add a comment for GetMaxBlobSizeBytes() method.

Consider adding a comment to describe the purpose of the GetMaxBlobSizeBytes() method, which is to provide the maximum allowed blob size for validation purposes.
+	// GetMaxBlobSizeBytes returns the maximum allowed blob size in the DA for validation.
testutil/types.go (1)
265-265: Inconsistent AppState JSON Structure Across Files

The AppState JSON structure for rollapp_params is inconsistent across different parts of the codebase. This might lead to issues if uniformity is expected.

In testutil/types.go: Includes da, commit, block_max_gas, and block_max_size.

In rpc/json/service_test.go: Includes da and commit, but lacks block_max_gas and block_max_size.

In cmd/dymint/commands/init.go: Includes da and version (instead of commit), and is nested under app_state.

Consider aligning the JSON structure across these occurrences to ensure consistency.

Analysis chain

Verify AppState JSON structure.

The AppState field in GenerateGenesis now includes a JSON string for rollapp_params. Ensure that this JSON structure is correctly formatted and aligns with the intended configuration.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the JSON structure of `AppState` in the codebase.

# Test: Search for occurrences of `rollapp_params` JSON structure. Expect: Correct formatting and alignment with intended configuration.
rg --type go 'rollapp_params'
Length of output: 837

Script:
#!/bin/bash
# Description: Verify the JSON structure consistency across different files.

# Extract and display JSON structures for `rollapp_params` in relevant files for comparison.
rg --type go 'rollapp_params' -A 3
Length of output: 1503
block/produce.go (1)

83-83: Improve logging for full bytes produced channel.

The log message for a full bytes produced channel has been streamlined. Ensure that this change improves clarity without losing essential information.

Consider adding more context to the log message if necessary to aid in debugging.
p2p/client.go (1)
215-217: Potential issue with error handling in SaveBlock method

The change to log errors instead of returning them in the SaveBlock method may disrupt error handling in calling functions. Several instances in the codebase expect SaveBlock to return an error, which is then checked or asserted. This inconsistency could lead to unhandled errors and unexpected behavior.

Review and ensure that the error handling strategy in SaveBlock aligns with the expectations of its callers.

Consider whether logging errors is appropriate or if returning errors might be necessary for proper error propagation.

Analysis chain

Verify the impact of logging errors instead of returning them.

The change in error handling within the SaveBlock method logs errors as debug messages instead of returning them immediately. This can aid in debugging but might affect error propagation and handling by the caller. Ensure that this change aligns with the overall error handling strategy of the application.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the impact of logging errors in the `SaveBlock` method.

# Test: Search for all calls to `SaveBlock` and check how errors are handled.
rg --type go -A 5 $'SaveBlock'
Length of output: 19089

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b027aaf and b512039.

Files ignored due to path filters (2)

go.sum is excluded by !**/*.sum
types/pb/dymint/state.pb.go is excluded by !**/*.pb.go

Files selected for processing (66)

block/block.go (1 hunks)
block/errors.go (1 hunks)
block/executor.go (3 hunks)
block/executor_test.go (5 hunks)
block/manager.go (7 hunks)
block/manager_test.go (18 hunks)
block/produce.go (5 hunks)
block/production_test.go (5 hunks)
block/pruning_test.go (1 hunks)
block/retriever.go (2 hunks)
block/state.go (3 hunks)
block/submit.go (7 hunks)
block/submit_loop_test.go (7 hunks)
block/submit_test.go (9 hunks)
cmd/dymint/commands/init.go (1 hunks)
cmd/dymint/commands/root.go (1 hunks)
config/config.go (6 hunks)
config/config_test.go (4 hunks)
config/defaults.go (3 hunks)
config/flags.go (7 hunks)
config/toml.go (2 hunks)
da/avail/avail.go (1 hunks)
da/celestia/celestia.go (1 hunks)
da/celestia/config.go (1 hunks)
da/da.go (1 hunks)
da/errors.go (1 hunks)
da/grpc/grpc.go (1 hunks)
da/local/local.go (1 hunks)
go.mod (1 hunks)
mocks/github.com/dymensionxyz/dymension/v3/x/rollapp/types/mock_QueryClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymension/v3/x/sequencer/types/mock_QueryClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/da/avail/mock_SubstrateApiI.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/da/celestia/types/mock_CelestiaRPCClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/da/mock_DataAvailabilityLayerClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/settlement/dymension/mock_CosmosClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go (5 hunks)
mocks/github.com/dymensionxyz/dymint/store/mock_Store.go (3 hunks)
mocks/github.com/tendermint/tendermint/abci/types/mock_Application.go (1 hunks)
mocks/github.com/tendermint/tendermint/proxy/mock_AppConnConsensus.go (1 hunks)
mocks/github.com/tendermint/tendermint/proxy/mock_AppConns.go (1 hunks)
node/node.go (9 hunks)
node/node_test.go (4 hunks)
p2p/block_sync_test.go (1 hunks)
p2p/client.go (1 hunks)
p2p/validator_test.go (3 hunks)
proto/types/dymint/state.proto (2 hunks)
proto/types/tendermint/abci/types.proto (1 hunks)
proto/types/tendermint/types/params.proto (1 hunks)
rpc/client/client_test.go (13 hunks)
rpc/json/service_test.go (4 hunks)
settlement/config.go (2 hunks)
settlement/dymension/dymension.go (7 hunks)
settlement/dymension/dymension_test.go (3 hunks)
settlement/grpc/grpc.go (2 hunks)
settlement/local/local.go (2 hunks)
settlement/local/local_test.go (4 hunks)
settlement/settlement.go (1 hunks)
testutil/block.go (5 hunks)
testutil/node.go (2 hunks)
testutil/rpc.go (1 hunks)
testutil/types.go (3 hunks)
types/metrics.go (1 hunks)
types/serialization.go (1 hunks)
types/serialization_test.go (3 hunks)
types/state.go (3 hunks)
version/version.go (1 hunks)

Files skipped from review due to trivial changes (10)

mocks/github.com/dymensionxyz/dymension/v3/x/rollapp/types/mock_QueryClient.go
mocks/github.com/dymensionxyz/dymension/v3/x/sequencer/types/mock_QueryClient.go
mocks/github.com/dymensionxyz/dymint/da/avail/mock_SubstrateApiI.go
mocks/github.com/dymensionxyz/dymint/da/celestia/types/mock_CelestiaRPCClient.go
mocks/github.com/dymensionxyz/dymint/da/mock_DataAvailabilityLayerClient.go
mocks/github.com/dymensionxyz/dymint/settlement/dymension/mock_CosmosClient.go
mocks/github.com/tendermint/tendermint/abci/types/mock_Application.go
mocks/github.com/tendermint/tendermint/proxy/mock_AppConnConsensus.go
proto/types/tendermint/types/params.proto
types/serialization.go

Files skipped from review as they are similar to previous changes (37)

block/block.go
block/errors.go
block/executor.go
block/manager.go
block/production_test.go
block/retriever.go
block/state.go
block/submit_loop_test.go
cmd/dymint/commands/init.go
cmd/dymint/commands/root.go
config/config.go
config/config_test.go
config/defaults.go
config/flags.go
config/toml.go
da/avail/avail.go
da/celestia/celestia.go
da/celestia/config.go
da/errors.go
da/grpc/grpc.go
mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go
mocks/github.com/dymensionxyz/dymint/store/mock_Store.go
mocks/github.com/tendermint/tendermint/proxy/mock_AppConns.go
node/node.go
node/node_test.go
p2p/validator_test.go
proto/types/tendermint/abci/types.proto
rpc/client/client_test.go
rpc/json/service_test.go
settlement/config.go
settlement/dymension/dymension.go
settlement/dymension/dymension_test.go
settlement/local/local.go
settlement/local/local_test.go
settlement/settlement.go
testutil/block.go
testutil/rpc.go

Additional context used

GitHub Check: CodeQL

block/produce.go

[notice] 151-151: Floating point arithmetic
Floating point arithmetic operations are not associative and a possible source of non-determinism

Additional comments not posted (36)

version/version.go (1)

3-6: LGTM! The changes improve version tracking.

The addition of the Commit variable alongside BuildVersion enhances the ability to track specific builds and commits, which is beneficial for debugging and deployment.

proto/types/dymint/state.proto (3)

32-32: Reserved field 12 for future compatibility.

Using the reserved keyword ensures that the field number 12 won't be reused accidentally, preventing potential conflicts with previous versions.

41-46: Introduction of RollappConsensusParams enhances parameter management.

The new RollappConsensusParams message encapsulates consensus parameters, providing a more structured and flexible way to manage these settings.

49-54: New Params message improves parameter clarity.

The Params message provides clear definitions for consensus parameters, enhancing readability and maintainability.
p2p/block_sync_test.go (1)
19-19: Verify the GetManager function signature change.

The removal of a nil argument suggests a change in the GetManager function signature. Ensure that this aligns with the intended refactoring and does not affect functionality.

Verification successful

GetManager Function Signature Change Verified

The GetManager function signature has been updated, and all instances across the codebase have been modified to align with this change. The removal of a nil argument in the test file is consistent with the new function signature.

Instances found in: p2p/block_sync_test.go, block/submit_test.go, block/pruning_test.go, block/production_test.go, block/manager_test.go.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the `GetManager` function signature change.

# Test: Search for the `GetManager` function definition. Expect: The signature matches the updated call.
rg --type go 'func GetManager'
Length of output: 627

Script:
#!/bin/bash
# Search for all instances where `GetManager` is called to ensure consistency across the codebase.
rg --type go 'GetManager(' -A 2
Length of output: 106

Script:
#!/bin/bash
# Corrected regex pattern to search for all instances where `GetManager` is called.
rg --type go 'GetManager\(' -A 2
Length of output: 3788
testutil/node.go (3)
33-40: Mock setup for EndBlock response looks good.

The mock setup for the EndBlock method now includes consensus parameter updates. This enhances the test environment by simulating realistic node behavior.

24-24: Update function signature to include genesis parameter.

The CreateNode function now requires a genesis parameter. Ensure that all calls to this function are updated to pass the appropriate genesis document.

Verification successful

All calls to CreateNode have been updated to include the genesis parameter.

The function is correctly invoked with the new parameter in the following files:

testutil/rpc.go

node/node_test.go

No further action is needed.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `CreateNode` include the new `genesis` parameter.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'CreateNode'
Length of output: 1080
60-60: Simplify settlement configuration.

The RollappID has been removed from the SettlementConfig, which may simplify the configuration process. Ensure that this change aligns with the overall architecture and that the ProposerPubKey is correctly utilized.

Verification successful

RollappID removal is consistent and does not affect the codebase.

The RollappID is no longer referenced in the codebase, confirming that its removal from SettlementConfig is consistent and does not introduce issues. The configuration process is indeed simplified as a result.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of `SettlementConfig` to ensure `RollappID` removal is consistent.

# Test: Search for `SettlementConfig` usage. Expect: No reliance on `RollappID`.
rg --type go -A 5 $'SettlementConfig'
Length of output: 7547

Script:
#!/bin/bash
# Description: Search for any remaining references to `RollappID` in the codebase.

rg --type go 'RollappID'
Length of output: 24
block/pruning_test.go (2)

21-28: Mock setup for EndBlock response in tests.

The mock setup for the EndBlock method now includes updates to RollappConsensusParams, enhancing the test's realism. Ensure that these parameters align with the expected test scenarios.

36-39: Improve manager initialization with mock DA client.

The manager.DAClient is now assigned a mock data availability client, and manager.Retriever is explicitly set. This enhances the test setup by providing more predictable behavior. Ensure that the mock client accurately reflects the expected interactions.
types/metrics.go (1)
28-30: Update metric name for clarity.

The metric name has been updated to reflect a focus on "blocks" rather than "batches." Ensure that this change is consistently applied in all related documentation and code.

Verification successful

Metric name update verified.

The metric name has been consistently updated to RollappPendingSubmissionsSkewNumBlocks across the codebase. No occurrences of the old name were found.

types/metrics.go

block/submit.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the consistent usage of the updated metric name across the codebase.

# Test: Search for the old and new metric names. Expect: Only occurrences of the new name.
rg --type go $'RollappPendingSubmissionsSkewNumBatches|RollappPendingSubmissionsSkewNumBlocks'
Length of output: 403
types/state.go (1)
34-34: Verify the impact of changing ConsensusParams type.

The type of ConsensusParams has been changed to dymint.RollappConsensusParams. Ensure that all usages of this field are compatible with the new type and that any necessary adjustments have been made throughout the codebase.

Verification successful

The impact of changing ConsensusParams type is verified.

The type change to dymint.RollappConsensusParams has been integrated across the codebase, with adjustments made in serialization, deserialization, and test scenarios. The change appears to be handled correctly, ensuring compatibility with the new type.

Usage in types/state.go and serialization logic.

Test scenarios in serialization_test.go and other test files.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all usages of `ConsensusParams` to ensure compatibility with the new type.

# Test: Search for `ConsensusParams` usage. Expect: Compatibility with `dymint.RollappConsensusParams`.
rg --type go -A 5 $'ConsensusParams'
Length of output: 30169
types/serialization_test.go (1)
Line range hint 102-130:
Verify test coverage for RollappConsensusParams.

The test has been updated to use pb.RollappConsensusParams. Ensure that all relevant scenarios are covered, including edge cases and potential failure modes. Consider adding more test cases if necessary to fully validate the new structure.

Verification successful

Test Coverage for RollappConsensusParams Appears Sufficient

The RollappConsensusParams is utilized in various test cases across multiple files, indicating that it is being tested under different scenarios and conditions. This suggests a reasonable level of test coverage for this component. If specific edge cases or failure modes are of concern, consider reviewing the individual test cases for completeness.

types/serialization_test.go

rpc/client/client_test.go

block/submit_test.go

block/production_test.go

block/manager_test.go

block/pruning_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify test coverage for `RollappConsensusParams`.

# Test: Search for additional test cases using `RollappConsensusParams`. Expect: Comprehensive coverage.
rg --type go -A 5 $'RollappConsensusParams'
Length of output: 16249

Script:
#!/bin/bash
# Description: Extract detailed information about test cases involving `RollappConsensusParams`.

# Extract test functions and their bodies where `RollappConsensusParams` is used.
rg --type go -A 20 'func Test.*' | rg -A 20 'RollappConsensusParams'
Length of output: 14866
da/local/local.go (1)

177-180: Implement the logic for GetMaxBlobSizeBytes or add a TODO comment.

The method currently returns a hardcoded value of 0, which seems to be a placeholder. Ensure to implement the actual logic or add a TODO comment indicating further development is needed.
testutil/types.go (1)
232-237: Ensure correct initialization of ConsensusParams.

The addition of ConsensusParams in GenerateState enhances the state generation logic. Ensure that the dymint.RollappConsensusParams structure is correctly initialized and aligns with the intended configuration.

Verification successful

Initialization of ConsensusParams is correct

The ConsensusParams in testutil/types.go is initialized correctly with dymint.RollappConsensusParams. The fields Da and Commit match the expected structure and configuration. This setup is consistent with the intended use of these parameters.

The Params struct in types/pb/dymint/state.pb.go includes the fields Da and Commit, confirming the correct initialization.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the initialization of `RollappConsensusParams` in the codebase.

# Test: Search for the initialization of `RollappConsensusParams`. Expect: Correct initialization with intended values.
rg --type go 'RollappConsensusParams'
Length of output: 6309

Script:
#!/bin/bash
# Search for the definition of `RollappConsensusParams` and `Params` in the codebase to verify their structure and expected values.
ast-grep --lang go --pattern 'type RollappConsensusParams struct { $$$ }'
ast-grep --lang go --pattern 'type Params struct { $$$ }'
Length of output: 2321
block/produce.go (2)
189-193: Validate configuration params with consensus params.

The addition of ValidateConfigWithRollappParams ensures alignment between configuration and consensus parameters. Ensure that this validation is comprehensive and covers all necessary parameters.

Verification successful

Comprehensive Validation Confirmed for Configuration and Consensus Parameters

The ValidateConfigWithRollappParams function effectively ensures alignment by validating key parameters such as versioning, batch sizes, and DA client types. No further action is required.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the validation logic for configuration and consensus parameters.

# Test: Search for the implementation of `ValidateConfigWithRollappParams`. Expect: Comprehensive validation covering all necessary parameters.
rg --type go 'ValidateConfigWithRollappParams'
Length of output: 462

Script:
#!/bin/bash
# Fetch the implementation details of `ValidateConfigWithRollappParams` function in `block/manager.go`.
rg --type go -A 20 'func (m \*Manager) ValidateConfigWithRollappParams' block/manager.go
Length of output: 94

Script:
#!/bin/bash
# Retry fetching the implementation details of `ValidateConfigWithRollappParams` function in `block/manager.go` with a broader pattern.
rg --type go -A 20 'func $m \*Manager$ ValidateConfigWithRollappParams' block/manager.go
Length of output: 1152
Line range hint 43-60:
Review block production logic.

The addition of logic to determine whether to produce an empty block refines block production under idle conditions. Ensure that this logic aligns with the intended behavior and does not introduce unintended side effects.

Verification successful

Block production logic verified successfully. The logic for determining when to produce an empty block is consistent with the intended behavior, and there are no apparent unintended side effects in the ProduceAndGossipBlock function.

The produceEmptyBlock condition correctly incorporates firstBlock, MaxIdleTime, and nextEmptyBlock checks.

The ProduceAndGossipBlock function handles the allowEmpty parameter appropriately and manages errors effectively.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the logic for producing empty blocks in the codebase.

# Test: Search for the logic related to empty block production. Expect: Correct implementation without unintended side effects.
rg --type go 'produceEmptyBlock'
Length of output: 238

Script:
#!/bin/bash
# Search for the definition and usage of 'firstBlock' in the codebase
rg --type go 'firstBlock'

# Search for the definition and usage of 'MaxIdleTime' in the codebase
rg --type go 'MaxIdleTime'

# Search for the definition and usage of 'nextEmptyBlock' in the codebase
rg --type go 'nextEmptyBlock'

# Search for the implementation of 'ProduceAndGossipBlock' function
ast-grep --lang go --pattern 'func ProduceAndGossipBlock($_, $_) ($_, $_, error) { $$$ }'
Length of output: 2869

Script:
#!/bin/bash
# Search for the implementation of 'ProduceAndGossipBlock' function with potential variations in parameters
rg --type go 'func ProduceAndGossipBlock'
Length of output: 43

Script:
#!/bin/bash
# Broaden the search for 'ProduceAndGossipBlock' function to locate its definition or usage
rg --type go 'ProduceAndGossipBlock'
Length of output: 923

Script:
#!/bin/bash
# Retrieve the implementation of the 'ProduceAndGossipBlock' function from 'block/produce.go'
ast-grep --lang go --pattern 'func (m *Manager) ProduceAndGossipBlock(ctx context.Context, allowEmpty bool) (*types.Block, *types.Commit, error) { $$$ }'
Length of output: 798
settlement/grpc/grpc.go (1)
51-51: LGTM! But verify the Init method usage in the codebase.

The change to pass rollappId directly as an argument enhances flexibility by decoupling it from the configuration. Ensure that all calls to Init are updated to match the new signature.

Verification successful

All Init method calls are correctly updated. The Init method calls throughout the codebase have been updated to match the new signature, with rollappId passed as a separate argument. No further action is needed.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all calls to the `Init` method match the new signature.

# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'Init($_, $_, $_, $_, $$$)'
Length of output: 47

Script:
#!/bin/bash
# Description: Search for all calls to the `Init` method in the Go codebase.

# Test: Use a simpler pattern to find all occurrences of the `Init` method.
rg --type go 'Init\(' -A 3
Length of output: 11718
block/submit.go (3)
31-34: Approve the transition from batch-based to block-based processing.

The changes to use maxBlockSkew and pendingSubmittedBlocks reflect a shift towards block-based processing, which can improve efficiency. Ensure that the logic correctly handles these parameters and that metrics are updated accordingly.

80-80: Ensure metrics are correctly updated for pending submissions.

The metrics for pending submissions now track blocks instead of batches. Verify that these metrics are correctly updated throughout the codebase.

44-45: Verify the logic for handling maxBlockSkew and pendingSubmittedBlocks.

Ensure that the logic in SubmitLoopInner correctly handles the new parameters for block-based processing.

Verification successful

Logic for Handling maxBlockSkew and pendingSubmittedBlocks Verified

The SubmitLoopInner function in block/submit.go correctly uses maxBlockSkew and pendingSubmittedBlocks to control the block submission process. The logic ensures that the number of pending blocks does not exceed the allowed skew, effectively managing the submission flow.

File: block/submit.go

Function: SubmitLoopInner

The implementation aligns with the intended functionality.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the logic handling `maxBlockSkew` and `pendingSubmittedBlocks`.

# Test: Check for the correct usage of these parameters in the codebase.
rg --type go -A 5 $'SubmitLoopInner($_, $_, $_, $_, $_, $_, $_)'
Length of output: 65

Script:
#!/bin/bash
# Search for the definition of the SubmitLoopInner function to examine its implementation.
ast-grep --lang go --pattern 'func SubmitLoopInner($$$) { $$$ }'
Length of output: 66

Script:
#!/bin/bash
# Search for Go functions that include `maxBlockSkew` or `pendingSubmittedBlocks` as parameters.
rg --type go 'func .*maxBlockSkew.*' -A 10
rg --type go 'func .*pendingSubmittedBlocks.*' -A 10
Length of output: 99

Script:
#!/bin/bash
# Search for occurrences of `maxBlockSkew` in Go files to understand its usage.
rg --type go 'maxBlockSkew' -A 5 -B 5

# Search for occurrences of `pendingSubmittedBlocks` in Go files to understand its usage.
rg --type go 'pendingSubmittedBlocks' -A 5 -B 5
Length of output: 3854
block/executor_test.go (3)
54-59: Approve the refactor to use dymint.RollappConsensusParams.

The refactor enhances the organization and potential extensibility of consensus parameter management in tests. Ensure all tests are updated to reflect this new structure.

98-107: Verify the mock response for EndBlock reflects the new consensus structure.

Ensure that the mock response for EndBlock is correctly updated to accommodate the new consensus parameters structure.

Verification successful

Mock Response for EndBlock Reflects New Consensus Structure

The search results indicate that the EndBlock mock responses have been updated to include the RollappConsensusParamUpdates with the new consensus structure, as seen in multiple test files.

Files with Updated Mock Responses:

block/executor_test.go

block/submit_test.go

block/manager_test.go

rpc/client/client_test.go

rpc/json/service_test.go

These updates reflect the new consensus parameters structure, confirming that the mock responses are correctly aligned with the changes.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the mock response for `EndBlock` in tests.

# Test: Search for `EndBlock` mock responses in the test files. Expect: Correct updates for the new structure.
rg --type go -A 5 $'EndBlock(mock.Anything).Return'
Length of output: 52

Script:
#!/bin/bash
# Broaden the search for `EndBlock` mock responses and any related changes in the codebase.

# Test: Search for `EndBlock` in Go files to find all occurrences and check if they reflect the new consensus structure.
rg --type go -A 10 'EndBlock'
Length of output: 41078
259-263: Ensure the rollapp parameters update is correctly tested.

The test checks for rollapp parameter updates. Verify that this test accurately reflects the expected behavior.

Verification successful

Rollapp Parameters Update is Correctly Tested

The assertions in block/executor_test.go correctly verify the rollapp parameter updates for Da, Commit, BlockMaxSize, and BlockMaxGas. These parameters are checked against their expected values, ensuring the test reflects the intended behavior.

Assertions found in block/executor_test.go:

assert.Equal(state.ConsensusParams.Params.Da, "celestia")

assert.Equal(state.ConsensusParams.Params.Commit, "abcde")

assert.Equal(state.ConsensusParams.Params.BlockMaxSize, int64(100))

assert.Equal(state.ConsensusParams.Params.BlockMaxGas, int64(100))
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the test for rollapp parameters update.

# Test: Check for assertions on rollapp parameters in the test files. Expect: Correct assertions for updated parameters.
rg --type go -A 5 $'assert.Equal(state.ConsensusParams.Params.$_)'
Length of output: 67

Script:
#!/bin/bash
# Search for the specific assertions in the block/executor_test.go file
fd 'executor_test.go' | xargs rg 'assert.Equal(state.ConsensusParams.Params.'
Length of output: 183

Script:
#!/bin/bash
# Correct the regex pattern and search for assertions in block/executor_test.go
fd 'executor_test.go' | xargs rg 'assert\.Equal\(state\.ConsensusParams\.Params\.'
Length of output: 338
block/submit_test.go (5)

37-37: LGTM: Manager instantiation streamlined.

The use of GetManager with fewer parameters simplifies the test setup.

Line range hint 106-139:
LGTM: Enhanced EndBlock simulation.

The inclusion of RollappConsensusParamUpdates in the EndBlock response improves test accuracy.

Line range hint 145-202:
LGTM: Effective simulation of failure and recovery.

The test accurately handles failure scenarios and validates recovery, aligning with the PR objectives.

Line range hint 215-244:
LGTM: Time-triggered submission test.

The test configuration and logic effectively validate time-based submission triggers.

294-316: LGTM: Batch size-triggered submission test.

The test effectively validates submission based on batch size constraints.

go.mod (1)

299-299: Verify impact of dependency change.

The replacement of tendermint with cometbft may introduce new functionality or changes. Verify compatibility and performance implications.

block/manager_test.go (6)

44-53: LGTM: Improved configuration and settlement initialization.

The use of NodeConfig and genesis.ChainID enhances configuration management and settlement layer interaction.

Line range hint 123-169:
LGTM: Accurate syncing and block production simulation.

The test effectively validates block production post-sync, with enhanced consensus parameter simulation.

183-188: LGTM: DA batch retrieval failure simulation.

The test accurately handles DA batch retrieval failures, aligning with the PR's error handling improvements.

201-218: LGTM: Block production and state update validation.

The test accurately validates block production and state updates with enhanced consensus parameter simulation.

Line range hint 275-348:
LGTM: Block production failure and recovery simulation.

The test accurately handles failure scenarios after committing, aligning with the PR's error handling improvements.

Line range hint 368-434:
LGTM: Batch creation with bytes limit validation.

The test accurately validates batch creation constraints based on bytes limit.

block/manager_test.go

coderabbitai

Actionable comments posted: 1

Outside diff range, codebase verification and nitpick comments (1)

block/submit_test.go (1)
37-37: Discrepancy in GetManager Function Call

The GetManager function call in block/submit_test.go does not match the actual function signature found in testutil/block.go. The current signature requires seven parameters, while the test call uses fewer. Please update the test to align with the function's signature.

File: block/submit_test.go

Function Signature: func GetManager(conf config.BlockManagerConfig, settlementlc settlement.ClientI, genesisHeight, storeInitialHeight, storeLastBlockHeight int64, proxyAppConns proxy.AppConns, mockStore store.Store) (*block.Manager, error)

Analysis chain

LGTM! But verify the function signature.

The GetManager function call is simplified by removing unnecessary parameters.

Ensure that the GetManager function signature matches this update.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the `GetManager` function signature.

# Test: Search for the `GetManager` function implementation. Expect: Correct number of parameters.
ast-grep --lang go --pattern $'func GetManager($_, $_, $_, $_, $_, $_, $_) ($_, $_) {
  $$$
}'
Length of output: 95

Script:
#!/bin/bash
# Description: Search for the `GetManager` function with any parameter count to verify its current signature.

# Test: Search for the `GetManager` function implementation with any number of parameters.
ast-grep --lang go --pattern $'func GetManager($$$) ($_, $_) {
  $$$
}'
Length of output: 702

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b512039 and 5257713.

Files ignored due to path filters (2)

go.sum is excluded by !**/*.sum
types/pb/dymint/state.pb.go is excluded by !**/*.pb.go

Files selected for processing (66)

block/block.go (1 hunks)
block/errors.go (1 hunks)
block/executor.go (3 hunks)
block/executor_test.go (5 hunks)
block/manager.go (7 hunks)
block/manager_test.go (18 hunks)
block/produce.go (5 hunks)
block/production_test.go (5 hunks)
block/pruning_test.go (1 hunks)
block/retriever.go (2 hunks)
block/state.go (3 hunks)
block/submit.go (7 hunks)
block/submit_loop_test.go (7 hunks)
block/submit_test.go (9 hunks)
cmd/dymint/commands/init.go (1 hunks)
cmd/dymint/commands/root.go (1 hunks)
config/config.go (6 hunks)
config/config_test.go (4 hunks)
config/defaults.go (3 hunks)
config/flags.go (7 hunks)
config/toml.go (2 hunks)
da/avail/avail.go (1 hunks)
da/celestia/celestia.go (1 hunks)
da/celestia/config.go (1 hunks)
da/da.go (1 hunks)
da/errors.go (1 hunks)
da/grpc/grpc.go (1 hunks)
da/local/local.go (1 hunks)
go.mod (1 hunks)
mocks/github.com/dymensionxyz/dymension/v3/x/rollapp/types/mock_QueryClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymension/v3/x/sequencer/types/mock_QueryClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/da/avail/mock_SubstrateApiI.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/da/celestia/types/mock_CelestiaRPCClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/da/mock_DataAvailabilityLayerClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/settlement/dymension/mock_CosmosClient.go (1 hunks)
mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go (5 hunks)
mocks/github.com/dymensionxyz/dymint/store/mock_Store.go (3 hunks)
mocks/github.com/tendermint/tendermint/abci/types/mock_Application.go (1 hunks)
mocks/github.com/tendermint/tendermint/proxy/mock_AppConnConsensus.go (1 hunks)
mocks/github.com/tendermint/tendermint/proxy/mock_AppConns.go (1 hunks)
node/node.go (9 hunks)
node/node_test.go (4 hunks)
p2p/block_sync_test.go (1 hunks)
p2p/client.go (1 hunks)
p2p/validator_test.go (3 hunks)
proto/types/dymint/state.proto (2 hunks)
proto/types/tendermint/abci/types.proto (1 hunks)
proto/types/tendermint/types/params.proto (1 hunks)
rpc/client/client_test.go (13 hunks)
rpc/json/service_test.go (4 hunks)
settlement/config.go (2 hunks)
settlement/dymension/dymension.go (7 hunks)
settlement/dymension/dymension_test.go (3 hunks)
settlement/grpc/grpc.go (2 hunks)
settlement/local/local.go (2 hunks)
settlement/local/local_test.go (4 hunks)
settlement/settlement.go (1 hunks)
testutil/block.go (5 hunks)
testutil/node.go (2 hunks)
testutil/rpc.go (1 hunks)
testutil/types.go (3 hunks)
types/metrics.go (1 hunks)
types/serialization.go (1 hunks)
types/serialization_test.go (3 hunks)
types/state.go (3 hunks)
version/version.go (1 hunks)

Files skipped from review due to trivial changes (13)

da/celestia/celestia.go
mocks/github.com/dymensionxyz/dymension/v3/x/rollapp/types/mock_QueryClient.go
mocks/github.com/dymensionxyz/dymension/v3/x/sequencer/types/mock_QueryClient.go
mocks/github.com/dymensionxyz/dymint/da/avail/mock_SubstrateApiI.go
mocks/github.com/dymensionxyz/dymint/da/celestia/types/mock_CelestiaRPCClient.go
mocks/github.com/dymensionxyz/dymint/da/mock_DataAvailabilityLayerClient.go
mocks/github.com/dymensionxyz/dymint/settlement/dymension/mock_CosmosClient.go
mocks/github.com/tendermint/tendermint/abci/types/mock_Application.go
mocks/github.com/tendermint/tendermint/proxy/mock_AppConnConsensus.go
mocks/github.com/tendermint/tendermint/proxy/mock_AppConns.go
settlement/config.go
types/metrics.go
types/serialization.go

Files skipped from review as they are similar to previous changes (39)

block/block.go
block/errors.go
block/executor.go
block/manager.go
block/manager_test.go
block/production_test.go
block/pruning_test.go
block/retriever.go
block/state.go
block/submit_loop_test.go
cmd/dymint/commands/init.go
cmd/dymint/commands/root.go
config/defaults.go
config/flags.go
config/toml.go
da/avail/avail.go
da/celestia/config.go
da/da.go
da/errors.go
da/grpc/grpc.go
da/local/local.go
mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go
node/node_test.go
p2p/client.go
p2p/validator_test.go
proto/types/tendermint/abci/types.proto
proto/types/tendermint/types/params.proto
rpc/client/client_test.go
rpc/json/service_test.go
settlement/dymension/dymension.go
settlement/dymension/dymension_test.go
settlement/grpc/grpc.go
settlement/local/local.go
settlement/local/local_test.go
settlement/settlement.go
testutil/block.go
testutil/rpc.go
testutil/types.go
version/version.go

Additional context used

GitHub Check: CodeQL

block/produce.go

[notice] 151-151: Floating point arithmetic
Floating point arithmetic operations are not associative and a possible source of non-determinism

Additional comments not posted (38)

proto/types/dymint/state.proto (3)

32-32: Correct use of reserved keyword and new field type.

The reserved keyword is correctly used to prevent future conflicts with field number 12. The introduction of RollappConsensusParams at position 18 aligns with the intended design changes.

Also applies to: 41-41

45-46: Logical encapsulation of parameters.

The encapsulation of Params within RollappConsensusParams is logical and aligns with the goal of modularizing consensus parameters.

49-53: Appropriate field types and names in Params.

The field types and names in the Params message are appropriate and consistent with the overall system design.

p2p/block_sync_test.go (1)

19-19: Alignment with updated function signature.

The removal of a nil argument in the GetManager function call aligns with the updated function signature. Ensure that this change does not affect the intended test logic.

testutil/node.go (1)

24-40: Enhanced node initialization and improved mock setup.

The addition of the genesis parameter enhances node initialization with specific configurations. The mock setup changes improve testing accuracy by providing a more comprehensive simulation of node behavior.

types/state.go (2)

34-34: LGTM! ConsensusParams type change is appropriate.

The change from tmproto.ConsensusParams to dymint.RollappConsensusParams aligns with the PR objectives of enhancing rollapp parameter handling.

67-84: LGTM! LoadConsensusFromAppState is well-implemented.

The method correctly unmarshals JSON data and handles errors appropriately, aligning with the PR's objective to load rollapp parameters from the application state.

types/serialization_test.go (1)

Line range hint 102-130:
LGTM! TestStateRoundTrip changes align with new consensus parameters.

The test cases have been updated to reflect the new pb.RollappConsensusParams structure, ensuring that the serialization logic is correctly validated.

config/config_test.go (2)

27-35: LGTM! BlockTime assertion change is appropriate.

The change from 1234s to 4s for BlockTime aligns with the updated configuration constraints and PR objectives.

Line range hint 64-135:
LGTM! New validation test cases enhance coverage.

The additional test cases effectively cover the new configuration constraints, ensuring robust validation of node configuration parameters.

config/config.go (6)

21-24: Introduction of constants enhances configurability.

The new constants MinBlockTime, MaxBlockTime, MaxBatchSubmitTime, and MaxBatchSkewBlocks provide clear limits for block timing parameters, which improves configurability and maintainability.

58-62: Updated BlockManagerConfig aligns with new configuration requirements.

The addition of BatchSubmitTime and BatchSkewBlocks to BlockManagerConfig and their validation logic ensures compliance with new configuration requirements.

130-132: Validation for BlockTime ensures it is within acceptable limits.

The validation logic for BlockTime now checks against MinBlockTime, ensuring robustness in block production timing.

134-135: Validation for BlockTime ensures it does not exceed MaxBlockTime.

The updated validation logic for BlockTime ensures it does not exceed MaxBlockTime, aligning with system requirements.

151-165: Validation for BatchSubmitTime ensures it is within acceptable limits.

The validation logic for BatchSubmitTime now checks that it is positive, greater than BlockTime, greater than MaxIdleTime, and does not exceed MaxBatchSubmitTime.

171-177: Validation for BatchSkewBlocks ensures it is within acceptable limits.

The validation logic for BatchSkewBlocks ensures it is greater than a calculated minimum and does not exceed MaxBatchSkewBlocks, enhancing robustness.

block/produce.go (2)

Line range hint 43-60: Improved block production logic and logging clarity.

The changes to ProduceBlockLoop enhance the logic for producing empty blocks and improve the clarity of logging messages.

189-194: Validation step ensures configuration alignment.

The introduction of a validation step to ensure configuration parameters align with rollapp consensus parameters enhances error handling and system robustness.

block/submit.go (3)

31-34: Updated SubmitLoop aligns with block-based submission logic.

The changes to SubmitLoop reflect the transition to a block-based submission approach, improving clarity and efficiency.

44-45: Enhanced submission logic in SubmitLoopInner.

The changes to SubmitLoopInner focus on block-based submission, improving the efficiency and clarity of the submission process.

259-261: Introduction of GetUnsubmittedBlocks enhances tracking.

The GetUnsubmittedBlocks method provides an efficient mechanism to track unsubmitted blocks, supporting the new block-based submission logic.

node/node.go (5)

61-61: LGTM!

The initialization of baseKV within the Node struct is appropriate.

121-122: Skip comment.

The existing comment from a previous review already addresses the TODO comment regarding dalcKV.

163-173: LGTM!

The inclusion of dalcKV in the BlockManager initialization enhances modularity.

192-192: LGTM!

The assignment of baseKV to the Node struct is consistent and appropriate.

131-131: LGTM! But verify the usage of genesis.ChainID.

The inclusion of genesis.ChainID in the settlementlc.Init method seems appropriate.

Ensure that genesis.ChainID is correctly utilized within the Init method.

block/executor_test.go (4)

54-59: LGTM!

The initialization of ConsensusParams using dymint.RollappConsensusParams is correct and aligns with the new structure.

98-107: LGTM!

The EndBlock mock response correctly includes RollappConsensusParamUpdates, ensuring proper test validation.

161-169: LGTM!

The initialization of ConsensusParams with additional fields like Da and Commit is appropriate and enhances test setup.

259-263: LGTM!

The assertions for checking updated rollapp parameters are correct and ensure proper validation.

block/submit_test.go (5)

15-16: LGTM!

The imports for abci and log packages are necessary and correctly included.

24-28: LGTM!

The imports for da, slmocks, testutil, and version packages are necessary and correctly included.

106-120: LGTM!

The EndBlock mock response correctly includes RollappConsensusParamUpdates, ensuring proper test validation.

124-126: LGTM!

The assignments of DAClient and Retriever to the manager are correct and ensure necessary functionality.

139-139: LGTM!

The CreateAndSubmitBatch method call with BatchSubmitBytes reflects the updated naming convention.

go.mod (1)

299-299: Verify compatibility with the new dependency.

The dependency github.com/tendermint/tendermint has been replaced with github.com/dymensionxyz/cometbft. Ensure that all parts of the codebase are compatible with this new version.

mocks/github.com/dymensionxyz/dymint/store/mock_Store.go (2)

29-43: LGTM! Enhanced error handling in Close method.

The changes to the Close method improve error handling by returning an error. This allows more robust testing of error scenarios.

63-68: Verify impact of changes on tests.

The Return and RunAndReturn methods now accommodate the error return type. Ensure that existing tests are updated to reflect these changes and verify their correctness.

block/produce.go

omritoptix

Not sure about measuring the skew using block as as I see it, it can easily collide with batch size. wrote a more elaborated comment about it in the submission thread.

block/manager.go

omritoptix · 2024-08-14T13:17:53Z

block/manager.go

+		return nil
+	}
+
+	if m.DAClient.GetMaxBlobSizeBytes() != 0 && m.DAClient.GetMaxBlobSizeBytes() < uint32(m.Conf.BatchSubmitBytes) {


why are we validating this m.DAClient.GetMaxBlobSizeBytes() if it's a constant?

we have to validate BatchSubmitBytes, according to the max blob value for the specific DA, that can change if the DA is updated.

hmm. not sure I get it.
if you'll add another DA, you'll add another const (which will probably not be zero).
so still don't understand what does it have to do with adding a DA..

omritoptix · 2024-08-14T13:22:40Z

block/produce.go

+	// validate configuration params and rollapp consensus params keep in line
+	err = m.ValidateConfigWithRollappParams()
+	if err != nil {
+		panic(err)


i think it's better to return here the error wrapping ErrUnrecoverable. why?

In general we handle errors in the higher level callee

we don't really want to panic cause that will get us in a restart loop which won't help, we want to have the higher level caller catch it and emit unhealthy event.

i removed the panic here, but then i added panic if error after applying in the p2p onReceivedBlock, otherwise the error will just be logged and can be missed.

yea so I see we call applyBlock (in case of full node):

from da

from p2p

seems like we're panicing here, and don't go into unhealthy mode - though we probably should - as I don't see the benefit of constant panicing.

@srene @danwt wdyt.

makes sense 👍

i replaced panic by health status event

block/retriever.go

omritoptix · 2024-08-14T13:41:13Z

config/config.go

 		return fmt.Errorf("batch_submit_max_time must be positive")
 	}

-	if c.BatchSubmitMaxTime < c.BlockTime {
+	if c.BatchSubmitTime < c.BlockTime {
 		return fmt.Errorf("batch_submit_max_time must be greater than block_time")


Suggested change

return fmt.Errorf("batch_submit_max_time must be greater than block_time")

return fmt.Errorf("batch_submit_time must be greater than block_time")

omritoptix · 2024-08-14T13:41:34Z

config/config.go

 		return fmt.Errorf("batch_submit_max_time must be greater than block_time")
 	}

-	if c.BatchSubmitMaxTime < c.MaxIdleTime {
+	if c.BatchSubmitTime < c.MaxIdleTime {
 		return fmt.Errorf("batch_submit_max_time must be greater than max_idle_time")


Suggested change

return fmt.Errorf("batch_submit_max_time must be greater than max_idle_time")

return fmt.Errorf("batch_submit_time must be greater than max_idle_time")

omritoptix · 2024-08-14T13:41:58Z

config/config.go

-	BatchSubmitMaxTime time.Duration `mapstructure:"batch_submit_max_time"`
-	// MaxBatchSkew is the number of batches which are waiting to be submitted. Block production will be paused if this limit is reached.
-	MaxBatchSkew uint64 `mapstructure:"max_supported_batch_skew"`
+	BatchSubmitTime time.Duration `mapstructure:"batch_submit_max_time"`


Suggested change

BatchSubmitTime time.Duration `mapstructure:"batch_submit_max_time"`

BatchSubmitTime time.Duration `mapstructure:"batch_submit_time"`

omritoptix · 2024-08-14T13:43:28Z

config/config.go

-	MaxBatchSkew uint64 `mapstructure:"max_supported_batch_skew"`
+	BatchSubmitTime time.Duration `mapstructure:"batch_submit_max_time"`
+	// BatchSkewBlocks is the number of blocks which are waiting to be submitted. Block production will be paused if this limit is reached.
+	BatchSkewBlocks uint64 `mapstructure:"batch_skew_blocks"`


In general not sure the idea of measuring skew by blocks is good (see my other comment). regradless I'd change the name to

Suggested change

BatchSkewBlocks uint64 `mapstructure:"batch_skew_blocks"`

MaxBlockSkew uint64 `mapstructure:"max_block_skew"`

rolled back to batch skew

config/config.go

block/block.go

danwt · 2024-08-14T16:33:38Z

proto/types/dymint/state.proto

+}
+
+message RollappConsensusParams {
+  Params params = 1;


I think there should not be this wrapper

proto/types/dymint/state.proto

block/manager.go

da/errors.go

config/config.go

block/submit_loop_test.go

danwt

There are still unresolved and unresponded feedbacks from previous reviews

block/manager.go

danwt · 2024-08-21T12:30:27Z

block/submit_loop_test.go

@@ -103,11 +106,15 @@ func testSubmitLoopInner(
 		timeSinceLast := time.Since(timeLastProgressT).Milliseconds()
 		require.True(t, timeSinceLast < absoluteMax, "too long since last update", "timeSinceLast", timeSinceLast, "max", absoluteMax)

+		pendingBlocks.Store(0)                    // no pending blocks to be submitted


I think strictly, this should not be 0 every time you submit batch, but I guess it makes no difference as only used for logging

Co-authored-by: Daniel T <[email protected]>

block/produce.go

@@ -147,7 +147,7 @@
 		return nil, nil, fmt.Errorf("load block: height: %d: %w: %w", newHeight, err, ErrNonRecoverable)
 	}

-	maxBlockDataSize := uint64(float64(m.Conf.BatchMaxSizeBytes) * types.MaxBlockSizeAdjustment)
+	maxBlockDataSize := uint64(float64(m.Conf.BatchSubmitBytes) * types.MaxBlockSizeAdjustment)


srene requested a review from a team as a code owner July 30, 2024 15:18

srene marked this pull request as draft July 30, 2024 15:18

github-actions bot added the dym-internal label Jul 30, 2024

srene self-assigned this Aug 2, 2024

github-advanced-security bot found potential problems Aug 2, 2024

View reviewed changes

block/manager.go Fixed Show fixed Hide fixed

block/manager.go Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Aug 2, 2024

View reviewed changes

block/manager.go Fixed Show fixed Hide fixed

srene marked this pull request as ready for review August 2, 2024 17:06

coderabbitai bot reviewed Aug 6, 2024

View reviewed changes

coderabbitai bot reviewed Aug 7, 2024

View reviewed changes

omritoptix changed the title ~~feat(config): rollapp params refactor~~ feat: rollapp consensus params Aug 11, 2024

omritoptix requested changes Aug 11, 2024

View reviewed changes

omritoptix reviewed Aug 11, 2024

View reviewed changes

omritoptix requested changes Aug 11, 2024

View reviewed changes

srene marked this pull request as draft August 12, 2024 08:38

srene force-pushed the srene/989-rollapp_params_checks branch from 48239d4 to 553b3df Compare August 13, 2024 11:45

github-advanced-security bot found potential problems Aug 13, 2024

View reviewed changes

block/produce.go Fixed Show fixed Hide fixed

srene marked this pull request as ready for review August 13, 2024 15:27

srene requested a review from omritoptix August 13, 2024 15:28

coderabbitai bot reviewed Aug 13, 2024

View reviewed changes

block/manager_test.go Show resolved Hide resolved

srene mentioned this pull request Aug 14, 2024

feat: new rollapp consensus params module dymensionxyz/dymension-rdk#495

Merged

12 tasks

srene force-pushed the srene/989-rollapp_params_checks branch from b512039 to 91b6dfe Compare August 14, 2024 07:02

coderabbitai bot reviewed Aug 14, 2024

View reviewed changes

block/produce.go Outdated Show resolved Hide resolved

omritoptix requested changes Aug 14, 2024

View reviewed changes

dymensionxyz deleted a comment from coderabbitai bot Aug 14, 2024

danwt requested changes Aug 14, 2024

View reviewed changes

srene requested review from danwt and omritoptix August 19, 2024 14:09

danwt mentioned this pull request Aug 21, 2024

Creating empty batches causes submit loop to get stuck #999

Closed

danwt reviewed Aug 21, 2024

View reviewed changes

srene and others added 22 commits August 22, 2024 12:10

lint fix

35e90eb

pendingblocks mod + log

51bf1ea

rollback to skew in batches + renaming vars

ebecf57

confg params

2fec147

param validation to applyblock + config

978b429

err handling

7600838

submit loop test fix

ad91c65

error wrapped

5e3a201

config check update

66b5977

ValidateConfigWithRollappParams moved

ddf92b9

rollapp params proto docstrings

7ab769b

proto consensus param refactor

d4a496b

removing commented code

f4a85a1

replaced panic by health status

aa724af

Update block/manager.go

f7edc71

Co-authored-by: Daniel T <[email protected]>

renaming

810c3e1

renames

6219bda

removing 0 max blob size case

a7bd89c

test fix

244aaee

error format

153637a

error conf var name fix

1d37cb3

merge conflicts

22f507f

srene force-pushed the srene/989-rollapp_params_checks branch from c7e6b30 to 22f507f Compare August 22, 2024 10:50

github-advanced-security bot found potential problems Aug 22, 2024

View reviewed changes

srene added 2 commits August 22, 2024 12:57

lint fix + log fix

f2bfb68

log removed

f6b100c

danwt added the security label Aug 22, 2024

omritoptix approved these changes Aug 23, 2024

View reviewed changes

omritoptix merged commit 54e2fa5 into main Aug 24, 2024
7 checks passed

omritoptix deleted the srene/989-rollapp_params_checks branch August 24, 2024 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: rollapp consensus params #991

feat: rollapp consensus params #991

srene commented Jul 30, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot left a comment

coderabbitai bot Aug 6, 2024

coderabbitai bot left a comment

coderabbitai bot left a comment

omritoptix left a comment

omritoptix Aug 11, 2024

srene Aug 13, 2024

danwt Aug 14, 2024

coderabbitai bot left a comment

coderabbitai bot left a comment

omritoptix left a comment

omritoptix Aug 14, 2024

srene Aug 14, 2024

omritoptix Aug 15, 2024

omritoptix Aug 14, 2024

srene Aug 14, 2024

omritoptix Aug 15, 2024

danwt Aug 21, 2024

srene Aug 21, 2024

omritoptix Aug 14, 2024

srene Aug 14, 2024

omritoptix Aug 14, 2024

srene Aug 14, 2024

omritoptix Aug 14, 2024

srene Aug 14, 2024

omritoptix Aug 14, 2024

srene Aug 14, 2024

danwt Aug 14, 2024

danwt left a comment

danwt Aug 21, 2024

	return fmt.Errorf("batch_submit_max_time must be greater than block_time")
	return fmt.Errorf("batch_submit_time must be greater than block_time")

	BatchSubmitTime time.Duration `mapstructure:"batch_submit_max_time"`
	BatchSubmitTime time.Duration `mapstructure:"batch_submit_time"`

	BatchSkewBlocks uint64 `mapstructure:"batch_skew_blocks"`
	MaxBlockSkew uint64 `mapstructure:"max_block_skew"`

feat: rollapp consensus params #991

feat: rollapp consensus params #991

Conversation

srene commented Jul 30, 2024 • edited by coderabbitai bot Loading

PR Standards

Opening a pull request should be able to meet the following requirements

Summary by CodeRabbit

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Aug 6, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

omritoptix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

omritoptix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srene commented Jul 30, 2024 •

edited by coderabbitai bot

Loading