From 26b276660fe45015c5ba5c3b45c72042ef63d5e0 Mon Sep 17 00:00:00 2001 From: Potuz Date: Wed, 1 Oct 2025 21:17:39 -0300 Subject: [PATCH] Avoid unnecessary calls to ExitInformation() (#15764) * Avoid unnecessary calls to ExitInformation() ExitInformation runs a loop over the whole validator set. This is needed in case that there are slashings or exits to be processed in a block (we could be caching or avoid this entirely post-Electra though). This PR removes these calls on normal state transition to this function. h/t to @terencechain for finding out this bug. In addition, on processing withdrawal requests and registry updates, we kept recomputing the exit information at the same time that the state is updated and the function that updates the state already takes care of tracking and updating the right exit information. So this PR removes the calls to compute this exit information on a loop. Notice that this bug has been present even before we had a function `ExitInformation()` so I will document here to help the reviewer Our previous behavior is to do this in a loop ``` st, err = validators.InitiateValidatorExit(ctx, st, vIdx, validators.ExitInformation(st)) ``` This is a bit problematic since `ExitInformation` loops over the whole validator set to compute the exit information (and the total active balance) and then the function `InitiateValidatorExit` actually recomputes the total active balance looping again over the whole validator set and overwriting the pointer returned by `ExitInformation`. On the other hand, the funciton `InitiateValidatorExit` does mutate the state `st` itself. So each call to `ExitInformation(st)` may actually return a different pointer. The function ExitInformation computes as follows ``` err := s.ReadFromEveryValidator(func(idx int, val state.ReadOnlyValidator) error { e := val.ExitEpoch() if e != farFutureEpoch { if e > exitInfo.HighestExitEpoch { exitInfo.HighestExitEpoch = e exitInfo.Churn = 1 } else if e == exitInfo.HighestExitEpoch { exitInfo.Churn++ } ``` So it simply increases the churn for each validator that has epoch equal to the highest exit epoch. The function `InitiateValidatorExit` mutates this pointer in the following way if the state is post-electra, it disregards completely this pointer and computes the highest exit epoch and updates churn inconditionally, so the pointer `exitInfo.HighestExitEpoch` will always have the right value and is not even neded to be computed before. We could even avoid the fist loop even. If the state is pre-Electra then the function itself updates correctly the exit info for the next iteration. * Only care about exits pre-Electra * Update beacon-chain/core/transition/transition_no_verify_sig.go Co-authored-by: terence * Radek's review --------- Co-authored-by: terence --- beacon-chain/core/blocks/attester_slashing.go | 6 +++++ beacon-chain/core/blocks/exit.go | 3 +++ beacon-chain/core/blocks/proposer_slashing.go | 6 +++++ .../core/electra/transition_no_verify_sig.go | 12 ++++++--- beacon-chain/core/electra/withdrawals.go | 16 ++++++++++- beacon-chain/core/epoch/epoch_processing.go | 17 +++++++----- .../transition/transition_no_verify_sig.go | 27 ++++++++++++++----- beacon-chain/core/validators/validator.go | 11 ++++++-- beacon-chain/rpc/eth/rewards/service.go | 6 ++++- .../v1alpha1/validator/proposer_slashings.go | 13 +++++---- changelog/potuz_avoid_exit_info.md | 3 +++ 11 files changed, 95 insertions(+), 25 deletions(-) create mode 100644 changelog/potuz_avoid_exit_info.md diff --git a/beacon-chain/core/blocks/attester_slashing.go b/beacon-chain/core/blocks/attester_slashing.go index ea70a90027..80d95f3973 100644 --- a/beacon-chain/core/blocks/attester_slashing.go +++ b/beacon-chain/core/blocks/attester_slashing.go @@ -42,6 +42,9 @@ func ProcessAttesterSlashings( slashings []ethpb.AttSlashing, exitInfo *validators.ExitInfo, ) (state.BeaconState, error) { + if exitInfo == nil && len(slashings) > 0 { + return nil, errors.New("exit info required to process attester slashings") + } var err error for _, slashing := range slashings { beaconState, err = ProcessAttesterSlashing(ctx, beaconState, slashing, exitInfo) @@ -59,6 +62,9 @@ func ProcessAttesterSlashing( slashing ethpb.AttSlashing, exitInfo *validators.ExitInfo, ) (state.BeaconState, error) { + if exitInfo == nil { + return nil, errors.New("exit info is required to process attester slashing") + } if err := VerifyAttesterSlashing(ctx, beaconState, slashing); err != nil { return nil, errors.Wrap(err, "could not verify attester slashing") } diff --git a/beacon-chain/core/blocks/exit.go b/beacon-chain/core/blocks/exit.go index d60398c4c0..954fb7485a 100644 --- a/beacon-chain/core/blocks/exit.go +++ b/beacon-chain/core/blocks/exit.go @@ -55,6 +55,9 @@ func ProcessVoluntaryExits( if len(exits) == 0 { return beaconState, nil } + if exitInfo == nil { + return nil, errors.New("exit info required to process voluntary exits") + } for idx, exit := range exits { if exit == nil || exit.Exit == nil { return nil, errors.New("nil voluntary exit in block body") diff --git a/beacon-chain/core/blocks/proposer_slashing.go b/beacon-chain/core/blocks/proposer_slashing.go index 850ff98eb1..f45ae2fc97 100644 --- a/beacon-chain/core/blocks/proposer_slashing.go +++ b/beacon-chain/core/blocks/proposer_slashing.go @@ -51,6 +51,9 @@ func ProcessProposerSlashings( slashings []*ethpb.ProposerSlashing, exitInfo *validators.ExitInfo, ) (state.BeaconState, error) { + if exitInfo == nil && len(slashings) > 0 { + return nil, errors.New("exit info required to process proposer slashings") + } var err error for _, slashing := range slashings { beaconState, err = ProcessProposerSlashing(ctx, beaconState, slashing, exitInfo) @@ -75,6 +78,9 @@ func ProcessProposerSlashing( if err = VerifyProposerSlashing(beaconState, slashing); err != nil { return nil, errors.Wrap(err, "could not verify proposer slashing") } + if exitInfo == nil { + return nil, errors.New("exit info is required to process proposer slashing") + } beaconState, err = validators.SlashValidator(ctx, beaconState, slashing.Header_1.Header.ProposerIndex, exitInfo) if err != nil { return nil, errors.Wrapf(err, "could not slash proposer index %d", slashing.Header_1.Header.ProposerIndex) diff --git a/beacon-chain/core/electra/transition_no_verify_sig.go b/beacon-chain/core/electra/transition_no_verify_sig.go index 04338031bd..77a11a0881 100644 --- a/beacon-chain/core/electra/transition_no_verify_sig.go +++ b/beacon-chain/core/electra/transition_no_verify_sig.go @@ -53,9 +53,15 @@ func ProcessOperations(ctx context.Context, st state.BeaconState, block interfac // 6110 validations are in VerifyOperationLengths bb := block.Body() // Electra extends the altair operations. - exitInfo := v.ExitInformation(st) - if err := helpers.UpdateTotalActiveBalanceCache(st, exitInfo.TotalActiveBalance); err != nil { - return nil, errors.Wrap(err, "could not update total active balance cache") + var exitInfo *v.ExitInfo + hasSlashings := len(bb.ProposerSlashings()) > 0 || len(bb.AttesterSlashings()) > 0 + hasExits := len(bb.VoluntaryExits()) > 0 + if hasSlashings || hasExits { + // ExitInformation is expensive to compute, only do it if we need it. + exitInfo = v.ExitInformation(st) + if err := helpers.UpdateTotalActiveBalanceCache(st, exitInfo.TotalActiveBalance); err != nil { + return nil, errors.Wrap(err, "could not update total active balance cache") + } } st, err = ProcessProposerSlashings(ctx, st, bb.ProposerSlashings(), exitInfo) if err != nil { diff --git a/beacon-chain/core/electra/withdrawals.go b/beacon-chain/core/electra/withdrawals.go index 5f78ab0c29..9cfbcc4641 100644 --- a/beacon-chain/core/electra/withdrawals.go +++ b/beacon-chain/core/electra/withdrawals.go @@ -13,6 +13,7 @@ import ( "github.com/OffchainLabs/prysm/v6/monitoring/tracing/trace" enginev1 "github.com/OffchainLabs/prysm/v6/proto/engine/v1" ethpb "github.com/OffchainLabs/prysm/v6/proto/prysm/v1alpha1" + "github.com/OffchainLabs/prysm/v6/runtime/version" "github.com/OffchainLabs/prysm/v6/time/slots" "github.com/ethereum/go-ethereum/common/hexutil" "github.com/pkg/errors" @@ -91,6 +92,18 @@ func ProcessWithdrawalRequests(ctx context.Context, st state.BeaconState, wrs [] ctx, span := trace.StartSpan(ctx, "electra.ProcessWithdrawalRequests") defer span.End() currentEpoch := slots.ToEpoch(st.Slot()) + if len(wrs) == 0 { + return st, nil + } + // It is correct to compute exitInfo once for all withdrawals in the block, as the ExitInfo pointer is + // updated within InitiateValidatorExit which is the only function that uses it. + var exitInfo *validators.ExitInfo + if st.Version() < version.Electra { + exitInfo = validators.ExitInformation(st) + } else { + // After Electra, the function InitiateValidatorExit ignores the exitInfo passed to it and recomputes it anyway. + exitInfo = &validators.ExitInfo{} + } for _, wr := range wrs { if wr == nil { return nil, errors.New("nil execution layer withdrawal request") @@ -148,7 +161,8 @@ func ProcessWithdrawalRequests(ctx context.Context, st state.BeaconState, wrs [] // Only exit validator if it has no pending withdrawals in the queue if pendingBalanceToWithdraw == 0 { var err error - st, err = validators.InitiateValidatorExit(ctx, st, vIdx, validators.ExitInformation(st)) + // exitInfo is updated within InitiateValidatorExit + st, err = validators.InitiateValidatorExit(ctx, st, vIdx, exitInfo) if err != nil { return nil, err } diff --git a/beacon-chain/core/epoch/epoch_processing.go b/beacon-chain/core/epoch/epoch_processing.go index 24ce0579d9..a061a3da29 100644 --- a/beacon-chain/core/epoch/epoch_processing.go +++ b/beacon-chain/core/epoch/epoch_processing.go @@ -96,12 +96,17 @@ func ProcessRegistryUpdates(ctx context.Context, st state.BeaconState) (state.Be } // Process validators eligible for ejection. - for _, idx := range eligibleForEjection { - // Here is fine to do a quadratic loop since this should - // barely happen - st, err = validators.InitiateValidatorExit(ctx, st, idx, validators.ExitInformation(st)) - if err != nil && !errors.Is(err, validators.ErrValidatorAlreadyExited) { - return nil, errors.Wrapf(err, "could not initiate exit for validator %d", idx) + if len(eligibleForEjection) > 0 { + // It is safe to compute exitInfo once for all ejections in the epoch, as the ExitInfo pointer is + // updated within InitiateValidatorExit which is the only function that uses it. + exitInfo := validators.ExitInformation(st) + for _, idx := range eligibleForEjection { + // Here is fine to do a quadratic loop since this should + // barely happen + st, err = validators.InitiateValidatorExit(ctx, st, idx, exitInfo) + if err != nil && !errors.Is(err, validators.ErrValidatorAlreadyExited) { + return nil, errors.Wrapf(err, "could not initiate exit for validator %d", idx) + } } } diff --git a/beacon-chain/core/transition/transition_no_verify_sig.go b/beacon-chain/core/transition/transition_no_verify_sig.go index 4bf9d89f48..4cfa37a789 100644 --- a/beacon-chain/core/transition/transition_no_verify_sig.go +++ b/beacon-chain/core/transition/transition_no_verify_sig.go @@ -10,6 +10,7 @@ import ( "github.com/OffchainLabs/prysm/v6/beacon-chain/core/electra" "github.com/OffchainLabs/prysm/v6/beacon-chain/core/helpers" "github.com/OffchainLabs/prysm/v6/beacon-chain/core/transition/interop" + "github.com/OffchainLabs/prysm/v6/beacon-chain/core/validators" v "github.com/OffchainLabs/prysm/v6/beacon-chain/core/validators" "github.com/OffchainLabs/prysm/v6/beacon-chain/state" "github.com/OffchainLabs/prysm/v6/consensus-types/blocks" @@ -378,9 +379,16 @@ func ProcessBlockForStateRoot( func altairOperations(ctx context.Context, st state.BeaconState, beaconBlock interfaces.ReadOnlyBeaconBlock) (state.BeaconState, error) { var err error - exitInfo := v.ExitInformation(st) - if err := helpers.UpdateTotalActiveBalanceCache(st, exitInfo.TotalActiveBalance); err != nil { - return nil, errors.Wrap(err, "could not update total active balance cache") + hasSlashings := len(beaconBlock.Body().ProposerSlashings()) > 0 || len(beaconBlock.Body().AttesterSlashings()) > 0 + // exitInfo is only needed for voluntary exits pre Electra. + hasExits := st.Version() < version.Electra && len(beaconBlock.Body().VoluntaryExits()) > 0 + exitInfo := &validators.ExitInfo{} + if hasSlashings || hasExits { + // ExitInformation is expensive to compute, only do it if we need it. + exitInfo = v.ExitInformation(st) + if err := helpers.UpdateTotalActiveBalanceCache(st, exitInfo.TotalActiveBalance); err != nil { + return nil, errors.Wrap(err, "could not update total active balance cache") + } } st, err = b.ProcessProposerSlashings(ctx, st, beaconBlock.Body().ProposerSlashings(), exitInfo) if err != nil { @@ -407,10 +415,15 @@ func altairOperations(ctx context.Context, st state.BeaconState, beaconBlock int // This calls phase 0 block operations. func phase0Operations(ctx context.Context, st state.BeaconState, beaconBlock interfaces.ReadOnlyBeaconBlock) (state.BeaconState, error) { var err error - - exitInfo := v.ExitInformation(st) - if err := helpers.UpdateTotalActiveBalanceCache(st, exitInfo.TotalActiveBalance); err != nil { - return nil, errors.Wrap(err, "could not update total active balance cache") + hasSlashings := len(beaconBlock.Body().ProposerSlashings()) > 0 || len(beaconBlock.Body().AttesterSlashings()) > 0 + hasExits := len(beaconBlock.Body().VoluntaryExits()) > 0 + var exitInfo *v.ExitInfo + if hasSlashings || hasExits { + // ExitInformation is expensive to compute, only do it if we need it. + exitInfo = v.ExitInformation(st) + if err := helpers.UpdateTotalActiveBalanceCache(st, exitInfo.TotalActiveBalance); err != nil { + return nil, errors.Wrap(err, "could not update total active balance cache") + } } st, err = b.ProcessProposerSlashings(ctx, st, beaconBlock.Body().ProposerSlashings(), exitInfo) if err != nil { diff --git a/beacon-chain/core/validators/validator.go b/beacon-chain/core/validators/validator.go index 29b56277b0..25b2f0f62b 100644 --- a/beacon-chain/core/validators/validator.go +++ b/beacon-chain/core/validators/validator.go @@ -98,7 +98,9 @@ func InitiateValidatorExit( if validator.ExitEpoch != params.BeaconConfig().FarFutureEpoch { return s, ErrValidatorAlreadyExited } - + if exitInfo == nil { + return nil, errors.New("exit info is required to process validator exit") + } // Compute exit queue epoch. if s.Version() < version.Electra { if err = initiateValidatorExitPreElectra(ctx, s, exitInfo); err != nil { @@ -177,6 +179,9 @@ func initiateValidatorExitPreElectra(ctx context.Context, s state.BeaconState, e // if exit_queue_churn >= get_validator_churn_limit(state): // exit_queue_epoch += Epoch(1) exitableEpoch := helpers.ActivationExitEpoch(time.CurrentEpoch(s)) + if exitInfo == nil { + return errors.New("exit info is required to process validator exit") + } if exitableEpoch > exitInfo.HighestExitEpoch { exitInfo.HighestExitEpoch = exitableEpoch exitInfo.Churn = 0 @@ -235,7 +240,9 @@ func SlashValidator( exitInfo *ExitInfo, ) (state.BeaconState, error) { var err error - + if exitInfo == nil { + return nil, errors.New("exit info is required to slash validator") + } s, err = InitiateValidatorExitForTotalBal(ctx, s, slashedIdx, exitInfo, primitives.Gwei(exitInfo.TotalActiveBalance)) if err != nil && !errors.Is(err, ErrValidatorAlreadyExited) { return nil, errors.Wrapf(err, "could not initiate validator %d exit", slashedIdx) diff --git a/beacon-chain/rpc/eth/rewards/service.go b/beacon-chain/rpc/eth/rewards/service.go index 0383c9e892..6e36451848 100644 --- a/beacon-chain/rpc/eth/rewards/service.go +++ b/beacon-chain/rpc/eth/rewards/service.go @@ -68,7 +68,11 @@ func (rs *BlockRewardService) GetBlockRewardsData(ctx context.Context, blk inter Code: http.StatusInternalServerError, } } - exitInfo := validators.ExitInformation(st) + var exitInfo *validators.ExitInfo + if len(blk.Body().ProposerSlashings()) > 0 || len(blk.Body().AttesterSlashings()) > 0 { + // ExitInformation is expensive to compute, only do it if we need it. + exitInfo = validators.ExitInformation(st) + } st, err = coreblocks.ProcessAttesterSlashings(ctx, st, blk.Body().AttesterSlashings(), exitInfo) if err != nil { return nil, &httputil.DefaultJsonError{ diff --git a/beacon-chain/rpc/prysm/v1alpha1/validator/proposer_slashings.go b/beacon-chain/rpc/prysm/v1alpha1/validator/proposer_slashings.go index 21fa2721d2..dd8ff2f5e4 100644 --- a/beacon-chain/rpc/prysm/v1alpha1/validator/proposer_slashings.go +++ b/beacon-chain/rpc/prysm/v1alpha1/validator/proposer_slashings.go @@ -12,13 +12,18 @@ import ( func (vs *Server) getSlashings(ctx context.Context, head state.BeaconState) ([]*ethpb.ProposerSlashing, []ethpb.AttSlashing) { var err error - + proposerSlashings := vs.SlashingsPool.PendingProposerSlashings(ctx, head, false /*noLimit*/) + attSlashings := vs.SlashingsPool.PendingAttesterSlashings(ctx, head, false /*noLimit*/) + validProposerSlashings := make([]*ethpb.ProposerSlashing, 0, len(proposerSlashings)) + validAttSlashings := make([]ethpb.AttSlashing, 0, len(attSlashings)) + if len(proposerSlashings) == 0 && len(attSlashings) == 0 { + return validProposerSlashings, validAttSlashings + } + // ExitInformation is expensive to compute, only do it if we need it. exitInfo := v.ExitInformation(head) if err := helpers.UpdateTotalActiveBalanceCache(head, exitInfo.TotalActiveBalance); err != nil { log.WithError(err).Warn("Could not update total active balance cache") } - proposerSlashings := vs.SlashingsPool.PendingProposerSlashings(ctx, head, false /*noLimit*/) - validProposerSlashings := make([]*ethpb.ProposerSlashing, 0, len(proposerSlashings)) for _, slashing := range proposerSlashings { _, err = blocks.ProcessProposerSlashing(ctx, head, slashing, exitInfo) if err != nil { @@ -27,8 +32,6 @@ func (vs *Server) getSlashings(ctx context.Context, head state.BeaconState) ([]* } validProposerSlashings = append(validProposerSlashings, slashing) } - attSlashings := vs.SlashingsPool.PendingAttesterSlashings(ctx, head, false /*noLimit*/) - validAttSlashings := make([]ethpb.AttSlashing, 0, len(attSlashings)) for _, slashing := range attSlashings { _, err = blocks.ProcessAttesterSlashing(ctx, head, slashing, exitInfo) if err != nil { diff --git a/changelog/potuz_avoid_exit_info.md b/changelog/potuz_avoid_exit_info.md new file mode 100644 index 0000000000..b21477b397 --- /dev/null +++ b/changelog/potuz_avoid_exit_info.md @@ -0,0 +1,3 @@ +### Fixed + +- Avoid unnecessary calls to `ExitInformation()`.