chore(store): Disallow removing from validator registry to return all correctly #1805
Conversation
WalkthroughThe changes improve the Changes
Poem
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: 3
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- mod/storage/pkg/beacondb/registry.go (2 hunks)
Additional comments not posted (2)
mod/storage/pkg/beacondb/registry.go (2)
167-170: Consider edge cases for filling empty values.The loop fills in empty values to maintain the order of the registry. Ensure that
keyval.Keyis always greater than or equal toindexto avoid potential panics.- for j := index; j < keyval.Key; j++ { + if keyval.Key >= index { + for j := index; j < keyval.Key; j++ { + vals[j] = emptyVal + } + }
172-176: Ensure that the index is within bounds.The index is incremented and used to access the
valsslice. Ensure that the index does not exceed the slice bounds to avoid potential panics.- vals[keyval.Key] = keyval.Value + if keyval.Key < uint64(len(vals)) { + vals[keyval.Key] = keyval.Value + } else { + return nil, fmt.Errorf("index out of bounds: %d", keyval.Key) + }
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1805 +/- ##
==========================================
- Coverage 25.47% 25.46% -0.01%
==========================================
Files 329 329
Lines 14612 14616 +4
Branches 21 21
==========================================
Hits 3722 3722
- Misses 10760 10764 +4
Partials 130 130
|
calbera
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (4)
- mod/node-core/pkg/components/storage/types.go (1 hunks)
- mod/state-transition/pkg/core/interfaces.go (1 hunks)
- mod/state-transition/pkg/core/state/interfaces.go (1 hunks)
- mod/storage/pkg/beacondb/registry.go (3 hunks)
Additional comments not posted (32)
mod/node-core/pkg/components/storage/types.go (32)
Line range hint
63-63:
LGTM!The
Contextmethod is straightforward and does not require any changes.
Line range hint
65-67:
LGTM!The
WithContextmethod is straightforward and does not require any changes.
Line range hint
69-69:
LGTM!The
Savemethod is straightforward and does not require any changes.
Line range hint
71-71:
LGTM!The
Copymethod is straightforward and does not require any changes.
Line range hint
73-75:
LGTM!The
GetLatestExecutionPayloadHeadermethod is straightforward and does not require any changes.
Line range hint
77-79:
LGTM!The
SetLatestExecutionPayloadHeadermethod is straightforward and does not require any changes.
Line range hint
81-81:
LGTM!The
GetEth1DepositIndexmethod is straightforward and does not require any changes.
Line range hint
83-85:
LGTM!The
SetEth1DepositIndexmethod is straightforward and does not require any changes.
Line range hint
87-87:
LGTM!The
GetBalancemethod is straightforward and does not require any changes.
Line range hint
89-91:
LGTM!The
SetBalancemethod is straightforward and does not require any changes.
Line range hint
93-93:
LGTM!The
GetSlotmethod is straightforward and does not require any changes.
Line range hint
95-97:
LGTM!The
SetSlotmethod is straightforward and does not require any changes.
Line range hint
99-99:
LGTM!The
GetForkmethod is straightforward and does not require any changes.
Line range hint
101-103:
LGTM!The
SetForkmethod is straightforward and does not require any changes.
Line range hint
105-105:
LGTM!The
GetGenesisValidatorsRootmethod is straightforward and does not require any changes.
Line range hint
107-109:
LGTM!The
SetGenesisValidatorsRootmethod is straightforward and does not require any changes.
Line range hint
111-111:
LGTM!The
GetLatestBlockHeadermethod is straightforward and does not require any changes.
Line range hint
113-115:
LGTM!The
SetLatestBlockHeadermethod is straightforward and does not require any changes.
Line range hint
117-117:
LGTM!The
GetBlockRootAtIndexmethod is straightforward and does not require any changes.
Line range hint
119-121:
LGTM!The
StateRootAtIndexmethod is straightforward and does not require any changes.
Line range hint
123-123:
LGTM!The
GetEth1Datamethod is straightforward and does not require any changes.
Line range hint
125-127:
LGTM!The
SetEth1Datamethod is straightforward and does not require any changes.
Line range hint
129-129:
LGTM!The
GetValidatorsmethod is straightforward and does not require any changes.
Line range hint
131-131:
LGTM!The
GetBalancesmethod is straightforward and does not require any changes.
Line range hint
133-133:
LGTM!The
GetNextWithdrawalIndexmethod is straightforward and does not require any changes.
Line range hint
135-137:
LGTM!The
SetNextWithdrawalIndexmethod is straightforward and does not require any changes.
Line range hint
139-139:
LGTM!The
GetNextWithdrawalValidatorIndexmethod is straightforward and does not require any changes.
Line range hint
141-143:
LGTM!The
SetNextWithdrawalValidatorIndexmethod is straightforward and does not require any changes.
Line range hint
145-145:
LGTM!The
GetTotalSlashingmethod is straightforward and does not require any changes.
Line range hint
147-149:
LGTM!The
SetTotalSlashingmethod is straightforward and does not require any changes.
Line range hint
151-151:
LGTM!The
GetRandaoMixAtIndexmethod is straightforward and does not require any changes.
Line range hint
62-62:
Review the removal ofRemoveValidatorAtIndexmethod.The removal of
RemoveValidatorAtIndexfrom theKVStoreinterface suggests a shift in how validators are managed. Ensure that the functionality previously provided by this method is now handled elsewhere in the codebase.Verification successful
Removal of
RemoveValidatorAtIndexmethod successfully verified.The
RemoveValidatorAtIndexmethod has been completely removed from the codebase, and no lingering references were found. This confirms that the functionality previously provided by this method is now handled elsewhere or is no longer needed.
- No occurrences of
RemoveValidatorAtIndexwere found in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `RemoveValidatorAtIndex` in the codebase. # Test: Search for the function usage. Expect: No occurrences of the removed method. rg --type go -A 5 $'RemoveValidatorAtIndex'Length of output: 42
Script:
#!/bin/bash # Description: Verify the usage of `RemoveValidatorAtIndex` across the entire codebase. # Test: Search for any references to `RemoveValidatorAtIndex`. Expect: No occurrences of the removed method. rg 'RemoveValidatorAtIndex'Length of output: 27
Enforces the invariant that iterating in order over the validators indexed map for
GetValidatorsreturns a slice where the index of validator corresponds to validator'sValidatorIndexSummary by CodeRabbit
New Features
Bug Fixes
Refactor
RemoveValidatorAtIndexmethod, improving usability and reducing complexity.