Handle errors as no-op for execution requests #14826

terencechain · 2025-01-25T17:19:47Z

In Electra, when processing execution requests, any failure—except for malformed requests (i.e., nil)—should be treated as a no-op per consensus specifications. However, there's some ambiguity regarding what constitutes a failure:

The execution request is invalid.
A bug in the client code processing the execution request.

While (1) has been addressed, (2) remains unclear in terms of whether it should be treated as a no-op or a failure state transition. I argue that it should be treated as a no-op because triggering a state transition failure would result in the block being marked invalid and added to the invalid block cache. This could lead to a fork from other consensus layer implementations. Such a scenario might arise if, for example, a bug exists in an Electra helper function or if a bug is introduced in future updates. Treating all failures as no-ops, except for malformed requests, provides a much safer approach.

What this PR accomplishes:

Errors are only triggered for nil requests.
For other failures, errors are logged, and processing continues. Logging errors is more appropriate in this context, as these cases don't indicate a fundamental implementation failure.
Added safeguards to the process option to only trigger errors for nil requests, preventing potential issues from being introduced later.

james-prysm · 2025-01-27T17:02:14Z

beacon-chain/core/electra/transition_no_verify_sig.go

@@ -84,14 +85,20 @@ func ProcessOperations(
 	}
 	st, err = ProcessDepositRequests(ctx, st, requests.Deposits)
 	if err != nil {
-		return nil, errors.Wrap(err, "could not process deposit requests")
+		if errors.Is(err, errNilExecutionRequest) {


no logs required here? because they are processed inside?

it's logged inside ProcessDepositRequests

james-prysm · 2025-01-27T17:06:23Z

beacon-chain/core/electra/withdrawals.go

 		}
 		amount := wr.Amount
 		isFullExitRequest := amount == params.BeaconConfig().FullExitRequestAmount
 		// If partial withdrawal queue is full, only full exits are processed
 		if n, err := st.NumPendingPartialWithdrawals(); err != nil {
-			return nil, err
+			log.WithError(err).Error("Could not get number of pending partial withdrawals")


i guess this and the logs below would potentially print many times for what isn't really affected by the loop above. wouldn't all of these continually fail? shouldn't this just print once?

we should probably have a way to wrap and return this error so it only logs once, unless we want to print multiple times in which case we should add fields for the unique identifier of the request that caused it.

james-prysm · 2025-01-27T17:06:43Z

beacon-chain/core/electra/withdrawals.go

@@ -113,7 +113,8 @@ func ProcessWithdrawalRequests(ctx context.Context, st state.BeaconState, wrs []
 		}
 		validator, err := st.ValidatorAtIndexReadOnly(vIdx)
 		if err != nil {
-			return nil, err
+			log.WithError(err).Error("Could not get validator at index")


if it exists in validator index by pubkey can it actually fail here?

if it is needed we can provide some of the request information along with the log

james-prysm · 2025-01-27T20:17:43Z

beacon-chain/core/electra/consolidations.go

@@ -209,7 +209,7 @@ func ProcessConsolidationRequests(ctx context.Context, st state.BeaconState, req
 		}

 		if npc, err := st.NumPendingConsolidations(); err != nil {
-			log.WithError(err).Error("failed to fetch number of pending consolidations")
+			log.WithError(err).Errorf("failed to fetch number of pending consolidations at index %d", i)


would it be better to use .WithField("index",i) instead?

jtraglia

In Electra, when processing execution requests, any failure—except for malformed requests (i.e., nil)—should be treated as a no-op per consensus specifications. However, there's some ambiguity regarding what constitutes a failure.

I don't believe the specifications are ambiguous here. See the comment I left below. If there's an error which should never happen (eg uint64 overflow), the specifications will throw an exception. It will not just continue.

I argue that it should be treated as a no-op because triggering a state transition failure would result in the block being marked invalid and added to the invalid block cache. This could lead to a fork from other consensus layer implementations.

I think I agree with this, but I'm not entirely sure. I originally brought this up (in the doc I shared with the team) because I noticed inconsistent uses of return and continue in a few spots. It seemed that the dominant pattern was to return when the error should never happen. This made sense to me. But marking the block as invalid and adding it to the invalid block cache does feel dangerous. Wouldn't there be a consensus error in either situation (return vs continue)?

jtraglia · 2025-01-27T20:40:39Z

beacon-chain/core/electra/withdrawals.go

 			withdrawableEpoch, err := exitQueueEpoch.SafeAddEpoch(params.BeaconConfig().MinValidatorWithdrawabilityDelay)
 			if err != nil {
-				return nil, errors.Wrap(err, "failed to add withdrawability delay to exit queue epoch")
+				log.WithError(err).Error("Could not compute withdrawable epoch")
+				continue


For example, this really never should happen. The specification would throw an exception here too.

... tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_consolidation_request.py:1221: in run_consolidation_processing spec.process_consolidation_request(state, consolidation) tests/core/pyspec/eth2spec/electra/minimal.py:5469: in process_consolidation_request if current_epoch < source_validator.activation_epoch + config.SHARD_COMMITTEE_PERIOD: venv/lib/python3.13/site-packages/remerkleable/basic.py:88: in __add__ return self.__class__(super().__add__(self.__class__.coerce_view(other))) cls = <class 'eth2spec.electra.minimal.Epoch'>, value = 18446744073709551679 def __new__(cls, value: int): if value < 0: raise ValueError(f"unsigned type {cls} must not be negative") byte_len = cls.type_byte_length() if value.bit_length() > (byte_len << 3): > raise ValueError(f"value out of bounds for {cls}") E ValueError: value out of bounds for <class 'eth2spec.electra.minimal.Epoch'>

Edit: wrong function but it's the same idea.

terencechain · 2025-01-27T21:19:57Z

I don't believe the specifications are ambiguous here

Sorry, I meant client implementation.

Wouldn't there be a consensus error in either situation (return vs continue)?

I think a client implementation bug is more likely to happen than something going out of bounds. The point is, if a client bug exists, it shouldn't be a footgun that causes the block to become invalid. An invalid block is the absolute worst-case scenario here, and we should avoid it

jtraglia · 2025-01-27T21:30:16Z

An invalid block is the absolute worst-case scenario here, and we should avoid it.

This makes sense to me. With this in mind, the PR looks good to me.

potuz

There are different kinds of errors that may happen on state transition:

A context deadline (we should check this and not declare the block invalid in this case) The best option I see is to make this change in validateStateTransition directly, and instead of marking the block as invalid check first if ctx.Err() != nil
An error on Prysm when processing, for example we can't get some fields or can't access some state required etc. These errors do not immediately indicate that the block is invalid, but continuing processing will alter the resulting state root and therefore declaring the block root as invalid later on. We should stop on these and not declare the block as invalid.
Errors because the block actually fails to process, these should be marked as invalid.

potuz · 2025-01-29T10:56:25Z

beacon-chain/core/electra/consolidations.go

 		} else if npc >= pcLimit {
 			return nil
 		}

 		activeBal, err := helpers.TotalActiveBalance(st)
 		if err != nil {
-			return err
+			log.WithError(err).Error("failed to fetch total active balance")


I think for these kinds of errors the request may be valid and we fail to get the active balance for whatever reason, by continuing we would have a consensus split anyway since the state root will fail to match. I think we need to return an error and not mark the block as invalid though.

beacon-chain/core/electra/error.go

prestonvanloon

Please add tests for this scenario

prestonvanloon · 2025-01-30T15:24:31Z

beacon-chain/core/electra/transition_no_verify_sig.go

+	for _, d := range requests.Deposits {
+		if d == nil {
+			return nil, errors.New("nil deposit request")
+		}
+	}


Shouldn't this validation logic be in ProcessDepositRequests?

The point of this PR is to separate out the error. ProcessDepositRequests should returns error typed execReqErr which that caller can handle it appropiately

I disagree with this. ProcessOperations should not be doing any input validations.

Discussed offline, this is planned to be reworked in a later change.

potuz · 2025-01-30T15:48:25Z

beacon-chain/blockchain/receive_block.go

+		if electra.IsExecutionRequestError(err) {
+			return nil, err
+		}


I wouldn't check this here, this leaks internal logic up the stack, instead, if you keep the error in ProcessOperations, you can check on the caller.

potuz · 2025-01-30T17:31:37Z

beacon-chain/core/electra/transition_no_verify_sig.go

 	}
 	st, err = ProcessWithdrawalRequests(ctx, st, requests.Withdrawals)
 	if err != nil {
-		return nil, errors.Wrap(err, "could not process withdrawal requests")
+		return nil, execReqErr{errors.Wrap(err, "could not process withdrawal requests")}


why not simply deal with this logic within the core function. If the spec expects you to simply continue on these cases, why not ditch the request right there and not return an error?

terencechain · 2025-01-30T18:01:28Z

We'll close this and we figured out a better solution. A new one will be opened soon

prestonvanloon · 2025-01-31T16:22:03Z

beacon-chain/core/electra/withdrawals.go

-		if wr == nil {
-			return nil, errors.New("nil execution layer withdrawal request")
-		}


Why remove this?

prestonvanloon · 2025-01-31T16:22:52Z

beacon-chain/core/electra/transition_no_verify_sig.go

+	for _, d := range requests.Deposits {
+		if d == nil {
+			return nil, errors.New("nil deposit request")
+		}
+	}


I disagree with this. ProcessOperations should not be doing any input validations.

prestonvanloon · 2025-01-31T22:01:04Z

beacon-chain/core/electra/transition_no_verify_sig.go

+	for _, d := range requests.Deposits {
+		if d == nil {
+			return nil, errors.New("nil deposit request")
+		}
+	}


Discussed offline, this is planned to be reworked in a later change.

prestonvanloon · 2025-01-31T22:01:22Z

beacon-chain/core/electra/transition_no_verify_sig_test.go

+	"github.com/prysmaticlabs/prysm/v5/testing/util"
+)
+
+func TestProcessOperationsWithNilRequests(t *testing.T) {


Thanks, this is a good start for testing this method.

terencechain requested a review from a team as a code owner January 25, 2025 17:19

terencechain requested review from kasey, saolyn and dB2510 January 25, 2025 17:19

james-prysm reviewed Jan 27, 2025

View reviewed changes

terencechain force-pushed the execution-reqs branch 2 times, most recently from 3ecd5c0 to a354632 Compare January 27, 2025 18:23

james-prysm reviewed Jan 27, 2025

View reviewed changes

jtraglia reviewed Jan 27, 2025

View reviewed changes

james-prysm added Electra electra hardfork >1 Approves Required labels Jan 28, 2025

potuz requested changes Jan 29, 2025

View reviewed changes

terencechain force-pushed the execution-reqs branch 2 times, most recently from b423c05 to 1e50745 Compare January 30, 2025 14:09

prestonvanloon reviewed Jan 30, 2025

View reviewed changes

beacon-chain/core/electra/error.go Show resolved Hide resolved

prestonvanloon requested changes Jan 30, 2025

View reviewed changes

prestonvanloon reviewed Jan 30, 2025

View reviewed changes

terencechain force-pushed the execution-reqs branch from 1e50745 to dd3091d Compare January 30, 2025 15:30

potuz reviewed Jan 30, 2025

View reviewed changes

terencechain closed this Jan 30, 2025

terencechain reopened this Jan 31, 2025

terencechain force-pushed the execution-reqs branch 3 times, most recently from 36838e0 to d950af3 Compare January 31, 2025 15:57

prestonvanloon reviewed Jan 31, 2025

View reviewed changes

potuz previously approved these changes Jan 31, 2025

View reviewed changes

terencechain added 2 commits January 31, 2025 10:08

Update electra core processing error handling

9960290

Add test for IsExecutionRequestError

eb8b62e

terencechain dismissed potuz’s stale review via b8c6e72 January 31, 2025 20:33

terencechain force-pushed the execution-reqs branch from d950af3 to b8c6e72 Compare January 31, 2025 20:33

Add TestProcessOperationsWithNilRequests

c86f8fa

terencechain force-pushed the execution-reqs branch from b8c6e72 to c86f8fa Compare January 31, 2025 20:35

prestonvanloon previously approved these changes Jan 31, 2025

View reviewed changes

gazelle

73341cb

prestonvanloon dismissed their stale review via 73341cb January 31, 2025 22:11

prestonvanloon approved these changes Jan 31, 2025

View reviewed changes

prestonvanloon enabled auto-merge January 31, 2025 22:11

prestonvanloon added this pull request to the merge queue Jan 31, 2025

Merged via the queue into develop with commit 910609a Jan 31, 2025
17 checks passed

prestonvanloon deleted the execution-reqs branch January 31, 2025 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle errors as no-op for execution requests #14826

Handle errors as no-op for execution requests #14826

terencechain commented Jan 25, 2025 •

edited

Loading

james-prysm Jan 27, 2025

terencechain Jan 27, 2025

james-prysm Jan 27, 2025 •

edited

Loading

james-prysm Jan 27, 2025

james-prysm Jan 27, 2025

james-prysm Jan 27, 2025

james-prysm Jan 27, 2025

jtraglia left a comment

jtraglia Jan 27, 2025 •

edited

Loading

terencechain commented Jan 27, 2025

jtraglia commented Jan 27, 2025

potuz left a comment

potuz Jan 29, 2025

prestonvanloon left a comment

prestonvanloon Jan 30, 2025

terencechain Jan 31, 2025

prestonvanloon Jan 31, 2025

prestonvanloon Jan 31, 2025

potuz Jan 30, 2025

potuz Jan 30, 2025

terencechain commented Jan 30, 2025

prestonvanloon Jan 31, 2025

prestonvanloon Jan 31, 2025

prestonvanloon Jan 31, 2025

prestonvanloon Jan 31, 2025

Handle errors as no-op for execution requests #14826

Handle errors as no-op for execution requests #14826

Conversation

terencechain commented Jan 25, 2025 • edited Loading

What this PR accomplishes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-prysm Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtraglia left a comment

Choose a reason for hiding this comment

jtraglia Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

terencechain commented Jan 27, 2025

jtraglia commented Jan 27, 2025

potuz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Jan 25, 2025 •

edited

Loading

james-prysm Jan 27, 2025 •

edited

Loading

jtraglia Jan 27, 2025 •

edited

Loading