feat: sequencer rotation #992
Conversation
…edundent' into mtsitrin/poc/sequencer_switch
…edundent' into mtsitrin/poc/sequencer_switch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (10)
settlement/local/local.go (1)
206-209: Clarify the logic ofGetAllSequencersinsettlement/local/local.go.The
GetAllSequencersmethod currently callsGetBondedSequencers, which contradicts the interface documentation stating that it should return all sequencers (both bonded and non-bonded). Ensure this implementation aligns with the intended functionality.
- File:
settlement/local/local.go- Lines: 206-209
Analysis chain
Clarify the logic of
GetAllSequencers.The method
GetAllSequencerscurrently callsGetBondedSequencers, which might not align with the expectation of retrieving all sequencers. Ensure this is the intended behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic of `GetAllSequencers`. # Test: Search for the method usage. Expect: Ensure it aligns with the intended purpose. rg --type go -A 5 $'GetAllSequencers'Length of output: 6109
settlement/dymension/dymension.go (5)
69-69: Consider using a more flexible context.Using
context.Background()directly may limit flexibility in context management. Consider passing a context as a parameter for better control.
279-320: Consider adding documentation for caching logic.Adding a comment to explain the caching logic for the proposer could improve code readability and maintainability.
324-366: Consider usingerrors.Joinfor error handling.Using
errors.Joincan improve error traceability and provide more context in error messages.
Line range hint
369-412: Consider usingerrors.Joinfor error handling.Using
errors.Joincan improve error traceability and provide more context in error messages.
414-456: Consider adding documentation for rotation logic.Adding a comment to explain the rotation logic could improve code readability and maintainability.
mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go (4)
29-57: Improve the panic message for clarity.Consider providing a more informative panic message to aid debugging when no return value is specified.
86-114: Improve the panic message for clarity.Consider providing a more informative panic message to aid debugging when no return value is specified.
201-229: Improve the panic message for clarity.Consider providing a more informative panic message to aid debugging when no return value is specified.
Line range hint
374-410: Improve the panic message for clarity.Consider providing a more informative panic message to aid debugging when no return value is specified.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
types/pb/dymint/state.pb.gois excluded by!**/*.pb.go
Files selected for processing (23)
- block/block.go (4 hunks)
- block/executor_test.go (10 hunks)
- block/manager.go (9 hunks)
- block/produce.go (4 hunks)
- block/pruning.go (1 hunks)
- block/sequencers.go (1 hunks)
- block/state.go (4 hunks)
- da/grpc/grpc.go (1 hunks)
- go.mod (10 hunks)
- mocks/github.com/dymensionxyz/dymint/settlement/mock_ClientI.go (4 hunks)
- mocks/github.com/dymensionxyz/dymint/store/mock_Store.go (7 hunks)
- proto/types/dymint/dymint.proto (3 hunks)
- proto/types/dymint/state.proto (3 hunks)
- settlement/dymension/dymension.go (13 hunks)
- settlement/dymension/events.go (1 hunks)
- settlement/grpc/grpc.go (2 hunks)
- settlement/local/local.go (2 hunks)
- settlement/settlement.go (2 hunks)
- types/block.go (3 hunks)
- types/hashing.go (1 hunks)
- types/serialization.go (6 hunks)
- types/serialization_test.go (7 hunks)
- types/validation.go (3 hunks)
Files skipped from review due to trivial changes (1)
- go.mod
Files skipped from review as they are similar to previous changes (12)
- block/block.go
- block/pruning.go
- block/state.go
- proto/types/dymint/dymint.proto
- proto/types/dymint/state.proto
- settlement/dymension/events.go
- settlement/grpc/grpc.go
- settlement/settlement.go
- types/block.go
- types/hashing.go
- types/serialization.go
- types/serialization_test.go
Additional comments not posted (28)
types/validation.go (3)
12-17: Refactor improves function clarity.Changing the parameter from
Sequencertotmcrypto.PubKeysimplifies the function's interface and usage.
40-42: Enhanced data integrity check.The addition of a data hash validation improves block integrity checks.
98-109: Streamlined signature validation.The use of
tmcrypto.PubKeyfor direct signature verification enhances the validation logic.da/grpc/grpc.go (1)
49-49: Improved channel initialization.Initializing the
syncedchannel before configuration checks ensures it is always available, enhancing method reliability.block/sequencers.go (5)
38-51: Correct implementation of proposer check.The function correctly checks if the local node is the proposer, considering both L2 and hub contexts.
53-70: Correct rotation progress check.The function correctly identifies when a rotation is in progress and returns the appropriate address.
86-107: Robust rotation completion logic.The function correctly implements the logic for completing the sequencer rotation, including validation and batch creation.
110-148: Correct implementation of last batch creation and posting.The function correctly handles the creation and posting of the last batch, ensuring it is marked and submitted appropriately.
14-34: Improve error handling inMonitorSequencerRotation.The function uses panic for error handling, which is not ideal for production systems. Consider returning an error instead.
- if err != nil { - panic("Error subscribing to events") - } + if err != nil { + return fmt.Errorf("error subscribing to events: %w", err) + }Likely invalid or redundant comment.
settlement/local/local.go (3)
195-201: LGTM: Correct integration withsettlement.Sequencer.The conversion of the public key and the return of a
settlement.Sequenceris handled correctly.
211-214: LGTM: Correct implementation ofGetBondedSequencers.The method correctly returns the proposer as a bonded sequencer.
216-218: Verify the placeholder status ofCheckRotationInProgress.The method currently returns nil, indicating a placeholder. Ensure that future logic is planned, and consider adding a TODO comment.
Verification successful
Consider adding a TODO comment for
CheckRotationInProgress.The
CheckRotationInProgressmethod insettlement/local/local.goreturns nil, indicating it is a placeholder. To clarify the intention and ensure future development, consider adding a TODO comment specifying the planned logic for this method.
settlement/local/local.go: Add a TODO comment in theCheckRotationInProgressmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the placeholder status of `CheckRotationInProgress`. # Test: Search for the method usage and TODO comments. Expect: Ensure future logic is planned. rg --type go -A 5 $'CheckRotationInProgress'Length of output: 6075
block/manager.go (3)
237-240: LGTM: Correct synchronization logic insyncFromSettlement.The method correctly updates the sequencer set and handles synchronization.
265-267: LGTM: Correct retrieval logic inGetProposerPubKey.The method correctly retrieves the proposer's public key from the state.
140-144: Verify chain halt logic inStart.Ensure that the logic for checking if the chain is halted is correct and aligns with the intended behavior.
Verification successful
Chain halt logic in
Startmethod is correctly implemented.The
isChainHaltedfunction appropriately checks for the presence of a proposer public key, which aligns with the expected behavior of ensuring the chain is not halted. This logic is consistent with the need for a proposer to be set for the chain to function properly. No further issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify chain halt logic in `Start`. # Test: Search for the method usage and related logic. Expect: Ensure it aligns with the intended behavior. rg --type go -A 5 $'isChainHalted'Length of output: 566
Script:
#!/bin/bash # Description: Check for documentation or comments related to chain halt behavior. # Search for comments or documentation related to isChainHalted or chain halt logic. rg --type go -A 3 'chain halt|isChainHalted'Length of output: 413
block/executor_test.go (2)
Line range hint
33-79:
LGTM: Correct test logic inTestCreateBlock.The test correctly uses the
SequencerSetand effectively tests the block creation process.
Line range hint
142-235:
LGTM: Correct test logic inTestApplyBlock.The test correctly uses the
SequencerSetand effectively tests the block application process, including validation of proposed transitions.block/produce.go (5)
44-44: Consider implications of using system time.Calling the system time may introduce non-determinism, which can affect the reproducibility of the block production process.
98-102: LGTM!The function is clear and effectively delegates to a helper method.
104-106: LGTM!The function is well-structured and effectively uses helper methods.
157-157: Floating point arithmetic can be non-deterministic.Floating point operations are not associative and could be a source of non-determinism, which may affect block consistency.
176-205: LGTM!The function is well-structured and handles commit creation effectively.
settlement/dymension/dymension.go (1)
Line range hint
456-473: LGTM!The function is straightforward and handles errors effectively.
mocks/github.com/dymensionxyz/dymint/store/mock_Store.go (5)
30-44: Enhancement in error handling forClosemethod.The modification to return an error from the
Closemethod is a positive change, enforcing error handling in the calling code.
190-216: Addition ofLoadBlockCidmethod.The new
LoadBlockCidmethod is a valuable addition, aligning with existing patterns and extending the mock's functionality.
420-448: Transition toLoadSequencers.Replacing
LoadValidatorswithLoadSequencersreflects the shift to sequencer management and aligns with the refactoring objectives.
699-727: Addition ofSaveBlockCidmethod.The
SaveBlockCidmethod is a well-implemented addition, enhancing the mock's capabilities and following established patterns.
819-847: Transition toSaveSequencers.Replacing
SaveValidatorswithSaveSequencerssupports the transition to sequencer management and aligns with the refactoring goals.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
settlement/dymension/dymension.go (2)
Line range hint
63-108: Consider using a more appropriate context thancontext.Background().Using
context.Background()is generally not recommended for lifecycle management. Consider using a context that can be canceled or has a timeout.// Consider passing a context to Init or using context.WithCancel for better lifecycle management.
Line range hint
109-209: Reminder: Address the TODO comment about using channels.The TODO comment suggests changing to a channel for event handling, which could improve efficiency.
Do you want me to help refactor this section to use a channel, or open a GitHub issue to track this task?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (17)
third_party/dymension/rollapp/types/genesis.pb.gois excluded by!**/*.pb.gothird_party/dymension/rollapp/types/liveness.pb.gois excluded by!**/*.pb.gothird_party/dymension/rollapp/types/metadata.pb.gois excluded by!**/*.pb.gothird_party/dymension/rollapp/types/params.pb.gois excluded by!**/*.pb.gothird_party/dymension/rollapp/types/rollapp.pb.gois excluded by!**/*.pb.gothird_party/dymension/rollapp/types/state_info.pb.gois excluded by!**/*.pb.gothird_party/dymension/rollapp/types/tx.pb.gois excluded by!**/*.pb.gothird_party/dymension/sequencer/types/description.pb.gois excluded by!**/*.pb.gothird_party/dymension/sequencer/types/events.pb.gois excluded by!**/*.pb.gothird_party/dymension/sequencer/types/genesis.pb.gois excluded by!**/*.pb.gothird_party/dymension/sequencer/types/metadata.pb.gois excluded by!**/*.pb.gothird_party/dymension/sequencer/types/operating_status.pb.gois excluded by!**/*.pb.gothird_party/dymension/sequencer/types/params.pb.gois excluded by!**/*.pb.gothird_party/dymension/sequencer/types/query.pb.gois excluded by!**/*.pb.gothird_party/dymension/sequencer/types/query.pb.gw.gois excluded by!**/*.pb.gw.gothird_party/dymension/sequencer/types/sequencer.pb.gois excluded by!**/*.pb.gothird_party/dymension/sequencer/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (7)
- settlement/dymension/dymension.go (13 hunks)
- third_party/dymension/rollapp/types/errors.go (1 hunks)
- third_party/dymension/rollapp/types/message_update_state.go (1 hunks)
- third_party/dymension/rollapp/types/params.go (1 hunks)
- third_party/dymension/sequencer/types/events.go (1 hunks)
- third_party/dymension/sequencer/types/keys.go (1 hunks)
- third_party/dymension/sequencer/types/params.go (1 hunks)
Files skipped from review due to trivial changes (1)
- third_party/dymension/rollapp/types/params.go
Files skipped from review as they are similar to previous changes (3)
- third_party/dymension/rollapp/types/errors.go
- third_party/dymension/rollapp/types/message_update_state.go
- third_party/dymension/sequencer/types/params.go
Additional comments not posted (10)
third_party/dymension/sequencer/types/events.go (1)
1-27: LGTM! Constants for event types and attributes are well-defined.The event types and attribute keys are clearly defined and organized, which should facilitate event handling and logging.
third_party/dymension/sequencer/types/keys.go (1)
1-104: LGTM! Key management functions and constants are well-structured.The functions for generating keys and constants for prefixes are clearly defined, aiding in consistent key management across the module.
settlement/dymension/dymension.go (8)
Line range hint
210-238: LGTM! Error handling ingetStateInfois well-implemented.The method uses
errors.Joinfor enhanced error traceability, and the retry logic is well-structured.
279-320: LGTM! Caching logic inGetProposeris efficient.The method efficiently retrieves and caches the proposer using slices.IndexFunc. Ensure that the caching logic aligns with the system's needs.
323-366: LGTM! Retrieval logic inGetAllSequencersis well-structured.The method effectively retrieves all sequencers and unpacks public keys using
protoCodec.UnpackAny.
Line range hint
368-412: LGTM! Retrieval logic inGetBondedSequencersis consistent and well-structured.The method effectively retrieves bonded sequencers with appropriate retry logic and error handling.
414-456: LGTM! Rotation check logic inCheckRotationInProgressis well-structured.The method effectively checks for proposer rotation. Ensure that the rotation logic aligns with the system's requirements.
Line range hint
457-473: LGTM! Transaction broadcasting inbroadcastBatchis well-handled.The method effectively broadcasts transactions and handles errors, logging the transaction hash upon success.
Line range hint
474-507: LGTM! Batch conversion logic inconvertBatchToMsgUpdateStateis well-structured.The method effectively converts a batch to a message for state updates. Ensure that address derivation logic is correct.
Line range hint
508-540: LGTM! Client options configuration ingetCosmosClientOptionsis well-structured.The method effectively configures options for the Cosmos client, with flexible handling of gas settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
proto/types/dymint/state.proto (2)
12-12: Unused Import Alert:The import statement for
types/dymint/dymint.protoinproto/types/dymint/state.protoappears to be unused, as there are no references to elements from this file withinstate.proto. Consider removing this import to clean up the code.Analysis chain
Ensure the new import is necessary.
The import statement for
dymint.protosuggests new functionality related to the Dymint protocol. Verify that this import is being used effectively in the file.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `dymint.proto` import. # Test: Search for references to `dymint` in the file. rg --type proto 'dymint' proto/types/dymint/state.protoLength of output: 85
Script:
#!/bin/bash # Description: Verify the usage of the `dymint.proto` import. # Test: Search for references to `dymint` in the file without specifying the file type. rg 'dymint' proto/types/dymint/state.protoLength of output: 164
28-29: Deprecation of validator fields not fully addressed.The
validatorsfield, along withnext_validatorsandlast_validators, is still referenced in several.protofiles across the codebase. Ensure these deprecated fields are removed or updated to align with the shift to usingsequencerSet.
proto/types/tendermint/types/types.protoproto/types/tendermint/state/types.protoproto/types/tendermint/abci/types.protoAnalysis chain
Deprecation of validator fields.
The
validatorsfield is marked as deprecated, which aligns with the shift to usingsequencerSet. Ensure that all references to these deprecated fields are removed or updated in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that deprecated validator fields are not used elsewhere. # Test: Search for the usage of deprecated validator fields in the codebase. rg --type proto 'validators|next_validators|last_validators'Length of output: 92
Script:
#!/bin/bash # Description: Verify that deprecated validator fields are not used elsewhere. # Test: Search for the usage of deprecated validator fields in the codebase. rg 'validators|next_validators|last_validators' --glob '*.proto'Length of output: 2162
block/submit_test.go (1)
174-179: Inconsistency in batch submission testing across the codebase.The
block/submit_test.gofile includes a test for handling batch submission failures, but similar error handling is not present inblock/pruning_test.go. This indicates a lack of consistency in testing error scenarios related to batch submissions across different test files. Consider adding similar error handling tests in other relevant test files to ensure uniformity.Analysis chain
Consistency in batch submission testing.
The test ensures that batch submission failures are handled correctly. Verify that similar scenarios are consistently tested across the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency in batch submission testing. # Test: Search for batch submission tests in the codebase. rg --type go 'CreateAndSubmitBatch'Length of output: 1101
Script:
#!/bin/bash # Description: Search for error assertions in tests involving CreateAndSubmitBatch. # Test: Search for assertions related to error handling in tests involving CreateAndSubmitBatch. rg --type go 'CreateAndSubmitBatch.*assert\.(Error|NoError)' -A 5Length of output: 67
Script:
#!/bin/bash # Description: Broaden the search to find any test cases involving CreateAndSubmitBatch. # Test: Search for any test cases involving CreateAndSubmitBatch and look for assertions or error handling. rg --type go 'CreateAndSubmitBatch' -A 10Length of output: 4761
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
types/pb/dymint/state.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- block/executor.go (5 hunks)
- block/pruning_test.go (1 hunks)
- block/sequencers.go (1 hunks)
- block/submit.go (3 hunks)
- block/submit_test.go (4 hunks)
- proto/types/dymint/state.proto (3 hunks)
- rpc/client/client.go (4 hunks)
- store/store.go (4 hunks)
Files skipped from review as they are similar to previous changes (5)
- block/executor.go
- block/pruning_test.go
- block/sequencers.go
- rpc/client/client.go
- store/store.go
Additional comments not posted (7)
proto/types/dymint/state.proto (1)
45-45: Addition ofsequencerSetfield.The
sequencerSetfield is added and marked as non-nullable. This change is crucial for managing sequencer information in the state. Ensure that this field is correctly initialized and used throughout the codebase.Verification successful
sequencerSetField Usage VerifiedThe
sequencerSetfield is correctly utilized in the codebase. It is part of the protobuf definition and is actively used in functions likeSaveSequencersinstore/store.go, where it is converted to a protobuf format and managed with error handling. This indicates proper initialization and usage of the field.
types/pb/dymint/state.pb.go: Protobuf definition includessequencerSet.store/store.go: FunctionSaveSequencershandlessequencerSet.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization and usage of the `sequencerSet` field. # Test: Search for the usage of `sequencerSet` in the codebase. rg --type proto 'sequencerSet'Length of output: 60
Script:
#!/bin/bash # Description: Search for the usage of `sequencerSet` in the codebase without specifying file type. # Test: Search for the usage of `sequencerSet` in the codebase. rg 'sequencerSet'Length of output: 716
block/submit_test.go (4)
148-148: Refinement ofproposertype.The
proposervariable is now of typesettlement.Sequencer, which aligns with the updated sequencer logic. Ensure that this change is consistent with the new approach to handling sequencers.
23-23: Ensure correct import usage.The import of the
settlementpackage indicates a shift to usingsettlement.Sequencer. Verify that this change is consistent with the new sequencer management logic.Verification successful
Import of
settlementpackage is consistent with sequencer management logic.The
settlement.Sequenceris used extensively across the codebase, including in theblock/submit_test.gofile, confirming that the import aligns with the new sequencer management logic. No issues were found with the import usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `settlement` package in the codebase. # Test: Search for the usage of `settlement.Sequencer` in the codebase. rg --type go 'settlement.Sequencer'Length of output: 5972
167-167: Ensure error handling consistency.The error handling for
ProduceApplyGossipBlockshould be consistent with other parts of the code. Verify that this method is correctly handling errors in all test scenarios.Verification successful
Error handling for
ProduceApplyGossipBlockis consistent across tests.The function
ProduceApplyGossipBlockconsistently checks for errors usingrequire.NoErrororassert.NoErrorin all relevant test files, ensuring that any errors are appropriately handled and reported during testing. This confirms that the error handling is consistent with other parts of the code.
- Instances found in
submit_test.go,pruning_test.go, andmanager_test.goall follow this pattern.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling for `ProduceApplyGossipBlock`. # Test: Search for error handling related to `ProduceApplyGossipBlock`. rg --type go 'ProduceApplyGossipBlock' -A 3Length of output: 2513
120-120: Update toProduceApplyGossipBlock.The method
ProduceAndGossipBlockhas been replaced withProduceApplyGossipBlock. Ensure that this change is reflected consistently across all relevant test cases.Verification successful
ProduceApplyGossipBlockConsistently Used Across CodebaseThe method
ProduceAndGossipBlockhas been successfully replaced withProduceApplyGossipBlockacross all relevant test cases and files. No instances of the old method were found, confirming the refactoring is complete.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ProduceApplyGossipBlock` in the codebase. # Test: Search for the usage of `ProduceApplyGossipBlock` in the codebase. rg --type go 'ProduceApplyGossipBlock'Length of output: 962
Script:
#!/bin/bash # Description: Verify if `ProduceAndGossipBlock` is still present in the codebase. # Test: Search for the usage of `ProduceAndGossipBlock` in the codebase. rg --type go 'ProduceAndGossipBlock'Length of output: 36
block/submit.go (2)
161-165: Handling the last batch scenario.The logic for marking the last batch is a valuable addition. Ensure that this logic is correctly integrated and tested throughout the codebase.
134-134: Update toCreateAndSubmitBatchsignature.The function now accepts a
lastBatchparameter, which is crucial for managing the last batch scenario. Ensure that all calls to this function are updated accordingly.Verification successful
All calls to
CreateAndSubmitBatchare correctly updated with the newlastBatchparameter. The search results show that each instance ofCreateAndSubmitBatchincludes thelastBatchparameter, ensuring proper function usage.
block/sequencers.go: Correctly usestruefor thelastBatchparameter.block/submit_test.goandblock/pruning_test.go: Correctly usefalsefor thelastBatchparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `CreateAndSubmitBatch` are updated with the new parameter. # Test: Search for the usage of `CreateAndSubmitBatch` in the codebase. rg --type go 'CreateAndSubmitBatch'Length of output: 1101
| Signature: tmSignature, | ||
| }, | ||
| } | ||
| maxBlockDataSize := uint64(float64(m.Conf.BatchMaxSizeBytes) * types.MaxBlockSizeAdjustment) |
Check notice
Code scanning / CodeQL
Floating point arithmetic Note
| go func() { | ||
| rotateC <- nextSeqAddr | ||
| }() |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note
|
@mtsitrin build failed |
was a faulty UT |
This PR refactors the sequencers management on dymint.
Closes #941
Main changes:
tendermint.ValidatorSet)nextSequencerHashfield in block headerBug fix
WIP:
will be added in separate PRs: