Electra: Spec review of process_slashings (#14766)

* Electra: Review process_slashings

* Update signature not to return the state, there is no need

* Update CHANGELOG.md

CHANGELOG.md

@@ -32,6 +32,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve
 - Limit consolidating by validator's effective balance.
 - Use 16-bit random value for proposer and sync committee selection filter.
 - Re-organize the content of the `*.proto` files (No functional change).
+- Updated spec definitions for `process_slashings` in godocs. Simplified `ProcessSlashings` API.
 - Updated spec definition electra `process_registry_updates`.
 - Updated Electra spec definition for `process_epoch`.
 - Update our `go-libp2p-pubsub` dependency.

beacon-chain/core/altair/epoch_spec_test.go

@@ -105,10 +105,9 @@ func TestProcessSlashings_NotSlashed(t *testing.T) {
 	}
 	s, err := state_native.InitializeFromProtoAltair(base)
 	require.NoError(t, err)
-	newState, err := epoch.ProcessSlashings(s)
-	require.NoError(t, err)
+	require.NoError(t, epoch.ProcessSlashings(s))
 	wanted := params.BeaconConfig().MaxEffectiveBalance
-	assert.Equal(t, wanted, newState.Balances()[0], "Unexpected slashed balance")
+	assert.Equal(t, wanted, s.Balances()[0], "Unexpected slashed balance")
 }
 
 func TestProcessSlashings_SlashedLess(t *testing.T) {
@@ -176,9 +175,8 @@ func TestProcessSlashings_SlashedLess(t *testing.T) {
 			original := proto.Clone(tt.state)
 			s, err := state_native.InitializeFromProtoAltair(tt.state)
 			require.NoError(t, err)
-			newState, err := epoch.ProcessSlashings(s)
-			require.NoError(t, err)
-			assert.Equal(t, tt.want, newState.Balances()[0], "ProcessSlashings({%v}) = newState; newState.Balances[0] = %d", original, newState.Balances()[0])
+			require.NoError(t, epoch.ProcessSlashings(s))
+			assert.Equal(t, tt.want, s.Balances()[0], "ProcessSlashings({%v}) = newState; newState.Balances[0] = %d", original, s.Balances()[0])
 		})
 	}
 }
@@ -192,6 +190,5 @@ func TestProcessSlashings_BadValue(t *testing.T) {
 	}
 	s, err := state_native.InitializeFromProtoAltair(base)
 	require.NoError(t, err)
-	_, err = epoch.ProcessSlashings(s)
-	require.ErrorContains(t, "addition overflows", err)
+	require.ErrorContains(t, "addition overflows", epoch.ProcessSlashings(s))
 }

beacon-chain/core/altair/transition.go

@@ -69,8 +69,7 @@ func ProcessEpoch(ctx context.Context, state state.BeaconState) error {
 	}
 
 	// Modified in Altair and Bellatrix.
