fix: bring back concurrent export genesis in v2 #21554
Conversation
WalkthroughWalkthroughThe changes involve modifying the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
bot
removed
C:server/v2
akhilkumarpilli
changed the title
akhilkumarpilli
requested review from
julienrbrt,
hieuvubk and
a team
as code owners
|
@akhilkumarpilli 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: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- runtime/v2/builder.go (1 hunks)
- runtime/v2/manager.go (4 hunks)
Additional context used
Path-based instructions (2)
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.
GitHub Check: CodeQL
runtime/v2/manager.go
[notice] 231-242: Spawning a Go routine
Spawning a Go routine may be a possible source of non-determinism
Additional comments not posted (1)
runtime/v2/manager.go (1)
Line range hint
198-253: Review ofExportGenesisForModulesFunction ChangesThe changes to the
ExportGenesisForModulesfunction introduce concurrency through goroutines, which can enhance performance but also bring challenges such as non-determinism and potential race conditions. Here are some specific observations and suggestions:
Function Signature Change: The addition of
appStf *stf.STF[T]andstate store.ReaderMapto the function signature is a significant change. Ensure that all calls to this function across the codebase have been updated to pass these new parameters.Concurrency with Goroutines: The use of goroutines to handle exports concurrently is a major shift towards improving performance. However, it's crucial to ensure that the shared resources, particularly
state, are handled safely to avoid race conditions.Error Handling: The structured approach to error handling within goroutines is commendable. It ensures that errors from individual module exports are captured and handled appropriately.
Potential Non-Determinism: As noted by the security bot, the introduction of goroutines could lead to non-deterministic behavior, especially in a distributed system like a blockchain. It's important to ensure that the order of operations and the state consistency are preserved across different executions.
Testing: Given the complexity introduced by these changes, comprehensive testing is crucial. Consider adding more unit tests and integration tests to cover concurrent executions and error scenarios.
Overall, the refactoring enhances the function's capability to handle asynchronous operations and improves its error handling. However, careful attention must be paid to concurrency issues and ensuring deterministic behavior.
Consider verifying the deterministic behavior and thread safety through additional tests or static analysis tools. If you need assistance with setting up tests or further analysis, please let me know.
Tools
GitHub Check: CodeQL
[notice] 231-242: Spawning a Go routine
Spawning a Go routine may be a possible source of non-determinism
runtime/v2/builder.go
Outdated
| state, err := a.app.db.StateAt(version) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to export genesis: %w", err) | ||
| } | ||
|
|
||
| genesisJson, err := a.app.moduleManager.ExportGenesisForModules(ctx, a.app.stf, state) |
There was a problem hiding this comment.
Approve changes to ExportGenesis with a suggestion for enhanced error handling.
The modifications to ExportGenesis function are well-aligned with the PR's objectives to restore concurrent export functionality and address the race condition. The addition of state retrieval at a specified version enhances the robustness and accuracy of the export process.
Suggestion:
Consider adding more specific error messages for different failure points within the function to aid in debugging and maintenance.
runtime/v2/manager.go
Outdated
| @@ -194,6 +195,8 @@ func (m *MM[T]) InitGenesisJSON( | |||
| // ExportGenesisForModules performs export genesis functionality for modules | |||
| func (m *MM[T]) ExportGenesisForModules( | |||
| ctx context.Context, | |||
| appStf *stf.STF[T], | |||
There was a problem hiding this comment.
Can we not leak stf in the module manager API?
akhilkumarpilli
requested review from
testinginprod and
facundomedica
as code owners
github-actions
bot
added
C:server/v2
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
runtime/v2/manager.go (1)
Line range hint
199-255: Approve the refactoring with a minor nitpick.The refactoring of the
ExportGenesisForModulesfunction to support asynchronous execution using goroutines and channels is a good approach to improve performance. The changes handle errors and collect results from the goroutines correctly. The use of thegenesisResulttype and theModuleIinterface improves modularity and reusability.However, consider renaming
moduleItomodulefor better readability:- var moduleI ModuleI + var module ModuleI if module, hasGenesis := mod.(appmodulev2.HasGenesis); hasGenesis { - moduleI = module.(ModuleI) + module = module.(ModuleI) } else if module, hasABCIGenesis := mod.(appmodulev2.HasABCIGenesis); hasABCIGenesis { - moduleI = module.(ModuleI) + module = module.(ModuleI) } else { continue }Overall, the changes align with the PR objectives and the AI-generated summary.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- runtime/v2/manager.go (4 hunks)
Additional context used
Path-based instructions (1)
runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
runtime/v2/manager.go (2)
233-243: Skip the static analysis flag.The static analysis tool flagged the use of goroutines as a possible source of non-determinism. However, the usage here is valid and does not introduce non-determinism as the goroutines are used to enable concurrent execution of independent tasks and the results are collected deterministically using channels.
Line range hint
260-269: LGTM!The
checkModulesExistsfunction is implemented correctly.
|
@akhilkumarpilli, could you base yourself on this branch: #21739? |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- runtime/v2/builder.go (1 hunks)
- runtime/v2/manager.go (4 hunks)
- server/v2/appmanager/appmanager.go (1 hunks)
Additional context used
Path-based instructions (3)
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.server/v2/appmanager/appmanager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (4)
server/v2/appmanager/appmanager.go (2)
92-93: LGTM!The addition of the guard clause to check if
exportGenesisis nil at the start of the function is a good practice. It simplifies the control flow, improves readability, and ensures that the function fails fast ifexportGenesisis not set, preventing potential nil pointer dereferences.
Line range hint
96-100: Looks good!The changes made to the
ExportGenesisfunction, such as directly callingexportGenesisand handling its errors in a straightforward manner, improve the clarity and maintainability of the code. The removal of the state retrieval from the database simplifies the function and focuses on the core functionality of exporting the genesis state. These changes align with the AI-generated summary and enhance the overall readability and maintainability of the code.runtime/v2/builder.go (1)
176-181: Approve changes toExportGenesiswith a minor suggestion for improved error handling.The modifications to the
ExportGenesisfunction align well with the PR's objectives of restoring concurrent export functionality and addressing the race condition. Retrieving the state at the specified version ensures that the genesis export is based on the most current state, enhancing the reliability of the exported data.The error handling for the state retrieval failure provides more context, improving the debuggability of the genesis export process. The approach of fetching the state here, instead of modifying the
ExportGenesisarguments, maintains backward compatibility, as mentioned in the past review comments.Minor Suggestion:
Consider wrapping the error returned byExportGenesisForModuleswith more context about the failure, similar to how it's done for the state retrieval error. This will provide even more clarity during debugging.runtime/v2/manager.go (1)
232-242: Ensure Thread-Safety of Concurrent State AccessThe use of goroutines to export genesis data concurrently is beneficial for performance. However, calling
appStf.RunWithCtxconcurrently with the sharedstatemay introduce data races or inconsistent state ifstateor its underlying components are not safe for concurrent access.Verify that
store.ReaderMapand any underlying stores support concurrent read operations without the risk of data races.To check if
store.ReaderMapis thread-safe, you can run the following script:
runtime/v2/manager.go
Outdated
| appStf appmanager.StateTransitionFunction[T], | ||
| state store.ReaderMap, |
There was a problem hiding this comment.
Avoid Leaking Internal Interfaces in Module Manager API
Passing appStf appmanager.StateTransitionFunction[T] and state store.ReaderMap directly into the ExportGenesisForModules function exposes internal interfaces and implementation details at the module manager level. This can lead to tight coupling and makes it harder to maintain and refactor the code in the future.
Consider abstracting these dependencies or providing a higher-level interface that encapsulates the state transition functionality without exposing the internal types.
github-actions
bot
removed
C:server/v2
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- runtime/v2/builder.go (1 hunks)
- runtime/v2/manager.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- runtime/v2/builder.go
Additional context used
Path-based instructions (1)
runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
runtime/v2/manager.go (2)
209-212: Improved encapsulation withgenesisResultstruct.The introduction of the
genesisResultstruct provides a cleaner way to encapsulate the exported genesis data and any errors that may occur during the export process. This improves code readability and maintainability.
Line range hint
218-243: Verify the correctness of the goroutine implementation.The use of goroutines and channels allows for concurrent execution of the
ExportGenesismethod for each module, potentially improving performance. However, spawning goroutines can be a source of non-determinism if not implemented correctly.To verify the correctness of the goroutine implementation, consider the following:
- Ensure that the goroutines are properly synchronized and do not introduce any race conditions.
- Verify that the goroutines are properly terminated and do not leak resources.
- Test the implementation thoroughly to ensure that it behaves as expected under different scenarios, such as when a module's
ExportGenesismethod returns an error.You can use the following script to search for potential synchronization issues:
| genesisData := make(map[string]json.RawMessage) | ||
| for moduleName := range channels { | ||
| res := <-channels[moduleName] | ||
| if res.err != nil { | ||
| return nil, fmt.Errorf("genesis export error in %s: %w", moduleName, res.err) | ||
| } | ||
|
|
||
| genesisData[moduleName] = res | ||
| genesisData[moduleName] = res.bz |
There was a problem hiding this comment.
Use a deterministic order when collecting genesis data.
Collecting the exported genesis data from the channels using a range loop over the channels map may result in non-deterministic ordering. This is because the order of iteration over a map is not guaranteed to be the same across different runs.
To ensure deterministic ordering, consider the following:
- Create a sorted slice of module names based on the keys of the
channelsmap. - Iterate over the sorted slice of module names to collect the exported genesis data from the channels in a deterministic order.
Here's an example of how you can modify the code to achieve deterministic ordering:
genesisData := make(map[string]json.RawMessage)
-for moduleName := range channels {
+moduleNames := make([]string, 0, len(channels))
+for moduleName := range channels {
+ moduleNames = append(moduleNames, moduleName)
+}
+sort.Strings(moduleNames)
+for _, moduleName := range moduleNames {
res := <-channels[moduleName]
if res.err != nil {
return nil, fmt.Errorf("genesis export error in %s: %w", moduleName, res.err)
}
genesisData[moduleName] = res.bz
}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.
| genesisData := make(map[string]json.RawMessage) | |
| for moduleName := range channels { | |
| res := <-channels[moduleName] | |
| if res.err != nil { | |
| return nil, fmt.Errorf("genesis export error in %s: %w", moduleName, res.err) | |
| } | |
| genesisData[moduleName] = res | |
| genesisData[moduleName] = res.bz | |
| genesisData := make(map[string]json.RawMessage) | |
| moduleNames := make([]string, 0, len(channels)) | |
| for moduleName := range channels { | |
| moduleNames = append(moduleNames, moduleName) | |
| } | |
| sort.Strings(moduleNames) | |
| for _, moduleName := range moduleNames { | |
| res := <-channels[moduleName] | |
| if res.err != nil { | |
| return nil, fmt.Errorf("genesis export error in %s: %w", moduleName, res.err) | |
| } | |
| genesisData[moduleName] = res.bz |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
runtime/v2/manager.go (1)
245-252: Use a deterministic order when collecting genesis data.Collecting the exported genesis data from the channels using a range loop over the
channelsmap may result in non-deterministic ordering. This is because the order of iteration over a map is not guaranteed to be the same across different runs.To ensure deterministic ordering, consider the following:
- Create a sorted slice of module names based on the keys of the
channelsmap.- Iterate over the sorted slice of module names to collect the exported genesis data from the channels in a deterministic order.
Here's an example of how you can modify the code to achieve deterministic ordering:
genesisData := make(map[string]json.RawMessage) -for moduleName := range channels { +moduleNames := make([]string, 0, len(channels)) +for moduleName := range channels { + moduleNames = append(moduleNames, moduleName) +} +sort.Strings(moduleNames) +for _, moduleName := range moduleNames { res := <-channels[moduleName] if res.err != nil { return nil, fmt.Errorf("genesis export error in %s: %w", moduleName, res.err) } genesisData[moduleName] = res.bz }
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- runtime/v2/builder.go (1 hunks)
- runtime/v2/manager.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- runtime/v2/builder.go
Additional context used
Path-based instructions (1)
runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
runtime/v2/manager.go (2)
Line range hint
198-256: Excellent refactoring of theExportGenesisForModulesfunction!The changes introduce several improvements:
- Asynchronous execution enables concurrent processing of module exports, enhancing performance.
- Better error handling with the
genesisResultstruct provides clearer visibility into module-specific issues.- State isolation through the
stateFactoryparameter ensures each module operates on its own context, improving robustness.- Modularity allows for targeted exports of specific modules, providing fine-grained control.
24-28: Appropriate addition of import statements.The new import statements,
"cosmossdk.io/core/store"and"cosmossdk.io/runtime/v2/services", are necessary to support the refactoredExportGenesisForModulesfunction:
"cosmossdk.io/core/store"provides thestore.WriterMaptype used in thestateFactoryparameter."cosmossdk.io/runtime/v2/services"offers theservices.NewGenesisContextfunction used to create a newgenesisContextfor each module's export operation.These import statements serve a clear purpose and are correctly added to facilitate the changes made in the
ExportGenesisForModulesfunction.
| go func(moduleI ModuleI, ch chan genesisResult) { | ||
| genesisCtx := services.NewGenesisContext(stateFactory()) | ||
| _, _ = genesisCtx.Run(ctx, func(ctx context.Context) error { | ||
| jm, err := moduleI.ExportGenesis(ctx) | ||
| if err != nil { | ||
| ch <- genesisResult{nil, err} | ||
| return err | ||
| } | ||
| ch <- genesisResult{jm, nil} | ||
| return nil | ||
| }) | ||
| }(moduleI, channels[moduleName]) |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note
Description
Closes: #21303
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