fix: fix sim test state persistence issue #19771
Conversation
WalkthroughWalkthroughThe changes encompass method visibility adjustments, simulation enhancements, and integration of state hash calculations in simulations. These updates aim to ensure consistency in method naming, improve simulation accuracy by adjusting parameters and calculations, and verify state integrity within the blockchain framework. Changes
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (3)
- baseapp/abci.go (3 hunks)
- x/gov/simulation/operations.go (8 hunks)
- x/simulation/simulate.go (1 hunks)
Additional comments: 7
x/simulation/simulate.go (1)
- 235-236: The addition of
app.WorkingHash()before the commit check is a crucial step towards ensuring state hash consistency. It's important to verify the implementation ofWorkingHash()to ensure it doesn't introduce any unintended side effects or significant performance overhead.Verification successful
The implementation of
WorkingHash()inbaseapp/abci.goaligns with its intended purpose of ensuring state hash consistency before commits. The method finalizes block state, writes all state transitions to the root multi-store, and computes the hash of all writes, which is crucial for the integrity and consistency of the application state. There's no evidence of unintended side effects or significant performance issues based on the provided context. The operation seems necessary and well-implemented for the application's functionality.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation details of WorkingHash to ensure no side effects or performance issues. grep -A 10 -B 10 "func (app \*BaseApp) WorkingHash()" $(fd --type f "abci.go")Length of output: 1787
x/gov/simulation/operations.go (3)
- 232-232: Adjustments to the
randomDepositfunction usage with new parameters enhance the realism of simulation. Consider adding comments explaining the rationale behind these adjustments to improve code maintainability.Also applies to: 341-341
- 281-293: Modifications in
simulateMsgSubmitProposalto adjustvoteTimeand deposit amounts aim to improve simulation accuracy. It's important to verify these changes don't negatively impact simulation performance or deviate from expected behavior.Also applies to: 310-310
- 594-619: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [564-616]
The adjustments in handling
minInitAmtandminAmtinrandomDepositare crucial for accurate simulation of deposits. Consider adding unit tests to ensure the correctness and expected behavior of these adjustments.Would you like assistance in creating unit tests for these adjustments?
baseapp/abci.go (3)
- 993-993: The method
WorkingHashhas been made public by changing its name fromworkingHashtoWorkingHash. This change aligns with the PR's objective to ensure consistency in method naming conventions and to allow external calls to this method for state hash consistency checks.- 907-907: The call to
app.WorkingHash()within theFinalizeBlockmethod ensures that the application hash (AppHash) is correctly updated before committing changes. This addition addresses the core issue of state persistence in simulation tests by ensuring that the state hash is consistent at every block height.- 893-893: Similarly, the call to
app.WorkingHash()within theFinalizeBlockmethod, but in the optimistic execution flow, ensures the application hash is correctly updated. This is crucial for maintaining state hash consistency across different execution paths.
| @@ -232,6 +232,8 @@ func SimulateFromSeed( | |||
|
|
|||
| logWriter.AddEntry(EndBlockEntry(blockHeight)) | |||
|
|
|||
| app.WorkingHash() | |||
There was a problem hiding this comment.
I think we should not make this function public but see if all operations are run in the context of a FinalizeBlock call. Given that this function is called in there
| minAmount := sdkmath.ZeroInt() | ||
| if useMinAmount { | ||
| minInitAmt := sdkmath.ZeroInt() | ||
| if useMinInitAmt { |
There was a problem hiding this comment.
Yeah, I was trying to keep the original code as much as possible. I made these changes to get the proposal to immediately enter the voting period. So the deposit amount has to satisfy both the "minimum initial deposit requirement" and the "minimum deposit requirement".
| @@ -990,7 +990,7 @@ func (app *BaseApp) Commit() (*abci.ResponseCommit, error) { | |||
| // disk in the Commit phase. This means when the ABCI client requests Commit(), the application | |||
| // state transitions will be flushed to disk and as a result, but we already have | |||
| // an application Merkle root. | |||
| func (app *BaseApp) workingHash() []byte { | |||
| func (app *BaseApp) WorkingHash() []byte { | |||
There was a problem hiding this comment.
We shouldn't make this one public, if calling this solves the issue then the problem is that there are operations running outside of a FinalizeBlock call context. Because this function is called at the end of FinalizeBlock.
|
Thank you so much for opening this PR! Left some comments but I'm pretty sure you found the issue |
julienrbrt
added
the
backport/v0.50.x
|
@facundomedica Thank you for the prompt review! So |
|
Gentle bump @hacheigriega, do you have time to implement Facu's suggestion, or one of us should take this over? |
|
Hey @julienrbrt, it seems to me that a complete fix would require a much more substantial refactor of the simulation testing mechanism. The current setup runs In short, I'm not sure how to improve this PR at the moment. Please let me know if you have a little more specific suggestions |
|
hey we are doing a refactor of simulations now. @hacheigriega would you be fine with closing this for now once that works completes ? |
Description
Related: #16796
Fix the issue where the simulation tests fail to commit and update the states at every height.
Also fix x/gov simulation operations so that its future operations are actually run.
Summary by CodeRabbit