-	state, err = e.ProcessSlashings(state)
-	if err != nil {
+	if err := e.ProcessSlashings(state); err != nil {
 		return err
 	}
 	state, err = e.ProcessEth1DataReset(state)

beacon-chain/core/electra/transition.go

@@ -81,8 +81,7 @@ func ProcessEpoch(ctx context.Context, state state.BeaconState) error {
 	if err := ProcessRegistryUpdates(ctx, state); err != nil {
 		return errors.Wrap(err, "could not process registry updates")
 	}
-	state, err = ProcessSlashings(state)
-	if err != nil {
+	if err := ProcessSlashings(state); err != nil {
 		return err
 	}
 	state, err = ProcessEth1DataReset(state)

beacon-chain/core/epoch/epoch_processing.go

@@ -141,20 +141,76 @@ func ProcessRegistryUpdates(ctx context.Context, st state.BeaconState) (state.Be
 	return st, nil
 }
 
-// ProcessSlashings processes the slashed validators during epoch processing,
+// ProcessSlashings processes the slashed validators during epoch processing. This is a state mutating method.
+//
+// Electra spec definition:
 //
 //	def process_slashings(state: BeaconState) -> None:
+//	    epoch = get_current_epoch(state)
+//	    total_balance = get_total_active_balance(state)
+//	    adjusted_total_slashing_balance = min(
+//	        sum(state.slashings) * PROPORTIONAL_SLASHING_MULTIPLIER_BELLATRIX,
+//	        total_balance
+//	    )
+//	    increment = EFFECTIVE_BALANCE_INCREMENT  # Factored out from total balance to avoid uint64 overflow
+//	    penalty_per_effective_balance_increment = adjusted_total_slashing_balance // (total_balance // increment)
+//	    for index, validator in enumerate(state.validators):
+//	        if validator.slashed and epoch + EPOCHS_PER_SLASHINGS_VECTOR // 2 == validator.withdrawable_epoch:
 //	            effective_balance_increments = validator.effective_balance // increment
+//	            # [Modified in Electra:EIP7251]
 //	            penalty = penalty_per_effective_balance_increment * effective_balance_increments
-func ProcessSlashings(st state.BeaconState) (state.BeaconState, error) {
+//	            decrease_balance(state, ValidatorIndex(index), penalty)
+//
+// Bellatrix spec definition:
+//
+//	def process_slashings(state: BeaconState) -> None:
+//	    epoch = get_current_epoch(state)
+//	    total_balance = get_total_active_balance(state)
+//	    adjusted_total_slashing_balance = min(
+//	        sum(state.slashings) * PROPORTIONAL_SLASHING_MULTIPLIER_BELLATRIX,  # [Modified in Bellatrix]
+//	        total_balance
+//	    )
+//	    for index, validator in enumerate(state.validators):
+//	        if validator.slashed and epoch + EPOCHS_PER_SLASHINGS_VECTOR // 2 == validator.withdrawable_epoch:
+//	            increment = EFFECTIVE_BALANCE_INCREMENT  # Factored out from penalty numerator to avoid uint64 overflow
+//	            penalty_numerator = validator.effective_balance // increment * adjusted_total_slashing_balance
+//	            penalty = penalty_numerator // total_balance * increment
+//	            decrease_balance(state, ValidatorIndex(index), penalty)
+//
+// Altair spec definition:
+//
+//	def process_slashings(state: BeaconState) -> None:
+//	    epoch = get_current_epoch(state)
+//	    total_balance = get_total_active_balance(state)
+//	    adjusted_total_slashing_balance = min(sum(state.slashings) * PROPORTIONAL_SLASHING_MULTIPLIER_ALTAIR, total_balance)
+//	    for index, validator in enumerate(state.validators):
+//	        if validator.slashed and epoch + EPOCHS_PER_SLASHINGS_VECTOR // 2 == validator.withdrawable_epoch:
+//	            increment = EFFECTIVE_BALANCE_INCREMENT  # Factored out from penalty numerator to avoid uint64 overflow
+//	            penalty_numerator = validator.effective_balance // increment * adjusted_total_slashing_balance
+//	            penalty = penalty_numerator // total_balance * increment
+//	            decrease_balance(state, ValidatorIndex(index), penalty)
+//
+// Phase0 spec definition:
+//
+//	def process_slashings(state: BeaconState) -> None:
+//	    epoch = get_current_epoch(state)
+//	    total_balance = get_total_active_balance(state)
+//	    adjusted_total_slashing_balance = min(sum(state.slashings) * PROPORTIONAL_SLASHING_MULTIPLIER, total_balance)
+//	    for index, validator in enumerate(state.validators):
+//	        if validator.slashed and epoch + EPOCHS_PER_SLASHINGS_VECTOR // 2 == validator.withdrawable_epoch:
+//	            increment = EFFECTIVE_BALANCE_INCREMENT  # Factored out from penalty numerator to avoid uint64 overflow
+//	            penalty_numerator = validator.effective_balance // increment * adjusted_total_slashing_balance
+//	            penalty = penalty_numerator // total_balance * increment
+//	            decrease_balance(state, ValidatorIndex(index), penalty)
+func ProcessSlashings(st state.BeaconState) error {
 	slashingMultiplier, err := st.ProportionalSlashingMultiplier()
 	if err != nil {
-		return nil, errors.Wrap(err, "could not get proportional slashing multiplier")
+		return errors.Wrap(err, "could not get proportional slashing multiplier")
 	}
 	currentEpoch := time.CurrentEpoch(st)
 	totalBalance, err := helpers.TotalActiveBalance(st)
 	if err != nil {
-		return nil, errors.Wrap(err, "could not get total active balance")
+		return errors.Wrap(err, "could not get total active balance")
 	}
 
 	// Compute slashed balances in the current epoch
@@ -166,7 +222,7 @@ func ProcessSlashings(st state.BeaconState) (state.BeaconState, error) {
 	for _, slashing := range slashings {
 		totalSlashing, err = math.Add64(totalSlashing, slashing)
 		if err != nil {
-			return nil, err
+			return err
 		}
 	}
 
@@ -200,14 +256,14 @@ func ProcessSlashings(st state.BeaconState) (state.BeaconState, error) {
 		return nil
 	})
 	if err != nil {
-		return nil, err
+		return err
 	}
 	if changed {
 		if err := st.SetBalances(bals); err != nil {
-			return nil, err
+			return err
 		}
 	}
-	return st, nil
+	return nil
 }
 
 // ProcessEth1DataReset processes updates to ETH1 data votes during epoch processing.

beacon-chain/core/epoch/epoch_processing_test.go

@@ -32,10 +32,9 @@ func TestProcessSlashings_NotSlashed(t *testing.T) {
 	}
 	s, err := state_native.InitializeFromProtoPhase0(base)
 	require.NoError(t, err)
-	newState, err := epoch.ProcessSlashings(s)
-	require.NoError(t, err)
+	require.NoError(t, epoch.ProcessSlashings(s))
 	wanted := params.BeaconConfig().MaxEffectiveBalance
-	assert.Equal(t, wanted, newState.Balances()[0], "Unexpected slashed balance")
+	assert.Equal(t, wanted, s.Balances()[0], "Unexpected slashed balance")
 }
 
 func TestProcessSlashings_SlashedLess(t *testing.T) {
@@ -111,9 +110,8 @@ func TestProcessSlashings_SlashedLess(t *testing.T) {
 			s, err := state_native.InitializeFromProtoPhase0(tt.state)
 			require.NoError(t, err)
 			helpers.ClearCache()
-			newState, err := epoch.ProcessSlashings(s)
-			require.NoError(t, err)
-			assert.Equal(t, tt.want, newState.Balances()[0], "ProcessSlashings({%v}) = newState; newState.Balances[0] = %d", original, newState.Balances()[0])
+			require.NoError(t, epoch.ProcessSlashings(s))
+			assert.Equal(t, tt.want, s.Balances()[0], "ProcessSlashings({%v}) = newState; newState.Balances[0] = %d", original, s.Balances()[0])
 		})
 	}
 }
@@ -365,8 +363,7 @@ func TestProcessSlashings_BadValue(t *testing.T) {
 	}
 	s, err := state_native.InitializeFromProtoPhase0(base)
 	require.NoError(t, err)
-	_, err = epoch.ProcessSlashings(s)
-	require.ErrorContains(t, "addition overflows", err)
+	require.ErrorContains(t, "addition overflows", epoch.ProcessSlashings(s))
 }
 
 func TestProcessHistoricalDataUpdate(t *testing.T) {
@@ -514,9 +511,8 @@ func TestProcessSlashings_SlashedElectra(t *testing.T) {
 			s, err := state_native.InitializeFromProtoElectra(tt.state)
 			require.NoError(t, err)
 			helpers.ClearCache()
-			newState, err := epoch.ProcessSlashings(s)
-			require.NoError(t, err)
-			assert.Equal(t, tt.want, newState.Balances()[0], "ProcessSlashings({%v}) = newState; newState.Balances[0] = %d", original, newState.Balances()[0])
+			require.NoError(t, epoch.ProcessSlashings(s))
+			assert.Equal(t, tt.want, s.Balances()[0], "ProcessSlashings({%v}); s.Balances[0] = %d", original, s.Balances()[0])
 		})
 	}
 }

