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
WalkthroughWalkthroughThis update enhances the Cosmos SDK's Changes
Sequence Diagram(s)sequenceDiagram
participant App as BaseApp
participant State as State Contexts
participant Query as Query System
App->>State: Request block header
State-->>App: Provide block header options
App->>State: Validate block headers
alt Valid header found
App->>Query: Create query context
else No valid header
App-->>Query: Return error
end
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 Configuration File (
|
![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. |
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
Bug Fixes
Tests