chore: set empty hash to hash of nothing #20775
Conversation
WalkthroughThe changes focus on improving hash and response handling across multiple files in the repository. Key updates include initializing hashes with non-nil values, transitioning from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CometBFT
participant Application
Client->>CometBFT: SubmitTx()
CometBFT->>Application: CheckTx()
Application->>CometBFT: CheckTxResponse{CheckTxResponse using abciproto}
CometBFT->>Client: RespondTxStatus()
Client->>CometBFT: QueryInfo()
CometBFT->>Application: Info()
Application->>CometBFT: InfoResponse{InfoResponse using abciproto}
CometBFT->>Client: SendInfo()
Assessment against linked issues
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- core/header/service.go (1 hunks)
- server/v2/cometbft/abci.go (9 hunks)
- server/v2/stf/core_header_service.go (2 hunks)
- server/v2/stf/stf.go (1 hunks)
- store/v2/root/store.go (2 hunks)
Additional context used
Path-based instructions (5)
server/v2/stf/core_header_service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/header/service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/stf.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (9)
server/v2/stf/core_header_service.go (3)
7-7: Approved addition ofstoreimport.The addition of
"cosmossdk.io/core/store"aligns with the new functions introduced for setting and getting header info, which interact with the store.
39-55: Validate error handling and null checks ingetHeaderInfo.Proper error handling and null checks are in place. However, consider adding a comment on line 49 to explain the return of an empty
header.Infowhen the value is nil.+ // Return an empty header.Info if the stored value is nil, indicating no previous header info was set.
18-18: Check the use of a constant forheaderInfoPrefix.Verification successful
The usage of the
headerInfoPrefixconstant is consistent within theserver/v2/stf/core_header_service.gofile.
server/v2/stf/core_header_service.go: The constant is used inSetandGetmethods.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `headerInfoPrefix` to ensure it's used correctly throughout the codebase. # Test: Search for the constant usage. Expect: Consistent and correct usage. rg --type go $'headerInfoPrefix'Length of output: 280
core/header/service.go (2)
40-40: Ensure hash size validation.The validation of
i.Hashlength againsthashSizeis crucial for maintaining data integrity and preventing errors during encoding.
49-49: Ensure app hash size validation.Similar to the hash size validation, validating
i.AppHashagainsthashSizeis essential for consistency and error prevention.store/v2/root/store.go (2)
5-5: Approved addition ofcrypto/sha256import.The addition of the SHA256 library is necessary for the new functionality to compute hashes of empty byte slices, as seen in the
LastCommitIDmethod.
152-155: Verify handling of empty hashes inLastCommitID.The logic to handle empty hashes by returning a SHA256 hash of an empty byte slice when
latestVersionis 0 is a crucial update for consistency. Ensure this behavior is correctly implemented across all relevant parts of the application.server/v2/cometbft/abci.go (2)
5-5: Approved addition ofcrypto/sha256import.The addition of the SHA256 library is necessary for the new functionality to compute hashes, as seen in the
InitChainand other methods.
107-107: Review changes in response structures and hash handling in various methods.The modifications in response structures and hash handling across multiple methods such as
CheckTx,Info,InitChain,PrepareProposal,ProcessProposal,VerifyVoteExtension, andExtendVoteare crucial for aligning with the new architectural changes. Ensure these changes do not introduce any regressions or inconsistencies.Also applies to: 141-141, 235-235, 283-283, 329-329, 362-362, 368-368, 541-541, 577-577
| // setHeaderInfo sets the header info in the state to be used by queries in the future. | ||
| func (s STF[T]) setHeaderInfo(state store.WriterMap, headerInfo header.Info) error { | ||
| // TODO storing header info is too low level here, stf should be stateless. | ||
| // We should have a keeper that does this. | ||
| runtimeStore, err := state.GetWriter(Identity) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| bz, err := headerInfo.Bytes() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = runtimeStore.Set([]byte{headerInfoPrefix}, bz) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Ensure statelessness in STF design.
The comment on line 22 suggests that storing header info at this level might not be ideal, indicating a potential architectural concern. Consider refactoring to use a dedicated keeper for state management.
- // TODO storing header info is too low level here, stf should be stateless.
- // We should have a keeper that does this.
+ // Refactor to use a dedicated keeper for managing header info.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.
| // setHeaderInfo sets the header info in the state to be used by queries in the future. | |
| func (s STF[T]) setHeaderInfo(state store.WriterMap, headerInfo header.Info) error { | |
| // TODO storing header info is too low level here, stf should be stateless. | |
| // We should have a keeper that does this. | |
| runtimeStore, err := state.GetWriter(Identity) | |
| if err != nil { | |
| return err | |
| } | |
| bz, err := headerInfo.Bytes() | |
| if err != nil { | |
| return err | |
| } | |
| err = runtimeStore.Set([]byte{headerInfoPrefix}, bz) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| // setHeaderInfo sets the header info in the state to be used by queries in the future. | |
| func (s STF[T]) setHeaderInfo(state store.WriterMap, headerInfo header.Info) error { | |
| // Refactor to use a dedicated keeper for managing header info. | |
| runtimeStore, err := state.GetWriter(Identity) | |
| if err != nil { | |
| return err | |
| } | |
| bz, err := headerInfo.Bytes() | |
| if err != nil { | |
| return err | |
| } | |
| err = runtimeStore.Set([]byte{headerInfoPrefix}, bz) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
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)
- server/v2/cometbft/abci.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/abci.go
Description
Closes: #20513
in server v1 we always set an empty hash when its needed via the lastcommitid check. this pr aims to recreate this
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
HashandAppHashto ensure they are non-nil, improving stability.Refactor
abcistruct declarations and return types withabciprotofor better code clarity and maintenance.Chores
crypto/sha256to enhance security-related computations.