testing/spectest/shared/altair/epoch_processing/slashings.go

@@ -29,7 +29,6 @@ func RunSlashingsTests(t *testing.T, config string) {
 }
 
 func processSlashingsWrapper(t *testing.T, st state.BeaconState) (state.BeaconState, error) {
-	st, err := epoch.ProcessSlashings(st)
-	require.NoError(t, err, "Could not process slashings")
+	require.NoError(t, epoch.ProcessSlashings(st), "Could not process slashings")
 	return st, nil
 }

testing/spectest/shared/bellatrix/epoch_processing/slashings.go

@@ -29,7 +29,6 @@ func RunSlashingsTests(t *testing.T, config string) {
 }
 
 func processSlashingsWrapper(t *testing.T, st state.BeaconState) (state.BeaconState, error) {
-	st, err := epoch.ProcessSlashings(st)
-	require.NoError(t, err, "Could not process slashings")
+	require.NoError(t, epoch.ProcessSlashings(st), "Could not process slashings")
 	return st, nil
 }

testing/spectest/shared/capella/epoch_processing/slashings.go

@@ -29,7 +29,6 @@ func RunSlashingsTests(t *testing.T, config string) {
 }
 
 func processSlashingsWrapper(t *testing.T, st state.BeaconState) (state.BeaconState, error) {
-	st, err := epoch.ProcessSlashings(st)
-	require.NoError(t, err, "Could not process slashings")
+	require.NoError(t, epoch.ProcessSlashings(st), "Could not process slashings")
 	return st, nil
 }

