fix(baseapp): align block header when query with latest height #21003
Conversation
use finalize state block header when CreateQueryContext with latest height and checkState block header height is fall behind
📝 Walkthrough📝 WalkthroughWalkthroughThis update introduces significant enhancements to the Cosmos SDK, particularly with the addition of the Changes
Sequence Diagram(s)sequenceDiagram
participant App as BaseApp
participant Validator as Transaction Validator
participant Context as Query Context
App->>Validator: Validate transaction
Validator-->>App: Return validation result
alt Valid transaction
App->>Context: Create query context
else Invalid transaction
App-->>Context: Return error
end
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- baseapp/abci.go (1 hunks)
- baseapp/baseapp_test.go (3 hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional context used
Path-based instructions (2)
baseapp/baseapp_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"baseapp/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (5)
baseapp/baseapp_test.go (4)
691-693: LGTM!The
mockABCIListenerstruct is correctly implemented for testing purposes.
695-697: LGTM!The
ListenFinalizeBlockmethod is correctly implemented to return nil.
699-701: LGTM!The
ListenCommitmethod correctly uses the providedListenCommitFnfunction to handle commit responses.
744-786: LGTM!The test function
TestABCI_CreateQueryContext_Before_Set_CheckStateis correctly implemented to validate the behavior of theCreateQueryContextmethod.baseapp/abci.go (1)
1248-1269: LGTM!The updated logic in the
CreateQueryContextmethod enhances the robustness of the context creation by ensuring that it only proceeds with a valid block header.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- baseapp/abci.go (3 hunks)
- baseapp/baseapp_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- baseapp/baseapp_test.go
Additional context used
Path-based instructions (1)
baseapp/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (10)
baseapp/abci.go (10)
1203-1204: Refactor: Delegate toCreateQueryContextWithCheckHeader.The
CreateQueryContextfunction now delegates toCreateQueryContextWithCheckHeaderwith an additionalcheckHeaderargument set totrue. This refactor is correct and aligns with the new logic introduced.
1206-1208: Function Documentation: Ensure Clarity.The function documentation clearly describes the purpose and parameters of
CreateQueryContextWithCheckHeader. This is a good practice.
Line range hint
1209-1213:
Error Handling: Check for Negative Height.The function correctly checks for a negative height and returns an appropriate error. This is a good practice for input validation.
Line range hint
1214-1216:
Use Custom Query Multi-Store if Provided.The function uses a custom query multi-store if provided, otherwise defaults to the commit multi-store. This adds flexibility to the function.
Line range hint
1217-1224:
Error Handling: Validate Height Against Latest Version.The function validates the provided height against the latest version of the multi-store, ensuring that queries are not made for future heights. This is a good practice.
Line range hint
1225-1231:
Error Handling: Validate Proof for Heights <= 1.The function correctly handles the case where proof is requested for heights less than or equal to 1, which is invalid.
Line range hint
1232-1240:
Cache Multi-Store with Version: Error Handling.The function attempts to cache the multi-store with the specified version and handles errors appropriately. This ensures that the state at the specified height is correctly loaded.
1254-1267: Header Validation: Loop Through States.The function loops through the available states (
checkStateandfinalizeBlockState) to find a valid block header. This ensures that the correct header is used for the query context.
1269-1275: Error Handling: No Valid Header Found.The function returns an error if no valid header is found in the available states. This ensures that the query context is only created with a valid header.
1276-1284: Create Query Context: Set Context Parameters.The function sets various parameters in the context, including gas meter, header info, and block height. This ensures that the context is correctly configured for the query.
|
Can you expand a bit more on why this is needed and if there were issues caused by this? Thank you! |
I was trying to use block timestamp as random seed for smart contract. Currently a query result of new block height may be returned with the previous block timestamp if the block is committed but state not being set.@facundomedica you can see the case in |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
baseapp/abci.go (1)
1203-1204: Update all instances ofCreateQueryContextto useCreateQueryContextWithCheckHeaderThe following instances of
CreateQueryContextneed to be updated to use the newCreateQueryContextWithCheckHeaderfunction:
baseapp/baseapp_test.go(multiple occurrences)baseapp/abci.go(multiple occurrences)baseapp/grpcserver.goPlease ensure these updates are made to maintain consistency and avoid potential errors.
Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
CreateQueryContextare correctly updated to use the newCreateQueryContextWithCheckHeaderfunction.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `CreateQueryContext` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'CreateQueryContext'Length of output: 4019
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- baseapp/abci.go (5 hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional context used
Path-based instructions (1)
baseapp/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
baseapp/abci.go (1)
Line range hint
1208-1282:
LGTM! Robust error handling and context management.The code changes are approved.
However, ensure that all function calls to
CreateQueryContextWithCheckHeaderare correctly updated and tested in the codebase.Verification successful
Function calls to
CreateQueryContextWithCheckHeaderhave been correctly updated.The function calls in
baseapp/abci.goandbaseapp/baseapp_test.gomatch the new signature, ensuring consistency across the codebase.
baseapp/abci.gobaseapp/baseapp_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `CreateQueryContextWithCheckHeader` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'CreateQueryContextWithCheckHeader'Length of output: 1744
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- baseapp/baseapp_test.go (3 hunks)
Additional context used
Path-based instructions (1)
baseapp/baseapp_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"
Additional comments not posted (6)
baseapp/baseapp_test.go (6)
691-693: LGTM! The mock listener is well-defined.The
mockABCIListenerstruct and itsListenCommitFnfunction pointer are correctly implemented for mocking purposes.
695-697: LGTM! TheListenFinalizeBlockmethod is correctly implemented.The method correctly returns nil, as expected for a mock implementation.
699-701: LGTM! TheListenCommitmethod is correctly implemented.The method correctly delegates to the
ListenCommitFnfunction pointer.
Line range hint
709-742:
LGTM! The test cases are comprehensive and well-structured.The function
TestABCI_CreateQueryContextcovers various scenarios for creating query contexts, ensuring robust error handling.
744-773: LGTM! The test cases for header checks are well-structured.The function
TestABCI_CreateQueryContextWithCheckHeadercovers scenarios with and without header checks, ensuring correct behavior.
775-817: LGTM! The test case ensures correct integration and query context creation.The function
TestABCI_CreateQueryContext_Before_Set_CheckStateensures that the query context is created successfully and the block height matches expectations after the commit phase.
mmsqe
force-pushed
the
align_query_height
branch
from
3ab599d
to
246e258
Compare
|
Hey I tried |
baseapp/abci.go
Outdated
| defer func() { | ||
| if err == nil { | ||
| rms, ok := app.cms.(*rootmulti.Store) | ||
| if ok { | ||
| cInfo, err := rms.GetCommitInfo(height) | ||
| if cInfo != nil && err == nil { | ||
| ctx = ctx.WithHeaderInfo(coreheader.Info{Height: height, Time: cInfo.Timestamp}) | ||
| } | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
why is this defer necessary?
There was a problem hiding this comment.
just replaced it with isLatest to avoid confusion
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- baseapp/abci.go (4 hunks)
- baseapp/baseapp_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- baseapp/baseapp_test.go
Additional context used
Path-based instructions (1)
baseapp/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (8)
baseapp/abci.go (8)
Line range hint
142-146:
Ensure the correctness of the new function usage.The
Infofunction now usesCreateQueryContextWithCheckHeader. Verify that this change aligns with the intended logic and that thecheckHeaderparameter is correctly set tofalse.
1203-1204: Delegation to new function looks good.The
CreateQueryContextfunction correctly delegates toCreateQueryContextWithCheckHeaderwith thecheckHeaderparameter set totrue.
1208-1209: Check for negative height.The function starts by checking for negative height values, which is a good practice to ensure valid input.
1240-1244: Iterate over state contexts to determine the appropriate header.The loop iterates over various state contexts to determine the appropriate header. This enhances the robustness of the context creation process.
1258-1264: Handle missing header scenario.The function handles the scenario where the header is not found in any state context, returning an explicit error. This improves error handling.
1270-1280: Deferred function for setting header info.The deferred function ensures that the header info is set correctly after the main logic executes, which is a good practice for maintaining context integrity.
1292-1299: Create new context with appropriate settings.The function creates a new context with the appropriate settings, including gas meter and header info. This ensures that the context is correctly initialized for queries.
Line range hint
1197-1199:
Verify the function usage.The
handleQueryGRPCfunction callsCreateQueryContext, which now delegates toCreateQueryContextWithCheckHeader. Ensure that this change aligns with the intended logic.
Might need some check in main to reproduce the fail test |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- baseapp/baseapp_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- baseapp/baseapp_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
baseapp/abci.go (1)
1203-1204: Update function calls to match the new signatureThe following function calls to
CreateQueryContextneed to be updated to match the new signature or replaced byCreateQueryContextWithCheckHeader:
baseapp/abci.go
- Line 1203:
ctx, err := app.CreateQueryContext(req.Height, req.Prove)baseapp/grpcserver.go
- Line 15:
sdkCtx, err := app.CreateQueryContext(height, false)baseapp/baseapp_test.go
- Line 220:
ctx, err := app.CreateQueryContext(height, tc.prove)- Line 303:
qCtx, qErr := app.CreateQueryContext(0, true)- Line 350:
ctx, err := bapp.CreateQueryContext(1, false)Ensure these calls are updated to use
CreateQueryContextWithCheckHeaderor the correct parameters are passed toCreateQueryContext.Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
CreateQueryContextmatch the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `CreateQueryContext` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'CreateQueryContext'Length of output: 4006
Script:
#!/bin/bash # Description: Verify all function calls to `CreateQueryContext` match the new signature or need to be updated. # Check the lines where `CreateQueryContext` is used in the codebase rg --type go -C 3 'CreateQueryContext' baseapp/baseapp_test.go baseapp/grpcserver.go baseapp/abci.goLength of output: 4696
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- baseapp/abci.go (5 hunks)
Additional context used
Path-based instructions (1)
baseapp/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
baseapp/abci.go (2)
Line range hint
1206-1295:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
CreateQueryContextWithCheckHeadermatch the new signature.Verification successful
Function usage verification successful.
All instances of
CreateQueryContextWithCheckHeadermatch the new function signature with three parameters:height,prove, andcheckHeader.
baseapp/baseapp_test.gobaseapp/abci.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `CreateQueryContextWithCheckHeader` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'CreateQueryContextWithCheckHeader'Length of output: 1739
142-144: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
CreateQueryContextWithCheckHeadermatch the new signature.Verification successful
Verified: The function
CreateQueryContextWithCheckHeaderis used consistently with the new signature.The code changes are correctly implemented, and all function calls match the new signature.
baseapp/abci.gobaseapp/baseapp_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `CreateQueryContextWithCheckHeader` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'CreateQueryContextWithCheckHeader'Length of output: 1739
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Could you create an issue about this explaining the problem? Then we'll assign people to review this. |
Just add #21615, see if it's ok. |
mmsqe
requested review from
aaronc,
testinginprod and
kocubinski
as code owners
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.md (1)
The changes to UnbondingDelegation in the v0.47.0 release of the Cosmos SDK introduce significant updates to the staking module, particularly in relation to unbonding delegations. Here are the key points:
New functionality: A new message type,
MsgCancelUnbondingDelegation, has been introduced. This allows delegators to cancel an ongoing unbonding delegation, which is a significant new feature in the staking module.Query improvements: There are numerous new or updated query functions related to UnbondingDelegation, such as
ValidatorUnbondingDelegations,DelegatorUnbondingDelegations, andUnbondingDelegation. This suggests improved capabilities for querying unbonding delegation information.Breaking changes: The CHANGELOG.md file explicitly mentions both API Breaking Changes and State Breaking changes. This indicates that there are significant changes that may affect existing implementations and potentially the state of the blockchain.
Potential structural changes: While we couldn't directly observe the UnbondingDelegation structure, the presence of many related functions suggests that there might be changes to how unbonding delegations are handled internally.
Developers working with the staking module, particularly those dealing with unbonding delegations, should carefully review these changes. The new cancellation feature and updated querying capabilities may require updates to existing implementations. Additionally, the mention of breaking changes in the changelog emphasizes the need for thorough testing and potentially migration strategies when upgrading to this version.
🔗 Analysis chain
Line range hint
145-240: Significant API changes require careful migrationThe v0.47.0 release introduces numerous API breaking changes that will require careful attention from developers updating their applications. Some key points to note:
- Many modules have undergone significant restructuring, including x/auth, x/bank, x/gov, and x/staking.
- Several function signatures and type definitions have changed, often to improve consistency or add new functionality.
- Some modules have been moved to their own go.mod files, which may affect dependency management.
- There are changes to how certain operations are performed, such as signing transactions and handling capabilities.
Developers should thoroughly review these changes and plan for a comprehensive migration process when upgrading to v0.47.0. It's recommended to update applications in a staged manner and thoroughly test all functionality after the upgrade.
To help verify the impact of these changes, you can run the following command to check for usage of deprecated or removed APIs:
This will help identify areas of your codebase that may need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of removed or changed APIs rg -i -e "RegisterCodec" -e "GetSignBytes" -e "Route" -e "Type" --type goLength of output: 380853
Script:
#!/bin/bash # Search for changes in UnbondingDelegation related APIs grep -R "func.*UnbondingDelegation" x/staking/ grep -R "type.*UnbondingDelegation" x/staking/ grep -R "MsgCancelUnbondingDelegation" x/staking/Length of output: 50406
Script:
#!/bin/bash # Compare UnbondingDelegation structure and check for removed APIs git show v0.46.0:x/staking/types/delegation.go | grep -A 20 "type UnbondingDelegation struct" echo "---" cat x/staking/types/delegation.go | grep -A 20 "type UnbondingDelegation struct" echo "---" git diff v0.46.0..HEAD x/staking/keeper/keeper.go | grep "func.*Unbonding.*Delegation.*("Length of output: 532
Script:
#!/bin/bash # Examine current UnbondingDelegation structure and related functions echo "UnbondingDelegation structure:" grep -n -A 20 "type UnbondingDelegation struct" x/staking/types/delegation.go echo -e "\nUnbondingDelegation related functions:" grep -n "func.*Unbonding.*Delegation" x/staking/types/*.go echo -e "\nChecking for breaking change comments:" grep -n -i "breaking change" x/staking/CHANGELOG.mdLength of output: 24003
baseapp/baseapp_test.go (1)
770-784: Improve test naming for better clarity in subtestsIn
TestABCI_CreateQueryContextWithCheckHeader, the subtests use the constant name"valid height with different initial height"int.Run. This can make it difficult to identify which specific test case fails during execution. To enhance clarity, consider adding anamefield to your test cases and using it int.Run, so that each subtest is uniquely identified.Apply this diff to improve subtest naming:
testCases := []struct { - checkHeader bool - expErr bool + name string + checkHeader bool + expErr bool }{ - {true, true}, - {false, false}, + {"checkHeader true, expect error", true, true}, + {"checkHeader false, expect no error", false, false}, } for _, tc := range testCases { - t.Run("valid height with different initial height", func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) {baseapp/abci.go (1)
1212-1214: Add detailed documentation for thecheckHeaderparameter.The new function
CreateQueryContextWithCheckHeaderintroduces acheckHeaderboolean parameter. Providing more detailed comments or examples on when to setcheckHeadertotrueorfalsewould improve code readability and help other developers understand the intended usage.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- baseapp/abci.go (4 hunks)
- baseapp/baseapp_test.go (4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"baseapp/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/baseapp_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"
🔇 Additional comments (9)
CHANGELOG.md (5)
Line range hint
1-39: Impressive list of new features!The v0.47.0 release introduces several important features that enhance the Cosmos SDK's functionality and developer experience. Some notable additions include:
- New
grpc-onlyflag for query-only mode.- Improved transaction simulation and block results querying.
- Support for creating permanent locked accounts.
- Enhanced governance features, including expedited proposals.
- New CLI commands for various modules.
These features demonstrate a strong focus on improving the SDK's capabilities and usability.
Line range hint
41-108: Significant improvements across the boardThe v0.47.0 release includes a wide range of improvements that enhance the Cosmos SDK's performance, maintainability, and developer experience. Some notable improvements include:
- Optimization of iteration on nested cached KV stores.
- Improved error messages and logging throughout the codebase.
- Refactoring of various modules to use collections for state management.
- Enhanced CLI commands and query capabilities.
- Improved genesis validation and export functionality.
These improvements demonstrate a commitment to continually refining and optimizing the Cosmos SDK.
Line range hint
110-143: Critical bug fixes enhance stabilityThe v0.47.0 release includes several important bug fixes that address various issues across the Cosmos SDK. Some notable fixes include:
- Resolving data races in store trace components and mempools.
- Fixing panics in various scenarios, such as when parsing telemetry configuration.
- Correcting behavior in modules like x/gov, x/bank, and x/staking.
- Addressing issues with genesis state handling and migration.
These bug fixes contribute to the overall stability and reliability of the Cosmos SDK, which is crucial for developers building applications on top of it.
Line range hint
1-266: Comprehensive update with significant improvements and breaking changesThe v0.47.0 release of the Cosmos SDK represents a major update with far-reaching implications for developers and network operators. This release introduces numerous new features, performance improvements, and bug fixes that enhance the overall functionality and reliability of the SDK.
However, it's crucial to note the extensive API and state machine breaking changes. These changes will require significant effort to migrate existing applications and coordinate network upgrades. Developers should plan for a thorough review and testing process when upgrading to this version.
Key takeaways:
- New features improve functionality across various modules.
- Performance optimizations and bug fixes enhance stability.
- Extensive API changes require careful code updates.
- State machine breaking changes necessitate coordinated network upgrades.
Overall, while the upgrade process may be challenging, the improvements offered by v0.47.0 provide a solid foundation for building more robust and efficient blockchain applications.
Line range hint
242-266: Critical state machine changes require coordinated upgradeThe v0.47.0 release includes several state machine breaking changes that are crucial to understand:
- Changes to how various modules store and process state, including x/bank, x/gov, and x/staking.
- Updates to consensus parameters and how they are managed.
- Modifications to how certain operations, like unbonding in x/staking, are handled.
These changes are critical and require careful planning for a coordinated upgrade across all nodes in a network. Failure to properly handle these changes could result in consensus failures or other serious issues.
Network operators should:
- Thoroughly test the upgrade process on a testnet before applying to mainnet.
- Ensure all validators and nodes are prepared to upgrade simultaneously.
- Have a rollback plan in case of unexpected issues during the upgrade.
To help verify the impact of these changes, you can use the following command to check for any remaining usage of old state keys or deprecated state handling:
This will help identify any areas of your codebase that may need updating to align with the new state machine changes.
baseapp/baseapp_test.go (2)
787-829: Questioning the order ofInitChainafter block commitmentsIn
TestABCI_CreateQueryContext_Before_Set_CheckState, the call toInitChainis made after finalizing and committing blocks. Typically,InitChainis called before processing any blocks to initialize the application state. Verify whether callingInitChainafter committing blocks is intentional and necessary for this test scenario, as it may not reflect the actual initialization sequence in production.
756-785: Unit tests adequately cover new functionalityThe newly added test
TestABCI_CreateQueryContextWithCheckHeadereffectively tests the behavior ofCreateQueryContextWithCheckHeaderwith differentcheckHeadervalues. The test ensures that whencheckHeaderistrue, an error is expected, and whenfalse, no error occurs and the block height is correctly set. This enhances the test coverage for the new functionality introduced.baseapp/abci.go (2)
145-145: Verify the implications of disabling header checks inCreateQueryContextWithCheckHeader.In the
Infomethod,CreateQueryContextWithCheckHeaderis called withcheckHeaderset tofalse. Please ensure that bypassing the header check here does not introduce any security vulnerabilities or inconsistent state, especially when retrieving the application version.
1209-1210: Ensure consistent behavior whencheckHeaderis set totrue.The
CreateQueryContextmethod now callsCreateQueryContextWithCheckHeaderwithcheckHeaderset totrue. Please verify that this change maintains the intended behavior across all uses ofCreateQueryContext, and that header checks are performed correctly.
| func (m mockABCIListener) ListenFinalizeBlock(_ context.Context, _ abci.FinalizeBlockRequest, _ abci.FinalizeBlockResponse) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (m *mockABCIListener) ListenCommit(ctx context.Context, commit abci.CommitResponse, pairs []*storetypes.StoreKVPair) error { | ||
| return m.ListenCommitFn(ctx, commit, pairs) | ||
| } |
There was a problem hiding this comment.
Inconsistent method receiver types in mockABCIListener methods
The methods ListenFinalizeBlock and ListenCommit of mockABCIListener use different receiver types: ListenFinalizeBlock uses a value receiver (m mockABCIListener), whereas ListenCommit uses a pointer receiver (m *mockABCIListener). According to Go best practices and the Uber Go Style Guide, methods on the same type should have consistent receiver types to avoid confusion and potential bugs. Consider changing ListenFinalizeBlock to use a pointer receiver for consistency.
Apply this diff to fix the inconsistent receiver:
-func (m mockABCIListener) ListenFinalizeBlock(_ context.Context, _ abci.FinalizeBlockRequest, _ abci.FinalizeBlockResponse) error {
+func (m *mockABCIListener) ListenFinalizeBlock(_ context.Context, _ abci.FinalizeBlockRequest, _ abci.FinalizeBlockResponse) error {📝 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.
| func (m mockABCIListener) ListenFinalizeBlock(_ context.Context, _ abci.FinalizeBlockRequest, _ abci.FinalizeBlockResponse) error { | |
| return nil | |
| } | |
| func (m *mockABCIListener) ListenCommit(ctx context.Context, commit abci.CommitResponse, pairs []*storetypes.StoreKVPair) error { | |
| return m.ListenCommitFn(ctx, commit, pairs) | |
| } | |
| func (m *mockABCIListener) ListenFinalizeBlock(_ context.Context, _ abci.FinalizeBlockRequest, _ abci.FinalizeBlockResponse) error { | |
| return nil | |
| } | |
| func (m *mockABCIListener) ListenCommit(ctx context.Context, commit abci.CommitResponse, pairs []*storetypes.StoreKVPair) error { | |
| return m.ListenCommitFn(ctx, commit, pairs) | |
| } |
| WithBlockHeader(*header). | ||
| WithBlockHeight(height) | ||
|
|
||
| if height != lastBlockHeight { | ||
| if !isLatest { |
There was a problem hiding this comment.
Handle errors when retrieving commit info for historical queries.
At lines 1295-1298, when attempting to retrieve the commit info to get the timestamp for historical queries, the code assumes GetCommitInfo succeeds if err == nil and cInfo != nil. However, it's possible for GetCommitInfo to return an error or nil cInfo. Ensure that errors are properly handled, and default values are used if necessary.
Apply this diff to handle potential errors:
if !isLatest {
rms, ok := app.cms.(*rootmulti.Store)
if ok {
cInfo, err := rms.GetCommitInfo(height)
- if cInfo != nil && err == nil {
+ if err != nil {
+ return ctx, errorsmod.Wrapf(err, "failed to get commit info for height %d", height)
+ }
+ if cInfo != nil {
ctx = ctx.WithHeaderInfo(coreheader.Info{Height: height, Time: cInfo.Timestamp})
+ } else {
+ ctx = ctx.WithHeaderInfo(coreheader.Info{Height: height})
+ }
}
}📝 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.
| WithBlockHeader(*header). | |
| WithBlockHeight(height) | |
| if height != lastBlockHeight { | |
| if !isLatest { | |
| WithBlockHeader(*header). | |
| WithBlockHeight(height) | |
| if !isLatest { | |
| rms, ok := app.cms.(*rootmulti.Store) | |
| if ok { | |
| cInfo, err := rms.GetCommitInfo(height) | |
| if err != nil { | |
| return ctx, errorsmod.Wrapf(err, "failed to get commit info for height %d", height) | |
| } | |
| if cInfo != nil { | |
| ctx = ctx.WithHeaderInfo(coreheader.Info{Height: height, Time: cInfo.Timestamp}) | |
| } else { | |
| ctx = ctx.WithHeaderInfo(coreheader.Info{Height: height}) | |
| } | |
| } | |
| } |
| var header *cmtproto.Header | ||
| isLatest := height == 0 | ||
| for _, state := range []*state{ | ||
| app.checkState, | ||
| app.finalizeBlockState, | ||
| } { | ||
| if state != nil { | ||
| // branch the commit multi-store for safety | ||
| h := state.Context().BlockHeader() | ||
| if isLatest { | ||
| lastBlockHeight = qms.LatestVersion() | ||
| } | ||
| if !checkHeader || !isLatest || isLatest && h.Height == lastBlockHeight { | ||
| header = &h | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if header == nil { | ||
| return sdk.Context{}, | ||
| errorsmod.Wrapf( | ||
| sdkerrors.ErrInvalidHeight, | ||
| "header height in all state context is not latest height (%d)", lastBlockHeight, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Ensure header is properly assigned to prevent nil dereference.
In the loop starting at line 1246, if neither app.checkState nor app.finalizeBlockState is non-nil or meet the conditions, header may remain nil, potentially leading to a nil pointer dereference later in the code. Please ensure that header is always assigned a valid value or add error handling for the case when it remains nil.
Apply this diff to add a check for header being nil:
}
+ if header == nil {
+ return sdk.Context{},
+ errorsmod.Wrapf(
+ sdkerrors.ErrInvalidHeight,
+ "failed to retrieve header for height %d", height,
+ )
+ }
// when a client did not provide a query height, manually inject the latestCommittable suggestion was skipped due to low confidence.
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 6cc6a6c) # Conflicts: # CHANGELOG.md
…ort #21003) (#22142) Co-authored-by: mmsqe <[email protected]> Co-authored-by: marbar3778 <[email protected]>
Description
Closes: #XXXX
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
x/validatemodule for transaction validation and handler management.crypto/keyringmodule.sims2framework and simplified message factories.Bug Fixes
BaseApp.Initmethod andtypes/mempoolinterface.Tests