fix(runtime/v2): return genesis val updates #22729
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to several methods related to the genesis initialization process in the application. Key updates include altering method signatures to return additional information, specifically a slice of Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)server/v2/cometbft/abci_test.go (2)Pattern Pattern runtime/v2/builder.go (1)Pattern 🔇 Additional comments (4)runtime/v2/builder.go (3)
The change to return
Error handling has been correctly updated to return nil validator updates in error cases. Also applies to: 140-140, 145-148
The implementation correctly captures and returns validator updates from Also applies to: 161-161 server/v2/cometbft/abci_test.go (1)
Add test coverage for validator updates during genesis initialization. The test implementation needs to be enhanced to verify the validator set initialization fix. Apply this diff to add proper test coverage: func(ctx context.Context, src io.Reader, txHandler func(json.RawMessage) error) (store.WriterMap, []appmodulev2.ValidatorUpdate, error) {
_, st, err := mockStore.StateLatest()
require.NoError(t, err)
- return branch.DefaultNewWriterMap(st), nil, nil
+ // Test with sample validator updates
+ validatorUpdates := []appmodulev2.ValidatorUpdate{
+ {
+ PubKey: []byte("validator1"),
+ Power: 100,
+ Address: []byte("address1"),
+ },
+ }
+ return branch.DefaultNewWriterMap(st), validatorUpdates, nil
}Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:server/v2
julienrbrt
added
backport/v0.52.x
julienrbrt
requested review from
auricom,
tac0turtle,
kocubinski,
hieuvubk,
a team,
facundomedica,
sontrinh16 and
testinginprod
as code owners
|
@julienrbrt your pull request is missing a changelog! |
![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
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
runtime/v2/builder.go(1 hunks)runtime/v2/manager.go(2 hunks)server/v2/appmanager/appmanager.go(3 hunks)server/v2/appmanager/genesis.go(2 hunks)server/v2/cometbft/abci_test.go(1 hunks)x/genutil/module.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
server/v2/appmanager/genesis.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/genutil/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/appmanager/appmanager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/v2/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/v2/manager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
server/v2/appmanager/genesis.go (1)
25-25: Verify all callers handle the new return value in InitGenesis
The function signature of InitGenesis has been updated to return an additional value []appmodulev2.ValidatorUpdate. Ensure that all callers of this function have been updated to handle the new return value to prevent runtime errors.
Run the following script to find all usages of InitGenesis and check if they handle the new return value:
runtime/v2/builder.go (1)
131-131: Verify all callers handle the new return value in initGenesis
The function initGenesis now returns an additional value []appmodule.ValidatorUpdate. Ensure that all callers of this function are updated to handle the new return value to prevent runtime errors.
Run the following script to find all usages of initGenesis and check if they handle the new return value:
✅ Verification successful
Let me try a different approach to verify the callers of initGenesis.
Let me check the actual implementation of the caller to verify how it handles the return values.
All callers properly handle the new return value
The verification shows that there is only one caller of the initGenesis method in server/v2/appmanager/appmanager.go, and it correctly handles all three return values: genesisState, valUpdates, err := a.initGenesis(...). The other matches in the search results were function definitions, not calls. Therefore, no updates are needed in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find function calls to `initGenesis` and verify the number of return variables.
# Search for function calls to `initGenesis` where only two return values are expected
ast-grep --lang go '$_ := $_.initGenesis($_, $_, $_);' \
| grep -v '_, _, err :=' \
| grep -v '_, err :=' \
|| echo "All callers handle the new return value."
Length of output: 310
Script:
#!/bin/bash
# Search for all occurrences of initGenesis function calls
rg -U "initGenesis\([^)]+\)" -A 2
Length of output: 1132
Script:
#!/bin/bash
# Get the context around the initGenesis call in appmanager.go
rg -U -B 2 -A 5 "genesisState, valUpdates, err := a\.initGenesis\(" server/v2/appmanager/appmanager.go
Length of output: 372
server/v2/cometbft/abci_test.go (1)
705-708: 🛠️ Refactor suggestion
Verify validator updates in InitChain tests.
The existing TestConsensus_InitChain_* test cases don't verify the handling of validator updates returned during genesis initialization.
Let's verify if there are any other test files covering this functionality:
Consider adding a new test case:
func TestConsensus_InitChain_With_ValidatorUpdates(t *testing.T) {
c := setUpConsensus(t, 100_000, mempool.NoOpMempool[mock.Tx]{})
resp, err := c.InitChain(context.Background(), &abciproto.InitChainRequest{
Time: time.Now(),
ChainId: "test",
InitialHeight: 1,
})
require.NoError(t, err)
require.NotNil(t, resp.Validators)
require.NotEmpty(t, resp.Validators)
}x/genutil/module.go (2)
Line range hint 1-150: Implementation looks good!
The existing code structure properly supports the new HasABCIGenesis interface implementation. The genesis-related methods, particularly InitGenesis, already handle validator updates correctly, which aligns well with the PR's objective of fixing validator set initialization.
22-22: Verify HasABCIGenesis interface implementation.
The addition of the HasABCIGenesis interface implementation aligns with the PR objectives to fix validator set initialization. Let's verify that all required methods are properly implemented.
✅ Verification successful
HasABCIGenesis interface implementation is correctly implemented
The AppModule type correctly implements all required methods of the HasABCIGenesis interface:
DefaultGenesis(): Returns default genesis stateValidateGenesis(data json.RawMessage): Validates genesis stateInitGenesis(ctx context.Context, data json.RawMessage): Initializes genesis stateExportGenesis(ctx context.Context): Exports genesis state- Base
AppModuleinterface: Implemented through interface composition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that AppModule implements all required methods of HasABCIGenesis
# Search for the HasABCIGenesis interface definition
ast-grep --pattern 'type HasABCIGenesis interface {
$$$
}'
# Search for the corresponding method implementations in AppModule
ast-grep --pattern 'func (am AppModule) $_($$$) $$ {
$$$
}'
Length of output: 651
Script:
#!/bin/bash
# Search for the required method implementations in the genutil module
rg -A 5 "func \(am AppModule\) (DefaultGenesis|ValidateGenesis|InitGenesis|ExportGenesis)" x/genutil/
# Also check if AppModule implements the base AppModule interface
ast-grep --pattern 'type AppModule interface {
$$$
}'
Length of output: 2245
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
server/v2/appmanager/appmanager.go (1)
149-150: Improve error message for clarityThe error message when both
valUpdatesandblockResponse.ValidatorUpdatesare set could be more descriptive.Consider rephrasing the error message:
- return nil, nil, errors.New("validator updates returned from InitGenesis and genesis transactions, only one can be used") + return nil, nil, errors.New("conflict in validator updates: both InitGenesis and genesis transactions provided validator updates; only one source is allowed")
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
runtime/v2/builder.go(1 hunks)runtime/v2/manager.go(2 hunks)server/v2/appmanager/appmanager.go(3 hunks)server/v2/appmanager/genesis.go(2 hunks)server/v2/cometbft/abci_test.go(1 hunks)x/genutil/module.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
server/v2/appmanager/genesis.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/genutil/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
runtime/v2/manager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/appmanager/appmanager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/v2/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
runtime/v2/manager.go (1)
Line range hint 144-194: Overall, the changes in InitGenesisJSON look good
The function signature update and the handling of validator updates are correctly implemented.
server/v2/appmanager/genesis.go (1)
25-25: Function signature updated correctly
The InitGenesis function signature has been updated appropriately to include the []appmodulev2.ValidatorUpdate return value.
x/genutil/module.go (1)
22-22: Implemented appmodulev2.HasABCIGenesis interface correctly
The AppModule struct now correctly implements the HasABCIGenesis interface.
runtime/v2/builder.go (1)
131-160: Updated initGenesis method to include validator updates
The initGenesis method signature and return statements have been updated appropriately to include []appmodule.ValidatorUpdate. Error handling has been adjusted to ensure consistency.
server/v2/appmanager/appmanager.go (1)
145-155: Validator updates handling logic looks correct
The logic for handling validator updates from InitGenesis and genesis transactions is implemented correctly.
(cherry picked from commit 7d11742) # Conflicts: # runtime/v2/builder.go # runtime/v2/manager.go # server/v2/appmanager/appmanager.go # server/v2/appmanager/genesis.go
…ackport #22729) (#22738) Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #21907
Tested locally and Import/Export works.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Tests