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

chore(forks): Handle MaxValidatorsPerWithdrawalsSweep based on planned fork upgrade #2194

Merged
merged 5 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions mod/chain-spec/pkg/chain/chain_spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,11 @@ type Spec[

// MaxValidatorsPerWithdrawalsSweep returns the maximum number of validators
// per withdrawal sweep.
MaxValidatorsPerWithdrawalsSweep() uint64
MaxValidatorsPerWithdrawalsSweep(
isPostUpgrade func(uint64, SlotT) bool,
chainID uint64,
slot SlotT,
) uint64
Comment on lines +158 to +162
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 30, 2024

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Update method comment to reflect new parameters

The MaxValidatorsPerWithdrawalsSweep method now accepts isPostUpgrade, chainID, and slot as parameters, but the method comment has not been updated accordingly. To improve clarity and maintain accurate documentation, please update the comment to describe these parameters.

Apply this change to the comment:

 // MaxValidatorsPerWithdrawalsSweep returns the maximum number of validators per
-// withdrawals sweep.
+// withdrawals sweep based on the upgrade status.
+//
+// Parameters:
+//   - isPostUpgrade: function to determine if the chain is post-upgrade
+//   - chainID: the chain identifier
+//   - slot: the current slot
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
MaxValidatorsPerWithdrawalsSweep(
isPostUpgrade func(uint64, SlotT) bool,
chainID uint64,
slot SlotT,
) uint64
// MaxValidatorsPerWithdrawalsSweep returns the maximum number of validators per
// withdrawals sweep based on the upgrade status.
//
// Parameters:
// - isPostUpgrade: function to determine if the chain is post-upgrade
// - chainID: the chain identifier
// - slot: the current slot
MaxValidatorsPerWithdrawalsSweep(
isPostUpgrade func(uint64, SlotT) bool,
chainID uint64,
slot SlotT,
) uint64

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fork choice should not be binary (pre/post) but I guess:

  • not much more we can do with current folder structure
  • we plan to solve the issue more structurally as soon as fork activates.

So fine with me

Copy link
Contributor

Choose a reason for hiding this comment

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

@abi87, understood. Thank you.


// Deneb Values

Expand Down Expand Up @@ -485,8 +489,15 @@ func (c chainSpec[
// withdrawals sweep.
func (c chainSpec[
DomainTypeT, EpochT, ExecutionAddressT, SlotT, CometBFTConfigT,
]) MaxValidatorsPerWithdrawalsSweep() uint64 {
return c.Data.MaxValidatorsPerWithdrawalsSweep
]) MaxValidatorsPerWithdrawalsSweep(
isPostUpgrade func(uint64, SlotT) bool,
chainID uint64, slot SlotT,
) uint64 {
if isPostUpgrade(chainID, slot) {
return c.Data.MaxValidatorsPerWithdrawalsSweepPostUpgrade
}

return c.Data.MaxValidatorsPerWithdrawalsSweepPreUpgrade
}

// MinEpochsForBlobsSidecarsRequest returns the minimum number of epochs for
Expand Down
20 changes: 14 additions & 6 deletions mod/chain-spec/pkg/chain/data.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,13 @@ type SpecData[

// HysteresisQuotient is the quotient used in effective balance calculations
HysteresisQuotient uint64 `mapstructure:"hysteresis-quotient"`
// HysteresisDownwardMultiplier is the multiplier for downward balance adjustments.
// HysteresisDownwardMultiplier is the multiplier for downward balance
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we need to sync our linters XD

// adjustments.
HysteresisDownwardMultiplier uint64 `mapstructure:"hysteresis-downward-multiplier"`
// HysteresisUpwardMultiplier is the multiplier for upward balance adjustments.
// HysteresisUpwardMultiplier is the multiplier for upward balance
// adjustments.
HysteresisUpwardMultiplier uint64 `mapstructure:"hysteresis-upward-multiplier"`

// Time parameters constants.
//
// SlotsPerEpoch is the number of slots per epoch.
Expand Down Expand Up @@ -128,10 +131,15 @@ type SpecData[
// MaxWithdrawalsPerPayload indicates the maximum number of withdrawal
// operations allowed in a single payload.
MaxWithdrawalsPerPayload uint64 `mapstructure:"max-withdrawals-per-payload"`
// MaxValidatorsPerWithdrawalsSweep specifies the maximum number of
// validator
// withdrawals allowed per sweep.
MaxValidatorsPerWithdrawalsSweep uint64 `mapstructure:"max-validators-per-withdrawals-sweep"`
// MaxValidatorsPerWithdrawalsSweepPreUpgrade specifies the maximum number
// of validator withdrawals allowed per sweep. Before the upgrade, this
// value is consistent with ETH2.0 spec, set to 2^14.
MaxValidatorsPerWithdrawalsSweepPreUpgrade uint64 `mapstructure:"max-validators-per-withdrawals-sweep-pre-upgrade"`
// MaxValidatorsPerWithdrawalsSweepPostUpgrade specifies the maximum number
// of validator withdrawals allowed per sweep. After the upgrade, this value
// is set to the largest prime number smaller than the minimum possible
// amount of total validators.
MaxValidatorsPerWithdrawalsSweepPostUpgrade uint64 `mapstructure:"max-validators-per-withdrawals-sweep-post-upgrade"`
Comment on lines +134 to +142
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)

Consider simplifying field names for readability

The field names MaxValidatorsPerWithdrawalsSweepPreUpgrade and MaxValidatorsPerWithdrawalsSweepPostUpgrade are quite long and may impact code readability. Consider shortening them to improve clarity, for example:

  • MaxValidatorsPerSweepPreUpgrade
  • MaxValidatorsPerSweepPostUpgrade


// Deneb Values
//
Expand Down
22 changes: 16 additions & 6 deletions mod/config/pkg/spec/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,11 @@ func BaseSpec() chain.SpecData[
any,
]{
// Gwei value constants.
MinDepositAmount: 1e9,
MaxEffectiveBalance: 32e9,
EjectionBalance: 16e9,
EffectiveBalanceIncrement: 1e9,
MinDepositAmount: 1e9,
MaxEffectiveBalance: 32e9,
EjectionBalance: 16e9,
EffectiveBalanceIncrement: 1e9,

HysteresisQuotient: 4,
HysteresisDownwardMultiplier: 1,
HysteresisUpwardMultiplier: 5,
Expand All @@ -67,6 +68,7 @@ func BaseSpec() chain.SpecData[
SlotsPerEpoch: 32,
MinEpochsToInactivityPenalty: 4,
SlotsPerHistoricalRoot: 8,

// Signature domains.
DomainTypeProposer: common.DomainType{
0x00, 0x00, 0x00, 0x00,
Expand All @@ -92,35 +94,43 @@ func BaseSpec() chain.SpecData[
DomainTypeApplicationMask: common.DomainType{
0x00, 0x00, 0x00, 0x01,
},

// Eth1-related values.
DepositContractAddress: common.NewExecutionAddressFromHex(
DefaultDepositContractAddress,
),
DepositEth1ChainID: 1,
Eth1FollowDistance: 1,
TargetSecondsPerEth1Block: 3,

// Fork-related values.
DenebPlusForkEpoch: 9999999999999998,
ElectraForkEpoch: 9999999999999999,

calbera marked this conversation as resolved.
Show resolved Hide resolved
// State list length constants.
EpochsPerHistoricalVector: 8,
EpochsPerSlashingsVector: 8,
HistoricalRootsLimit: 8,
ValidatorRegistryLimit: 1099511627776,

// Max operations per block constants.
MaxDepositsPerBlock: 16,

// Slashing
ProportionalSlashingMultiplier: 1,

// Capella values.
MaxWithdrawalsPerPayload: 16,
MaxValidatorsPerWithdrawalsSweep: 1 << 14,
MaxWithdrawalsPerPayload: 16,
MaxValidatorsPerWithdrawalsSweepPreUpgrade: 1 << 14,

Copy link
Collaborator

@abi87 abi87 Dec 2, 2024

Choose a reason for hiding this comment

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

Wait, are we changing this for every network?
I think we want 16 set in Boonet (post fork) and every other network but Bartio.
So I would keep base values to 1<<14 and fix them in their chain specs

// Deneb values.
MinEpochsForBlobsSidecarsRequest: 4096,
MaxBlobCommitmentsPerBlock: 16,
MaxBlobsPerBlock: 6,
FieldElementsPerBlob: 4096,
BytesPerBlob: 131072,
KZGCommitmentInclusionProofDepth: 17,

// Comet values.
CometValues: cmtConsensusParams,
}
Expand Down
14 changes: 7 additions & 7 deletions mod/config/pkg/spec/boonet.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,27 +34,27 @@ func BoonetChainSpec() (chain.Spec[
math.Slot,
any,
], error) {
testnetSpec := BaseSpec()
boonetSpec := BaseSpec()

// Chain ID is 80000.
testnetSpec.DepositEth1ChainID = BoonetEth1ChainID
boonetSpec.DepositEth1ChainID = BoonetEth1ChainID

// BGT contract address.
testnetSpec.EVMInflationAddress = common.NewExecutionAddressFromHex(
boonetSpec.EVMInflationAddress = common.NewExecutionAddressFromHex(
"0x289274787bAF083C15A45a174b7a8e44F0720660",
)

// 2.5 BERA per block minting.
// BERA per block minting.
//
// TODO: determine correct value for boonet upgrade.
testnetSpec.EVMInflationPerBlock = 2.5e9
boonetSpec.EVMInflationPerBlock = 2.5e9
calbera marked this conversation as resolved.
Show resolved Hide resolved

// 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.
//
// TODO: Determine correct value for boonet upgrade.
testnetSpec.MaxValidatorsPerWithdrawalsSweep = 43
boonetSpec.MaxValidatorsPerWithdrawalsSweepPostUpgrade = 43

return chain.NewChainSpec(testnetSpec)
return chain.NewChainSpec(boonetSpec)
}
8 changes: 6 additions & 2 deletions mod/node-core/pkg/services/version/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ func (rs *ReportingService[_, _]) Start(ctx context.Context) error {
for {
select {
case <-ticker.C:
// since the eth client can be updated separately for beacon node
// since the eth client can be updated separately for beacon
// node
// we need to fetch the version every time
ethVersion, err = rs.GetEthVersion(ctx)
if err != nil {
Expand Down Expand Up @@ -175,7 +176,10 @@ func (rs *ReportingService[_, _]) GetEthVersion(
// Get the client version from the execution layer.
info, err := rs.client.GetClientVersionV1(ctx)
if err != nil {
return ethVersion, fmt.Errorf("failed to get client version: %w", err)
return ethVersion, fmt.Errorf(
"failed to get client version: %w",
err,
)
}

// the spec says we should have at least one client version
Expand Down
4 changes: 3 additions & 1 deletion mod/state-transition/pkg/core/state/statedb.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,9 @@ func (s *StateDB[
}

bound := min(
totalValidators, s.cs.MaxValidatorsPerWithdrawalsSweep(),
totalValidators, s.cs.MaxValidatorsPerWithdrawalsSweep(
IsPostUpgrade, s.cs.DepositEth1ChainID(), slot,
),
Comment on lines +254 to +256
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

Undefined variable IsPostUpgrade

The variable IsPostUpgrade is used as a parameter but is not defined within the current scope. This will result in a compilation error. Please define IsPostUpgrade before using it.

Apply this diff to define IsPostUpgrade:

 slot, err := s.GetSlot()
 if err != nil {
     return nil, err
 }
+// Determine if the chain is post-upgrade based on the current slot
+IsPostUpgrade := s.IsPostUpgrade(slot)

 bound := min(
     totalValidators, s.cs.MaxValidatorsPerWithdrawalsSweep(
         IsPostUpgrade, s.cs.DepositEth1ChainID(), slot,
     ),
 )

Committable suggestion skipped: line range outside the PR's diff.

)

// Iterate through indices to find the next validators to withdraw.
Expand Down
44 changes: 44 additions & 0 deletions mod/state-transition/pkg/core/state/upgrade.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// SPDX-License-Identifier: BUSL-1.1
//
// Copyright (C) 2024, Berachain Foundation. All rights reserved.
// Use of this software is governed by the Business Source License included
// in the LICENSE file of this repository and at www.mariadb.com/bsl11.
//
// ANY USE OF THE LICENSED WORK IN VIOLATION OF THIS LICENSE WILL AUTOMATICALLY
// TERMINATE YOUR RIGHTS UNDER THIS LICENSE FOR THE CURRENT AND ALL OTHER
// VERSIONS OF THE LICENSED WORK.
//
// THIS LICENSE DOES NOT GRANT YOU ANY RIGHT IN ANY TRADEMARK OR LOGO OF
// LICENSOR OR ITS AFFILIATES (PROVIDED THAT YOU MAY USE A TRADEMARK OR LOGO OF
// LICENSOR AS EXPRESSLY REQUIRED BY THIS LICENSE).
//
// TO THE EXTENT PERMITTED BY APPLICABLE LAW, THE LICENSED WORK IS PROVIDED ON
// AN “AS IS” BASIS. LICENSOR HEREBY DISCLAIMS ALL WARRANTIES AND CONDITIONS,
// EXPRESS OR IMPLIED, INCLUDING (WITHOUT LIMITATION) WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE, NON-INFRINGEMENT, AND
// TITLE.

package state

import (
"github.com/berachain/beacon-kit/mod/config/pkg/spec"
"github.com/berachain/beacon-kit/mod/primitives/pkg/math"
)

// IsPostUpgrade returns true if the chain is post-upgrade (Fork2 on Boonet).
//
// TODO: Jank. Refactor into better fork version management.
calbera marked this conversation as resolved.
Show resolved Hide resolved
func IsPostUpgrade(chainID uint64, slot math.Slot) bool {
switch chainID {
case spec.BartioChainID:
return false
case spec.BoonetEth1ChainID:
if slot < math.U64(spec.BoonetFork2Height) {
return false
}

return true
default:
return true
Comment on lines +41 to +42
Copy link
Collaborator

Choose a reason for hiding this comment

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

default behaviour equals Boonet post fork. This is the general principle we are following, right?

}
}
calbera marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 6 additions & 2 deletions mod/state-transition/pkg/core/state_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -563,8 +563,12 @@ func (sp *StateProcessor[

var (
hysteresisIncrement = sp.cs.EffectiveBalanceIncrement() / sp.cs.HysteresisQuotient()
downwardThreshold = math.Gwei(hysteresisIncrement * sp.cs.HysteresisDownwardMultiplier())
upwardThreshold = math.Gwei(hysteresisIncrement * sp.cs.HysteresisUpwardMultiplier())
downwardThreshold = math.Gwei(
hysteresisIncrement * sp.cs.HysteresisDownwardMultiplier(),
)
upwardThreshold = math.Gwei(
hysteresisIncrement * sp.cs.HysteresisUpwardMultiplier(),
)

idx math.U64
balance math.Gwei
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,7 @@ func TestTransitionMaxWithdrawals(t *testing.T) {
csData := spec.BaseSpec()
csData.DepositEth1ChainID = spec.BoonetEth1ChainID
csData.MaxWithdrawalsPerPayload = 2
csData.MaxValidatorsPerWithdrawalsSweepPostUpgrade = 2
cs, err := chain.NewChainSpec(csData)
require.NoError(t, err)

Expand Down
14 changes: 12 additions & 2 deletions mod/state-transition/pkg/core/state_processor_withdrawals.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/berachain/beacon-kit/mod/config/pkg/spec"
"github.com/berachain/beacon-kit/mod/errors"
"github.com/berachain/beacon-kit/mod/primitives/pkg/math"
"github.com/berachain/beacon-kit/mod/state-transition/pkg/core/state"
"github.com/davecgh/go-spew/spew"
)

Expand Down Expand Up @@ -93,6 +94,7 @@ func (sp *StateProcessor[
st,
expectedWithdrawals,
payloadWithdrawals,
slot,
)

case sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID &&
Expand All @@ -116,13 +118,15 @@ func (sp *StateProcessor[
st,
expectedWithdrawals,
payloadWithdrawals,
slot,
)

default:
return sp.processWithdrawalsDefault(
st,
expectedWithdrawals,
payloadWithdrawals,
slot,
)
}
}
Expand All @@ -134,6 +138,7 @@ func (sp *StateProcessor[
st BeaconStateT,
expectedWithdrawals []WithdrawalT,
payloadWithdrawals []WithdrawalT,
slot math.Slot,
) error {
for i, wd := range expectedWithdrawals {
// Ensure the withdrawals match the local state.
Expand Down Expand Up @@ -185,7 +190,9 @@ func (sp *StateProcessor[
return err
}
nextValidatorIndex += math.ValidatorIndex(
sp.cs.MaxValidatorsPerWithdrawalsSweep())
sp.cs.MaxValidatorsPerWithdrawalsSweep(
state.IsPostUpgrade, spec.BartioChainID, slot,
))
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

Call 'state.IsPostUpgrade' function instead of passing it as a reference

In lines 194-195, state.IsPostUpgrade is passed as a function reference to MaxValidatorsPerWithdrawalsSweep, but it should be called with the appropriate parameters to return a boolean value. This could lead to a logical error or compile-time issue.

Apply this diff to fix the issue:

 nextValidatorIndex += math.ValidatorIndex(
-	sp.cs.MaxValidatorsPerWithdrawalsSweep(
-		state.IsPostUpgrade, spec.BartioChainID, slot,
+	sp.cs.MaxValidatorsPerWithdrawalsSweep(
+		state.IsPostUpgrade(spec.BartioChainID, slot),
+		spec.BartioChainID, slot,
 	))

This ensures that state.IsPostUpgrade is invoked properly with spec.BartioChainID and slot as arguments, returning the expected boolean value needed by MaxValidatorsPerWithdrawalsSweep.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
sp.cs.MaxValidatorsPerWithdrawalsSweep(
state.IsPostUpgrade, spec.BartioChainID, slot,
))
sp.cs.MaxValidatorsPerWithdrawalsSweep(
state.IsPostUpgrade(spec.BartioChainID, slot), spec.BartioChainID, slot,
))

nextValidatorIndex %= math.ValidatorIndex(totalValidators)
}

Expand All @@ -199,6 +206,7 @@ func (sp *StateProcessor[
st BeaconStateT,
expectedWithdrawals []WithdrawalT,
payloadWithdrawals []WithdrawalT,
slot math.Slot,
) error {
// Enforce that first withdrawal is EVM inflation
if len(payloadWithdrawals) == 0 {
Expand Down Expand Up @@ -259,7 +267,9 @@ func (sp *StateProcessor[
return err
}
nextValidatorIndex += math.ValidatorIndex(
sp.cs.MaxValidatorsPerWithdrawalsSweep())
sp.cs.MaxValidatorsPerWithdrawalsSweep(
state.IsPostUpgrade, sp.cs.DepositEth1ChainID(), slot,
))
abi87 marked this conversation as resolved.
Show resolved Hide resolved
nextValidatorIndex %= math.ValidatorIndex(totalValidators)
}

Expand Down
Loading