Fix panic from missing batch in force-start-height#744
Conversation
peterargue
requested review from
janezpodhostnik,
m-Peter and
zhangchiqing
as code owners
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
![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
🔭 Outside diff range comments (1)
bootstrap/bootstrap.go (1)
Storage account owner verification needs implementation as per architectural pattern
The verification is indeed missing and should be implemented. This is confirmed by:
- A comment in
storage/pebble/register_storage.goindicating that owner checks are a caller responsibility- No existing implementation of storage account owner verification was found in the codebase
🔗 Analysis chain
Line range hint
619-621: Consider implementing the storage account owner verification.The TODO comment indicates a missing security check for verifying the storage account owner. This verification is important for ensuring the integrity of the storage system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing storage account owner verification logic rg -A 5 "verify.*storage.*account.*owner" # Search for storage account related security measures ast-grep --pattern 'func $_(storage $_, account $_) { $$$ verify$_($_, $_) $$$ }'Length of output: 417
Script:
#!/bin/bash # Search for storage account security patterns rg -i "storage.+account.+(check|validate|verify|auth)" -A 5 # Search for ownership validation patterns rg -i "(owner|ownership).+(check|validate|verify)" -A 5 # Look for storage account related functions ast-grep --pattern 'func $_($_) { $$$ storage$_account$_ $$$ }'Length of output: 1333
🧹 Nitpick comments (1)
bootstrap/bootstrap.go (1)
624-629: Improve error message specificity.While the batch commit logic is correct, the error message could be more informative by including the number of updates that failed to commit.
- return nil, nil, fmt.Errorf("could not commit setup updates: %w", err) + return nil, nil, fmt.Errorf("could not commit %d setup updates: %w", batch.Count(), err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bootstrap/bootstrap.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: Test
🔇 Additional comments (2)
bootstrap/bootstrap.go (2)
560-567: LGTM! Well-structured batch handling.The batch creation and cleanup logic is well-implemented with proper resource management using
defer. Fatal error logging on batch close failure is appropriate for the setup phase.
572-572: LGTM! Fixed panic by providing batch parameter.This change directly addresses the PR objective by providing the required batch parameter to
SetLatestCadenceHeight, preventing the panic condition.
3 participants
Closes: #???
Description
Fixes panic when running with
--force-start-heightthat was caused by a missingbatchin the call toSetLatestCadenceHeight.Also combines the db setup updates into a single batch to ensure the changes are only made if the setup is successful
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit