chore(forks): Handle MaxValidatorsPerWithdrawalsSweep based on planned fork upgrade #2194
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)mod/state-transition/pkg/core/state_processor_withdrawals.go (2)
In lines 194-195, Apply this diff to fix the issue: sp.cs.MaxValidatorsPerWithdrawalsSweep(
- state.IsPostUpgrade, sp.cs.DepositEth1ChainID(), slot,
+ state.IsPostUpgrade(sp.cs.DepositEth1ChainID(), slot),
+ sp.cs.DepositEth1ChainID(), slot,
))
Similarly, in lines 271-272, Apply this diff to fix the issue: sp.cs.MaxValidatorsPerWithdrawalsSweep(
- state.IsPostUpgrade, sp.cs.DepositEth1ChainID(), slot,
+ state.IsPostUpgrade(sp.cs.DepositEth1ChainID(), slot),
+ sp.cs.DepositEth1ChainID(), slot,
))🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2194 +/- ##
==========================================
+ Coverage 27.49% 27.61% +0.12%
==========================================
Files 365 366 +1
Lines 16812 16867 +55
Branches 12 13 +1
==========================================
+ Hits 4622 4658 +36
- Misses 11881 11900 +19
Partials 309 309
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range comments (1)
mod/config/pkg/spec/base.go (1)
Line range hint
1-136: Consider documenting the upgrade strategyThe introduction of pre/post upgrade distinctions in the spec suggests an upgrade mechanism. Consider adding documentation in the package comments explaining:
- The upgrade strategy and its implications
- How pre/post upgrade values are determined and used
- The relationship between fork epochs and upgrade-specific values
This will help maintainers understand the upgrade process and its impact on the system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
mod/chain-spec/pkg/chain/chain_spec.go(2 hunks)mod/chain-spec/pkg/chain/data.go(2 hunks)mod/config/pkg/spec/base.go(3 hunks)mod/config/pkg/spec/boonet.go(1 hunks)mod/state-transition/pkg/core/state/statedb.go(1 hunks)mod/state-transition/pkg/core/state/upgrade.go(1 hunks)mod/state-transition/pkg/core/state_processor_withdrawals.go(7 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
mod/chain-spec/pkg/chain/chain_spec.go
[warning] 498-498: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block
(revive)
mod/chain-spec/pkg/chain/data.go
132-132: File is not goimports-ed
(goimports)
🔇 Additional comments (16)
mod/state-transition/pkg/core/state/upgrade.go (1)
31-44: Verify chain ID constants and fork height
Let's verify the chain ID constants and fork height values are correctly defined and consistent across the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Chain ID constants and fork heights are correctly defined and consistently used
The verification shows that:
BartioChainIDandBoonetEth1ChainIDare properly defined in the spec package, withBoonetEth1ChainID = 80000BoonetFork2Heightis defined as99999999999999inmod/config/pkg/spec/special_cases.go- The constants are consistently used across the codebase for fork logic, particularly in:
- State transitions
- Deposit validations
- Withdrawal processing
- Block building
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify chain ID constants and fork height definitions
# Check for chain ID constant definitions
echo "Checking chain ID definitions..."
rg -A 2 "BartioChainID|BoonetEth1ChainID"
# Check for fork height constant definition and usage
echo "Checking fork height definition and usage..."
rg -A 2 "BoonetFork2Height"
# Check for other potential fork version checks
echo "Checking for other fork version checks..."
ast-grep --pattern 'if $_ < $_Fork$_Height'
Length of output: 9726
mod/config/pkg/spec/boonet.go (4)
37-37: LGTM! Variable renaming improves clarity.
The renaming from testnetSpec to boonetSpec better reflects the context and purpose of this configuration.
52-57: 🧹 Nitpick (assertive)
Address TODO and verify the upgrade-related property rename.
Two important points need attention:
- The TODO indicates this value needs to be determined for the boonet upgrade.
- The property has been renamed to include "PostUpgrade", suggesting a distinction between pre and post upgrade behavior.
Would you like me to help create a GitHub issue to track the determination of this value?
✅ Verification successful
The pre/post upgrade property distinction is well-defined, but the post-upgrade value needs determination
The codebase shows a clear and intentional distinction between pre and post-upgrade behavior:
- Pre-upgrade: Value is 2^14 (16384), consistent with ETH2.0 spec
- Post-upgrade: Value is intentionally set to 43 as the largest prime number below the minimum validator set size (46)
The TODO can be addressed as the post-upgrade value's rationale is already documented in both the code comment and the property documentation. The value of 43 appears to be a deliberate choice rather than a placeholder.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related pre/post upgrade properties and their usage
rg "MaxValidatorsPerWithdrawalsSweep(Pre|Post)Upgrade"
Length of output: 950
Script:
#!/bin/bash
# Let's check the documentation and usage context for these properties
rg -A 3 "MaxValidatorsPerWithdrawalsSweepPreUpgrade specifies" mod/chain-spec/pkg/chain/data.go
rg -A 3 "MaxValidatorsPerWithdrawalsSweepPostUpgrade specifies" mod/chain-spec/pkg/chain/data.go
# Let's also check where and how these values are used
ast-grep --pattern 'MaxValidatorsPerWithdrawalsSweep$_'
Length of output: 839
40-40: Verify the Chain ID constant value.
The comment indicates Chain ID should be 80000. Let's verify the constant is correctly defined.
✅ Verification successful
Chain ID constant is correctly defined as 80000
The constant BoonetEth1ChainID is correctly defined in mod/config/pkg/spec/chain_ids.go with the expected value of 80000, and it's properly used in the boonet configuration file. The comment in the code also accurately reflects this value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BoonetEth1ChainID constant definition
rg -A 1 "BoonetEth1ChainID\s*=\s*"
Length of output: 36
Script:
#!/bin/bash
# Let's try a broader search for the constant name without the assignment operator
rg "BoonetEth1ChainID" --type go
Length of output: 1365
Script:
#!/bin/bash
# Let's also check for any chain ID related constants in the config package
fd . -e go -E "vendor/*" --exec grep -l "ChainID" {}
Length of output: 1380
Script:
#!/bin/bash
# Let's look at the content of the file containing our target line
cat mod/config/pkg/spec/boonet.go
Length of output: 2134
43-45:
Verify the EVM inflation address.
This address is critical for token minting. Please ensure this is the correct production address for the Boonet environment.
mod/config/pkg/spec/base.go (3)
58-62: LGTM! Improved readability with scientific notation
The Gwei value constants are correctly defined using scientific notation, making the values more readable while maintaining the standard Ethereum specifications.
133-133: LGTM! Proper consensus parameters assignment
The CometValues assignment is correct and properly initialized with the default consensus parameters.
123-125: Verify the post-upgrade withdrawal sweep configuration
The renaming to MaxValidatorsPerWithdrawalsSweepPreUpgrade suggests there should be a corresponding post-upgrade value. Please ensure this is properly configured elsewhere in the codebase.
✅ Verification successful
Pre/post upgrade withdrawal sweep configuration is properly implemented
The codebase has a complete implementation for handling both pre and post-upgrade withdrawal sweep configurations:
- The
MaxValidatorsPerWithdrawalsSweepmethod in the chain spec properly handles both cases using anisPostUpgradefunction - Pre-upgrade value is set to 1 << 14 (16384)
- Post-upgrade value is configurable per network (e.g., set to 43 in boonet)
- The implementation includes proper documentation and type safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the post-upgrade configuration
echo "Searching for MaxValidatorsPerWithdrawalsSweepPostUpgrade configuration..."
rg "MaxValidatorsPerWithdrawalsSweepPostUpgrade" -A 2
echo "Checking for related interface methods..."
ast-grep --pattern 'interface Spec {
$$$
MaxValidatorsPerWithdrawalsSweep($$$) $$$
$$$
}'
Length of output: 1414
Script:
#!/bin/bash
# Let's verify the interface and implementation details
echo "Checking for the interface definition..."
ast-grep --pattern 'interface Spec {
$$$
MaxValidatorsPerWithdrawalsSweep($$$) $$$
}'
echo "\nChecking implementation of MaxValidatorsPerWithdrawalsSweep..."
rg "func.*MaxValidatorsPerWithdrawalsSweep" -A 3
echo "\nChecking for upgrade-related conditions..."
rg "IsWithdrawalsSweepUpgradeActive|WithdrawalsSweepUpgradeHeight" -A 2
Length of output: 545
Script:
#!/bin/bash
# Let's check the chain spec implementation more broadly
echo "Checking chain spec implementation..."
rg "MaxValidatorsPerWithdrawalsSweep" -B 2 -A 2 mod/chain-spec/pkg/chain/chain_spec.go
echo "\nChecking for upgrade height configuration..."
rg "WithdrawalsSweepUpgrade" mod/chain-spec/pkg/chain/data.go
echo "\nChecking for usage of withdrawal sweep values..."
rg "MaxValidatorsPerWithdrawalsSweep" mod/config/pkg/spec/base.go -B 2 -A 2
Length of output: 1384
mod/chain-spec/pkg/chain/data.go (2)
136-140: 🧹 Nitpick (assertive)
Documentation needs clarification on post-upgrade value determination
The documentation for MaxValidatorsPerWithdrawalsSweepPostUpgrade needs more detail:
- What determines the "minimum possible amount of total validators"?
- Why is a prime number specifically chosen?
- How is this value calculated or determined?
This information is crucial for understanding the system's behavior post-upgrade.
132-140: 🧹 Nitpick (assertive)
Verify upgrade transition handling
The split of MaxValidatorsPerWithdrawalsSweep into pre/post upgrade variants suggests a significant change in withdrawal processing. Please ensure:
- The transition between pre and post upgrade values is handled gracefully
- There are no edge cases during the upgrade transition
- The change in withdrawal sweep size doesn't impact system stability
✅ Verification successful
Let me gather more information about the withdrawal processing logic and upgrade handling.
Let me gather more specific information about the upgrade transition handling.
Based on the gathered information, I can now provide a final response about the withdrawal sweep transition handling.
Transition handling is properly implemented with clear upgrade boundaries
The upgrade transition for withdrawal sweep sizes is handled safely through:
- A clear upgrade boundary check via
IsPostUpgrade()that uses a well-defined height (BoonetFork2Height) - A clean switch between pre/post values in
MaxValidatorsPerWithdrawalsSweep()based on the upgrade status - Conservative default values:
- Pre-upgrade: ETH2.0 spec compatible value (2^14)
- Post-upgrade: Smaller prime number (43) based on minimum validator set size
The implementation avoids edge cases by:
- Using a high upgrade height (99999999999999) as a safety barrier
- Having a clear chain ID-based branching logic
- Maintaining backward compatibility with pre-upgrade values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for upgrade transition handling code
rg -A 10 "IsPostUpgrade|UpgradeStatus"
# Search for withdrawal processing logic that might be affected
ast-grep --pattern 'func $_($$) {
$$$
withdrawal
$$$
}'
Length of output: 3505
Script:
#!/bin/bash
# Search for withdrawal sweep size usage
rg -A 5 "MaxValidatorsPerWithdrawalsSweep"
# Look for withdrawal processing functions
rg -A 10 "func.*[Ww]ithdrawal.*"
# Search for upgrade-related configuration
rg -A 5 "BoonetFork2Height|Fork2"
Length of output: 118081
Script:
#!/bin/bash
# Look for MaxValidatorsPerWithdrawalsSweep usage in state processor
rg -A 10 "MaxValidatorsPerWithdrawalsSweep.*IsPostUpgrade"
# Check for upgrade-related state transitions
rg -A 10 "func.*IsPostUpgrade"
# Look for upgrade height configuration
rg -A 5 "BoonetFork2Height.*="
Length of output: 1001
🧰 Tools
🪛 golangci-lint (1.62.2)
132-132: File is not goimports-ed
(goimports)
mod/state-transition/pkg/core/state_processor_withdrawals.go (5)
29-29: Importing the 'state' package for upgrade checks
The addition of the import "github.com/berachain/beacon-kit/mod/state-transition/pkg/core/state" is appropriate as it allows access to state.IsPostUpgrade needed for upgrade-related logic.
97-97: Passing 'slot' parameter enhances chain-specific logic
Including the slot parameter in the call to processWithdrawalsBartio enables the function to apply chain-specific logic based on the current slot, which aligns with the PR objectives.
121-121: Passing 'slot' parameter to 'processWithdrawalsBartio'
Adding slot to the function call ensures consistency and allows for proper handling of withdrawals before BoonetFork2Height on the Boonet chain.
129-129: Passing 'slot' parameter to 'processWithdrawalsDefault'
Including slot in the call to processWithdrawalsDefault provides necessary context for processing withdrawals in the default case, supporting the new upgrade logic.
141-141: Adding 'slot' parameter to 'processWithdrawalsBartio' function signature
The slot parameter is correctly added to the function signature, allowing internal logic to utilize the current slot information.
mod/chain-spec/pkg/chain/chain_spec.go (1)
158-162: Ensure all implementations and usages of MaxValidatorsPerWithdrawalsSweep are updated
Changing the method signature of a public interface can have widespread impact. Please verify that all implementations and calls to MaxValidatorsPerWithdrawalsSweep are updated to match the new signature.
Run the following script to verify all declarations and usages of MaxValidatorsPerWithdrawalsSweep:
✅ Verification successful
Based on the search results, I can generate the final response as we have comprehensive information about both the interface definition and all its usages:
All implementations and usages of MaxValidatorsPerWithdrawalsSweep are correctly aligned with the new signature
The verification shows that:
- The interface definition in
chain_spec.gomatches the implementation in the same file - All three usage locations correctly provide the required parameters:
state_processor_withdrawals.go(2 occurrences)statedb.go(1 occurrence)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all implementations and usages of MaxValidatorsPerWithdrawalsSweep are updated to the new signature.
# Search for all method declarations and interface definitions
rg 'MaxValidatorsPerWithdrawalsSweep\(' -A 3 -B 3
# Search for all calls to MaxValidatorsPerWithdrawalsSweep
rg 'MaxValidatorsPerWithdrawalsSweep\(' --type go
Length of output: 3337
| sp.cs.MaxValidatorsPerWithdrawalsSweep( | ||
| state.IsPostUpgrade, spec.BartioChainID, slot, | ||
| )) |
There was a problem hiding this comment.
Call 'state.IsPostUpgrade' function instead of passing it as a reference
In lines 194-195, state.IsPostUpgrade is passed as a function reference to MaxValidatorsPerWithdrawalsSweep, but it should be called with the appropriate parameters to return a boolean value. This could lead to a logical error or compile-time issue.
Apply this diff to fix the issue:
nextValidatorIndex += math.ValidatorIndex(
- sp.cs.MaxValidatorsPerWithdrawalsSweep(
- state.IsPostUpgrade, spec.BartioChainID, slot,
+ sp.cs.MaxValidatorsPerWithdrawalsSweep(
+ state.IsPostUpgrade(spec.BartioChainID, slot),
+ spec.BartioChainID, slot,
))This ensures that state.IsPostUpgrade is invoked properly with spec.BartioChainID and slot as arguments, returning the expected boolean value needed by MaxValidatorsPerWithdrawalsSweep.
📝 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.
| sp.cs.MaxValidatorsPerWithdrawalsSweep( | |
| state.IsPostUpgrade, spec.BartioChainID, slot, | |
| )) | |
| sp.cs.MaxValidatorsPerWithdrawalsSweep( | |
| state.IsPostUpgrade(spec.BartioChainID, slot), spec.BartioChainID, slot, | |
| )) |
| totalValidators, s.cs.MaxValidatorsPerWithdrawalsSweep( | ||
| IsPostUpgrade, s.cs.DepositEth1ChainID(), slot, | ||
| ), |
There was a problem hiding this comment.
Undefined variable IsPostUpgrade
The variable IsPostUpgrade is used as a parameter but is not defined within the current scope. This will result in a compilation error. Please define IsPostUpgrade before using it.
Apply this diff to define IsPostUpgrade:
slot, err := s.GetSlot()
if err != nil {
return nil, err
}
+// Determine if the chain is post-upgrade based on the current slot
+IsPostUpgrade := s.IsPostUpgrade(slot)
bound := min(
totalValidators, s.cs.MaxValidatorsPerWithdrawalsSweep(
IsPostUpgrade, s.cs.DepositEth1ChainID(), slot,
),
)Committable suggestion skipped: line range outside the PR's diff.
| if isPostUpgrade(chainID, slot) { | ||
| return c.Data.MaxValidatorsPerWithdrawalsSweepPostUpgrade | ||
| } else { | ||
| return c.Data.MaxValidatorsPerWithdrawalsSweepPreUpgrade | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify conditional logic by removing unnecessary else clause
Since the if block ends with a return statement, the else clause is unnecessary. Removing it improves code readability and aligns with Go's idiomatic practices.
Apply the following diff to simplify the code:
func (c chainSpec[
DomainTypeT, EpochT, ExecutionAddressT, SlotT, CometBFTConfigT,
]) MaxValidatorsPerWithdrawalsSweep(
isPostUpgrade func(uint64, SlotT) bool,
chainID uint64, slot SlotT,
) uint64 {
if isPostUpgrade(chainID, slot) {
return c.Data.MaxValidatorsPerWithdrawalsSweepPostUpgrade
- } else {
+ }
return c.Data.MaxValidatorsPerWithdrawalsSweepPreUpgrade
- }
}📝 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.
| if isPostUpgrade(chainID, slot) { | |
| return c.Data.MaxValidatorsPerWithdrawalsSweepPostUpgrade | |
| } else { | |
| return c.Data.MaxValidatorsPerWithdrawalsSweepPreUpgrade | |
| } | |
| if isPostUpgrade(chainID, slot) { | |
| return c.Data.MaxValidatorsPerWithdrawalsSweepPostUpgrade | |
| } | |
| return c.Data.MaxValidatorsPerWithdrawalsSweepPreUpgrade |
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 498-498: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block
(revive)
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/state-transition/pkg/core/state_processor_staking_test.go(1 hunks)mod/state-transition/pkg/core/state_processor_withdrawals.go(8 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/state-transition/pkg/core/state_processor_staking_test.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:154-168
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In the `TestTransitionUpdateValidators` function in `mod/state-transition/pkg/core/state_processor_staking_test.go`, adding additional assertions for balance relationships is not necessary.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:424-440
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In the `TestTransitionHittingValidatorsCap` function in `mod/state-transition/pkg/core/state_processor_staking_test.go`, adding additional assertions to verify validator set size after initialization is unnecessary.
🪛 golangci-lint (1.62.2)
mod/state-transition/pkg/core/state_processor_withdrawals.go
84-84: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
87-87: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🔇 Additional comments (8)
mod/state-transition/pkg/core/state_processor_withdrawals.go (7)
104-104: Addition of slot parameter enhances withdrawal processing
Including the slot parameter in the processWithdrawalsBartio call ensures that withdrawal logic accurately reflects the current chain state.
128-128: Passing slot parameter aligns with updated function signature
Updating the processWithdrawalsBartio call with the slot parameter maintains consistency with the function's revised signature.
136-136: Updated call to processWithdrawalsDefault is appropriate
Incorporating the slot parameter into the processWithdrawalsDefault call ensures proper handling based on the current slot.
148-148: Function signature update with slot parameter is correct
Adding slot math.Slot to processWithdrawalsBartio enhances the function's capability to process withdrawals accurately.
200-202: Call state.IsPostUpgrade function instead of passing it as a reference
This issue was previously identified. Ensure that state.IsPostUpgrade is called with the correct arguments to return a boolean value.
216-216: Addition of slot parameter to processWithdrawalsDefault is appropriate
Including the slot parameter allows for withdrawal logic that reflects the current chain conditions.
277-279: Call state.IsPostUpgrade function instead of passing it as a reference
This issue mirrors a previous concern. Ensure proper invocation of state.IsPostUpgrade with the necessary parameters.
mod/state-transition/pkg/core/state_processor_staking_test.go (1)
437-437: Test configuration updated correctly with MaxValidatorsPerWithdrawalsSweepPostUpgrade
Setting MaxValidatorsPerWithdrawalsSweepPostUpgrade to 2 ensures the test reflects the intended post-upgrade behavior.
| for i, wd := range expectedWithdrawals { | ||
| fmt.Printf("expectedWithdrawal %d: %s\n", i, spew.Sdump(wd)) | ||
| } | ||
| for i, wd := range payloadWithdrawals { | ||
| fmt.Printf("payloadWithdrawal %d: %s\n", i, spew.Sdump(wd)) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fmt.Printf with appropriate logging methods
The use of fmt.Printf is discouraged according to the project's coding guidelines and flagged by static analysis tools. Consider using the designated logging package for debug output.
Apply this diff to fix the issue:
- fmt.Printf("expectedWithdrawal %d: %s\n", i, spew.Sdump(wd))
+ logger.Debugf("expectedWithdrawal %d: %s", i, spew.Sdump(wd))Similarly, replace the other instance:
- fmt.Printf("payloadWithdrawal %d: %s\n", i, spew.Sdump(wd))
+ logger.Debugf("payloadWithdrawal %d: %s", i, spew.Sdump(wd))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
84-84: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
87-87: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (2)
mod/node-core/pkg/services/version/version.go (1)
Line range hint
201-203: Track the TODO comment for metric cleanup.The TODO comment indicates that this counter should be deleted as it's superseded by the
beacon_kit.runtime.versionmetric. This cleanup task should be tracked.Would you like me to create a GitHub issue to track the removal of this deprecated metric counter?
mod/chain-spec/pkg/chain/chain_spec.go (1)
Line range hint
315-316: Correct the typo in the method commentThere's a typo in the comment for the
DenebPlusForkEpochmethod. The comment saysDenebPlusForEpochinstead ofDenebPlusForkEpoch. Updating it will enhance readability and prevent confusion.Apply this change to fix the typo:
-// DenebPlusForEpoch returns the epoch of the Deneb+ fork. +// DenebPlusForkEpoch returns the epoch of the Deneb+ fork.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
mod/chain-spec/pkg/chain/chain_spec.go(2 hunks)mod/chain-spec/pkg/chain/data.go(2 hunks)mod/node-core/pkg/services/version/version.go(2 hunks)mod/state-transition/pkg/core/state_processor.go(1 hunks)mod/state-transition/pkg/core/state_processor_withdrawals.go(7 hunks)
🔇 Additional comments (6)
mod/node-core/pkg/services/version/version.go (2)
125-126: LGTM! Comment formatting improvement.
The split of the comment into two lines improves readability while maintaining the same meaning.
179-182: LGTM! Error formatting improvement.
The multi-line error formatting follows Go's best practices and improves readability.
mod/state-transition/pkg/core/state_processor_withdrawals.go (2)
193-195:
Call state.IsPostUpgrade instead of passing it as a function reference
In lines 194-195, state.IsPostUpgrade is passed as a function reference to sp.cs.MaxValidatorsPerWithdrawalsSweep, but it should be called with the appropriate parameters to return a boolean value. This could lead to logical errors or runtime issues.
Apply this diff to fix the issue:
nextValidatorIndex += math.ValidatorIndex(
sp.cs.MaxValidatorsPerWithdrawalsSweep(
- state.IsPostUpgrade, spec.BartioChainID, slot,
+ state.IsPostUpgrade(spec.BartioChainID, slot), spec.BartioChainID, slot,
))270-272:
Call state.IsPostUpgrade instead of passing it as a function reference
Similarly, in lines 271-272, state.IsPostUpgrade is passed as a reference instead of being called. This should be corrected to prevent potential logical errors.
Apply this diff to fix the issue:
nextValidatorIndex += math.ValidatorIndex(
sp.cs.MaxValidatorsPerWithdrawalsSweep(
- state.IsPostUpgrade, sp.cs.DepositEth1ChainID(), slot,
+ state.IsPostUpgrade(sp.cs.DepositEth1ChainID(), slot), sp.cs.DepositEth1ChainID(), slot,
))mod/state-transition/pkg/core/state_processor.go (1)
566-571: Improved readability of hysteresis threshold calculations
The refactoring of the hysteresis threshold calculations enhances code readability without altering functionality. The calculations are now more clearly structured.
mod/chain-spec/pkg/chain/chain_spec.go (1)
496-500: Implementation correctly handles upgrade logic
The MaxValidatorsPerWithdrawalsSweep method correctly returns the appropriate value based on the upgrade status determined by isPostUpgrade. The logic is clear and follows best practices.
| // MaxValidatorsPerWithdrawalsSweepPreUpgrade specifies the maximum number | ||
| // of validator withdrawals allowed per sweep. Before the upgrade, this | ||
| // value is consistent with ETH2.0 spec, set to 2^14. | ||
| MaxValidatorsPerWithdrawalsSweepPreUpgrade uint64 `mapstructure:"max-validators-per-withdrawals-sweep-pre-upgrade"` | ||
| // MaxValidatorsPerWithdrawalsSweepPostUpgrade specifies the maximum number | ||
| // of validator withdrawals allowed per sweep. After the upgrade, this value | ||
| // is set to the largest prime number smaller than the minimum possible | ||
| // amount of total validators. | ||
| MaxValidatorsPerWithdrawalsSweepPostUpgrade uint64 `mapstructure:"max-validators-per-withdrawals-sweep-post-upgrade"` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider simplifying field names for readability
The field names MaxValidatorsPerWithdrawalsSweepPreUpgrade and MaxValidatorsPerWithdrawalsSweepPostUpgrade are quite long and may impact code readability. Consider shortening them to improve clarity, for example:
MaxValidatorsPerSweepPreUpgradeMaxValidatorsPerSweepPostUpgrade
| MaxValidatorsPerWithdrawalsSweep( | ||
| isPostUpgrade func(uint64, SlotT) bool, | ||
| chainID uint64, | ||
| slot SlotT, | ||
| ) uint64 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Update method comment to reflect new parameters
The MaxValidatorsPerWithdrawalsSweep method now accepts isPostUpgrade, chainID, and slot as parameters, but the method comment has not been updated accordingly. To improve clarity and maintain accurate documentation, please update the comment to describe these parameters.
Apply this change to the comment:
// MaxValidatorsPerWithdrawalsSweep returns the maximum number of validators per
-// withdrawals sweep.
+// withdrawals sweep based on the upgrade status.
+//
+// Parameters:
+// - isPostUpgrade: function to determine if the chain is post-upgrade
+// - chainID: the chain identifier
+// - slot: the current slot📝 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.
| MaxValidatorsPerWithdrawalsSweep( | |
| isPostUpgrade func(uint64, SlotT) bool, | |
| chainID uint64, | |
| slot SlotT, | |
| ) uint64 | |
| // MaxValidatorsPerWithdrawalsSweep returns the maximum number of validators per | |
| // withdrawals sweep based on the upgrade status. | |
| // | |
| // Parameters: | |
| // - isPostUpgrade: function to determine if the chain is post-upgrade | |
| // - chainID: the chain identifier | |
| // - slot: the current slot | |
| MaxValidatorsPerWithdrawalsSweep( | |
| isPostUpgrade func(uint64, SlotT) bool, | |
| chainID uint64, | |
| slot SlotT, | |
| ) uint64 |
There was a problem hiding this comment.
Fork choice should not be binary (pre/post) but I guess:
- not much more we can do with current folder structure
- we plan to solve the issue more structurally as soon as fork activates.
So fine with me
There was a problem hiding this comment.
@abi87, understood. Thank you.
| @@ -45,10 +45,13 @@ type SpecData[ | |||
|
|
|||
| // HysteresisQuotient is the quotient used in effective balance calculations | |||
| HysteresisQuotient uint64 `mapstructure:"hysteresis-quotient"` | |||
| // HysteresisDownwardMultiplier is the multiplier for downward balance adjustments. | |||
| // HysteresisDownwardMultiplier is the multiplier for downward balance | |||
There was a problem hiding this comment.
nit: we need to sync our linters XD
mod/config/pkg/spec/base.go
Outdated
| MaxWithdrawalsPerPayload: 16, | ||
| MaxValidatorsPerWithdrawalsSweepPreUpgrade: 1 << 14, | ||
|
|
There was a problem hiding this comment.
Wait, are we changing this for every network?
I think we want 16 set in Boonet (post fork) and every other network but Bartio.
So I would keep base values to 1<<14 and fix them in their chain specs
| default: | ||
| return true |
There was a problem hiding this comment.
default behaviour equals Boonet post fork. This is the general principle we are following, right?
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/config/pkg/spec/base.go(3 hunks)
🔇 Additional comments (2)
mod/config/pkg/spec/base.go (2)
58-61: LGTM! Improved formatting for better readability.
The reformatting of Gwei value constants improves code readability while maintaining the same values.
106-109: Consider adding documentation for fork epoch values
The fork epoch values are set to effectively infinite values, which is a good practice for disabling forks until they're ready. Consider adding a comment explaining this intention.
+ // Fork epochs are set to effectively infinite values to disable them until ready
DenebPlusForkEpoch: 9999999999999998,
ElectraForkEpoch: 9999999999999999,| MaxWithdrawalsPerPayload: 16, | ||
| MaxValidatorsPerWithdrawalsSweepPreUpgrade: 1 << 14, | ||
| MaxValidatorsPerWithdrawalsSweepPostUpgrade: 1 << 14, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
💡 Codebase verification
Based on the gathered information, I can now generate the final response:
Network-specific withdrawal sweep limits need adjustment
The current implementation shows inconsistencies with the stated requirements:
- Only Boonet correctly overrides
MaxValidatorsPerWithdrawalsSweepPostUpgradeto 43 with a documented rationale - Other networks (Betnet, Testnet, Devnet) inherit the base value of
1 << 14(16384), which contradicts the requirement that they should use 16 - No network overrides the pre-upgrade value, all inheriting
1 << 14from base spec
Required changes:
- Move the default values from base spec to network-specific implementations
- Set post-upgrade value to 16 for Betnet, Testnet, and Devnet
- Review and adjust pre-upgrade values for each network if they differ from ETH2.0 spec
🔗 Analysis chain
Verify network-specific withdrawal sweep limits
The values for both MaxValidatorsPerWithdrawalsSweepPreUpgrade and MaxValidatorsPerWithdrawalsSweepPostUpgrade are set to 1 << 14 in the base spec. However, based on previous discussions, these values should potentially vary by network:
- Boonet should use 16 post-fork
- Other networks except Bartio should use 16
- Bartio might need different values
Consider moving these default values to their respective network-specific chain specs instead of setting them in the base spec.
Run the following script to verify the current implementation across different networks:
Let me gather more information about the network-specific configurations by checking the content of the network spec files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check withdrawal sweep configurations across different network specs
# Search for MaxValidatorsPerWithdrawalsSweep configurations in chain specs
echo "Searching for withdrawal sweep configurations across network specs..."
rg -A 2 "MaxValidatorsPerWithdrawalsSweep(Pre|Post)Upgrade"
# Look for network-specific chain spec files
echo -e "\nListing network-specific chain spec files..."
fd -e go . | grep -i "spec.*\.go" | grep -iv "base\.go"
Length of output: 2965
Script:
#!/bin/bash
# Check network-specific configurations
echo "Checking boonet configuration..."
cat mod/config/pkg/spec/boonet.go
echo -e "\nChecking betnet configuration..."
cat mod/config/pkg/spec/betnet.go
echo -e "\nChecking testnet configuration..."
cat mod/config/pkg/spec/testnet.go
echo -e "\nChecking devnet configuration..."
cat mod/config/pkg/spec/devnet.go
Length of output: 7520
There was a problem hiding this comment.
not needed yet. We will revisit once we clean chain specs structurally
There was a problem hiding this comment.
@abi87, understood. We'll revisit this after the chain specs are structurally cleaned.
(_/)
(╥﹏╥)
/ \
✏️ Learnings added
Learnt from: abi87
PR: berachain/beacon-kit#2194
File: mod/config/pkg/spec/base.go:123-125
Timestamp: 2024-12-02T16:30:06.768Z
Learning: Adjustments to network-specific withdrawal sweep limits will be revisited after the chain specs are structurally cleaned.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation