Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(blockchain): introducing validator size cap size #2119

Merged
merged 102 commits into from
Dec 2, 2024
Merged

Conversation

abi87
Copy link
Collaborator

@abi87 abi87 commented Nov 1, 2024

This PR:

  • Introduces a cap on the validator set size
  • Let highest staking validators be part of the validator set. So if a deposit is made that would breach the cap, the validator with the lowest staked amount will be evicted (or not added to the validator set if the lowest staking validator would be the one just being added)
  • Introduces processEffectiveBalanceUpdates

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a method to retrieve the maximum number of validators allowed in the active set.
    • Added a field for the maximum number of active validators per epoch.
    • Enhanced validator management by enforcing a cap on the validator set size.
    • Added a constant for the validator set cap in the configuration.
    • Introduced a new field for the validator set cap in the testnet specification.
    • Added a method to set the withdrawable epoch for validators.
    • Introduced new error handling for exceeding the validator set cap.
  • Bug Fixes

    • Improved validation logic for genesis deposits with checks against the validator set cap.
  • Tests

    • Expanded test coverage for validator behavior under various conditions, including eviction scenarios and validator cap enforcement.
    • Added new tests for scenarios involving validator cap limits and eviction logic.

@abi87 abi87 self-assigned this Nov 1, 2024
Copy link
Contributor

coderabbitai bot commented Nov 1, 2024

Walkthrough

The changes introduce several enhancements primarily focused on validator management within the blockchain framework. A new method ValidatorSetCap() is added to the Spec interface to retrieve the maximum number of validators allowed. Additionally, a ValidatorSetCap field is introduced in the SpecData struct. The StateProcessor struct is updated with methods to manage validator sets, including enforcing a cap on the number of validators and handling evictions. New error handling and testing functions are also added to ensure robust validator management.

Changes

File Path Change Summary
mod/chain-spec/pkg/chain/chain_spec.go Added method ValidatorSetCap() uint64 to Spec interface and chainSpec struct.
mod/chain-spec/pkg/chain/data.go Added field ValidatorSetCap uint64 \mapstructure:"validator-set-cap-size"`toSpecData`.
mod/consensus-types/pkg/types/validator.go Added method SetWithdrawableEpoch(e math.Epoch) to Validator struct.
mod/state-transition/pkg/core/errors.go Added error variable ErrValSetCapExceeded for exceeding validator set cap.
mod/state-transition/pkg/core/state_processor_staking.go Added methods nextEpochValidatorSet and lowestStakeVal, modified addValidatorToRegistry to enforce validator cap.
mod/state-transition/pkg/core/state_processor_staking_test.go Added tests for validator cap scenarios and new helper functions for testing.
mod/state-transition/pkg/core/types.go Added method SetWithdrawableEpoch(math.Epoch) to Validator interface.
mod/config/pkg/spec/base.go Added constant ValidatorSetCap with value 256 in BaseSpec.
mod/state-transition/pkg/core/core_test.go Added variable dummyExecutionPayload for enhanced test setup.
mod/state-transition/pkg/core/state_processor_validators.go Updated processValidatorsSetUpdates to use nextEpochValidatorSet for active validators.
mod/state-transition/pkg/core/validation_deposits.go Updated validateGenesisDeposits and validateNonGenesisDeposits to check against validator set cap.
mod/config/pkg/spec/boonet.go Added field ValidatorSetCap to boonetSpec structure with value 256.
mod/chain-spec/pkg/chain/errors.go Added error variable ErrInvalidValidatorSetCap for exceeding validator registry limit.

Possibly related PRs

Suggested reviewers

  • itsdevbear
  • abi87

Poem

🐇 In the land of code where validators play,
A cap on their numbers keeps chaos at bay.
With methods and structs, we build and refine,
Ensuring our network is stable and fine.
So hop along, friends, let’s celebrate this feat,
For our validators now stand strong on their feet! 🎉


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@abi87 abi87 changed the base branch from main to fix-maximum-number-withdrawals November 1, 2024 09:51
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 66.10169% with 40 lines in your changes missing coverage. Please review.

Project coverage is 27.99%. Comparing base (fd7cca5) to head (0307a03).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ate-transition/pkg/core/state_processor_staking.go 74.39% 14 Missing and 7 partials ⚠️
...d/state-transition/pkg/core/validation_deposits.go 17.64% 12 Missing and 2 partials ⚠️
mod/chain-spec/pkg/chain/chain_spec.go 40.00% 2 Missing and 1 partial ⚠️
mod/consensus-types/pkg/types/validator.go 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2119      +/-   ##
==========================================
+ Coverage   27.61%   27.99%   +0.38%     
==========================================
  Files         366      366              
  Lines       16867    16972     +105     
  Branches       13       13              
==========================================
+ Hits         4658     4752      +94     
- Misses      11900    11903       +3     
- Partials      309      317       +8     
Files with missing lines Coverage Δ
mod/config/pkg/spec/base.go 100.00% <100.00%> (ø)
mod/config/pkg/spec/boonet.go 100.00% <100.00%> (ø)
...-transition/pkg/core/state_processor_validators.go 91.30% <100.00%> (+10.48%) ⬆️
mod/consensus-types/pkg/types/validator.go 94.35% <0.00%> (-1.55%) ⬇️
mod/chain-spec/pkg/chain/chain_spec.go 18.62% <40.00%> (+3.16%) ⬆️
...d/state-transition/pkg/core/validation_deposits.go 36.97% <17.64%> (-4.38%) ⬇️
...ate-transition/pkg/core/state_processor_staking.go 69.82% <74.39%> (+3.15%) ⬆️

... and 2 files with indirect coverage changes

// TODO: consider moving this to BeaconState directly
func (sp *StateProcessor[
_, _, _, _, _, _, _, _, _, _, _, _, ValidatorT, _, _, _, _,
]) smallest(currentVals []ValidatorT) (
Copy link
Contributor

@calbera calbera Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to lowestStakeVal(vals []ValidatorT) or smth similar and keep as helper (no receiver StateProcessor)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tricky to get rid of the receiver (which I agree is not needed) and get the typing right. We have other methods here that could be free functions and are not for this reason.
I would rather defer everything to a general cleanup of the typing system

// Adding the validator would breach the cap. Find the validator
// with the smallest stake among current and candidate validators
// and kick it out.
smallestVal, err := sp.smallest(nextEpochVals)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename to lowestStakeVal

Copy link
Contributor

@fridrik01 fridrik01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and readable!

@@ -507,3 +511,11 @@ func (c chainSpec[
]) GetCometBFTConfigForSlot(_ SlotT) CometBFTConfigT {
return c.Data.CometValues
}

// GetValidatorSetCapS retrieves the maximum number of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// GetValidatorSetCapS retrieves the maximum number of
// GetValidatorSetCap retrieves the maximum number of

Base automatically changed from validator-set-updates-optimizations to main November 28, 2024 14:59
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between bca903b and e8db0d9.

📒 Files selected for processing (11)
  • mod/chain-spec/pkg/chain/chain_spec.go (2 hunks)
  • mod/chain-spec/pkg/chain/data.go (1 hunks)
  • mod/config/pkg/spec/base.go (1 hunks)
  • mod/consensus-types/pkg/types/validator.go (1 hunks)
  • mod/state-transition/pkg/core/core_test.go (1 hunks)
  • mod/state-transition/pkg/core/errors.go (1 hunks)
  • mod/state-transition/pkg/core/state_processor_staking.go (3 hunks)
  • mod/state-transition/pkg/core/state_processor_staking_test.go (2 hunks)
  • mod/state-transition/pkg/core/state_processor_validators.go (1 hunks)
  • mod/state-transition/pkg/core/types.go (1 hunks)
  • mod/state-transition/pkg/core/validation_deposits.go (2 hunks)
🧰 Additional context used
📓 Learnings (7)
mod/chain-spec/pkg/chain/chain_spec.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
mod/chain-spec/pkg/chain/data.go (3)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:150-151
Timestamp: 2024-11-12T11:12:56.773Z
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.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
mod/config/pkg/spec/base.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
mod/state-transition/pkg/core/state_processor_staking.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking.go:202-207
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking.go`, within the `addValidatorToRegistry` function, when a new validator's effective balance is less than or equal to the smallest current validator's effective balance, it's acceptable to mark the new validator as withdrawable and add it to the registry.
mod/state-transition/pkg/core/state_processor_staking_test.go (8)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:986-1000
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In the Go test file `mod/state-transition/pkg/core/state_processor_staking_test.go`, documenting helper functions like `generateTestExecutionAddress` is not necessary.
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:1002-1010
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, helper functions like `generateTestPK` do not need additional documentation.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:424-424
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In Go 1.23, when looping a specific number of times, it's acceptable to use `for idx := range N` where `N` is an integer.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:416-416
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, setting up the state via genesis is acceptable, and there is no need to set the state artificially in tests.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:588-588
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, using `for idx := range cs.GetValidatorSetCapSize()` is acceptable, even if `GetValidatorSetCapSize()` returns an integer value.
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.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:175-197
Timestamp: 2024-11-12T11:12:56.774Z
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.
mod/state-transition/pkg/core/state_processor_validators.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2143
File: mod/state-transition/pkg/core/state_processor_validators.go:44-44
Timestamp: 2024-11-28T14:42:16.342Z
Learning: In `state_processor_validators.go`, within the function `processValidatorsSetUpdates`, validators whose effective balance equals the ejection balance should not be considered active. Therefore, the comparison should use `>` instead of `>=` when comparing effective balances to the ejection balance.
mod/state-transition/pkg/core/validation_deposits.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-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.
🪛 golangci-lint (1.62.2)
mod/state-transition/pkg/core/state_processor_staking.go

[high] 170-170: G115: integer overflow conversion int -> uint32

(gosec)

mod/state-transition/pkg/core/validation_deposits.go

[high] 49-49: G115: integer overflow conversion int -> uint32

(gosec)


[high] 90-90: G115: integer overflow conversion int -> uint32

(gosec)

🔇 Additional comments (17)
mod/state-transition/pkg/core/state_processor_validators.go (1)

37-39: LGTM! Verify cap enforcement integration.

The switch to nextEpochValidatorSet aligns with the validator cap implementation. The timing (before slot update) is correct for next epoch preparation.

Let's verify the cap enforcement implementation:

mod/state-transition/pkg/core/errors.go (2)

26-28: 🛠️ Refactor suggestion

Improve naming consistency and error message clarity

  1. The variable name uses plural "Validators" while the error message uses singular "validator"
  2. The error message could be more descriptive

Apply these changes:

-// ErrValidatorSetCapHit is returned when we try and add a validator
-// that would breach validator set size cap.
-ErrHitValidatorsSetCap = errors.New("hit validator set size cap")
+// ErrHitValidatorSetCap is returned when attempting to add a validator
+// that would exceed the validator set size cap.
+ErrHitValidatorSetCap = errors.New("cannot add validator: maximum validator set size reached")

The changes:

  • Fix pluralization consistency
  • Make the error message more descriptive and actionable
  • Improve comment clarity by removing informal language ("we try and")

26-28: Verify error handling in related functions

Let's ensure this error is properly handled in all relevant validator management functions.

✅ Verification successful

Let me gather more specific information about the error handling implementation.


Let me check one more aspect to understand the validator set cap implementation and its error handling.


Error handling is properly implemented and verified

The error ErrHitValidatorsSetCap is correctly integrated into the codebase:

  • It's properly used in deposit validation logic with detailed error messages
  • The error is checked against the validator set cap via GetValidatorSetCap()
  • Implementation exists in both genesis and non-genesis deposit validation
  • Test coverage confirms the cap enforcement
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check usage of the new error variable and related error handling

# Find all references to the error variable
rg -l "ErrHitValidatorsSetCap"

# Find validator set cap related functions that should handle this error
ast-grep --pattern 'func $_($$) error {
  $$$
  validator$$$
  $$$
}'

Length of output: 195


Script:

#!/bin/bash
# Check the actual implementation where this error is used
rg -A 10 "ErrHitValidatorsSetCap" mod/state-transition/pkg/core/validation_deposits.go

# Find validator set cap related functions
ast-grep --pattern 'func $_($_) error {
  $$$
  if $$ >= GetValidatorSetCap() {
    $$$
  }
  $$$
}'

Length of output: 671


Script:

#!/bin/bash
# Find the validator set cap implementation and related checks
rg -A 5 "GetValidatorSetCap" 

# Look for the complete context of deposit validation
rg -B 5 -A 15 "validateNonGenesisDeposits" mod/state-transition/pkg/core/validation_deposits.go

Length of output: 9362

mod/chain-spec/pkg/chain/data.go (2)

158-161: LGTM! Documentation and type choice are consistent.

The new field follows the established patterns:

  • Documentation follows the minimal style used throughout the struct
  • Type uint32 is appropriate as the validator cap won't approach maxUint64
  • Placement under "Berachain Values" section is logical

158-161: Validation between ValidatorSetCap and ValidatorRegistryLimit is still needed.

While the note in the documentation indicates that ValidatorSetCap must be smaller than ValidatorRegistryLimit, runtime validation is still needed to enforce this constraint.

#!/bin/bash
# Description: Check if validation exists between ValidatorSetCap and ValidatorRegistryLimit
# Expected: No results as validation is pending
rg -A 5 "ValidatorSetCap.*ValidatorRegistryLimit|ValidatorRegistryLimit.*ValidatorSetCap" --type go
mod/state-transition/pkg/core/types.go (2)

254-255: 🧹 Nitpick (assertive)

Add documentation for the new method.

Please add a documentation comment for SetWithdrawableEpoch consistent with other methods in the interface. The comment should explain the purpose and any constraints on setting the withdrawable epoch.


254-255: Verify the implementation of SetWithdrawableEpoch.

The method addition looks good. Let's verify its implementation and usage across the codebase, particularly in relation to validator eviction logic.

✅ Verification successful

Let me analyze the implementation and usage based on the results:

  1. The implementation is found in mod/consensus-types/pkg/types/validator.go and it's a straightforward setter that assigns the epoch parameter to the WithdrawableEpoch field.

  2. The method is used in mod/state-transition/pkg/core/state_processor_staking.go in two scenarios:

    • During validator addition process
    • During validator eviction process where it's used to mark the smallest validator for withdrawal

Let's verify the test coverage and related validator state transition logic:


Let's verify the validator state transition logic specifically around withdrawal epoch setting:


Based on the comprehensive analysis of the implementation and usage of SetWithdrawableEpoch, I can now provide the final response:

The SetWithdrawableEpoch implementation is properly integrated and verified

The implementation is correctly used in the validator lifecycle:

  • It's properly implemented in mod/consensus-types/pkg/types/validator.go
  • It's used appropriately in state processor for validator eviction logic
  • The field is properly tested in validator tests
  • The method is used consistently with the validator state transition logic, particularly in staking operations where it's set during validator addition and eviction
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation and usage of SetWithdrawableEpoch

# Find the implementation
echo "=== Implementation of SetWithdrawableEpoch ==="
ast-grep --pattern 'func ($v $_) SetWithdrawableEpoch($_) {
  $$$
}'

# Find usages in validator eviction logic
echo -e "\n=== Usage in validator eviction logic ==="
rg -A 5 'SetWithdrawableEpoch'

# Find test coverage
echo -e "\n=== Test coverage ==="
rg -A 5 'TestSetWithdrawableEpoch|test.*SetWithdrawableEpoch'

Length of output: 2457


Script:

#!/bin/bash
# Check validator state transition tests and validator eviction tests
echo "=== Validator state transition and eviction tests ==="
rg -A 10 "func Test.*ValidatorEviction|func Test.*ValidatorState" 

# Check the validator struct definition and withdrawable epoch field
echo -e "\n=== Validator struct definition ==="
ast-grep --pattern 'type Validator struct {
  $$$
}'

# Check if there are any direct modifications to WithdrawableEpoch field
echo -e "\n=== Direct WithdrawableEpoch modifications ==="
rg -A 3 "WithdrawableEpoch\s*=\s*"

Length of output: 4100


Script:

#!/bin/bash
# Check state processor implementation for validator state transitions
echo "=== State processor validator transitions ==="
rg -A 15 "func.*ProcessValidatorExit|func.*ProcessValidatorActivation" 

# Check validator lifecycle tests
echo -e "\n=== Validator lifecycle tests ==="
rg -A 10 "Test.*ValidatorLifecycle|Test.*ValidatorTransition"

# Check validator withdrawal related constants and configs
echo -e "\n=== Validator withdrawal related constants ==="
rg "WithdrawableEpoch|MinWithdrawDelay|WithdrawalDelay" --type go

Length of output: 6931

mod/consensus-types/pkg/types/validator.go (1)

328-328: Add documentation comment for the method.

Please add a documentation comment explaining the purpose and usage of this method.

mod/chain-spec/pkg/chain/chain_spec.go (2)

202-204: 🧹 Nitpick (assertive)

Complete the documentation comment.

The documentation comment is incomplete and should be completed for clarity.

-       // GetValidatorSetCap retrieves the maximum number of
-       // validators allowed in the active set.
+       // GetValidatorSetCap retrieves the maximum number of validators
+       // allowed in the active set.

Likely invalid or redundant comment.


541-547: 🧹 Nitpick (assertive)

Fix documentation typo.

The documentation comment has a typo in the method name ("GetValidatorSetCapS").

-// GetValidatorSetCapS retrieves the maximum number of
-// validators allowed in the active set.
+// GetValidatorSetCap retrieves the maximum number of validators
+// allowed in the active set.

Likely invalid or redundant comment.

mod/state-transition/pkg/core/state_processor_staking.go (4)

197-197: 🧹 Nitpick (assertive)

Fix typo in comment: "exiting" should be "existing"

The comment should read "mark existing validator for eviction and add candidate" instead of "mark exiting validator..."


218-218: 🧹 Nitpick (assertive)

Consider renaming withdrawEpoch to nextEpoch for clarity

Renaming the variable to nextEpoch can improve readability, as it represents the next epoch.


189-189: 🧹 Nitpick (assertive)

Consider renaming val to newVal or candidateVal for clarity

Renaming val to newVal or candidateVal enhances clarity by indicating that it represents the new validator being added.


245-265: 🧹 Nitpick (assertive)

Optimize the smallest function for large validator sets

Sorting the entire validator list in the smallest function may be inefficient for large validator sets. Consider using a min-heap or maintaining a reference to the validator with the smallest effective balance to improve performance.

mod/state-transition/pkg/core/state_processor_staking_test.go (3)

590-590: Consider removing the TODO comment about artificial state setup

Based on previous discussions, setting up the state via genesis is acceptable, and there's no need to artificially set the state in tests. The TODO comment on line 590 can be removed.


725-725: Consider removing the TODO comment about artificial state setup

As previously agreed, setting up the state via genesis is acceptable, and there's no need to artificially set the state in tests. The TODO comment on line 725 can be removed.


711-713: 🧹 Nitpick (assertive)

Synchronize function comment with function name

The comment refers to TestTransitionHittingValidatorsCap, but the function is named TestTransitionHittingValidatorsCap_ExtraBig. Update the comment to match the function name to avoid confusion.

⛔ Skipped due to learnings
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.

mod/config/pkg/spec/base.go Show resolved Hide resolved
mod/config/pkg/spec/base.go Show resolved Hide resolved
mod/state-transition/pkg/core/validation_deposits.go Outdated Show resolved Hide resolved
mod/state-transition/pkg/core/core_test.go Show resolved Hide resolved
mod/consensus-types/pkg/types/validator.go Show resolved Hide resolved
mod/state-transition/pkg/core/state_processor_staking.go Outdated Show resolved Hide resolved
require.NoError(t, err)
require.Equal(t, extraValDeposit.Pubkey, extraVal.Pubkey)
require.Equal(t, math.Slot(1), extraVal.WithdrawableEpoch)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Verify that WithdrawableEpoch is set correctly

In the test, WithdrawableEpoch is set to math.Slot(1). According to the consensus specifications, when a validator is scheduled for withdrawal, WithdrawableEpoch should be set to currentEpoch + cs.MinValidatorWithdrawabilityDelay(). Please verify that setting it to math.Slot(1) is appropriate.

smallVal1, err := st.ValidatorByIndex(smallVal1Idx)
require.NoError(t, err)
require.Equal(t, math.Slot(1), smallVal1.WithdrawableEpoch)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Verify that WithdrawableEpoch is set correctly

Similar to the previous comment, on line 963, WithdrawableEpoch is set to math.Slot(1) for the validator being scheduled for withdrawal. Ensure that this aligns with the consensus specifications, which typically require WithdrawableEpoch to be currentEpoch + cs.MinValidatorWithdrawabilityDelay().

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between e8db0d9 and 4774160.

📒 Files selected for processing (1)
  • mod/config/pkg/spec/boonet.go (1 hunks)
🔇 Additional comments (1)
mod/config/pkg/spec/boonet.go (1)

59-60: Please clarify the validator set cap value and its relationships

The cap is set to 46, which matches the "at least 46 validators" mentioned in the MaxValidatorsPerWithdrawalsSweep comment. This raises some concerns:

  1. If 46 is the minimum expected validators, using it as a cap might restrict system growth
  2. The relationship between MaxValidatorsPerWithdrawalsSweep (43) and ValidatorSetCap (46) needs explanation

Let's check if these values are referenced elsewhere:

✅ Verification successful

The validator set cap value is correctly set based on system requirements

The code analysis reveals that the relationship between MaxValidatorsPerWithdrawalsSweep (43) and ValidatorSetCap (46) is intentionally designed:

  1. The comment explicitly states that 46 is the expected minimum number of validators in the total set
  2. 43 is chosen as a prime number smaller than the minimum validators (46) for the withdrawal sweep
  3. The cap of 46 is not a restrictive maximum but rather ensures the minimum required validators are present
  4. This setup allows for efficient withdrawal processing while maintaining the necessary validator set size

The values are used consistently across the codebase in validator management, state transitions, and withdrawal processing.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for references to these specific numbers to understand their significance
echo "Searching for references to 46 validators:"
rg -A 2 "46.*validator"

echo "Searching for references to 43 validators:"
rg -A 2 "43.*validator"

Length of output: 241


Script:

#!/bin/bash
# Let's try a broader search for these numbers and related context
echo "Searching for '46' references:"
rg -B 2 -A 2 '\b46\b'

echo -e "\nSearching for '43' references:"
rg -B 2 -A 2 '\b43\b'

echo -e "\nSearching for ValidatorSetCap references:"
rg -B 2 -A 2 'ValidatorSetCap'

echo -e "\nSearching for MaxValidatorsPerWithdrawalsSweep references:"
rg -B 2 -A 2 'MaxValidatorsPerWithdrawalsSweep'

Length of output: 26518

@@ -56,5 +56,7 @@ func BoonetChainSpec() (chain.Spec[
// TODO: Determine correct value for boonet upgrade.
testnetSpec.MaxValidatorsPerWithdrawalsSweep = 43

testnetSpec.ValidatorSetCap = 46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add documentation for ValidatorSetCap

Please add a comment explaining:

  1. The purpose of ValidatorSetCap
  2. Why 46 was chosen as the specific value
  3. Any implications for validator selection and eviction
+       // ValidatorSetCap defines the maximum number of validators allowed in the validator set.
+       // When this cap is reached, only validators with higher stakes than the current minimum
+       // stake in the set will be admitted, causing the lowest staked validator to be evicted.
        testnetSpec.ValidatorSetCap = 46
📝 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.

Suggested change
testnetSpec.ValidatorSetCap = 46
// ValidatorSetCap defines the maximum number of validators allowed in the validator set.
// When this cap is reached, only validators with higher stakes than the current minimum
// stake in the set will be admitted, causing the lowest staked validator to be evicted.
testnetSpec.ValidatorSetCap = 46

@abi87 abi87 requested a review from calbera November 29, 2024 11:34
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between 4774160 and 78d6429.

📒 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/config/pkg/spec/boonet.go (1 hunks)
  • mod/state-transition/pkg/core/state_processor_staking.go (3 hunks)
  • mod/state-transition/pkg/core/state_processor_staking_test.go (2 hunks)
🧰 Additional context used
📓 Learnings (4)
mod/chain-spec/pkg/chain/chain_spec.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
mod/chain-spec/pkg/chain/data.go (3)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:150-151
Timestamp: 2024-11-12T11:12:56.773Z
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.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
mod/state-transition/pkg/core/state_processor_staking.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking.go:202-207
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking.go`, within the `addValidatorToRegistry` function, when a new validator's effective balance is less than or equal to the smallest current validator's effective balance, it's acceptable to mark the new validator as withdrawable and add it to the registry.
mod/state-transition/pkg/core/state_processor_staking_test.go (8)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:986-1000
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In the Go test file `mod/state-transition/pkg/core/state_processor_staking_test.go`, documenting helper functions like `generateTestExecutionAddress` is not necessary.
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:1002-1010
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, helper functions like `generateTestPK` do not need additional documentation.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:424-424
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In Go 1.23, when looping a specific number of times, it's acceptable to use `for idx := range N` where `N` is an integer.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:416-416
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, setting up the state via genesis is acceptable, and there is no need to set the state artificially in tests.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:588-588
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, using `for idx := range cs.GetValidatorSetCapSize()` is acceptable, even if `GetValidatorSetCapSize()` returns an integer value.
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.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:175-197
Timestamp: 2024-11-12T11:12:56.774Z
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.
🪛 golangci-lint (1.62.2)
mod/state-transition/pkg/core/state_processor_staking.go

[high] 170-170: G115: integer overflow conversion int -> uint32

(gosec)

🔇 Additional comments (11)
mod/config/pkg/spec/boonet.go (2)

52-54: Add documentation for ValidatorSetCap

The purpose and implications of this configuration value should be documented.

+       // ValidatorSetCap defines the maximum number of validators allowed in the validator set.
+       // When this cap is reached, only validators with higher stakes than the current minimum
+       // stake in the set will be admitted, causing the lowest staked validator to be evicted.
        testnetSpec.ValidatorSetCap = 46

52-56: 🧹 Nitpick (assertive)

Verify the relationship between validator caps

The comment explains that MaxValidatorsPerWithdrawalsSweep (43) is intentionally smaller than ValidatorSetCap (46), but both values have TODO comments indicating they need review. Please ensure these values are properly coordinated when finalizing them for the boonet upgrade.

Consider documenting:

  1. Why MaxValidatorsPerWithdrawalsSweep needs to be smaller than ValidatorSetCap
  2. The implications of these specific numbers on network performance and security
  3. The rationale for using a prime number for MaxValidatorsPerWithdrawalsSweep

Run this script to verify the relationship between these values across the codebase:

✅ Verification successful

Based on the search results, I can now generate a final response as there's clear evidence about the relationship between these values and their purpose in the codebase.

The relationship between validator caps is well-defined but needs documentation

The codebase reveals that MaxValidatorsPerWithdrawalsSweep must be prime relative to ValidatorSetCap to ensure all validators are eventually visited during withdrawal sweeps. This relationship is documented in mod/chain-spec/pkg/chain/data.go but not in the boonet configuration.

  • Add a comment in mod/config/pkg/spec/boonet.go explaining that 43 was chosen not just because it's smaller than 46, but specifically because it's prime relative to the validator set cap to ensure complete validator coverage during withdrawal sweeps
  • Consider documenting that ValidatorSetCap must be smaller than ValidatorRegistryLimit (as noted in chain/data.go)
  • The values are used in withdrawal processing (state_processor_withdrawals.go) and deposit validation (validation_deposits.go), so their coordination is critical for network operation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any other validator-related caps or limits that might need coordination

# Search for validator-related constants and configurations
rg -A 2 "ValidatorSetCap|MaxValidatorsPerWithdrawalsSweep" 

# Look for validator-related functions that might be affected by these caps
ast-grep --pattern 'func $_($$$ validator $$$) {
  $$$
}'