testing/spectest/shared/deneb/epoch_processing/slashings.go

@@ -26,7 +26,6 @@ func RunSlashingsTests(t *testing.T, config string) {
 }
 
 func processSlashingsWrapper(t *testing.T, st state.BeaconState) (state.BeaconState, error) {
-	st, err := epoch.ProcessSlashings(st)
-	require.NoError(t, err, "Could not process slashings")
+	require.NoError(t, epoch.ProcessSlashings(st), "Could not process slashings")
 	return st, nil
 }

testing/spectest/shared/electra/epoch_processing/slashings.go

@@ -26,7 +26,6 @@ func RunSlashingsTests(t *testing.T, config string) {
 }
 
 func processSlashingsWrapper(t *testing.T, st state.BeaconState) (state.BeaconState, error) {
-	st, err := electra.ProcessSlashings(st)
-	require.NoError(t, err, "Could not process slashings")
+	require.NoError(t, electra.ProcessSlashings(st), "Could not process slashings")
 	return st, nil
 }

testing/spectest/shared/phase0/epoch_processing/slashings.go

@@ -31,10 +31,9 @@ func RunSlashingsTests(t *testing.T, config string) {
 	}
 }
 
-func processSlashingsWrapper(t *testing.T, s state.BeaconState) (state.BeaconState, error) {
-	s, err := epoch.ProcessSlashings(s)
-	require.NoError(t, err, "Could not process slashings")
-	return s, nil
+func processSlashingsWrapper(t *testing.T, st state.BeaconState) (state.BeaconState, error) {
+	require.NoError(t, epoch.ProcessSlashings(st), "Could not process slashings")
+	return st, nil
 }
 
 func processSlashingsPrecomputeWrapper(t *testing.T, state state.BeaconState) (state.BeaconState, error) {