chore(cometBFTService): simplified CometBFTService #2026
Conversation
WalkthroughThe changes introduced in this pull request involve significant modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)
🔇 Additional comments (15)
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
CodeRabbit Configuration File (
|
| @@ -42,6 +42,8 @@ import ( | |||
| "github.com/sourcegraph/conc/iter" | |||
| ) | |||
|
|
|||
| var errInvalidHeight = errors.New("invalid height") | |||
There was a problem hiding this comment.
nit, which allows to find all places where req.Height is checked more easily
| @@ -57,7 +59,6 @@ func (s *Service[LoggerT]) InitChain( | |||
|
|
|||
| // On a new chain, we consider the init chain block height as 0, even though | |||
| // req.InitialHeight is 1 by default. | |||
| initHeader := cmtproto.Header{ChainID: req.ChainId, Time: req.Time} | |||
There was a problem hiding this comment.
initHeader is really used only in defer call, to prepare context for next block. Moved it to defer to simplify logic and reduce its scope. This is different from cosmos SDK BaseApp where the header is used to setup the state
| @@ -70,14 +71,13 @@ func (s *Service[LoggerT]) InitChain( | |||
| // proposing | |||
| // or processing the first block or not. | |||
| s.initialHeight = req.InitialHeight | |||
| if s.initialHeight == 0 { // If initial height is 0, set it to 1 | |||
There was a problem hiding this comment.
non informative comment, dropped
| @@ -90,19 +90,19 @@ func (s *Service[LoggerT]) InitChain( | |||
| // InitChain represents the state of the application BEFORE the first | |||
| // block, i.e. the genesis block. This means that when processing the | |||
| // app's InitChain handler, the block height is zero by default. | |||
| // However, after Commit is called | |||
There was a problem hiding this comment.
nit: Commit is not really called for setting initial state
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2026 +/- ##
==========================================
+ Coverage 22.38% 22.42% +0.04%
==========================================
Files 358 358
Lines 16042 16012 -30
Branches 12 12
==========================================
Hits 3591 3591
+ Misses 12302 12272 -30
Partials 149 149
|
| if s.finalizeBlockState == nil { | ||
| return nil, errors.New("finalizeBlockState is nil") | ||
| } |
There was a problem hiding this comment.
redundant check since s.setState(execModeFinalize) is called right before. We don't do these checks after setState is called for proposals.
| return ctx | ||
| } | ||
| ctx, _ = s.finalizeBlockState.Context().CacheContext() | ||
| if height != s.initialHeight || s.finalizeBlockState == nil { |
There was a problem hiding this comment.
nit: reduced indentation. This is really a personal preference, I am happy to revert it if we like the current version better
| // initialHeight is used to determine if we are | ||
| // proposing or processing the first block or not. | ||
| initialHeight int64 |
There was a problem hiding this comment.
I like an explanation of attributes where we declare them. Happy to revert if we don't agree on this (or it breaks codebase conventions)
abi87
force-pushed
the
cometBFT-some-more-cleanup-2
branch
from
a14da18
to
cfe9f30
Compare
abi87
changed the title
initHeaderinitChainer + nits
| return &cmtabci.InitChainResponse{ | ||
| ConsensusParams: res.ConsensusParams, |
There was a problem hiding this comment.
ConsensusParams is not set by the response generated in initChainer, so this effectively assigning default value of ConsensusParams.
Replaced with ConsensusParams from request, which I believe is what we need to do
| if s.finalizeBlockState == nil { | ||
| return nil, errors.New("finalizeBlockState is nil") | ||
| } |
There was a problem hiding this comment.
redundant check since s.setState(execModeFinalize) is called right before. We don't do these checks after setState is called for proposals.
abi87
force-pushed
the
cometBFT-some-more-cleanup-2
branch
from
cc31d0d
to
daa2785
Compare
| @@ -42,7 +42,8 @@ import ( | |||
| "github.com/sourcegraph/conc/iter" | |||
| ) | |||
|
|
|||
| //nolint:gocognit // todo fix. | |||
There was a problem hiding this comment.
side-effect of InitChain changes
abi87
force-pushed
the
cometBFT-some-more-cleanup-2
branch
from
daa2785
to
f54e688
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
mod/consensus/pkg/cometbft/service/abci.go (1)
Line range hint
472-486: Avoid panicking inworkingHash; handlenilfinalizeBlockStategracefullyUsing
panicwhens.finalizeBlockStateisnilcan cause the application to crash unexpectedly. It's better to handle this scenario by returning an error or logging an appropriate message.Consider modifying the function to return an error instead of panicking:
-func (s *Service[LoggerT]) workingHash() []byte { +func (s *Service[LoggerT]) workingHash() ([]byte, error) { // Write the FinalizeBlock state into branched storage and commit the // MultiStore. if s.finalizeBlockState == nil { - panic(fmt.Errorf("workingHash: %w", errNilFinalizeBlockState)) + return nil, fmt.Errorf("workingHash: %w", errNilFinalizeBlockState) } s.finalizeBlockState.ms.Write() // Get the hash of all writes to return the app hash. commitHash := s.sm.CommitMultiStore().WorkingHash() s.logger.Debug( "hash of all writes", "workingHash", fmt.Sprintf("%X", commitHash), ) - return commitHash + return commitHash, nil }Update the calling code to handle the returned error appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- mod/consensus/pkg/cometbft/service/abci.go (10 hunks)
- mod/consensus/pkg/cometbft/service/service.go (2 hunks)
🔇 Additional comments (4)
mod/consensus/pkg/cometbft/service/abci.go (4)
45-48: Introduction of standard error variables looks goodDefining
errInvalidHeightanderrNilFinalizeBlockStatestandardizes error handling throughout the service, enhancing maintainability and consistency.
263-268: Review the resetting ofs.finalizeBlockStateinProcessProposalResetting
s.finalizeBlockStatewithinProcessProposalwhenreq.Height > s.initialHeightmay have unintended side effects ifs.finalizeBlockStateis expected to persist between proposals or rounds. Please ensure that this reset aligns with the application's state management and does not interfere with other operations.
495-498: Verify context selection logic ingetContextForProposalIn
getContextForProposal, the context froms.finalizeBlockStateis used whenheight == s.initialHeightands.finalizeBlockStateis notnil. Please confirm that using the context fromfinalizeBlockStatein this scenario is intentional and that it won't cause issues with state consistency.
162-165:⚠️ Potential issueHandle missing 'beacon' key in
genesisStateAccessing
genesisState["beacon"]without verifying its existence could lead to a runtime error if the key is missing. Please ensure that the 'beacon' key exists before using it.Consider modifying the code to handle the missing key:
if err := json.Unmarshal(appStateBytes, &genesisState); err != nil { return nil, err } + beaconState, exists := genesisState["beacon"] + if !exists { + return nil, errors.New("'beacon' key missing in genesisState") + } valUpdates, err := s.Middleware.InitGenesis( ctx, - []byte(genesisState["beacon"]), + []byte(beaconState), ) if err != nil { return nil, err }Likely invalid or redundant comment.
| // A state is explicitly returned to avoid false positives from | ||
| // nilaway tool. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify the comment regarding static analysis tool.
The comment references the "nilaway tool," which might be unclear or incorrectly named. For better clarity, consider specifying the correct tool name or generalizing the statement.
Suggested revision:
-// A state is explicitly returned to avoid false positives from
-// nilaway tool.
+// Explicitly returning the state helps avoid false positives from
+// static analysis tools that check for nil references.📝 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.
| // A state is explicitly returned to avoid false positives from | |
| // nilaway tool. | |
| // Explicitly returning the state helps avoid false positives from | |
| // static analysis tools that check for nil references. |
There was a problem hiding this comment.
I like being specific (a different tool may give different results) but @itsdevbear and @ocnc may have a different opinion
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/consensus/pkg/cometbft/service/abci.go (10 hunks)
🔇 Additional comments (1)
mod/consensus/pkg/cometbft/service/abci.go (1)
106-109: Update usage ofinitChainerto match the new signatureThe
initChainermethod now acceptsappStateBytes []byteinstead of the entireInitChainRequest. This change simplifies the function parameters. Ensure that all invocations ofinitChainerare updated accordingly, and thatappStateBytescorrectly represents the application state.
| if req.Height < 1 { | ||
| return fmt.Errorf("invalid height: %d", req.Height) | ||
| return fmt.Errorf("FinalizeBlock: %w %v", errInvalidHeight, req.Height) | ||
| } |
There was a problem hiding this comment.
Looks good, nit: error message generally starts with smaller case letter. Similar comments at few other places.
There was a problem hiding this comment.
Thanks for spotting this, @nidhi-singh02 . Fixed
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/consensus/pkg/cometbft/service/abci.go (10 hunks)
🔇 Additional comments (3)
mod/consensus/pkg/cometbft/service/abci.go (3)
46-47: Error messages should start with lowercase lettersAccording to Go conventions, error messages should start with lowercase letters unless they begin with a proper noun or acronyms.
128-142: Ensure both validator lists are sorted before comparisonCurrently, only
req.Validatorsis sorted before the comparison withresValidators. To ensure accurate matching of validators, both lists should be sorted.
471-471: Avoid panicking; handlenilfinalizeBlockStategracefullyPanicking when
s.finalizeBlockStateisnilcan lead to undesirable crashes. It's better to handle this scenario by returning an error to maintain application stability.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/consensus/pkg/cometbft/service/abci.go (10 hunks)
🔇 Additional comments (3)
mod/consensus/pkg/cometbft/service/abci.go (3)
45-48: Added error variables for standardized error handlingThe introduction of
errInvalidHeightanderrNilFinalizeBlockStateimproves error consistency across the codebase, enhancing maintainability.
158-159: SimplifiedinitChainermethod signatureUpdating the
initChainerfunction to accept onlyappStateBytes []byteenhances clarity and reduces unnecessary parameter passing.
106-109: Ensure all calls toinitChainerhave updated signaturesThe
initChainermethod signature has been updated to acceptappStateBytes []byteinstead of the entire request. Please verify that all invocations ofinitChainerin the codebase have been updated accordingly.Run the following script to find all usages of
initChainer:✅ Verification successful
Verification Passed: All calls to
initChaineruse the updated signatureinitChainer(ctx sdk.Context, appStateBytes []byte).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `initChainer` and display their signatures. # Expected: All calls should match the new signature `initChainer(ctx sdk.Context, appStateBytes []byte)` rg --type go --no-heading --line-number 'initChainer\('Length of output: 229
Script:
#!/bin/bash # Description: Retrieve the line where `initChainer` is called to inspect its arguments. # Fetch the specific line from abci.go sed -n '106p' mod/consensus/pkg/cometbft/service/abci.goLength of output: 94
Script:
#!/bin/bash # Description: Retrieve lines 106-109 from abci.go to inspect arguments passed to `initChainer`. sed -n '106,109p' mod/consensus/pkg/cometbft/service/abci.goLength of output: 156
| ) | ||
| } | ||
|
|
||
| // check validators | ||
| if len(req.Validators) > 0 { |
There was a problem hiding this comment.
q: this allows an empty validator set. I saw that cosmos sdk baseApp does the same. But should we? In which case we are fine having no validators defined at genesis? @ocnc @itsdevbear
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/consensus/pkg/cometbft/service/abci.go (10 hunks)
🔇 Additional comments (10)
mod/consensus/pkg/cometbft/service/abci.go (10)
45-47: Consistent Error Handling: Define Error VariablesDefining error variables
errInvalidHeightanderrNilFinalizeBlockStateenhances consistency in error handling and makes the code more maintainable.
88-88: EnsurefinalizeBlockStateis Properly ResetSetting
s.finalizeBlockStateby callings.resetState()ensures that the state is clean before initialization.
94-95: Correct Block Height After GenesisAdjusting the height in the
initHeaderafter genesis handling ensures that the block height reflects the true state of the blockchain.
106-109: SimplifyinitChainerParametersUpdating the
initChainermethod to acceptappStateBytesinstead of the entireInitChainRequestsimplifies the function signature and focuses on the necessary data.
146-146: Set Appropriate Consensus Parameters inInitChainResponseAssigning
ConsensusParamsfrom the request ensures that the consensus parameters are correctly initialized in the response.
158-158: Handle Potential Unmarshal Errors GracefullyEnsure that any errors during JSON unmarshalling of
appStateBytesare properly handled and logged, providing clarity in case of initialization issues.
216-216: ResetprepareProposalStateBefore UseResetting
s.prepareProposalStateensures a clean state for each proposal preparation, preventing unintended state carryover.
272-272: ResetfinalizeBlockStateAppropriatelyResetting
s.finalizeBlockStatewhen processing proposals after the initial height ensures that the state is accurate and prevents stale data usage.
315-315: InitializefinalizeBlockStateif NilInitializing
s.finalizeBlockStateif it'snilprevents potential nil pointer dereferences during finalization.
114-120: Verify Validator Set ConsistencyThe code checks that the length of the validators in the request matches the genesis validators, and compares their properties. Ensure that this logic correctly enforces validator consistency at genesis.
Run the following script to confirm validator consistency:
| return nil, fmt.Errorf( | ||
| "prepareProposal, height %v: %w", | ||
| req.Height, | ||
| errInvalidHeight, | ||
| ) |
There was a problem hiding this comment.
Correct Error Formatting in PrepareProposal
When using fmt.Errorf with %w for error wrapping, %w should not be combined with other verbs. Modify the error message to ensure proper error unwrapping.
Suggested Change:
-return nil, fmt.Errorf(
- "prepareProposal, height %v: %w",
- req.Height,
- errInvalidHeight,
-)
+return nil, fmt.Errorf(
+ "prepareProposal at height %v: %w",
+ req.Height,
+ errInvalidHeight,
+)📝 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.
| return nil, fmt.Errorf( | |
| "prepareProposal, height %v: %w", | |
| req.Height, | |
| errInvalidHeight, | |
| ) | |
| return nil, fmt.Errorf( | |
| "prepareProposal at height %v: %w", | |
| req.Height, | |
| errInvalidHeight, | |
| ) |
| return nil, fmt.Errorf( | ||
| "processProposal, height %v: %w", | ||
| req.Height, | ||
| errInvalidHeight, | ||
| ) |
There was a problem hiding this comment.
Correct Error Formatting in ProcessProposal
As in PrepareProposal, ensure that error messages in ProcessProposal use %w correctly for wrapping errors.
Suggested Change:
-return nil, fmt.Errorf(
- "processProposal, height %v: %w",
- req.Height,
- errInvalidHeight,
-)
+return nil, fmt.Errorf(
+ "processProposal at height %v: %w",
+ req.Height,
+ errInvalidHeight,
+)📝 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.
| return nil, fmt.Errorf( | |
| "processProposal, height %v: %w", | |
| req.Height, | |
| errInvalidHeight, | |
| ) | |
| return nil, fmt.Errorf( | |
| "processProposal at height %v: %w", | |
| req.Height, | |
| errInvalidHeight, | |
| ) |
| @@ -449,7 +444,7 @@ | |||
| context.Context, *cmtabci.CommitRequest, | |||
| ) (*cmtabci.CommitResponse, error) { | |||
| if s.finalizeBlockState == nil { | |||
| return nil, errors.New("finalizeBlockState is nil") | |||
| return nil, fmt.Errorf("commit: %w", errNilFinalizeBlockState) | |||
There was a problem hiding this comment.
Improve Error Message in Commit
Provide a more descriptive error message when s.finalizeBlockState is nil during commit to aid in debugging.
Suggested Change:
-return nil, fmt.Errorf("commit: %w", errNilFinalizeBlockState)
+return nil, fmt.Errorf("commit failed: %w", errNilFinalizeBlockState)📝 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.
| return nil, fmt.Errorf("commit: %w", errNilFinalizeBlockState) | |
| return nil, fmt.Errorf("commit failed: %w", errNilFinalizeBlockState) |
| return fmt.Errorf( | ||
| "finalizeBlock, height %v: %w", | ||
| req.Height, | ||
| errInvalidHeight, | ||
| ) |
There was a problem hiding this comment.
Correct Error Formatting in validateFinalizeBlockHeight
Ensure proper error wrapping by adjusting the use of %w in fmt.Errorf.
Suggested Change:
-return fmt.Errorf(
- "finalizeBlock, height %v: %w",
- req.Height,
- errInvalidHeight,
-)
+return fmt.Errorf(
+ "finalizeBlock at height %v: %w",
+ req.Height,
+ errInvalidHeight,
+)📝 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.
| return fmt.Errorf( | |
| "finalizeBlock, height %v: %w", | |
| req.Height, | |
| errInvalidHeight, | |
| ) | |
| return fmt.Errorf( | |
| "finalizeBlock at height %v: %w", | |
| req.Height, | |
| errInvalidHeight, | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- mod/consensus/pkg/cometbft/service/abci.go (10 hunks)
- mod/consensus/pkg/cometbft/service/service.go (2 hunks)
🔇 Additional comments (13)
mod/consensus/pkg/cometbft/service/abci.go (11)
46-50: Good practice: Consistent error handling with defined error variablesDefining error variables like
errInvalidHeightanderrNilFinalizeBlockStateenhances consistency and maintainability in error handling across the codebase.
89-89: InitializefinalizeBlockStateinInitChainResetting
s.finalizeBlockStateat the beginning ofInitChainensures that the state is properly initialized before processing the genesis block, preventing any residual state from affecting the initialization process.
97-101: Set initial block header after genesis handlingInitializing
initHeaderwith the correctChainID,Time, andHeightensures that the application context reflects the true block height after genesis. This is crucial for the correct operation of subsequent blocks.
107-110: SimplifyinitChainerparameters for clarityBy updating
initChainerto accept onlyappStateBytes, the function signature is simplified, making it clearer and more focused on its purpose. This improves readability and reduces unnecessary parameter passing.
147-147: UseConsensusParamsfrom the requestReturning
ConsensusParams: req.ConsensusParamsensures that the consensus parameters are correctly initialized in the response, matching the parameters provided in theInitChainRequest.
159-159: UnmarshalappStateBytesintogenesisStateProperly unmarshalling
appStateBytesintogenesisStateensures that the genesis state is correctly initialized from the application state bytes provided in the request.
170-170: Convert validator updates tocmtabci.ValidatorUpdateUsing
iter.MapErrto convert validator updates ensures compatibility with the expected types in the response, facilitating correct communication with the consensus engine.
214-214: ResetprepareProposalStateinPrepareProposalResetting
s.prepareProposalStateensures a clean state for each proposal preparation, preventing potential state leakage between proposals. This is important for maintaining the integrity of each individual proposal.
265-267: Document the rationale for state resettingThe added comments clearly explain the need to reset
finalizeBlockStateto prevent issues in subsequent rounds ofProcessProposal. This documentation improves code maintainability by providing context for future developers.
268-270: Reset proposal and finalize block states appropriatelyEnsuring that both
processProposalStateandfinalizeBlockStateare reset maintains the integrity of the state machine between proposals, preventing unintended side effects from previous states.
313-313: InitializefinalizeBlockStateif nil ininternalFinalizeBlockChecking if
s.finalizeBlockStateisniland resetting it if necessary ensures that the block finalization process has a valid state to work with, preventing potential runtime errors.mod/consensus/pkg/cometbft/service/service.go (2)
212-218: LGTMThe
resetStatefunction correctly initializes a new state and enhances clarity in state management.
68-68:⚠️ Potential issueVerify initialization of
initialHeight.Ensure that
initialHeightis properly initialized before it is used in the codebase to prevent potential runtime errors.Run the following script to check where
initialHeightis assigned a value:✅ Verification successful
Initialization of
initialHeightVerified Successfully.
initialHeightis correctly initialized usingreq.InitialHeightand defaults to1whenreq.InitialHeightis0. All usages occur after these assignments, ensuring no potential runtime errors related to its initialization.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find assignments to `initialHeight` in the codebase. # Test: Search for assignments to `initialHeight`. Expect: Should find at least one assignment. rg --type go --pcre2 'initialHeight\s*='Length of output: 257
Script:
#!/bin/bash # Description: Find all usages of `initialHeight` to ensure it's properly initialized before use. # Search for usages of `initialHeight` that are not assignments. rg --type go --pcre2 'initialHeight(?!\s*=)'Length of output: 627
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/consensus/pkg/cometbft/service/abci.go (10 hunks)
🔇 Additional comments (17)
mod/consensus/pkg/cometbft/service/abci.go (17)
46-49: Define standardized error variablesThe introduction of
errInvalidHeightanderrNilFinalizeBlockStateensures consistent error handling throughout the service.
76-76: EnsureinitialHeightdefaults to 1 when unsetSetting
s.initialHeightto 1 when it's zero ensures that the block height starts correctly.
89-89: InitializefinalizeBlockStateappropriatelyResetting
finalizeBlockStatehere prepares the state for initialization.
98-101: InitializeinitHeaderwith correct valuesThe
initHeaderis correctly initialized withChainID,Time, andInitialHeight.
107-110: UpdateinitChainerinvocation with new parametersPassing
appStateBytessimplifies the function call and aligns with the updatedinitChainersignature.
147-148: SetConsensusParamsfrom the requestBy assigning
ConsensusParamsfrom the request, the initial consensus parameters are correctly established.
156-157: SimplifyinitChainerfunction signatureUpdating
initChainerto acceptappStateBytesreduces dependencies and improves modularity.
159-159: Handle JSON unmarshalling errorsProperly handling errors during unmarshalling ensures robustness in processing the genesis state.
170-170: Convert validator updates effectivelyUsing
iter.MapErrfor transforming validator updates enhances code readability and efficiency.
207-211: Correct error wrapping inPrepareProposalThe error message correctly uses
%wto wraperrInvalidHeight, ensuring proper error propagation.
214-214: ResetprepareProposalStateappropriatelyResetting the state prevents residual data from affecting proposal preparation.
256-260: Correct error wrapping inProcessProposalConsistent error formatting with
%wensures errors are wrapped properly forProcessProposal.
265-270: Reset states to avoid stale dataResetting
processProposalStateandfinalizeBlockStateappropriately prevents state contamination between blocks.
313-313: InitializefinalizeBlockStateif nilEnsuring
finalizeBlockStateis reset when nil maintains state integrity during block finalization.
386-390: Proper error wrapping invalidateFinalizeBlockHeightUsing
%wcorrectly wrapserrInvalidHeight, aiding in error unwrapping and handling.
445-445: Return error whenfinalizeBlockStateis nil inCommitReturning an error instead of proceeding prevents potential nil pointer dereferences.
480-480:⚠️ Potential issueAvoid panicking; handle nil
finalizeBlockStategracefullyPanicking can crash the application unexpectedly. Consider logging the error and returning nil to handle this case more gracefully.
Suggested Change:
if s.finalizeBlockState == nil { - panic(fmt.Errorf("workingHash: %w", errNilFinalizeBlockState)) + s.logger.Error("workingHash failed: finalizeBlockState is nil") + return nil }Likely invalid or redundant comment.
abi87
force-pushed
the
cometBFT-some-more-cleanup-2
branch
from
bfe5ffc
to
197ddbf
Compare
abi87
force-pushed
the
cometBFT-some-more-cleanup-2
branch
from
197ddbf
to
57e8eab
Compare
| // First check for an abort signal after beginBlock, as it's the first place | ||
| // we spend any significant amount of time. | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| default: | ||
| // continue |
There was a problem hiding this comment.
useful in cosmos sdk baseApp but not here since we don't call delivarTx. Same applies below
| @@ -447,7 +417,7 @@ func (s *Service[LoggerT]) Commit( | |||
| context.Context, *cmtabci.CommitRequest, | |||
| ) (*cmtabci.CommitResponse, error) { | |||
| if s.finalizeBlockState == nil { | |||
| return nil, errors.New("finalizeBlockState is nil") | |||
| panic(fmt.Errorf("commit: %w", errNilFinalizeBlockState)) | |||
There was a problem hiding this comment.
CometBFT calls ordering guarantees that this cannot happen. CosmosSDK baseApp, which is our model here, does not do this check, but we need to appease nilaway.
Turning this into a panic, as we do with workingHash, since I believe it's a similar situation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (2)
mod/consensus/pkg/cometbft/service/service.go (1)
Line range hint
228-247: Consider creating an issue for the TODO comment.The
convertValidatorUpdatefunction seems to work correctly, but the TODO comment suggests that there's room for improvement in terms of type conversion and abstraction.Would you like me to create a GitHub issue to track the potential improvement mentioned in the TODO comment? This would help ensure that the suggestion to simplify the type conversion process (possibly using a codec or a passed-in function) is not forgotten and can be addressed in future refactoring efforts.
mod/consensus/pkg/cometbft/service/abci.go (1)
Line range hint
156-167: Check for the existence of the "beacon" key ingenesisStateIn the
initChainerfunction, you are accessinggenesisState["beacon"]without verifying that the "beacon" key exists. This could lead to a panic or a runtime error if the "beacon" key is missing from the genesis state.Consider adding a check to ensure that the "beacon" key is present before using it. Here's a possible modification:
if err := json.Unmarshal(appStateBytes, &genesisState); err != nil { return nil, err } +beaconState, ok := genesisState["beacon"] +if !ok { + return nil, errors.New("missing 'beacon' key in genesisState") +} valUpdates, err := s.Middleware.InitGenesis( ctx, - []byte(genesisState["beacon"]), + beaconState, )This change enhances the robustness of your code by preventing potential nil pointer dereferences or panics when the "beacon" key is absent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- mod/consensus/pkg/cometbft/service/abci.go (12 hunks)
- mod/consensus/pkg/cometbft/service/service.go (2 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/consensus/pkg/cometbft/service/abci.go (1)
Learnt from: abi87 PR: berachain/beacon-kit#2026 File: mod/consensus/pkg/cometbft/service/abci.go:114-118 Timestamp: 2024-09-30T21:58:05.156Z Learning: The team prefers to address ensuring a non-empty validator set at genesis in future PRs.
| // prepareProposalState is used for PrepareProposal, which is set based on the | ||
| // previous block's state. This state is never committed. In case of multiple | ||
| // consensus rounds, the state is always reset to the previous block's state. | ||
| prepareProposalState *state | ||
|
|
||
| // processProposalState is used for ProcessProposal, which is set based on the | ||
| // previous block's state. This state is never committed. In case of multiple | ||
| // consensus rounds, the state is always reset to the previous block's state. | ||
| processProposalState *state | ||
| finalizeBlockState *state | ||
| interBlockCache storetypes.MultiStorePersistentCache | ||
| paramStore *params.ConsensusParamsStore | ||
| initialHeight int64 | ||
| minRetainBlocks uint64 | ||
|
|
||
| // finalizeBlockState is used for FinalizeBlock, which is set based on the | ||
| // previous block's state. This state is committed. finalizeBlockState is set | ||
| // on InitChain and FinalizeBlock and set to nil on Commit. | ||
| finalizeBlockState *state | ||
|
|
||
| interBlockCache storetypes.MultiStorePersistentCache | ||
| paramStore *params.ConsensusParamsStore | ||
|
|
||
| // initialHeight is the initial height at which we start the node | ||
| initialHeight int64 | ||
| minRetainBlocks uint64 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider minor comment improvements.
The addition of separate state fields for different stages of block processing (prepare proposal, process proposal, and finalize block) is a good design choice. It clearly separates concerns and should make the code easier to reason about and maintain.
The comments explaining the purpose and lifecycle of each state are helpful. However, consider adding a brief explanation for the initialHeight and minRetainBlocks fields to maintain consistency in documentation.
Consider adding comments for the new fields:
// initialHeight is the initial block height at which the node starts
initialHeight int64
// minRetainBlocks is the minimum number of recent blocks to retain
minRetainBlocks uint64| // resetState provides a fresh state which can be used to reset | ||
| // prepareProposal/processProposal/finalizeBlock State. | ||
| // A state is explicitly returned to avoid false positives from | ||
| // nilaway tool. | ||
| func (s *Service[LoggerT]) resetState() *state { | ||
| ms := s.sm.CommitMultiStore().CacheMultiStore() | ||
| baseState := &state{ | ||
| return &state{ | ||
| ms: ms, | ||
| ctx: sdk.NewContext(ms, false, servercmtlog.WrapSDKLogger(s.logger)), | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider clarifying the comment about static analysis.
The resetState method is a good addition that simplifies state management for different stages of block processing. It provides a consistent way to create a fresh state with a cached multi-store and a new context.
The comment explaining the purpose of explicitly returning the state is helpful. However, it could be more specific about which static analysis tool it's referring to.
Consider updating the comment to be more specific:
// resetState provides a fresh state which can be used to reset
// prepareProposal/processProposal/finalizeBlock State.
// A state is explicitly returned to avoid false positives from
// static analysis tools that check for nil references (e.g., nilaway).
abi87
changed the title
initChainer + nits
A group of changes, listed by relevance (top to bottom):
ConsensusParamsinInitChainResponsefinalizeBlockStatesetStateto appeasenilawayctx.Donesince we don't calldeliveryTxSummary by CodeRabbit
Summary by CodeRabbit
New Features
resetStatefunction.Bug Fixes
Documentation