feature(state-processor): update validators EffectiveBalance only when epoch turns
#2142
Conversation
WalkthroughThe changes in this pull request introduce new methods and fields related to hysteresis parameters in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2142 +/- ##
==========================================
+ Coverage 26.28% 26.51% +0.22%
==========================================
Files 360 360
Lines 16243 16244 +1
Branches 12 12
==========================================
+ Hits 4270 4307 +37
+ Misses 11705 11658 -47
- Partials 268 279 +11
|
abi87
force-pushed
the
complete-validator-set-updates
branch
2 times, most recently
from
25975af
to
d451b3f
Compare
abi87
force-pushed
the
complete-validator-set-updates
branch
from
d451b3f
to
d4ec3e7
Compare
| ) | ||
| switch { | ||
| case dep.Amount >= maxBalance: | ||
| require.Equal(t, maxBalance, val.EffectiveBalance) | ||
| case dep.Amount >= minBalance && dep.Amount < maxBalance: | ||
| case dep.Amount > minBalance && dep.Amount < maxBalance: |
There was a problem hiding this comment.
this is the correct check. Genesis validator whose balance is equal to EjectionBalance should be immediately ejected. Is it?
| @@ -197,34 +198,26 @@ func (sp *StateProcessor[ | |||
| ]) ProcessSlots( | |||
| st BeaconStateT, slot math.Slot, | |||
| ) (transition.ValidatorUpdates, error) { | |||
| var ( | |||
| validatorUpdates transition.ValidatorUpdates | |||
| epochValidatorUpdates transition.ValidatorUpdates | |||
There was a problem hiding this comment.
nit: reduced epochValidatorUpdates scope (it's only used when processing epochs)
| @@ -336,6 +329,9 @@ func (sp *StateProcessor[ | |||
| if err := sp.processRewardsAndPenalties(st); err != nil { | |||
| return nil, err | |||
| } | |||
| if err := sp.processEffectiveBalanceUpdates(st); err != nil { | |||
There was a problem hiding this comment.
this is one of the main point of the PR: along Eth 2.0 specs, EffectiveBalances must be updated only once per epoch, not every slot
| @@ -380,14 +375,6 @@ func (sp *StateProcessor[ | |||
| ) | |||
| } | |||
|
|
|||
| parentBlockRoot := latestBlockHeader.HashTreeRoot() | |||
There was a problem hiding this comment.
nit: just moved below to match better operation ordering in Eth2.0 specs
| func (sp *StateProcessor[ | ||
| _, _, _, BeaconStateT, _, DepositT, _, _, _, _, _, _, _, _, _, _, _, | ||
| ]) processDeposits( | ||
| st BeaconStateT, | ||
| deposits []DepositT, | ||
| ) error { | ||
| // Ensure the deposits match the local state. |
There was a problem hiding this comment.
nit: dropped to make code closed to Eth 2.0 specs
| activeVals := make([]ValidatorT, 0, len(vals)) | ||
| for _, val := range vals { | ||
| if val.GetEffectiveBalance() > math.U64(sp.cs.EjectionBalance()) { | ||
| activeVals = append(activeVals, val) | ||
| } | ||
| } |
There was a problem hiding this comment.
you may ask why we didn't do it so far. The reason is so far:
- Validator are activated as soon as they are added
- They never reduces their stake
- In fact this is not exactly true. We do reduce stake if a validator's balance goes beyond MaxEffectiveBalance but we reduce it just enough to get it to MaxEffectiveBalance. So the validator will stay active.
We need to change this to allow a capped validator set where validators may leave due to a validator with more stake coming in
| HysteresisQuotient: uint64(4), | ||
| HysteresisDownwardMultiplier: uint64(1), | ||
| HysteresisUpwardMultiplier: uint64(5), |
There was a problem hiding this comment.
default values from Eth 2.0 specs, similar to other quantities here
| //nolint:lll | ||
| func (sp *StateProcessor[ | ||
| _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _, | ||
| ]) processEffectiveBalanceUpdates( |
There was a problem hiding this comment.
this is straight from Eth 2.0 specs.
In essence, at each epoch, check if a validator balance has moved from its effective balance more than a certain amount. If so update the balance
|
|
||
| // create test inputs | ||
| var ( | ||
| genDeposits = []*types.Deposit{ |
There was a problem hiding this comment.
improved test expressiveness and added extra cases here
| case dep.Amount > minBalance && dep.Amount < maxBalance: | ||
| // Effective balance must be a multiple of increment. | ||
| // If balance is not, effective balance is rounded down | ||
| if dep.Amount%increment == 0 { |
There was a problem hiding this comment.
improved checks here. Hopefully the effective balance amount invariants are clearer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range comments (2)
mod/state-transition/pkg/core/state_processor_staking_test.go (1)
Line range hint
76-115: Consider using test data constants or helper functions.While the genesis setup is correct, consider extracting the test data setup into constants or helper functions to improve readability and make it easier to maintain different test scenarios.
Example refactor:
+const ( + validator1PubKey = [48]byte{0x01} + validator2PubKey = [48]byte{0x02} + validator3PubKey = [48]byte{0x03} +) + +func createGenesisDeposit(pubkey [48]byte, balanceOffset math.Gwei) *types.Deposit { + return &types.Deposit{ + Pubkey: pubkey, + Credentials: emptyCredentials, + Amount: balanceOffset, + Index: uint64(0), + } +}mod/state-transition/pkg/core/state_processor_genesis_test.go (1)
Line range hint
209-304: Consider reducing test duplication.The test cases for regular and Bartio networks share significant code. Consider extracting common setup and validation logic into helper functions:
+func setupGenesisTest(t *testing.T, cs chain.Spec[...]) (*StateProcessor, *TestBeaconStateT, []*types.Deposit, []*types.Deposit) { + // Extract common setup code here + return sp, beaconState, genDeposits, goodDeposits +} +func validateGenesisState(t *testing.T, bs *TestBeaconStateT, genDeposits []*types.Deposit, goodDeposits []*types.Deposit, genVals []*types.Validator) { + // Extract common validation code here +} func TestInitialize(t *testing.T) { - // Current setup code + sp, beaconState, genDeposits, goodDeposits := setupGenesisTest(t, spec.BetnetChainSpec()) // Test execution - // Current validation code + validateGenesisState(t, beaconState, genDeposits, goodDeposits, genVals) // Network-specific validation for _, dep := range goodDeposits { checkValidatorNonBartio(t, cs, beaconState, dep) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
mod/chain-spec/pkg/chain/chain_spec.go(2 hunks)mod/chain-spec/pkg/chain/data.go(1 hunks)mod/config/pkg/spec/testnet.go(1 hunks)mod/state-transition/pkg/core/state_processor.go(7 hunks)mod/state-transition/pkg/core/state_processor_committee.go(2 hunks)mod/state-transition/pkg/core/state_processor_genesis.go(3 hunks)mod/state-transition/pkg/core/state_processor_genesis_test.go(7 hunks)mod/state-transition/pkg/core/state_processor_staking.go(2 hunks)mod/state-transition/pkg/core/state_processor_staking_test.go(5 hunks)
🧰 Additional context used
📓 Learnings (2)
mod/chain-spec/pkg/chain/data.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:150-151
Timestamp: 2024-11-06T21:50:24.685Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, when adding new fields to the `SpecData` struct, follow the existing pattern for documentation, which may be minimal.
mod/state-transition/pkg/core/state_processor_genesis_test.go (3)
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:82-108
Timestamp: 2024-10-31T22:12:16.428Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, the `deposits` array is part of the test setup and not individual test cases, so adding comments to explain each deposit is not necessary.
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:79-176
Timestamp: 2024-10-29T22:31:53.888Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, adding additional tests requires resetting the persistence component, which complicates the implementation.
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:145-175
Timestamp: 2024-10-29T22:31:04.468Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, refactoring to extract assertion helpers will be revisited when adding unit tests for `Transition`.
🔇 Additional comments (20)
mod/state-transition/pkg/core/state_processor_committee.go (2)
24-24: LGTM: Required import for balance threshold comparison.
The addition of the math package is necessary for the new validator filtering logic.
43-43: Verify ejection balance threshold implementation.
Let's verify that the ejection balance threshold is being used correctly according to Ethereum 2.0 specifications.
✅ Verification successful
Based on the comprehensive search results, I can now verify the ejection balance implementation. The code shows:
- The ejection balance is properly defined in the chain spec configuration (
mod/config/pkg/spec/testnet.go) with the standard value of 16 ETH (16e9 Gwei) - The implementation in
state_processor_committee.gocorrectly uses this threshold to filter active validators by comparing their effective balance against the ejection balance - The validator state transitions and balance updates are handled consistently with the Ethereum 2.0 specifications, including proper handling of effective balances
Ejection balance implementation is correctly aligned with Ethereum 2.0 specifications
The implementation follows the standard Ethereum 2.0 validator lifecycle where:
- Validators with effective balance above ejection balance (16 ETH) are considered active
- The comparison is done using the proper chain spec configuration value
- The balance checks are integrated into the validator state processing logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of EjectionBalance to ensure consistent implementation
rg -A 2 "EjectionBalance\(\)"
# Search for any tests verifying this threshold
rg -l "TestProcess.*Committee.*" --type go
Length of output: 1855
Script:
#!/bin/bash
# Let's check the chain spec implementation and tests
ast-grep --pattern 'type ChainSpec struct {
$$$
EjectionBalance $$$
$$$
}'
# Check for committee-related tests
rg -A 5 "func TestProcess.*Committee" --type go
# Check for any constants or configurations related to ejection balance
rg -A 2 "EjectionBalance.*=" --type go
# Look for validator effective balance related code
rg -A 3 "GetEffectiveBalance|EffectiveBalance" --type go
Length of output: 64458
mod/config/pkg/spec/testnet.go (1)
63-66: LGTM! Values align with Ethereum specifications
The Gwei value constants are correctly defined and match the Ethereum mainnet specifications.
mod/state-transition/pkg/core/state_processor_genesis.go (3)
24-24: LGTM: Import addition is appropriate.
The added import for consensus-types is necessary for the new effective balance computation functionality.
178-180: LGTM: Proper initialization of withdrawal validator index.
The initialization of NextWithdrawalValidatorIndex is consistent with other state initializations and follows the expected pattern.
135-139: Verify effective balance computation parameters.
The effective balance computation uses configuration parameters from the chain spec. Let's verify these align with the Ethereum 2.0 specification.
✅ Verification successful
Effective balance parameters verified and match Ethereum 2.0 specification
The verification confirms that:
MaxEffectiveBalanceis set to 32 ETH (32e9 Gwei)EffectiveBalanceIncrementis set to 1 ETH (1e9 Gwei)- The
ComputeEffectiveBalancefunction implementation correctly rounds down to the nearest increment and caps at max effective balance
These values and implementation align with the Ethereum 2.0 specification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify effective balance parameters match Ethereum 2.0 spec
# Expected values:
# - EFFECTIVE_BALANCE_INCREMENT = 1 ETH = 1_000_000_000 (Gwei)
# - MAX_EFFECTIVE_BALANCE = 32 ETH = 32_000_000_000 (Gwei)
# Search for the parameter definitions
echo "Searching for effective balance parameters..."
rg -A 2 "EffectiveBalanceIncrement|MaxEffectiveBalance" --type go
# Search for any test cases verifying these values
echo "Searching for related test cases..."
rg -A 5 "TestEffectiveBalance|TestComputeEffectiveBalance" --type go
Length of output: 9834
Script:
#!/bin/bash
# Let's check the actual values in the test configuration and any test cases
# that verify effective balance computation
# First, let's look for test configuration files
echo "Searching for test configuration..."
rg -A 10 "MaxEffectiveBalance.*32e9|EffectiveBalanceIncrement.*1e9" --type go
# Then, let's look for the actual ComputeEffectiveBalance function implementation
echo "Searching for ComputeEffectiveBalance implementation..."
ast-grep --pattern 'func ComputeEffectiveBalance($$$) $$ {
$$$
}'
# Finally, let's look for any test cases that verify the computation
echo "Searching for test cases with balance computations..."
rg -A 10 "ComputeEffectiveBalance\(" --type go
Length of output: 4765
mod/chain-spec/pkg/chain/data.go (1)
46-50: Verify the field placement impact on existing configurations.
Since these fields are used for configuration via mapstructure, ensure that:
- Existing configuration files are updated to include these new fields
- Default values are properly handled in case these fields are missing in config files
✅ Verification successful
Default values are provided, no configuration updates required
The verification shows that:
- Default values are properly initialized in
mod/config/pkg/spec/testnet.go:HysteresisQuotient: uint64(4), HysteresisDownwardMultiplier: uint64(1), HysteresisUpwardMultiplier: uint64(5),
- The fields are properly used in state processing logic (
mod/state-transition/pkg/core/state_processor.go) - No existing configuration files contain these fields, which is fine as they have default values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing configuration files and verify if they need updates
echo "Searching for configuration files..."
fd -e yaml -e json -e toml | while read -r file; do
echo "Checking $file for hysteresis configuration..."
rg -i "hysteresis|balance.*multiplier" "$file" || echo "No hysteresis config found in $file"
done
# Search for any default value initialization
echo "Checking for default value initialization..."
rg "hysteresis.*=.*[0-9]+"
Length of output: 16639
Script:
#!/bin/bash
# Let's check for default value initialization in Go files and any config loading logic
echo "Checking for default value initialization and config loading in Go files..."
rg -A 5 -B 5 "HysteresisQuotient|HysteresisDownwardMultiplier|HysteresisUpwardMultiplier"
# Check for any test files that might show usage of these fields
echo "Checking test files for usage patterns..."
fd -e go -E vendor | xargs rg "Test.*Hysteresis"
# Look for any config loading or validation logic
echo "Checking for config validation and loading logic..."
rg "func.*[Vv]alidate.*Config"
Length of output: 6227
mod/state-transition/pkg/core/state_processor_staking_test.go (2)
Line range hint 40-75: LGTM! Well-structured test setup.
The test setup is comprehensive, with clear initialization of mocks and proper derivation of balance constants from the chain specification.
220-222: 🧹 Nitpick (assertive)
Address the TODO comment about genesis order assumption.
The TODO indicates a potential fragility in the test. The assumption about preserved genesis order could make the test brittle.
Let's verify if the order assumption is necessary:
Would you like help implementing a more robust validation approach that doesn't depend on order?
mod/state-transition/pkg/core/state_processor_staking.go (1)
110-110: LGTM: Simplified balance update logic.
The direct use of IncreaseBalance simplifies the implementation and better aligns with Eth 2.0 specifications.
mod/state-transition/pkg/core/state_processor_genesis_test.go (2)
Line range hint 112-158: LGTM! Clear separation of valid and invalid deposits.
The test execution and validation logic is well-structured:
- Clear separation between all deposits and valid deposits
- Proper validation of processed validator count
- Correct verification of deposit index tracking
63-111: 🧹 Nitpick (assertive)
LGTM! Consider documenting the test cases.
The test setup has been improved with:
- Clear variable names for balance thresholds
- Comprehensive test cases covering edge cases
- Better organization of test inputs
Consider adding a comment block explaining what each test case validates:
// Test cases:
// 1. Exact max balance (maxBalance)
// 2. Min balance + increment
// 3. Exact min balance
// 4. Double max balance
// 5. Below min balance
// 6. Non-increment aligned balance (1.5 increments above min)
// 7. Slightly above max balance
// 8. Almost one increment above min balance⛔ Skipped due to learnings
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:82-108
Timestamp: 2024-10-31T22:12:16.428Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, the `deposits` array is part of the test setup and not individual test cases, so adding comments to explain each deposit is not necessary.
mod/chain-spec/pkg/chain/chain_spec.go (2)
254-270: LGTM! Implementation follows established patterns.
The implementations are clean, consistent with the codebase's style, and correctly handle generic type parameters.
48-53: Verify the usage of hysteresis parameters in balance updates.
These parameters are crucial for effective balance calculations. Let's verify their usage:
Also applies to: 254-270
✅ Verification successful
Hysteresis parameters are correctly integrated and used in balance calculations
The verification shows proper integration:
- Parameters are correctly defined in the chain spec interface and data structures
- They are properly used in
processEffectiveBalanceUpdatesfor calculating balance thresholds - The implementation aligns with Ethereum 2.0 specifications with standard values (quotient: 4, downward: 1, upward: 5)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find where these hysteresis parameters are used in balance calculations
# Search for references to these parameters in balance update logic
echo "Searching for hysteresis parameter usage in balance calculations..."
rg -l "HysteresisQuotient|HysteresisDownwardMultiplier|HysteresisUpwardMultiplier" --type go
# Search for balance update related functions that might use these parameters
echo "Searching for balance update functions..."
ast-grep --pattern 'func $FUNC_NAME($$$) {
$$$
(HysteresisQuotient|HysteresisDownwardMultiplier|HysteresisUpwardMultiplier)
$$$
}'
Length of output: 623
Script:
#!/bin/bash
# Let's examine the actual usage in state_processor.go and the implementation in data.go
echo "Checking state_processor.go implementation..."
rg -A 10 -B 10 "Hysteresis" mod/state-transition/pkg/core/state_processor.go
echo -e "\nChecking data.go implementation..."
rg -A 5 -B 5 "Hysteresis" mod/chain-spec/pkg/chain/data.go
echo -e "\nChecking testnet.go implementation..."
rg -A 5 -B 5 "Hysteresis" mod/config/pkg/spec/testnet.go
Length of output: 2321
mod/state-transition/pkg/core/state_processor.go (6)
26-26: Import of types package is appropriate and necessary.
The added import statement is required for using types.ComputeEffectiveBalance in processEffectiveBalanceUpdates.
Line range hint 201-230: Correct accumulation of validator updates in ProcessSlots.
Initializing res and aggregating epoch validator updates ensures that all validator changes are properly captured and returned.
332-334: Adding processEffectiveBalanceUpdates in processEpoch aligns with specifications.
Including the call to processEffectiveBalanceUpdates ensures that effective balances are updated once per epoch, as per the Ethereum 2.0 specifications.
361-361: Improved error message clarity on slot mismatch.
The updated error message provides clearer information on expected and actual slot numbers, aiding in debugging.
Line range hint 366-407: Comprehensive block header verification added.
The added checks for block slot order, parent block root matching, and proposer slashing status enhance the robustness of block header processing.
504-554: processEffectiveBalanceUpdates correctly implements balance hysteresis updates.
The new function appropriately adjusts validators' effective balances using hysteresis thresholds, in accordance with Ethereum 2.0 specifications.
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/state-transition/pkg/core/state_processor_genesis.go(4 hunks)
🔇 Additional comments (1)
mod/state-transition/pkg/core/state_processor_genesis.go (1)
115-142: Refactor error handling in the validator activation loop for better maintainability
The error handling pattern within the validator activation loop is repeated multiple times. Consider extracting this logic into a helper function to reduce code duplication and improve readability.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
mod/state-transition/pkg/core/state_processor_staking_test.go(5 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:175-197
Timestamp: 2024-11-09T15:54:58.858Z
Learning: In Go test files within the `mod/state-transition/pkg/core/` directory, when a test is already substantial in size, avoid adding verbose code or comments that increase its length further.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:154-168
Timestamp: 2024-11-09T15:55:33.658Z
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.
🔇 Additional comments (3)
mod/state-transition/pkg/core/state_processor_staking_test.go (3)
77-97: LGTM! Well-structured test data setup.
The genesis deposits are well-designed with meaningful amounts that test different scenarios:
- Validator 1: Minimum balance + increment
- Validator 2: Near max balance (max - 6 increments)
- Validator 3: Near max balance (max - 3 increments)
124-128: LGTM! Good test case design.
The deposit amount of 2 * increment is well-chosen to test the hysteresis threshold, and the comment explains this reasoning.
174-236: LGTM! Comprehensive epoch transition test.
The test effectively verifies that:
- Effective balances remain unchanged during the epoch
- Effective balances are updated at epoch transition
- Other validators remain unaffected
| validators, err := st.GetValidators() | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
nit: just reduced validators scope, since this is only used right below to set genesis validators root
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
mod/state-transition/pkg/core/errors.go(1 hunks)mod/state-transition/pkg/core/state_processor_genesis.go(1 hunks)mod/state-transition/pkg/core/state_processor_staking_test.go(5 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:175-197
Timestamp: 2024-11-09T15:54:58.858Z
Learning: In Go test files within the `mod/state-transition/pkg/core/` directory, when a test is already substantial in size, avoid adding verbose code or comments that increase its length further.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:154-168
Timestamp: 2024-11-09T15:55:33.658Z
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.
🔇 Additional comments (10)
mod/state-transition/pkg/core/errors.go (1)
33-34: LGTM! Well-documented error variable.
The new ErrProposerMismatch error is well-defined and follows the established pattern. Its purpose is clear, and it integrates well with the existing error handling framework.
mod/state-transition/pkg/core/state_processor_genesis.go (6)
47-47: LGTM: Parameter rename improves readability
The parameter rename from executionPayloadHeader to execPayloadHeader follows Go naming conventions.
55-57: LGTM: Proper error handling for slot initialization
The error handling for slot initialization follows the established pattern and ensures proper state setup.
71-77: LGTM: Clean Eth1 data initialization
The initialization of eth1Data with an empty root, zero deposit count, and the execution payload's block hash is appropriate for genesis state.
82-94: LGTM: Proper genesis block initialization
The block body and header initialization with appropriate genesis values (zero slot, zero proposer index, empty roots) is correct.
157-169: LGTM: Proper genesis state initialization
The initialization of withdrawal indices and slashing with appropriate error handling is correct for genesis state.
127-141: Verify the Bartio validators root constant
The implementation looks correct, but let's verify the hardcoded validators root constant.
✅ Verification successful
Let me gather more context about this Bartio-specific constant.
Let me check the test files to verify if this constant is being tested.
Let me check the complete test implementation and chain spec for Bartio.
Let me check the chain spec implementation for Bartio.
Based on the comprehensive analysis of the codebase, I can now generate the final response.
Bartio validators root constant is correctly implemented and verified
The hardcoded validators root 0x9147586693b6e8faa837715c0f3071c2000045b54233901c2e7871b15872bc43 is properly implemented and verified:
- It's consistently used with chain ID 80084 across the codebase
- The implementation is thoroughly tested in
TestInitializeBartio - The chain ID is properly defined in the testnet chain spec
- Special case handling for Bartio is well-documented and tested
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of the Bartio validators root to ensure consistency
rg -l "9147586693b6e8faa837715c0f3071c2000045b54233901c2e7871b15872bc43"
Length of output: 129
Script:
#!/bin/bash
# Get the context around the Bartio validators root constant
rg -B 5 -A 5 "9147586693b6e8faa837715c0f3071c2000045b54233901c2e7871b15872bc43"
# Look for any Bartio-related constants or configurations
rg -i "bartio"
Length of output: 7690
Script:
#!/bin/bash
# Check the test files for validation of this constant
rg -B 10 -A 10 "TestInitializeBartio" mod/state-transition/pkg/core/state_processor_genesis_test.go
# Also check if there are any other validator root related tests
rg -i "validatorsroot.*test"
Length of output: 769
Script:
#!/bin/bash
# Get the complete test implementation
cat mod/state-transition/pkg/core/state_processor_genesis_test.go
# Check the chain spec implementation
rg -B 5 -A 5 "TestnetChainSpec" mod/config/pkg/spec/
Length of output: 10805
Script:
#!/bin/bash
# Get the testnet chain spec implementation
cat mod/config/pkg/spec/testnet.go
# Also check for any Bartio-specific chain ID references
rg "80084" --type go
Length of output: 4493
mod/state-transition/pkg/core/state_processor_staking_test.go (3)
41-43: LGTM! Clear and focused test documentation.
The function rename and documentation changes accurately reflect the test's purpose of demonstrating the lifecycle of a validator's balance updates.
117-237: LGTM! Comprehensive test coverage of balance update lifecycle.
The test effectively validates:
- Immediate balance updates after deposits
- Delayed effective balance updates at epoch transitions
- Correct validator indexing
Line range hint 239-422: LGTM! Good coverage of validator creation lifecycle.
The test comprehensively validates the creation of new validators and their balance handling, complementing the update test.
| // STEP 0: Setup initial state via genesis | ||
| var ( | ||
| genDeposits = []*types.Deposit{ | ||
| { | ||
| Pubkey: [48]byte{0x01}, | ||
| Credentials: emptyCredentials, | ||
| Amount: maxBalance - 3*minBalance, | ||
| Amount: minBalance + increment, | ||
| Index: uint64(0), | ||
| }, | ||
| { | ||
| Pubkey: [48]byte{0x02}, | ||
| Credentials: emptyCredentials, | ||
| Amount: maxBalance - 6*minBalance, | ||
| Amount: maxBalance - 6*increment, | ||
| Index: uint64(1), | ||
| }, | ||
| { | ||
| Pubkey: [48]byte{0x03}, | ||
| Credentials: emptyCredentials, | ||
| Amount: maxBalance - 3*increment, | ||
| Index: uint64(2), | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider documenting the significance of deposit amounts.
The deposit amounts use multiples of increment (e.g., maxBalance - 6*increment, maxBalance - 3*increment), but their significance isn't immediately clear. Consider adding comments explaining why these specific amounts were chosen.
abi87
changed the title
EffectiveBalance only when epoch turns
| // Ensure the block is within the acceptable range. | ||
| // TODO: move this is in the wrong spot. | ||
| deposits := blk.GetBody().GetDeposits() | ||
| if uint64(len(deposits)) > sp.cs.MaxDepositsPerBlock() { | ||
| return errors.Wrapf(ErrExceedsBlockDepositLimit, | ||
| "expected: %d, got: %d", | ||
| sp.cs.MaxDepositsPerBlock(), len(deposits), |
There was a problem hiding this comment.
fixed todo. Moved this to processDeposit
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
mod/chain-spec/pkg/chain/chain_spec.go(2 hunks)mod/chain-spec/pkg/chain/data.go(1 hunks)mod/state-transition/pkg/core/state_processor_genesis.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/state-transition/pkg/core/state_processor_genesis.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_genesis.go:109-118
Timestamp: 2024-11-09T21:00:02.468Z
Learning: In `InitializePreminedBeaconStateFromEth1` in `mod/state-transition/pkg/core/state_processor_genesis.go`, we enforce that there is a single deposit per validator in the genesis format; therefore, handling multiple deposits per validator is not necessary.
🔇 Additional comments (10)
mod/state-transition/pkg/core/state_processor_genesis.go (7)
47-47: LGTM: Parameter rename is appropriate.
The parameter rename from executionPayloadHeader to execPayloadHeader improves conciseness while maintaining clarity.
55-57: LGTM: Proper error handling for slot initialization.
The error handling for SetSlot(0) follows Go's best practices and ensures initialization failures are properly reported.
71-78: LGTM: Clean eth1Data initialization.
The eth1Data initialization is well-structured, using the builder pattern and proper error handling.
82-94: LGTM: Clean block initialization.
The block body and header initialization is well-structured with appropriate default values for genesis.
101-101: LGTM: Proper randao mix initialization.
Correctly uses the execution payload header's block hash for randao mix initialization.
116-126: LGTM: Improved validators root handling.
The code cleanly handles both Bartio and non-Bartio cases with proper error handling. The validators scope has been appropriately reduced to this specific section.
138-158: LGTM: Comprehensive state initialization.
All necessary state fields are properly initialized with appropriate error handling.
mod/chain-spec/pkg/chain/chain_spec.go (3)
48-60: LGTM! Documentation and interface methods are well-defined.
The new hysteresis-related methods are properly documented and follow the interface's established patterns. The documentation clearly explains the purpose and impact of each parameter.
261-277: LGTM! Implementation is consistent with existing patterns.
The implementation of hysteresis-related methods follows the established pattern of the codebase, maintaining consistency with other getter methods.
48-60: 🧹 Nitpick (assertive)
Consider documenting the hysteresis parameter values.
Since these parameters are crucial for effective balance calculations and validator updates, consider documenting the expected/default values in the interface documentation. This would help other developers understand the typical ranges and their impact on the hysteresis mechanism.
Let's verify if these values are documented elsewhere:
Also applies to: 261-277
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/state-transition/pkg/core/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
mod/state-transition/pkg/core/README.md
[uncategorized] ~7-~7: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...rever, as we have not (yet) implemented withdrawals facilities. - Withdrawals are automatic...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[formatting] ~8-~8: Consider inserting a comma after an introductory phrase for better readability.
Context: ...ance goes beyond MaxEffectiveBalance. In this case enough balance is scheduled for withdra...
(IN_THAT_CASE_COMMA)
[uncategorized] ~9-~9: A comma may be missing after the conjunctive/linking adverb ‘Moreover’.
Context: ...1] because of the insufficient balance. Moreover the whole deposited balance is schedule...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~12-~12: Possible missing comma found.
Context: ...which are not correctly handled. [^1]: Technically a validator is made in the BeaconKit st...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~12-~12: A comma may be missing after the conjunctive/linking adverb ‘Moreover’.
Context: ...never returned to the consensus engine. Moreover the deposit should be evicted at the ne...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
abi87
force-pushed
the
complete-validator-set-updates
branch
from
2d871a7
to
b6d62c5
Compare
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/state-transition/pkg/core/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
mod/state-transition/pkg/core/README.md
[formatting] ~8-~8: Consider inserting a comma after an introductory phrase for better readability.
Context: ...ance goes beyond MaxEffectiveBalance. In this case enough balance is scheduled for withdra...
(IN_THAT_CASE_COMMA)
[uncategorized] ~11-~11: Possible missing comma found.
Context: ...or withdrawal at the next epoch. [^1]: Technically a validator is made in the BeaconKit st...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~11-~11: A comma may be missing after the conjunctive/linking adverb ‘Moreover’.
Context: ...never returned to the consensus engine. Moreover the deposit should be evicted at the ne...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🔇 Additional comments (3)
mod/state-transition/pkg/core/README.md (3)
7-8: 🧹 Nitpick (assertive)
Improve clarity and fix grammar in withdrawal conditions.
The withdrawal mechanism description could be clearer, and there's a missing comma after "In this case".
Apply these changes:
-Any validator whose effective balance is above `EjectionBalance` will stay a validator forever, as we have not (yet) implemented withdrawals facilities.
-Withdrawals are automatically generated only if a validator effective balance goes beyond `MaxEffectiveBalance`. In this case enough balance is scheduled for withdrawal, just enough to make validator's effective balance equal to `MaxEffectiveBalance`. Since `MaxEffectiveBalance` > `EjectionBalance`, the validator will keep being a validator.
+Any validator whose effective balance is above `EjectionBalance` will stay a validator forever, as we have not (yet) implemented withdrawal facilities.
+Withdrawals are automatically generated only if a validator's effective balance goes beyond `MaxEffectiveBalance`. In this case, enough balance is scheduled for withdrawal to make the validator's effective balance equal to `MaxEffectiveBalance`. Since `MaxEffectiveBalance` > `EjectionBalance`, the validator will remain active.🧰 Tools
🪛 LanguageTool
[formatting] ~8-~8: Consider inserting a comma after an introductory phrase for better readability.
Context: ...ance goes beyond MaxEffectiveBalance. In this case enough balance is scheduled for withdra...
(IN_THAT_CASE_COMMA)
11-11: 🧹 Nitpick (assertive)
Fix grammar and expand temporary validator state explanation.
The footnote needs grammatical improvements (missing commas) and could benefit from more detailed explanation.
Apply these changes:
-[^1]: Technically a validator is made in the BeaconKit state to track the deposit, but such a validator is never returned to the consensus engine. Moreover the deposit should be evicted at the next epoch.
+[^1]: Technically, a validator is made in the BeaconKit state to track the deposit, but such a validator is never returned to the consensus engine. Moreover, the deposit should be evicted at the next epoch.🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Possible missing comma found.
Context: ...or withdrawal at the next epoch. [^1]: Technically a validator is made in the BeaconKit st...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~11-~11: A comma may be missing after the conjunctive/linking adverb ‘Moreover’.
Context: ...never returned to the consensus engine. Moreover the deposit should be evicted at the ne...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
1-11: 🛠️ Refactor suggestion
Document epoch-based effective balance updates.
The README should include information about the new epoch-based effective balance update mechanism, as this is a key change mentioned in the PR objectives.
🧰 Tools
🪛 LanguageTool
[formatting] ~8-~8: Consider inserting a comma after an introductory phrase for better readability.
Context: ...ance goes beyond MaxEffectiveBalance. In this case enough balance is scheduled for withdra...
(IN_THAT_CASE_COMMA)
[uncategorized] ~11-~11: Possible missing comma found.
Context: ...or withdrawal at the next epoch. [^1]: Technically a validator is made in the BeaconKit st...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~11-~11: A comma may be missing after the conjunctive/linking adverb ‘Moreover’.
Context: ...never returned to the consensus engine. Moreover the deposit should be evicted at the ne...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (1)
mod/state-transition/pkg/core/state_processor_staking.go (1)
Line range hint
183-186: Track Bartio-specific workaround for removalThe TODO indicates this is a temporary workaround. Consider creating a tracking issue for its removal.
Would you like me to create a GitHub issue to track the removal of the Bartio-specific code path?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
mod/config/pkg/spec/special_cases.go(1 hunks)mod/state-transition/pkg/core/state_processor.go(7 hunks)mod/state-transition/pkg/core/state_processor_genesis.go(1 hunks)mod/state-transition/pkg/core/state_processor_staking.go(2 hunks)
🧰 Additional context used
📓 Learnings (2)
mod/state-transition/pkg/core/state_processor_genesis.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_genesis.go:109-118
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `InitializePreminedBeaconStateFromEth1` in `mod/state-transition/pkg/core/state_processor_genesis.go`, we enforce that there is a single deposit per validator in the genesis format; therefore, handling multiple deposits per validator is not necessary.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_genesis.go:46-159
Timestamp: 2024-11-12T11:12:56.773Z
Learning: When reviewing the `InitializePreminedBeaconStateFromEth1` function in `mod/state-transition/pkg/core/state_processor_genesis.go`, additional documentation of the initialization sequence is not necessary as per the user's preference.
mod/state-transition/pkg/core/state_processor_staking.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking.go:105-106
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In this project, detailed TODO comments may not be necessary; concise TODOs are acceptable.
🔇 Additional comments (12)
mod/config/pkg/spec/special_cases.go (1)
29-29: Verify validator root hash usage
Given this PR's focus on validator balance updates, we should verify how this constant is used in the validator processing logic.
✅ Verification successful
BartioValRoot usage verified
No issues found with the usage of BartioValRoot in validator processing logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of BartioValRoot in validator-related code
# Search for usage of the constant
rg -A 5 "BartioValRoot"
# Look for validator-related files that might use this
ast-grep --pattern 'func $_ ($$$) {
$$$
BartioValRoot
$$$
}'
Length of output: 803
mod/state-transition/pkg/core/state_processor_genesis.go (4)
42-42: LGTM: Parameter rename maintains consistency
The rename from executionPayloadHeader to execPayloadHeader aligns with naming conventions.
50-52: LGTM: Proper error handling for slot initialization
The error handling follows Go best practices and ensures proper initialization of the genesis slot.
66-72: LGTM: Clean Eth1Data initialization
The initialization is well-structured with proper error handling and uses a clean method chaining pattern.
137-149: LGTM: Proper initialization of withdrawal and slashing state
The initialization sequence follows a consistent pattern with proper error handling.
mod/state-transition/pkg/core/state_processor_staking.go (2)
Line range hint 119-171: LGTM! Comprehensive validator creation logic
The implementation correctly handles both genesis and non-genesis cases, with proper signature verification against fork data.
42-54: Verify deposit limit check implementation against Eth2.0 specs
The deposit limit check has been simplified, but we should verify its alignment with the Ethereum 2.0 specifications.
mod/state-transition/pkg/core/state_processor.go (5)
Line range hint 206-235: Validator updates are correctly aggregated in ProcessSlots
The refactoring to use a single res variable to collect validator updates during slot processing improves code clarity and ensures that all updates are properly accumulated and returned.
337-339: Including processEffectiveBalanceUpdates in epoch processing
Adding processEffectiveBalanceUpdates within processEpoch aligns with Ethereum 2.0 specifications by ensuring effective balances are updated once per epoch.
371-373: Validating that block slot is newer than latest block header
The added check ensures that the block's slot is greater than the latest block header's slot, preventing the processing of stale or duplicate blocks and maintaining chain consistency.
399-404: Verifying block's parent root matches the latest block header
Ensuring that the block's parent root matches the hash tree root of the latest block header strengthens chain integrity by validating the correct parent-child relationship between blocks.
408-412: Preventing slashed proposers from proposing blocks
Adding a check to verify that the proposer is not slashed prevents penalized validators from proposing new blocks, adhering to consensus rules and enhancing network security.
| @@ -26,5 +26,5 @@ const ( | |||
| BartioChainID uint64 = 80084 | |||
|
|
|||
| //nolint:lll // temporary. | |||
| BArtioValRoot = "0x9147586693b6e8faa837715c0f3071c2000045b54233901c2e7871b15872bc43" | |||
| BartioValRoot = "0x9147586693b6e8faa837715c0f3071c2000045b54233901c2e7871b15872bc43" | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
💡 Codebase verification
Create a GitHub issue to track removal of Bartio special cases
No existing issue found. Multiple TODOs referencing Bartio special cases were found in:
mod/state-transition/pkg/core/state_processor_payload.gomod/state-transition/pkg/core/state/statedb.gomod/beacon/blockchain/execution_engine.go
🔗 Analysis chain
Track temporary code for future removal
The comment indicates this is temporary special case handling "to be removed". While the constant rename improves consistency, we should ensure this technical debt is tracked.
Let me help create a GitHub issue to track the removal of these special cases. Would you like me to:
- Create an issue to track the removal of Bartio special cases
- Include a timeline recommendation based on the validator balance processing improvements mentioned in the PR?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing issues about Bartio special cases
# Search for related issues
gh issue list --search "Bartio special cases" --state all
# Check for other references to Bartio in the codebase
rg -i "bartio.*special.*case"
Length of output: 425
| var blkBody BeaconBlockBodyT | ||
| blkBody = blkBody.Empty(version.ToUint32(genesisVersion)) | ||
|
|
||
| var blkHeader BeaconBlockHeaderT | ||
| blkHeader = blkHeader.New( | ||
| 0, // slot | ||
| 0, // proposer index | ||
| common.Root{}, // parent block root | ||
| common.Root{}, // state root | ||
| blkBody.HashTreeRoot(), // body root | ||
|
|
||
| ) | ||
| if err := st.SetLatestBlockHeader(blkHeader); err != nil { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding error handling for block initialization
While the initialization sequence is correct, consider adding error handling for blkBody.Empty() and blkBody.HashTreeRoot() calls to handle potential failures gracefully.
var blkBody BeaconBlockBodyT
-blkBody = blkBody.Empty(version.ToUint32(genesisVersion))
+blkBody, err = blkBody.Empty(version.ToUint32(genesisVersion))
+if err != nil {
+ return nil, err
+}
+
+bodyRoot, err := blkBody.HashTreeRoot()
+if err != nil {
+ return nil, err
+}
var blkHeader BeaconBlockHeaderT
blkHeader = blkHeader.New(
0, // slot
0, // proposer index
common.Root{}, // parent block root
common.Root{}, // state root
- blkBody.HashTreeRoot(), // body root
+ bodyRoot, // body root
)Committable suggestion skipped: line range outside the PR's diff.
| validatorsRoot := common.Root(hex.MustToBytes(spec.BartioValRoot)) | ||
| if sp.cs.DepositEth1ChainID() != spec.BartioChainID { | ||
| validators, err := st.GetValidators() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else if err = st. | ||
| SetGenesisValidatorsRoot(validators.HashTreeRoot()); err != nil { | ||
| validatorsRoot = validators.HashTreeRoot() | ||
| } | ||
| if err := st.SetGenesisValidatorsRoot(validatorsRoot); err != nil { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider documenting the Bartio special case
While the implementation is correct, adding a brief comment explaining why Bartio genesis requires a special validators root would improve maintainability.
+// Special case for Bartio genesis: use a predefined validators root instead of computing it
+// from the validators list to ensure deterministic genesis state across different networks.
validatorsRoot := common.Root(hex.MustToBytes(spec.BartioValRoot))📝 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.
| validatorsRoot := common.Root(hex.MustToBytes(spec.BartioValRoot)) | |
| if sp.cs.DepositEth1ChainID() != spec.BartioChainID { | |
| validators, err := st.GetValidators() | |
| if err != nil { | |
| return nil, err | |
| } | |
| } else if err = st. | |
| SetGenesisValidatorsRoot(validators.HashTreeRoot()); err != nil { | |
| validatorsRoot = validators.HashTreeRoot() | |
| } | |
| if err := st.SetGenesisValidatorsRoot(validatorsRoot); err != nil { | |
| // Special case for Bartio genesis: use a predefined validators root instead of computing it | |
| // from the validators list to ensure deterministic genesis state across different networks. | |
| validatorsRoot := common.Root(hex.MustToBytes(spec.BartioValRoot)) | |
| if sp.cs.DepositEth1ChainID() != spec.BartioChainID { | |
| validators, err := st.GetValidators() | |
| if err != nil { | |
| return nil, err | |
| } | |
| validatorsRoot = validators.HashTreeRoot() | |
| } | |
| if err := st.SetGenesisValidatorsRoot(validatorsRoot); err != nil { |
There was a problem hiding this comment.
Possible TODO
Mainnet follows: If balance below ejection balance after 1 epoch --> trigger withdrawal for validator
This PR fixes validators' balances and effective balances to make it closer to Eth 2.0 specs. In more details:
processEffectiveBalanceUpdatesto update balances at each epoch turnNote: the PR is somewhat large, but most of the code (> 300LOC) comes from added unit tests.
Left for upcoming PRs:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests