From 7ff58bbd035aa4cce8405000095248433cae3fbf Mon Sep 17 00:00:00 2001 From: Potuz Date: Thu, 16 Jan 2025 21:42:16 -0300 Subject: [PATCH 1/5] move credential helpers to ReadOnlyValidator methods --- beacon-chain/core/electra/consolidations.go | 12 +++- .../core/electra/effective_balance_updates.go | 3 +- beacon-chain/core/electra/upgrade.go | 2 +- beacon-chain/core/electra/withdrawals.go | 4 +- beacon-chain/core/fulu/upgrade.go | 2 +- beacon-chain/core/helpers/validators.go | 68 ++----------------- beacon-chain/core/helpers/validators_test.go | 20 ++++-- beacon-chain/state/interfaces.go | 3 + .../state/state-native/readonly_validator.go | 22 ++++++ 9 files changed, 59 insertions(+), 77 deletions(-) diff --git a/beacon-chain/core/electra/consolidations.go b/beacon-chain/core/electra/consolidations.go index 5dd6581e4839..51f33a3f278c 100644 --- a/beacon-chain/core/electra/consolidations.go +++ b/beacon-chain/core/electra/consolidations.go @@ -9,6 +9,7 @@ import ( "github.com/pkg/errors" "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/helpers" "github.com/prysmaticlabs/prysm/v5/beacon-chain/state" + state_native "github.com/prysmaticlabs/prysm/v5/beacon-chain/state/state-native" "github.com/prysmaticlabs/prysm/v5/config/params" "github.com/prysmaticlabs/prysm/v5/consensus-types/primitives" "github.com/prysmaticlabs/prysm/v5/encoding/bytesutil" @@ -233,13 +234,18 @@ func ProcessConsolidationRequests(ctx context.Context, st state.BeaconState, req return fmt.Errorf("failed to fetch source validator: %w", err) // This should never happen. } + roSrcV, err := state_native.NewValidator(srcV) + if err != nil { + return err + } + tgtV, err := st.ValidatorAtIndexReadOnly(tgtIdx) if err != nil { return fmt.Errorf("failed to fetch target validator: %w", err) // This should never happen. } // Verify source withdrawal credentials - if !helpers.HasExecutionWithdrawalCredentials(srcV) { + if !roSrcV.HasExecutionWithdrawalCredentials() { continue } // Confirm source_validator.withdrawal_credentials[12:] == consolidation_request.source_address @@ -248,7 +254,7 @@ func ProcessConsolidationRequests(ctx context.Context, st state.BeaconState, req } // Target validator must have their withdrawal credentials set appropriately. - if !helpers.HasCompoundingWithdrawalCredential(tgtV) { + if !tgtV.HasCompoundingWithdrawalCredentials() { continue } @@ -364,7 +370,7 @@ func IsValidSwitchToCompoundingRequest(st state.BeaconState, req *enginev1.Conso return false } - if !helpers.HasETH1WithdrawalCredential(srcV) { + if !srcV.HasETH1WithdrawalCredentials() { return false } diff --git a/beacon-chain/core/electra/effective_balance_updates.go b/beacon-chain/core/electra/effective_balance_updates.go index c87d232adb2b..eb697d56a4ad 100644 --- a/beacon-chain/core/electra/effective_balance_updates.go +++ b/beacon-chain/core/electra/effective_balance_updates.go @@ -3,7 +3,6 @@ package electra import ( "fmt" - "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/helpers" "github.com/prysmaticlabs/prysm/v5/beacon-chain/state" "github.com/prysmaticlabs/prysm/v5/config/params" ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1" @@ -49,7 +48,7 @@ func ProcessEffectiveBalanceUpdates(st state.BeaconState) error { balance := bals[idx] effectiveBalanceLimit := params.BeaconConfig().MinActivationBalance - if helpers.HasCompoundingWithdrawalCredential(val) { + if val.HasCompoundingWithdrawalCredentials() { effectiveBalanceLimit = params.BeaconConfig().MaxEffectiveBalanceElectra } diff --git a/beacon-chain/core/electra/upgrade.go b/beacon-chain/core/electra/upgrade.go index 47c3bf500c0a..a815461b4246 100644 --- a/beacon-chain/core/electra/upgrade.go +++ b/beacon-chain/core/electra/upgrade.go @@ -194,7 +194,7 @@ func UpgradeToElectra(beaconState state.BeaconState) (state.BeaconState, error) if val.ActivationEpoch() == params.BeaconConfig().FarFutureEpoch { preActivationIndices = append(preActivationIndices, primitives.ValidatorIndex(index)) } - if helpers.HasCompoundingWithdrawalCredential(val) { + if val.HasCompoundingWithdrawalCredentials() { compoundWithdrawalIndices = append(compoundWithdrawalIndices, primitives.ValidatorIndex(index)) } return nil diff --git a/beacon-chain/core/electra/withdrawals.go b/beacon-chain/core/electra/withdrawals.go index 9e54b66fe04d..62156188ccd8 100644 --- a/beacon-chain/core/electra/withdrawals.go +++ b/beacon-chain/core/electra/withdrawals.go @@ -116,7 +116,7 @@ func ProcessWithdrawalRequests(ctx context.Context, st state.BeaconState, wrs [] return nil, err } // Verify withdrawal credentials - hasCorrectCredential := helpers.HasExecutionWithdrawalCredentials(validator) + hasCorrectCredential := validator.HasExecutionWithdrawalCredentials() wc := validator.GetWithdrawalCredentials() isCorrectSourceAddress := bytes.Equal(wc[12:], wr.SourceAddress) if !hasCorrectCredential || !isCorrectSourceAddress { @@ -165,7 +165,7 @@ func ProcessWithdrawalRequests(ctx context.Context, st state.BeaconState, wrs [] hasExcessBalance := vBal > params.BeaconConfig().MinActivationBalance+pendingBalanceToWithdraw // Only allow partial withdrawals with compounding withdrawal credentials - if helpers.HasCompoundingWithdrawalCredential(validator) && hasSufficientEffectiveBalance && hasExcessBalance { + if validator.HasCompoundingWithdrawalCredentials() && hasSufficientEffectiveBalance && hasExcessBalance { // Spec definition: // to_withdraw = min( // state.balances[index] - MIN_ACTIVATION_BALANCE - pending_balance_to_withdraw, diff --git a/beacon-chain/core/fulu/upgrade.go b/beacon-chain/core/fulu/upgrade.go index ff2961609074..aa27f5d91450 100644 --- a/beacon-chain/core/fulu/upgrade.go +++ b/beacon-chain/core/fulu/upgrade.go @@ -85,7 +85,7 @@ func UpgradeToFulu(beaconState state.BeaconState) (state.BeaconState, error) { if val.ActivationEpoch() == params.BeaconConfig().FarFutureEpoch { preActivationIndices = append(preActivationIndices, primitives.ValidatorIndex(index)) } - if helpers.HasCompoundingWithdrawalCredential(val) { + if val.HasCompoundingWithdrawalCredentials() { compoundWithdrawalIndices = append(compoundWithdrawalIndices, primitives.ValidatorIndex(index)) } return nil diff --git a/beacon-chain/core/helpers/validators.go b/beacon-chain/core/helpers/validators.go index 6c5c1e6a1ea5..879876c25ed5 100644 --- a/beacon-chain/core/helpers/validators.go +++ b/beacon-chain/core/helpers/validators.go @@ -14,7 +14,6 @@ import ( "github.com/prysmaticlabs/prysm/v5/beacon-chain/state" fieldparams "github.com/prysmaticlabs/prysm/v5/config/fieldparams" "github.com/prysmaticlabs/prysm/v5/config/params" - "github.com/prysmaticlabs/prysm/v5/consensus-types/interfaces" "github.com/prysmaticlabs/prysm/v5/consensus-types/primitives" "github.com/prysmaticlabs/prysm/v5/crypto/hash" "github.com/prysmaticlabs/prysm/v5/encoding/bytesutil" @@ -515,63 +514,6 @@ func LastActivatedValidatorIndex(ctx context.Context, st state.ReadOnlyBeaconSta return lastActivatedvalidatorIndex, nil } -// hasETH1WithdrawalCredential returns whether the validator has an ETH1 -// Withdrawal prefix. It assumes that the caller has a lock on the state -func HasETH1WithdrawalCredential(val interfaces.WithWithdrawalCredentials) bool { - if val == nil { - return false - } - return isETH1WithdrawalCredential(val.GetWithdrawalCredentials()) -} - -func isETH1WithdrawalCredential(creds []byte) bool { - return bytes.HasPrefix(creds, []byte{params.BeaconConfig().ETH1AddressWithdrawalPrefixByte}) -} - -// HasCompoundingWithdrawalCredential checks if the validator has a compounding withdrawal credential. -// New in Electra EIP-7251: https://eips.ethereum.org/EIPS/eip-7251 -// -// Spec definition: -// -// def has_compounding_withdrawal_credential(validator: Validator) -> bool: -// """ -// Check if ``validator`` has an 0x02 prefixed "compounding" withdrawal credential. -// """ -// return is_compounding_withdrawal_credential(validator.withdrawal_credentials) -func HasCompoundingWithdrawalCredential(v interfaces.WithWithdrawalCredentials) bool { - if v == nil { - return false - } - return IsCompoundingWithdrawalCredential(v.GetWithdrawalCredentials()) -} - -// IsCompoundingWithdrawalCredential checks if the credentials are a compounding withdrawal credential. -// -// Spec definition: -// -// def is_compounding_withdrawal_credential(withdrawal_credentials: Bytes32) -> bool: -// return withdrawal_credentials[:1] == COMPOUNDING_WITHDRAWAL_PREFIX -func IsCompoundingWithdrawalCredential(creds []byte) bool { - return bytes.HasPrefix(creds, []byte{params.BeaconConfig().CompoundingWithdrawalPrefixByte}) -} - -// HasExecutionWithdrawalCredentials checks if the validator has an execution withdrawal credential or compounding credential. -// New in Electra EIP-7251: https://eips.ethereum.org/EIPS/eip-7251 -// -// Spec definition: -// -// def has_execution_withdrawal_credential(validator: Validator) -> bool: -// """ -// Check if ``validator`` has a 0x01 or 0x02 prefixed withdrawal credential. -// """ -// return has_compounding_withdrawal_credential(validator) or has_eth1_withdrawal_credential(validator) -func HasExecutionWithdrawalCredentials(v interfaces.WithWithdrawalCredentials) bool { - if v == nil { - return false - } - return HasCompoundingWithdrawalCredential(v) || HasETH1WithdrawalCredential(v) -} - // IsSameWithdrawalCredentials returns true if both validators have the same withdrawal credentials. // // return a.withdrawal_credentials[12:] == b.withdrawal_credentials[12:] @@ -606,10 +548,10 @@ func IsFullyWithdrawableValidator(val state.ReadOnlyValidator, balance uint64, e // Electra / EIP-7251 logic if fork >= version.Electra { - return HasExecutionWithdrawalCredentials(val) && val.WithdrawableEpoch() <= epoch + return val.HasExecutionWithdrawalCredentials() && val.WithdrawableEpoch() <= epoch } - return HasETH1WithdrawalCredential(val) && val.WithdrawableEpoch() <= epoch + return val.HasETH1WithdrawalCredentials() && val.WithdrawableEpoch() <= epoch } // IsPartiallyWithdrawableValidator returns whether the validator is able to perform a @@ -650,7 +592,7 @@ func isPartiallyWithdrawableValidatorElectra(val state.ReadOnlyValidator, balanc hasMaxBalance := val.EffectiveBalance() == maxEB hasExcessBalance := balance > maxEB - return HasExecutionWithdrawalCredentials(val) && + return val.HasExecutionWithdrawalCredentials() && hasMaxBalance && hasExcessBalance } @@ -670,7 +612,7 @@ func isPartiallyWithdrawableValidatorElectra(val state.ReadOnlyValidator, balanc func isPartiallyWithdrawableValidatorCapella(val state.ReadOnlyValidator, balance uint64, epoch primitives.Epoch) bool { hasMaxBalance := val.EffectiveBalance() == params.BeaconConfig().MaxEffectiveBalance hasExcessBalance := balance > params.BeaconConfig().MaxEffectiveBalance - return HasETH1WithdrawalCredential(val) && hasExcessBalance && hasMaxBalance + return val.HasETH1WithdrawalCredentials() && hasExcessBalance && hasMaxBalance } // ValidatorMaxEffectiveBalance returns the maximum effective balance for a validator. @@ -686,7 +628,7 @@ func isPartiallyWithdrawableValidatorCapella(val state.ReadOnlyValidator, balanc // else: // return MIN_ACTIVATION_BALANCE func ValidatorMaxEffectiveBalance(val state.ReadOnlyValidator) uint64 { - if HasCompoundingWithdrawalCredential(val) { + if val.HasCompoundingWithdrawalCredentials() { return params.BeaconConfig().MaxEffectiveBalanceElectra } return params.BeaconConfig().MinActivationBalance diff --git a/beacon-chain/core/helpers/validators_test.go b/beacon-chain/core/helpers/validators_test.go index e1c3dceb27f6..0e154e7ea434 100644 --- a/beacon-chain/core/helpers/validators_test.go +++ b/beacon-chain/core/helpers/validators_test.go @@ -910,13 +910,19 @@ func TestProposerIndexFromCheckpoint(t *testing.T) { func TestHasETH1WithdrawalCredentials(t *testing.T) { creds := []byte{0xFA, 0xCC} v := ðpb.Validator{WithdrawalCredentials: creds} - require.Equal(t, false, helpers.HasETH1WithdrawalCredential(v)) + roV, err := state_native.NewValidator(v) + require.NoError(t, err) + require.Equal(t, false, roV.HasETH1WithdrawalCredentials()) creds = []byte{params.BeaconConfig().ETH1AddressWithdrawalPrefixByte, 0xCC} v = ðpb.Validator{WithdrawalCredentials: creds} - require.Equal(t, true, helpers.HasETH1WithdrawalCredential(v)) + roV, err = state_native.NewValidator(v) + require.NoError(t, err) + require.Equal(t, true, roV.HasETH1WithdrawalCredentials()) // No Withdrawal cred v = ðpb.Validator{} - require.Equal(t, false, helpers.HasETH1WithdrawalCredential(v)) + roV, err = state_native.NewValidator(v) + require.NoError(t, err) + require.Equal(t, false, roV.HasETH1WithdrawalCredentials()) } func TestHasCompoundingWithdrawalCredential(t *testing.T) { @@ -935,7 +941,9 @@ func TestHasCompoundingWithdrawalCredential(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, helpers.HasCompoundingWithdrawalCredential(tt.validator)) + roV, err := state_native.NewValidator(tt.validator) + require.NoError(t, err) + assert.Equal(t, tt.want, roV.HasCompoundingWithdrawalCredentials()) }) } } @@ -959,7 +967,9 @@ func TestHasExecutionWithdrawalCredentials(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, helpers.HasExecutionWithdrawalCredentials(tt.validator)) + roV, err := state_native.NewValidator(tt.validator) + require.NoError(t, err) + assert.Equal(t, tt.want, roV.HasExecutionWithdrawalCredentials()) }) } } diff --git a/beacon-chain/state/interfaces.go b/beacon-chain/state/interfaces.go index 15b5544be80f..3161f2c6c720 100644 --- a/beacon-chain/state/interfaces.go +++ b/beacon-chain/state/interfaces.go @@ -119,6 +119,9 @@ type ReadOnlyValidator interface { Copy() *ethpb.Validator Slashed() bool IsNil() bool + HasETH1WithdrawalCredentials() bool + HasCompoundingWithdrawalCredentials() bool + HasExecutionWithdrawalCredentials() bool } // ReadOnlyValidators defines a struct which only has read access to validators methods. diff --git a/beacon-chain/state/state-native/readonly_validator.go b/beacon-chain/state/state-native/readonly_validator.go index 19ae4e5cc4ea..b7ea3f2da1a0 100644 --- a/beacon-chain/state/state-native/readonly_validator.go +++ b/beacon-chain/state/state-native/readonly_validator.go @@ -4,6 +4,7 @@ import ( "github.com/pkg/errors" "github.com/prysmaticlabs/prysm/v5/beacon-chain/state" fieldparams "github.com/prysmaticlabs/prysm/v5/config/fieldparams" + "github.com/prysmaticlabs/prysm/v5/config/params" "github.com/prysmaticlabs/prysm/v5/consensus-types/primitives" ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1" ) @@ -88,6 +89,27 @@ func (v readOnlyValidator) IsNil() bool { return v.validator == nil } +// HasETH1WithdrawalCredentials returns true if the validator has an ETH1 withdrawal credentials. +func (v readOnlyValidator) HasETH1WithdrawalCredentials() bool { + if v.IsNil() { + return false + } + return v.validator.WithdrawalCredentials[0] == params.BeaconConfig().ETH1AddressWithdrawalPrefixByte +} + +// HasCompoundingWithdrawalCredentials returns true if the validator has a compounding withdrawal credentials. +func (v readOnlyValidator) HasCompoundingWithdrawalCredentials() bool { + if v.IsNil() { + return false + } + return v.validator.WithdrawalCredentials[0] == params.BeaconConfig().CompoundingWithdrawalPrefixByte +} + +// HasExecutionWithdrawalCredentials returns true if the validator has an execution withdrawal credentials. +func (v readOnlyValidator) HasExecutionWithdrawalCredentials() bool { + return v.HasETH1WithdrawalCredentials() || v.HasCompoundingWithdrawalCredentials() +} + // Copy returns a new validator from the read only validator func (v readOnlyValidator) Copy() *ethpb.Validator { pubKey := v.PublicKey() From 8fc4d71747a9750793922d4017b92de5fe32aab7 Mon Sep 17 00:00:00 2001 From: Potuz Date: Thu, 16 Jan 2025 21:51:39 -0300 Subject: [PATCH 2/5] Changelog + Gazelle --- beacon-chain/core/helpers/BUILD.bazel | 1 - changelog/potuz_credentials_as_methods.md | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog/potuz_credentials_as_methods.md diff --git a/beacon-chain/core/helpers/BUILD.bazel b/beacon-chain/core/helpers/BUILD.bazel index 090a57458a6d..d742cab03729 100644 --- a/beacon-chain/core/helpers/BUILD.bazel +++ b/beacon-chain/core/helpers/BUILD.bazel @@ -25,7 +25,6 @@ go_library( "//beacon-chain/state:go_default_library", "//config/fieldparams:go_default_library", "//config/params:go_default_library", - "//consensus-types/interfaces:go_default_library", "//consensus-types/primitives:go_default_library", "//container/slice:go_default_library", "//container/trie:go_default_library", diff --git a/changelog/potuz_credentials_as_methods.md b/changelog/potuz_credentials_as_methods.md new file mode 100644 index 000000000000..b413c68cc599 --- /dev/null +++ b/changelog/potuz_credentials_as_methods.md @@ -0,0 +1,2 @@ +### Changed +- Remove helpers to check for execution/compounding withdrawal credentials and expose them as methods. \ No newline at end of file From 8333f94115d98ba11ac3bb17249c6372f0332a16 Mon Sep 17 00:00:00 2001 From: Potuz Date: Thu, 16 Jan 2025 22:29:07 -0300 Subject: [PATCH 3/5] Fix nil tests --- beacon-chain/core/helpers/validators_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/beacon-chain/core/helpers/validators_test.go b/beacon-chain/core/helpers/validators_test.go index 0e154e7ea434..7374d3498def 100644 --- a/beacon-chain/core/helpers/validators_test.go +++ b/beacon-chain/core/helpers/validators_test.go @@ -919,10 +919,6 @@ func TestHasETH1WithdrawalCredentials(t *testing.T) { require.NoError(t, err) require.Equal(t, true, roV.HasETH1WithdrawalCredentials()) // No Withdrawal cred - v = ðpb.Validator{} - roV, err = state_native.NewValidator(v) - require.NoError(t, err) - require.Equal(t, false, roV.HasETH1WithdrawalCredentials()) } func TestHasCompoundingWithdrawalCredential(t *testing.T) { @@ -937,7 +933,6 @@ func TestHasCompoundingWithdrawalCredential(t *testing.T) { {"Does not have compounding withdrawal credential", ðpb.Validator{WithdrawalCredentials: bytesutil.PadTo([]byte{0x00}, 32)}, false}, - {"Handles nil case", nil, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -963,7 +958,6 @@ func TestHasExecutionWithdrawalCredentials(t *testing.T) { {"Does not have compounding withdrawal credential or eth1 withdrawal credential", ðpb.Validator{WithdrawalCredentials: bytesutil.PadTo([]byte{0x00}, 32)}, false}, - {"Handles nil case", nil, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 098dda65122c0fdd3ed820d822ca28ccf7594376 Mon Sep 17 00:00:00 2001 From: Potuz Date: Thu, 16 Jan 2025 22:38:23 -0300 Subject: [PATCH 4/5] Fix more nil tests --- beacon-chain/core/electra/effective_balance_updates.go | 2 +- beacon-chain/core/electra/effective_balance_updates_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon-chain/core/electra/effective_balance_updates.go b/beacon-chain/core/electra/effective_balance_updates.go index eb697d56a4ad..a16515faeb50 100644 --- a/beacon-chain/core/electra/effective_balance_updates.go +++ b/beacon-chain/core/electra/effective_balance_updates.go @@ -39,7 +39,7 @@ func ProcessEffectiveBalanceUpdates(st state.BeaconState) error { // Update effective balances with hysteresis. validatorFunc := func(idx int, val state.ReadOnlyValidator) (newVal *ethpb.Validator, err error) { - if val == nil { + if val.IsNil() { return nil, fmt.Errorf("validator %d is nil in state", idx) } if idx >= len(bals) { diff --git a/beacon-chain/core/electra/effective_balance_updates_test.go b/beacon-chain/core/electra/effective_balance_updates_test.go index b66de1d1156b..765f417d6b95 100644 --- a/beacon-chain/core/electra/effective_balance_updates_test.go +++ b/beacon-chain/core/electra/effective_balance_updates_test.go @@ -77,7 +77,7 @@ func TestProcessEffectiveBalnceUpdates(t *testing.T) { Validators: []*eth.Validator{ { EffectiveBalance: params.BeaconConfig().MinActivationBalance / 2, - WithdrawalCredentials: nil, + WithdrawalCredentials: make([]byte, 32), }, }, Balances: []uint64{ From c335361639a7854eaee53fb2f3d72a21658bd1a6 Mon Sep 17 00:00:00 2001 From: Potuz Date: Fri, 17 Jan 2025 06:55:05 -0300 Subject: [PATCH 5/5] Fix another nil test --- beacon-chain/rpc/eth/events/events_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon-chain/rpc/eth/events/events_test.go b/beacon-chain/rpc/eth/events/events_test.go index 98259a5b1b05..44eb1b4b9828 100644 --- a/beacon-chain/rpc/eth/events/events_test.go +++ b/beacon-chain/rpc/eth/events/events_test.go @@ -461,7 +461,7 @@ func TestStreamEvents_OperationsEvents(t *testing.T) { defer testSync.cleanup() st := tc.getState() - v := ð.Validator{ExitEpoch: math.MaxUint64, EffectiveBalance: params.BeaconConfig().MinActivationBalance} + v := ð.Validator{ExitEpoch: math.MaxUint64, EffectiveBalance: params.BeaconConfig().MinActivationBalance, WithdrawalCredentials: make([]byte, 32)} require.NoError(t, st.SetValidators([]*eth.Validator{v})) currentSlot := primitives.Slot(0) // to avoid slot processing