From 996029013bb740d0e2c78d95e0fdb2f617161dbe Mon Sep 17 00:00:00 2001 From: terence tsao Date: Wed, 29 Jan 2025 05:57:40 -1000 Subject: [PATCH 1/4] Update electra core processing error handling --- beacon-chain/blockchain/BUILD.bazel | 1 + beacon-chain/blockchain/receive_block.go | 3 ++- beacon-chain/core/electra/BUILD.bazel | 1 + beacon-chain/core/electra/error.go | 16 ++++++++++++++ .../core/electra/transition_no_verify_sig.go | 22 +++++++++++++++---- beacon-chain/core/electra/withdrawals.go | 3 --- changelog/tt_fix_electra_core_processing.md | 3 +++ 7 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 beacon-chain/core/electra/error.go create mode 100644 changelog/tt_fix_electra_core_processing.md diff --git a/beacon-chain/blockchain/BUILD.bazel b/beacon-chain/blockchain/BUILD.bazel index 33d653b6028e..78ca6aaf7f7d 100644 --- a/beacon-chain/blockchain/BUILD.bazel +++ b/beacon-chain/blockchain/BUILD.bazel @@ -43,6 +43,7 @@ go_library( "//beacon-chain/cache:go_default_library", "//beacon-chain/core/altair:go_default_library", "//beacon-chain/core/blocks:go_default_library", + "//beacon-chain/core/electra:go_default_library", "//beacon-chain/core/epoch/precompute:go_default_library", "//beacon-chain/core/feed:go_default_library", "//beacon-chain/core/feed/state:go_default_library", diff --git a/beacon-chain/blockchain/receive_block.go b/beacon-chain/blockchain/receive_block.go index 0b4167869ce7..30f89b538bb7 100644 --- a/beacon-chain/blockchain/receive_block.go +++ b/beacon-chain/blockchain/receive_block.go @@ -7,6 +7,7 @@ import ( "time" "github.com/pkg/errors" + "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/electra" "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/feed" statefeed "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/feed/state" "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/helpers" @@ -468,7 +469,7 @@ func (s *Service) validateStateTransition(ctx context.Context, preState state.Be stateTransitionStartTime := time.Now() postState, err := transition.ExecuteStateTransition(ctx, preState, signed) if err != nil { - if ctx.Err() != nil { + if ctx.Err() != nil || electra.IsExecutionRequestError(err) { return nil, err } return nil, invalidBlock{error: err} diff --git a/beacon-chain/core/electra/BUILD.bazel b/beacon-chain/core/electra/BUILD.bazel index ee2a0addf949..4696fe2347ff 100644 --- a/beacon-chain/core/electra/BUILD.bazel +++ b/beacon-chain/core/electra/BUILD.bazel @@ -8,6 +8,7 @@ go_library( "consolidations.go", "deposits.go", "effective_balance_updates.go", + "error.go", "registry_updates.go", "transition.go", "transition_no_verify_sig.go", diff --git a/beacon-chain/core/electra/error.go b/beacon-chain/core/electra/error.go new file mode 100644 index 000000000000..522b75696a59 --- /dev/null +++ b/beacon-chain/core/electra/error.go @@ -0,0 +1,16 @@ +package electra + +import "github.com/pkg/errors" + +type execReqErr struct { + error +} + +// IsExecutionRequestError returns true if the error has `execReqErr`. +func IsExecutionRequestError(e error) bool { + if e == nil { + return false + } + var d execReqErr + return errors.As(e, &d) +} diff --git a/beacon-chain/core/electra/transition_no_verify_sig.go b/beacon-chain/core/electra/transition_no_verify_sig.go index e9f8f99f9f6d..018e519abc2a 100644 --- a/beacon-chain/core/electra/transition_no_verify_sig.go +++ b/beacon-chain/core/electra/transition_no_verify_sig.go @@ -2,7 +2,6 @@ package electra import ( "context" - "fmt" "github.com/pkg/errors" "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/blocks" @@ -82,16 +81,31 @@ func ProcessOperations( if err != nil { return nil, errors.Wrap(err, "could not get execution requests") } + for _, d := range requests.Deposits { + if d == nil { + return nil, errors.New("nil deposit request") + } + } st, err = ProcessDepositRequests(ctx, st, requests.Deposits) if err != nil { - return nil, errors.Wrap(err, "could not process deposit requests") + return nil, execReqErr{errors.Wrap(err, "could not process deposit requests")} + } + for _, w := range requests.Withdrawals { + if w == nil { + return nil, errors.New("nil withdrawal request") + } } 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")} + } + for _, c := range requests.Consolidations { + if c == nil { + return nil, errors.New("nil consolidation request") + } } if err := ProcessConsolidationRequests(ctx, st, requests.Consolidations); err != nil { - return nil, fmt.Errorf("could not process consolidation requests: %w", err) + return nil, execReqErr{errors.Wrap(err, "could not process consolidation requests")} } return st, nil } diff --git a/beacon-chain/core/electra/withdrawals.go b/beacon-chain/core/electra/withdrawals.go index 62156188ccd8..29bb67987011 100644 --- a/beacon-chain/core/electra/withdrawals.go +++ b/beacon-chain/core/electra/withdrawals.go @@ -92,9 +92,6 @@ func ProcessWithdrawalRequests(ctx context.Context, st state.BeaconState, wrs [] defer span.End() currentEpoch := slots.ToEpoch(st.Slot()) for _, wr := range wrs { - if wr == nil { - return nil, errors.New("nil execution layer withdrawal request") - } amount := wr.Amount isFullExitRequest := amount == params.BeaconConfig().FullExitRequestAmount // If partial withdrawal queue is full, only full exits are processed diff --git a/changelog/tt_fix_electra_core_processing.md b/changelog/tt_fix_electra_core_processing.md new file mode 100644 index 000000000000..cbb9dd721489 --- /dev/null +++ b/changelog/tt_fix_electra_core_processing.md @@ -0,0 +1,3 @@ +### Changed + +- Update electra core processing to not mark block bad if execution request error. \ No newline at end of file From eb8b62e6473ca7f7162eb2f743f676f4942435d7 Mon Sep 17 00:00:00 2001 From: terence tsao Date: Thu, 30 Jan 2025 05:29:52 -1000 Subject: [PATCH 2/4] Add test for IsExecutionRequestError --- beacon-chain/core/electra/BUILD.bazel | 2 ++ beacon-chain/core/electra/error_test.go | 45 +++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 beacon-chain/core/electra/error_test.go diff --git a/beacon-chain/core/electra/BUILD.bazel b/beacon-chain/core/electra/BUILD.bazel index 4696fe2347ff..a7a161c1dd3d 100644 --- a/beacon-chain/core/electra/BUILD.bazel +++ b/beacon-chain/core/electra/BUILD.bazel @@ -56,6 +56,7 @@ go_test( "deposit_fuzz_test.go", "deposits_test.go", "effective_balance_updates_test.go", + "error_test.go", "export_test.go", "registry_updates_test.go", "transition_test.go", @@ -87,6 +88,7 @@ go_test( "@com_github_ethereum_go_ethereum//common:go_default_library", "@com_github_ethereum_go_ethereum//common/hexutil:go_default_library", "@com_github_google_gofuzz//:go_default_library", + "@com_github_pkg_errors//:go_default_library", "@com_github_sirupsen_logrus//:go_default_library", "@com_github_sirupsen_logrus//hooks/test:go_default_library", ], diff --git a/beacon-chain/core/electra/error_test.go b/beacon-chain/core/electra/error_test.go new file mode 100644 index 000000000000..808117fba22d --- /dev/null +++ b/beacon-chain/core/electra/error_test.go @@ -0,0 +1,45 @@ +package electra + +import ( + "testing" + + "github.com/pkg/errors" +) + +func TestIsExecutionRequestError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "nil error", + err: nil, + want: false, + }, + { + name: "random error", + err: errors.New("some error"), + want: false, + }, + { + name: "execution request error", + err: execReqErr{errors.New("execution request failed")}, + want: true, + }, + { + name: "wrapped execution request error", + err: errors.Wrap(execReqErr{errors.New("execution request failed")}, "wrapped"), + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsExecutionRequestError(tt.err) + if got != tt.want { + t.Errorf("IsExecutionRequestError(%v) = %v, want %v", tt.err, got, tt.want) + } + }) + } +} From c86f8fa78d419ee28f04c466a40c29262d8657b6 Mon Sep 17 00:00:00 2001 From: terence tsao Date: Fri, 31 Jan 2025 10:33:37 -1000 Subject: [PATCH 3/4] Add TestProcessOperationsWithNilRequests --- .../electra/transition_no_verify_sig_test.go | 61 +++++++++++++++++++ beacon-chain/core/electra/withdrawals.go | 3 + 2 files changed, 64 insertions(+) create mode 100644 beacon-chain/core/electra/transition_no_verify_sig_test.go diff --git a/beacon-chain/core/electra/transition_no_verify_sig_test.go b/beacon-chain/core/electra/transition_no_verify_sig_test.go new file mode 100644 index 000000000000..3eee062d8038 --- /dev/null +++ b/beacon-chain/core/electra/transition_no_verify_sig_test.go @@ -0,0 +1,61 @@ +package electra_test + +import ( + "context" + "testing" + + "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/electra" + "github.com/prysmaticlabs/prysm/v5/consensus-types/blocks" + enginev1 "github.com/prysmaticlabs/prysm/v5/proto/engine/v1" + ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1" + "github.com/prysmaticlabs/prysm/v5/testing/require" + "github.com/prysmaticlabs/prysm/v5/testing/util" +) + +func TestProcessOperationsWithNilRequests(t *testing.T) { + tests := []struct { + name string + modifyBlk func(blockElectra *ethpb.SignedBeaconBlockElectra) + errMsg string + }{ + { + name: "Nil deposit request", + modifyBlk: func(blk *ethpb.SignedBeaconBlockElectra) { + blk.Block.Body.ExecutionRequests.Deposits = []*enginev1.DepositRequest{nil} + }, + errMsg: "nil deposit request", + }, + { + name: "Nil withdrawal request", + modifyBlk: func(blk *ethpb.SignedBeaconBlockElectra) { + blk.Block.Body.ExecutionRequests.Withdrawals = []*enginev1.WithdrawalRequest{nil} + }, + errMsg: "nil withdrawal request", + }, + { + name: "Nil consolidation request", + modifyBlk: func(blk *ethpb.SignedBeaconBlockElectra) { + blk.Block.Body.ExecutionRequests.Consolidations = []*enginev1.ConsolidationRequest{nil} + }, + errMsg: "nil consolidation request", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + st, ks := util.DeterministicGenesisStateElectra(t, 128) + blk, err := util.GenerateFullBlockElectra(st, ks, util.DefaultBlockGenConfig(), 1) + require.NoError(t, err) + + tc.modifyBlk(blk) + + b, err := blocks.NewSignedBeaconBlock(blk) + require.NoError(t, err) + + require.NoError(t, st.SetSlot(1)) + + _, err = electra.ProcessOperations(context.Background(), st, b.Block()) + require.ErrorContains(t, tc.errMsg, err) + }) + } +} diff --git a/beacon-chain/core/electra/withdrawals.go b/beacon-chain/core/electra/withdrawals.go index 29bb67987011..62156188ccd8 100644 --- a/beacon-chain/core/electra/withdrawals.go +++ b/beacon-chain/core/electra/withdrawals.go @@ -92,6 +92,9 @@ func ProcessWithdrawalRequests(ctx context.Context, st state.BeaconState, wrs [] defer span.End() currentEpoch := slots.ToEpoch(st.Slot()) for _, wr := range wrs { + if wr == nil { + return nil, errors.New("nil execution layer withdrawal request") + } amount := wr.Amount isFullExitRequest := amount == params.BeaconConfig().FullExitRequestAmount // If partial withdrawal queue is full, only full exits are processed From 73341cbfda3cd126c5f8c9677f01e4cbdc7595fe Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Fri, 31 Jan 2025 16:10:37 -0600 Subject: [PATCH 4/4] gazelle --- beacon-chain/core/electra/BUILD.bazel | 2 ++ 1 file changed, 2 insertions(+) diff --git a/beacon-chain/core/electra/BUILD.bazel b/beacon-chain/core/electra/BUILD.bazel index a7a161c1dd3d..0cde0db8b1ef 100644 --- a/beacon-chain/core/electra/BUILD.bazel +++ b/beacon-chain/core/electra/BUILD.bazel @@ -59,11 +59,13 @@ go_test( "error_test.go", "export_test.go", "registry_updates_test.go", + "transition_no_verify_sig_test.go", "transition_test.go", "upgrade_test.go", "validator_test.go", "withdrawals_test.go", ], + data = glob(["testdata/**"]), embed = [":go_default_library"], deps = [ "//beacon-chain/core/helpers:go_default_library",