Length of output: 9452

mod/chain-spec/pkg/chain/data.go (1)

159-162: LGTM! Implementation aligns with requirements.

The ValidatorSetCap field implementation:

  • Uses appropriate type (uint32) as the validator cap won't approach maxUint64
  • Follows the existing documentation pattern
  • Documents the constraint with ValidatorRegistryLimit
mod/state-transition/pkg/core/state_processor_staking.go (5)

24-25: Imports added appropriately

The addition of "bytes" and "slices" packages is necessary for byte comparison and slice manipulation used in the code below.


151-207: Validator set cap enforcement implemented correctly

The addValidatorToRegistry function effectively enforces the validator set cap by comparing the candidate validator's effective balance with the lowest in the current set and handling additions or evictions accordingly. This aligns with the PR objectives.

🧰 Tools
🪛 golangci-lint (1.62.2)

[high] 170-170: G115: integer overflow conversion int -> uint32

(gosec)


170-170: Fix potential integer overflow when casting to uint32

Casting len(nextEpochVals) from int to uint32 may lead to an integer overflow if the validator set is large. Consider casting sp.cs.GetValidatorSetCap() to int instead:

-if uint32(len(nextEpochVals)) < sp.cs.GetValidatorSetCap() {
+if len(nextEpochVals) < int(sp.cs.GetValidatorSetCap()) {
🧰 Tools
🪛 golangci-lint (1.62.2)

[high] 170-170: G115: integer overflow conversion int -> uint32

(gosec)


209-236: Efficient estimation of the next epoch's validator set

The nextEpochValidatorSet function correctly filters out validators with low effective balances and those scheduled for withdrawal in the next epoch, providing an accurate set for upcoming operations.


238-266: Consider optimizing the lowestStakeVal function

The function currently sorts the entire validator slice to find the lowest stake validator, which may be inefficient for large validator sets. Consider iterating through the slice to track the minimum value without sorting:

func (*StateProcessor[...]) lowestStakeVal(currentVals []ValidatorT) (ValidatorT, error) {
    if len(currentVals) == 0 {
        return ValidatorT{}, errors.New("no validators available")
    }
    minVal := currentVals[0]
    for _, val := range currentVals[1:] {
        if val.GetEffectiveBalance() < minVal.GetEffectiveBalance() ||
           (val.GetEffectiveBalance() == minVal.GetEffectiveBalance() && bytes.Compare(val.GetPubkey()[:], minVal.GetPubkey()[:]) < 0) {
            minVal = val
        }
    }
    return minVal, nil
}

This approach reduces the time complexity from O(n log n) to O(n).

mod/state-transition/pkg/core/state_processor_staking_test.go (3)

656-656: ⚠️ Potential issue

Verify that WithdrawableEpoch is set correctly

On line 656, WithdrawableEpoch is set to math.Slot(1) when scheduling the withdrawal of the validator. According to the consensus specifications, WithdrawableEpoch should be set to currentEpoch + cs.MinValidatorWithdrawabilityDelay(). Setting it to math.Slot(1) may not reflect the correct protocol behavior.

Please update the test to calculate expectedWithdrawableEpoch correctly:

-	require.Equal(t, math.Slot(1), extraVal.WithdrawableEpoch)
+	expectedWithdrawableEpoch := st.CurrentEpoch() + cs.MinValidatorWithdrawabilityDelay()
+	require.Equal(t, expectedWithdrawableEpoch, extraVal.WithdrawableEpoch)

963-963: ⚠️ Potential issue

Verify that WithdrawableEpoch is set correctly

On line 963, WithdrawableEpoch is set to math.Slot(1) for the validator scheduled for withdrawal. As per the consensus specifications, WithdrawableEpoch should be currentEpoch + cs.MinValidatorWithdrawabilityDelay(). Please ensure that the test reflects the correct value.

Consider updating the test with the correct WithdrawableEpoch calculation:

-	require.Equal(t, math.Slot(1), smallVal1.WithdrawableEpoch)
+	expectedWithdrawableEpoch := st.CurrentEpoch() + cs.MinValidatorWithdrawabilityDelay()
+	require.Equal(t, expectedWithdrawableEpoch, smallVal1.WithdrawableEpoch)

663-663: 🧹 Nitpick (assertive)

Fix typo in comment

The comment on line 663 has a typo: "enqueuued" should be "enqueued".

Apply this diff to correct the typo:

-	// for rejected validator are enqueuued then
+	// for rejected validators are enqueued then

Likely invalid or redundant comment.

mod/chain-spec/pkg/chain/data.go Outdated Show resolved Hide resolved
mod/chain-spec/pkg/chain/chain_spec.go Outdated Show resolved Hide resolved
mod/chain-spec/pkg/chain/chain_spec.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between 78d6429 and e8b4dfd.

📒 Files selected for processing (6)
  • mod/chain-spec/pkg/chain/chain_spec.go (2 hunks)
  • mod/chain-spec/pkg/chain/data.go (1 hunks)
  • mod/config/pkg/spec/base.go (1 hunks)
  • mod/config/pkg/spec/boonet.go (1 hunks)
  • mod/state-transition/pkg/core/errors.go (1 hunks)
  • mod/state-transition/pkg/core/state_processor_staking_test.go (2 hunks)
🧰 Additional context used
📓 Learnings (4)
mod/config/pkg/spec/base.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
mod/chain-spec/pkg/chain/data.go (3)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:150-151
Timestamp: 2024-11-12T11:12:56.773Z
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.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
mod/chain-spec/pkg/chain/chain_spec.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
mod/state-transition/pkg/core/state_processor_staking_test.go (8)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:986-1000
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In the Go test file `mod/state-transition/pkg/core/state_processor_staking_test.go`, documenting helper functions like `generateTestExecutionAddress` is not necessary.
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-424
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In Go 1.23, when looping a specific number of times, it's acceptable to use `for idx := range N` where `N` is an integer.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:416-416
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, setting up the state via genesis is acceptable, and there is no need to set the state artificially in tests.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:588-588
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, using `for idx := range cs.GetValidatorSetCapSize()` is acceptable, even if `GetValidatorSetCapSize()` returns an integer value.
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.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:175-197
Timestamp: 2024-11-12T11:12:56.774Z
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#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:1002-1010
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, helper functions like `generateTestPK` do not need additional documentation.
🔇 Additional comments (16)
mod/state-transition/pkg/core/state_processor_staking_test.go (9)

1117-1125: No need to document helper functions

The helper functions generateTestExecutionAddress and generateTestPK are self-explanatory and used within the test file. Adding documentation is not necessary in this context.


578-581: 🧹 Nitpick (assertive)

Update function comment to match function name

The comment for TestTransitionHittingValidatorsCap_ExtraSmall refers to TestTransitionHittingValidatorsCap. Please update the comment to match the function name for consistency and clarity.


591-591: 🧹 Nitpick (assertive)

Remove unnecessary TODO comment

The TODO comment suggests considering setting the state artificially instead of via genesis. Based on previous discussions, setting up the state via genesis is acceptable and preferred in this context. You can remove this TODO to clean up the code.


598-599: ⚠️ Potential issue

Verify usage of range over integer

Using range over an integer value returned by cs.GetValidatorSetCap() may not work as expected since range is typically used with slices, arrays, maps, or channels. Consider using a traditional for loop.

Apply this diff to fix the loop:

-for idx := range cs.GetValidatorSetCap() {
+for idx := uint64(0); idx < cs.GetValidatorSetCap(); idx++ {

657-657: ⚠️ Potential issue

Ensure WithdrawableEpoch is set correctly

Setting WithdrawableEpoch to math.Slot(1) may not reflect the correct protocol behavior. According to the consensus specifications, when a validator is scheduled for withdrawal, WithdrawableEpoch should be set to currentEpoch + cs.MinValidatorWithdrawabilityDelay(). This ensures that the validator remains active for the required delay period before withdrawal.

Apply this diff to set and verify the WithdrawableEpoch correctly:

-require.Equal(t, math.Slot(1), extraVal.WithdrawableEpoch)
+expectedWithdrawableEpoch := st.CurrentEpoch() + cs.MinValidatorWithdrawabilityDelay()
+require.Equal(t, expectedWithdrawableEpoch, extraVal.WithdrawableEpoch)

664-664: 🧹 Nitpick (assertive)

Fix typo in comment

Change "enqueuued" to "enqueued" in the comment on line 664 for better readability.


800-800: ⚠️ Potential issue

Ensure WithdrawableEpoch is set correctly for evicted validator

In line 800, setting smallVal.WithdrawableEpoch to math.Slot(1) may not comply with the consensus specifications. It should be set to currentEpoch + cs.MinValidatorWithdrawabilityDelay() when scheduling a validator for withdrawal.

Apply this diff to correct the WithdrawableEpoch:

-require.Equal(t, math.Slot(1), smallVal.WithdrawableEpoch)
+expectedWithdrawableEpoch := st.CurrentEpoch() + cs.MinValidatorWithdrawabilityDelay()
+require.Equal(t, expectedWithdrawableEpoch, smallVal.WithdrawableEpoch)

963-964: ⚠️ Potential issue

Verify WithdrawableEpoch setting for scheduled withdrawal

At line 963, smallVal1.WithdrawableEpoch is set to math.Slot(1). As per specifications, it should be currentEpoch + cs.MinValidatorWithdrawabilityDelay(). This ensures protocol compliance and accurate test results.

Apply this diff:

-require.Equal(t, math.Slot(1), smallVal1.WithdrawableEpoch)
+expectedWithdrawableEpoch := st.CurrentEpoch() + cs.MinValidatorWithdrawabilityDelay()
+require.Equal(t, expectedWithdrawableEpoch, smallVal1.WithdrawableEpoch)

1008-1009: ⚠️ Potential issue

Ensure correct WithdrawableEpoch for second evicted validator

Similarly, in lines 1008-1009, smallVal2.WithdrawableEpoch should be set to currentEpoch + cs.MinValidatorWithdrawabilityDelay() instead of math.Slot(1) to align with the consensus protocol.

Apply this diff:

-require.Equal(t, math.Slot(1), smallVal2.WithdrawableEpoch)
+expectedWithdrawableEpoch := st.CurrentEpoch() + cs.MinValidatorWithdrawabilityDelay()
+require.Equal(t, expectedWithdrawableEpoch, smallVal2.WithdrawableEpoch)
mod/config/pkg/spec/boonet.go (1)

52-53: 🧹 Nitpick (assertive)

Add documentation for ValidatorSetCap

Please add a comment explaining the purpose of ValidatorSetCap and why the value 256 was chosen. This aids in understanding the design decisions and implications for the validator set.

Apply this diff:

+    // ValidatorSetCap defines the maximum number of validators allowed in the validator set.
+    // The value 256 is chosen based on network parameters and desired decentralization level.
     boonetSpec.ValidatorSetCap = 256
mod/config/pkg/spec/base.go (2)

138-139: 🧹 Nitpick (assertive)

Group validator-related constants for better organization

Consider moving ValidatorSetCap near other validator-related constants like ValidatorRegistryLimit to enhance code readability and organization.


139-139: 🧹 Nitpick (assertive)

Provide documentation for ValidatorSetCap

Adding a comment explaining the purpose of ValidatorSetCap will improve code clarity.

Apply this diff:

+   // ValidatorSetCap defines the maximum number of validators that can be active in an epoch.
    ValidatorSetCap: 256,
mod/chain-spec/pkg/chain/data.go (2)

166-169: 🧹 Nitpick (assertive)

Add comprehensive documentation for ValidatorSetCap

Please provide detailed comments explaining the purpose of ValidatorSetCap, any constraints, and its relationship with other parameters. This will aid in maintaining and configuring the system correctly.

Apply this diff:

     // ValidatorSetCap is the maximum number of validators that can be active
     // for a given epoch.
     // Note: ValidatorSetCap must be smaller than ValidatorRegistryLimit.
+    // This cap ensures efficient operation and helps maintain network performance.
     ValidatorSetCap uint32 `mapstructure:"validator-set-cap-size"`

169-169: 🧹 Nitpick (assertive)

Consider type consistency for ValidatorSetCap

All other numeric configuration values in SpecData use uint64. For consistency, consider changing ValidatorSetCap to uint64, unless there is a specific reason for using uint32.

mod/chain-spec/pkg/chain/chain_spec.go (2)

552-558: 🧹 Nitpick (assertive)

Remove redundant documentation.

The implementation's documentation is redundant since it's already documented in the interface. This follows the pattern of other getter methods in this file.

-// GetValidatorSetCap retrieves the maximum number of
-// validators allowed in the active set.
 func (c chainSpec[
 	DomainTypeT, EpochT, ExecutionAddressT, SlotT, CometBFTConfigT,
 ]) GetValidatorSetCap() uint32 {
 	return c.Data.ValidatorSetCap
 }

Likely invalid or redundant comment.


206-208: 🧹 Nitpick (assertive)

Fix documentation style for consistency.

The documentation comment should be on a single line for better readability and consistency with other interface methods.

-	// GetValidatorSetCap retrieves the maximum number of
-	// validators allowed in the active set.
+	// GetValidatorSetCap retrieves the maximum number of validators allowed in the active set.
⛔ Skipped due to learnings
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.

mod/state-transition/pkg/core/errors.go Outdated Show resolved Hide resolved
@@ -203,6 +203,10 @@ type Spec[

// Berachain Values

// GetValidatorSetCap retrieves the maximum number of
// validators allowed in the active set.
GetValidatorSetCap() uint32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit can rename to ValidatorSetCap() for consistency with other value retrieval functions. Only the retrieval for the entire config of Comet GetCometBFTConfigForSlot has a Get prefix for some reason.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between e8b4dfd and 4f90bfe.

📒 Files selected for processing (1)
  • mod/config/pkg/spec/boonet.go (1 hunks)
🔇 Additional comments (1)
mod/config/pkg/spec/boonet.go (1)

50-50: 🧹 Nitpick (assertive)

Add documentation for ValidatorSetCap

Please add a comment explaining the purpose and implications of ValidatorSetCap.

+       // ValidatorSetCap defines the maximum number of validators allowed in the validator set.
+       // When this cap is reached, only validators with higher stakes than the current minimum
+       // stake in the set will be admitted, causing the lowest staked validator to be evicted.
        boonetSpec.ValidatorSetCap = 256

Comment on lines 53 to 55
// validators in the total validators set. We choose a prime number smaller
// than the minimum amount of total validators possible.
//
// TODO: Determine correct value for boonet upgrade.
boonetSpec.MaxValidatorsPerWithdrawalsSweepPostUpgrade = 43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update the comment to reflect current validator set size

The comment references expecting "at least 46 validators" which seems outdated now that the ValidatorSetCap is 256. Please update the comment to accurately reflect the current validator set size expectations.

-       // MaxValidatorsPerWithdrawalsSweep is 43 because we expect at least 46
-       // validators in the total validators set. We choose a prime number smaller
-       // than the minimum amount of total validators possible.
+       // MaxValidatorsPerWithdrawalsSweepPostUpgrade is set to 43 (a prime number)
+       // to ensure efficient processing of withdrawals while staying below the
+       // validator set cap of 256.
        boonetSpec.MaxValidatorsPerWithdrawalsSweepPostUpgrade = 43
📝 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.

Suggested change
// validators in the total validators set. We choose a prime number smaller
// than the minimum amount of total validators possible.
//
// TODO: Determine correct value for boonet upgrade.
boonetSpec.MaxValidatorsPerWithdrawalsSweepPostUpgrade = 43
// MaxValidatorsPerWithdrawalsSweepPostUpgrade is set to 43 (a prime number)
// to ensure efficient processing of withdrawals while staying below the
// validator set cap of 256.
boonetSpec.MaxValidatorsPerWithdrawalsSweepPostUpgrade = 43

mod/config/pkg/spec/boonet.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between 4f90bfe and 39b83ca.

📒 Files selected for processing (7)
  • mod/chain-spec/pkg/chain/chain_spec.go (3 hunks)
  • mod/chain-spec/pkg/chain/data.go (1 hunks)
  • mod/chain-spec/pkg/chain/errors.go (1 hunks)
  • mod/config/pkg/spec/boonet.go (1 hunks)
  • mod/state-transition/pkg/core/state_processor_staking.go (3 hunks)
  • mod/state-transition/pkg/core/state_processor_staking_test.go (2 hunks)
  • mod/state-transition/pkg/core/validation_deposits.go (2 hunks)
🧰 Additional context used
📓 Learnings (5)
mod/chain-spec/pkg/chain/data.go (3)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:150-151
Timestamp: 2024-11-12T11:12:56.773Z
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.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
mod/chain-spec/pkg/chain/chain_spec.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/chain_spec.go:515-521
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/chain_spec.go`, the `GetValidatorSetCapSize` method should return `uint32` because it sufficiently covers our validator set size; there is no need to use `uint64`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:151-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, the `ValidatorSetCapSize` field in the `SpecData` struct should use `uint32` instead of `uint64`, as the validator cap will not approach `maxUint64`.
mod/state-transition/pkg/core/validation_deposits.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/validation_deposits.go:49-49
Timestamp: 2024-11-29T11:21:04.160Z
Learning: In `mod/state-transition/pkg/core/validation_deposits.go`, when converting `len(deposits)` to `uint32` in the condition `if uint32(len(deposits)) > sp.cs.GetValidatorSetCap()`, it's acceptable not to add an explicit bounds check for potential integer overflow.
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.
mod/state-transition/pkg/core/state_processor_staking.go (3)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking.go:170-170
Timestamp: 2024-11-29T11:56:33.420Z
Learning: Casting `len(nextEpochVals)` from `int` to `uint32` is safe in `mod/state-transition/pkg/core/state_processor_staking.go` because `len(nextEpochVals)` will not exceed the maximum value representable by `uint32`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_validators.go:39-39
Timestamp: 2024-11-29T11:59:15.967Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking.go`, within the `nextEpochValidatorSet` function, validators with an `EffectiveBalance` equal to the `EjectionBalance` are intentionally excluded from the active validator set.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking.go:202-207
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking.go`, within the `addValidatorToRegistry` function, when a new validator's effective balance is less than or equal to the smallest current validator's effective balance, it's acceptable to mark the new validator as withdrawable and add it to the registry.
mod/state-transition/pkg/core/state_processor_staking_test.go (8)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:986-1000
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In the Go test file `mod/state-transition/pkg/core/state_processor_staking_test.go`, documenting helper functions like `generateTestExecutionAddress` is not necessary.
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-424
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In Go 1.23, when looping a specific number of times, it's acceptable to use `for idx := range N` where `N` is an integer.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:416-416
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, setting up the state via genesis is acceptable, and there is no need to set the state artificially in tests.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:588-588
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, using `for idx := range cs.GetValidatorSetCapSize()` is acceptable, even if `GetValidatorSetCapSize()` returns an integer value.
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.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:175-197
Timestamp: 2024-11-12T11:12:56.774Z
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#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:1002-1010
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, helper functions like `generateTestPK` do not need additional documentation.
🪛 golangci-lint (1.62.2)
mod/state-transition/pkg/core/state_processor_staking_test.go

612-612: unnecessary conversion

(unconvert)


747-747: unnecessary conversion

(unconvert)


908-908: unnecessary conversion

(unconvert)

🔇 Additional comments (4)
mod/state-transition/pkg/core/state_processor_staking.go (1)

246-265: 🧹 Nitpick (assertive)

Optimize lowestStakeVal function to improve performance

The lowestStakeVal function sorts the entire list of validators to find the one with the smallest effective balance. This approach can be inefficient for large validator sets. Consider iterating through the validators to find the minimum without sorting.

Here's a suggested refactor:

 func (*StateProcessor[
 	_, _, _, _, _, _, _, _, _, _, _, _, ValidatorT, _, _, _, _,
 ]) lowestStakeVal(currentVals []ValidatorT) (
 	ValidatorT,
 	error,
 ) {
-	// TODO: consider heapifying slice instead. We only care about the smallest
-	slices.SortFunc(currentVals, func(lhs, rhs ValidatorT) int {
-		var (
-			val1Stake = lhs.GetEffectiveBalance()
-			val2Stake = rhs.GetEffectiveBalance()
-		)
-		switch {
-		case val1Stake < val2Stake:
-			return -1
-		case val1Stake > val2Stake:
-			return 1
-		default:
-			// validators pks are guaranteed to be different
-			var (
-				val1Pk = lhs.GetPubkey()
-				val2Pk = rhs.GetPubkey()
-			)
-			return bytes.Compare(val1Pk[:], val2Pk[:])
-		}
-	})
-	return currentVals[0], nil
+	if len(currentVals) == 0 {
+		var emptyVal ValidatorT
+		return emptyVal, fmt.Errorf("no validators provided")
+	}
+	minVal := currentVals[0]
+	for _, val := range currentVals[1:] {
+		if val.GetEffectiveBalance() < minVal.GetEffectiveBalance() ||
+			(val.GetEffectiveBalance() == minVal.GetEffectiveBalance() &&
+				bytes.Compare(val.GetPubkey()[:], minVal.GetPubkey()[:]) < 0) {
+			minVal = val
+		}
+	}
+	return minVal, nil
 }

Likely invalid or redundant comment.

mod/state-transition/pkg/core/state_processor_staking_test.go (1)

800-800: Verify WithdrawableEpoch is set correctly in tests

In the test case, WithdrawableEpoch is set to math.Slot(1). According to consensus specifications, when scheduling a validator for withdrawal, WithdrawableEpoch should be set to currentEpoch + cs.MinValidatorWithdrawabilityDelay(). Please verify that setting it to math.Slot(1) aligns with the expected behavior.

mod/state-transition/pkg/core/validation_deposits.go (1)

50-54: 🧹 Nitpick (assertive)

Improve error message clarity

The error message in the validateGenesisDeposits function could be more descriptive to aid debugging. Consider rephrasing it to provide clearer context.

Suggested change:

 return fmt.Errorf("validator set cap exceeded: cap is %d, but deposits count is %d: %w",
 	sp.cs.ValidatorSetCap(),
 	len(deposits),
 	ErrHitValidatorsSetCap,
 )
⛔ Skipped due to learnings
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/validation_deposits.go:49-49
Timestamp: 2024-11-29T11:21:04.160Z
Learning: In `mod/state-transition/pkg/core/validation_deposits.go`, when converting `len(deposits)` to `uint32` in the condition `if uint32(len(deposits)) > sp.cs.GetValidatorSetCap()`, it's acceptable not to add an explicit bounds check for potential integer overflow.
mod/chain-spec/pkg/chain/chain_spec.go (1)

261-263: LGTM!

The validation logic correctly ensures that the validator set cap doesn't exceed the validator registry limit.

Comment on lines +578 to +581
// TestTransitionHittingValidatorsCap shows that the extra
// validator added when validators set is at cap gets never activated
// and its deposit is returned at after next epoch starts.
func TestTransitionHittingValidatorsCap_ExtraSmall(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Update function comment to match the function name

The comment refers to TestTransitionHittingValidatorsCap, but the function is named TestTransitionHittingValidatorsCap_ExtraSmall. Please update the comment to reflect the correct function name for clarity.

mod/chain-spec/pkg/chain/errors.go Show resolved Hide resolved
mod/config/pkg/spec/boonet.go Show resolved Hide resolved
mod/chain-spec/pkg/chain/data.go Show resolved Hide resolved
mod/chain-spec/pkg/chain/chain_spec.go Show resolved Hide resolved
mod/chain-spec/pkg/chain/chain_spec.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between 39b83ca and 309156952cfb4b38f35e1849b8cb059b795ba8e3.

📒 Files selected for processing (1)
  • mod/state-transition/pkg/core/state_processor_staking_test.go (2 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/state-transition/pkg/core/state_processor_staking_test.go (8)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:986-1000
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In the Go test file `mod/state-transition/pkg/core/state_processor_staking_test.go`, documenting helper functions like `generateTestExecutionAddress` is not necessary.
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-424
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In Go 1.23, when looping a specific number of times, it's acceptable to use `for idx := range N` where `N` is an integer.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:416-416
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, setting up the state via genesis is acceptable, and there is no need to set the state artificially in tests.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:588-588
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, using `for idx := range cs.GetValidatorSetCapSize()` is acceptable, even if `GetValidatorSetCapSize()` returns an integer value.
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.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:175-197
Timestamp: 2024-11-12T11:12:56.774Z
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#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:1002-1010
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, helper functions like `generateTestPK` do not need additional documentation.
🔇 Additional comments (5)
mod/state-transition/pkg/core/state_processor_staking_test.go (5)

1117-1125: No need to document helper functions

Noting that you chose not to add documentation for generateTestPK and generateTestExecutionAddress, which is acceptable as per previous decisions.


578-581: 🧹 Nitpick (assertive)

Update function comment to match function name

The function comment refers to TestTransitionHittingValidatorsCap, but the function is named TestTransitionHittingValidatorsCap_ExtraSmall. Please update the comment to reflect the correct function name for clarity.

⛔ Skipped due to learnings
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.
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#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:175-197
Timestamp: 2024-11-12T11:12:56.774Z
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#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:145-175
Timestamp: 2024-11-12T11:12:56.773Z
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`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:1002-1010
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, helper functions like `generateTestPK` do not need additional documentation.
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.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/core_test.go:304-313
Timestamp: 2024-12-02T17:25:33.673Z
Learning: The user prefers not to add the `setupStateWithValidators` test utility function in `mod/state-transition/pkg/core/core_test.go` for testing the validator size cap feature, as they deem it unnecessary.

801-801: ⚠️ Potential issue

Verify that WithdrawableEpoch is set correctly

When a validator is scheduled for withdrawal, WithdrawableEpoch should be set to currentEpoch + cs.MinValidatorWithdrawabilityDelay(). Please ensure that setting it to math.Slot(1) aligns with the consensus specifications.

Apply this diff to set WithdrawableEpoch correctly:

-require.Equal(t, math.Slot(1), smallVal.WithdrawableEpoch)
+expectedWithdrawableEpoch := st.CurrentEpoch() + cs.MinValidatorWithdrawabilityDelay()
+require.Equal(t, expectedWithdrawableEpoch, smallVal.WithdrawableEpoch)

Likely invalid or redundant comment.


963-963: ⚠️ Potential issue

Verify that WithdrawableEpoch is set correctly

For the validator smallVal1, WithdrawableEpoch is set to math.Slot(1). According to consensus specifications, it should be currentEpoch + cs.MinValidatorWithdrawabilityDelay() when scheduling for withdrawal. Please verify this value.

Apply this diff to set WithdrawableEpoch correctly:

-require.Equal(t, math.Slot(1), smallVal1.WithdrawableEpoch)
+expectedWithdrawableEpoch := st.CurrentEpoch() + cs.MinValidatorWithdrawabilityDelay()
+require.Equal(t, expectedWithdrawableEpoch, smallVal1.WithdrawableEpoch)

Likely invalid or redundant comment.


657-657: ⚠️ Potential issue

Verify that WithdrawableEpoch is set correctly

When scheduling a validator for withdrawal, WithdrawableEpoch should be set to currentEpoch + cs.MinValidatorWithdrawabilityDelay(). Setting it to math.Slot(1) may not reflect the correct protocol behavior. Please verify that this is intended.

Apply this diff to set WithdrawableEpoch correctly:

-require.Equal(t, math.Slot(1), extraVal.WithdrawableEpoch)
+expectedWithdrawableEpoch := st.CurrentEpoch() + cs.MinValidatorWithdrawabilityDelay()
+require.Equal(t, expectedWithdrawableEpoch, extraVal.WithdrawableEpoch)

Likely invalid or redundant comment.

)

// STEP 0: Setup genesis with GetValidatorSetCap validators
// TODO: consider instead setting state artificially
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Remove TODO comment if no longer applicable

The TODO comment suggests considering setting the state artificially. Since the current setup via genesis is acceptable, you may remove the TODO comment to avoid confusion.

require.Equal(t, extraValDeposit.Amount, extraValBalance)

// STEP 2: move the chain to the next epoch and show withdrawals
// for rejected validator are enqueuued then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Fix typo in comment

Change "enqueuued" to "enqueued" in the comment for better clarity.

Comment on lines +711 to +714
// TestTransitionHittingValidatorsCap shows that if the extra
// validator added when validators set is at cap improves amount staked
// an existing validator is removed at the beginning of next epoch.
func TestTransitionHittingValidatorsCap_ExtraBig(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Synchronize function comment with function name

The function comment refers to TestTransitionHittingValidatorsCap, but the function is named TestTransitionHittingValidatorsCap_ExtraBig. Please update the comment to match the function name to maintain consistency.

// show that eviction mechanism works fine even if multiple evictions
// happen in the same epoch.
//
// //nolint:maintidx // TODO: simplify
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Simplify the function to remove linter suppression

The comment //nolint:maintidx // TODO: simplify indicates that TestTransitionValidatorCap_DoubleEviction is complex and suppressed from linting. Refactoring this function to reduce complexity can eliminate the need for linter suppression and improve maintainability.

Would you like assistance in refactoring this function to reduce its complexity?

@abi87 abi87 force-pushed the validator-size-cap branch from 3091569 to 355d99b Compare December 2, 2024 19:49
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between 355d99b and 0307a03.

📒 Files selected for processing (2)
  • mod/state-transition/pkg/core/errors.go (1 hunks)
  • mod/state-transition/pkg/core/validation_deposits.go (2 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/state-transition/pkg/core/validation_deposits.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/validation_deposits.go:49-49
Timestamp: 2024-11-29T11:21:04.160Z
Learning: In `mod/state-transition/pkg/core/validation_deposits.go`, when converting `len(deposits)` to `uint32` in the condition `if uint32(len(deposits)) > sp.cs.GetValidatorSetCap()`, it's acceptable not to add an explicit bounds check for potential integer overflow.
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.
🔇 Additional comments (3)
mod/state-transition/pkg/core/validation_deposits.go (3)

87-96: LGTM! Well-documented validator set cap check.

The implementation correctly enforces the validator set cap after all other genesis validations. The comment clearly explains the rationale.


46-55: 🛠️ Refactor suggestion

Consider refactoring duplicate validator cap check logic.

The validator set cap check is duplicated between the Boonet case and the default case. Consider extracting this logic into a helper method:

+func (sp *StateProcessor[_, _, _, BeaconStateT, _, DepositT, _, _, _, _, _, _, _, _, _, _, _]) validateValidatorSetCap(deposits []DepositT) error {
+	if uint64(len(deposits)) > sp.cs.ValidatorSetCap() {
+		return fmt.Errorf("cannot process deposits: validator set cap of %d would be exceeded (deposits count: %d): %w",
+			sp.cs.ValidatorSetCap(),
+			len(deposits),
+			ErrValSetCapExceeded,
+		)
+	}
+	return nil
+}

Then use this helper in both places:

-		if uint64(len(deposits)) > sp.cs.ValidatorSetCap() {
-			return fmt.Errorf("validator set cap %d, deposits count %d: %w",
-				sp.cs.ValidatorSetCap(),
-				len(deposits),
-				ErrValSetCapExceeded,
-			)
-		}
+		if err := sp.validateValidatorSetCap(deposits); err != nil {
+			return err
+		}
⛔ Skipped due to learnings
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/validation_deposits.go:49-49
Timestamp: 2024-11-29T11:21:04.160Z
Learning: In `mod/state-transition/pkg/core/validation_deposits.go`, when converting `len(deposits)` to `uint32` in the condition `if uint32(len(deposits)) > sp.cs.GetValidatorSetCap()`, it's acceptable not to add an explicit bounds check for potential integer overflow.

Line range hint 102-102: Verify validator set cap enforcement for non-genesis deposits.

The PR objective mentions evicting validators with the lowest stake when the cap is exceeded, but validateNonGenesisDeposits doesn't enforce the validator set cap. Please verify if this is intentional or if the cap enforcement should be handled elsewhere.

Comment on lines +26 to +28
// ErrValSetCapExceeded is returned when the number of genesis deposits
// exceeds the validator set cap.
ErrValSetCapExceeded = errors.New("validator set cap exceeded at genesis")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM! Consider enhancing the error message.

The error variable is well-defined and follows the existing pattern. Consider making the error message more descriptive:

-	ErrValSetCapExceeded = errors.New("validator set cap exceeded at genesis")
+	ErrValSetCapExceeded = errors.New("number of validators would exceed the maximum allowed validator set cap at genesis")
📝 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.

Suggested change
// ErrValSetCapExceeded is returned when the number of genesis deposits
// exceeds the validator set cap.
ErrValSetCapExceeded = errors.New("validator set cap exceeded at genesis")
// ErrValSetCapExceeded is returned when the number of genesis deposits
// exceeds the validator set cap.
ErrValSetCapExceeded = errors.New("number of validators would exceed the maximum allowed validator set cap at genesis")

@abi87 abi87 merged commit eb7c5e8 into main Dec 2, 2024
16 checks passed
@abi87 abi87 deleted the validator-size-cap branch December 2, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants