From 4d9804905497a603ea25e0128c6c256e8eb24e1d Mon Sep 17 00:00:00 2001 From: Potuz Date: Sun, 3 Nov 2024 08:04:12 -0300 Subject: [PATCH 1/4] Use ROBlock earlier in the pipeline (#14609) --- CHANGELOG.md | 1 + beacon-chain/blockchain/process_block_test.go | 12 ++++++--- beacon-chain/blockchain/receive_block.go | 27 ++++++++++--------- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4c4ca276cb..6d32badecc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve - Simplified `ExitedValidatorIndices`. - Simplified `EjectedValidatorIndices`. - `engine_newPayloadV4`,`engine_getPayloadV4` are changes due to new execution request serialization decisions, [PR](https://github.com/prysmaticlabs/prysm/pull/14580) +- Use ROBlock earlier in block syncing pipeline. ### Deprecated diff --git a/beacon-chain/blockchain/process_block_test.go b/beacon-chain/blockchain/process_block_test.go index 7ed483861f1..053eb5fc478 100644 --- a/beacon-chain/blockchain/process_block_test.go +++ b/beacon-chain/blockchain/process_block_test.go @@ -1520,7 +1520,9 @@ func TestStore_NoViableHead_NewPayload(t *testing.T) { require.NoError(t, err) preStateVersion, preStateHeader, err := getStateVersionAndPayload(preState) require.NoError(t, err) - _, err = service.validateExecutionOnBlock(ctx, preStateVersion, preStateHeader, wsb, root) + rowsb, err := consensusblocks.NewROBlockWithRoot(wsb, root) + require.NoError(t, err) + _, err = service.validateExecutionOnBlock(ctx, preStateVersion, preStateHeader, rowsb) require.ErrorContains(t, "received an INVALID payload from execution engine", err) // Check that forkchoice's head and store's headroot are the previous head (since the invalid block did // not finish importing and it was never imported to forkchoice). Check @@ -1714,7 +1716,9 @@ func TestStore_NoViableHead_Liveness(t *testing.T) { require.NoError(t, err) preStateVersion, preStateHeader, err := getStateVersionAndPayload(preState) require.NoError(t, err) - _, err = service.validateExecutionOnBlock(ctx, preStateVersion, preStateHeader, wsb, root) + rowsb, err := consensusblocks.NewROBlockWithRoot(wsb, root) + require.NoError(t, err) + _, err = service.validateExecutionOnBlock(ctx, preStateVersion, preStateHeader, rowsb) require.ErrorContains(t, "received an INVALID payload from execution engine", err) // Check that forkchoice's head and store's headroot are the previous head (since the invalid block did @@ -1964,7 +1968,9 @@ func TestNoViableHead_Reboot(t *testing.T) { require.NoError(t, err) preStateVersion, preStateHeader, err := getStateVersionAndPayload(preState) require.NoError(t, err) - _, err = service.validateExecutionOnBlock(ctx, preStateVersion, preStateHeader, wsb, root) + rowsb, err := consensusblocks.NewROBlockWithRoot(wsb, root) + require.NoError(t, err) + _, err = service.validateExecutionOnBlock(ctx, preStateVersion, preStateHeader, rowsb) require.ErrorContains(t, "received an INVALID payload from execution engine", err) // Check that the headroot/state are not in DB and restart the node diff --git a/beacon-chain/blockchain/receive_block.go b/beacon-chain/blockchain/receive_block.go index e6afed07685..ff8c4d9187e 100644 --- a/beacon-chain/blockchain/receive_block.go +++ b/beacon-chain/blockchain/receive_block.go @@ -18,6 +18,7 @@ import ( "github.com/prysmaticlabs/prysm/v5/config/features" "github.com/prysmaticlabs/prysm/v5/consensus-types/blocks" consensus_blocks "github.com/prysmaticlabs/prysm/v5/consensus-types/blocks" + consensusblocks "github.com/prysmaticlabs/prysm/v5/consensus-types/blocks" "github.com/prysmaticlabs/prysm/v5/consensus-types/interfaces" "github.com/prysmaticlabs/prysm/v5/consensus-types/primitives" "github.com/prysmaticlabs/prysm/v5/encoding/bytesutil" @@ -84,7 +85,12 @@ func (s *Service) ReceiveBlock(ctx context.Context, block interfaces.ReadOnlySig } currentCheckpoints := s.saveCurrentCheckpoints(preState) - postState, isValidPayload, err := s.validateExecutionAndConsensus(ctx, preState, blockCopy, blockRoot) + roblock, err := consensus_blocks.NewROBlockWithRoot(blockCopy, blockRoot) + if err != nil { + return err + } + + postState, isValidPayload, err := s.validateExecutionAndConsensus(ctx, preState, roblock) if err != nil { return err } @@ -101,10 +107,6 @@ func (s *Service) ReceiveBlock(ctx context.Context, block interfaces.ReadOnlySig if err := s.savePostStateInfo(ctx, blockRoot, blockCopy, postState); err != nil { return errors.Wrap(err, "could not save post state info") } - roblock, err := consensus_blocks.NewROBlockWithRoot(blockCopy, blockRoot) - if err != nil { - return err - } args := &postBlockProcessConfig{ ctx: ctx, roblock: roblock, @@ -188,8 +190,7 @@ func (s *Service) updateCheckpoints( func (s *Service) validateExecutionAndConsensus( ctx context.Context, preState state.BeaconState, - block interfaces.SignedBeaconBlock, - blockRoot [32]byte, + block consensusblocks.ROBlock, ) (state.BeaconState, bool, error) { preStateVersion, preStateHeader, err := getStateVersionAndPayload(preState) if err != nil { @@ -208,7 +209,7 @@ func (s *Service) validateExecutionAndConsensus( var isValidPayload bool eg.Go(func() error { var err error - isValidPayload, err = s.validateExecutionOnBlock(ctx, preStateVersion, preStateHeader, block, blockRoot) + isValidPayload, err = s.validateExecutionOnBlock(ctx, preStateVersion, preStateHeader, block) if err != nil { return errors.Wrap(err, "could not notify the engine of the new payload") } @@ -559,16 +560,16 @@ func (s *Service) sendBlockAttestationsToSlasher(signed interfaces.ReadOnlySigne } // validateExecutionOnBlock notifies the engine of the incoming block execution payload and returns true if the payload is valid -func (s *Service) validateExecutionOnBlock(ctx context.Context, ver int, header interfaces.ExecutionData, signed interfaces.ReadOnlySignedBeaconBlock, blockRoot [32]byte) (bool, error) { - isValidPayload, err := s.notifyNewPayload(ctx, ver, header, signed) +func (s *Service) validateExecutionOnBlock(ctx context.Context, ver int, header interfaces.ExecutionData, block consensusblocks.ROBlock) (bool, error) { + isValidPayload, err := s.notifyNewPayload(ctx, ver, header, block) if err != nil { s.cfg.ForkChoiceStore.Lock() - err = s.handleInvalidExecutionError(ctx, err, blockRoot, signed.Block().ParentRoot()) + err = s.handleInvalidExecutionError(ctx, err, block.Root(), block.Block().ParentRoot()) s.cfg.ForkChoiceStore.Unlock() return false, err } - if signed.Version() < version.Capella && isValidPayload { - if err := s.validateMergeTransitionBlock(ctx, ver, header, signed); err != nil { + if block.Block().Version() < version.Capella && isValidPayload { + if err := s.validateMergeTransitionBlock(ctx, ver, header, block); err != nil { return isValidPayload, err } } From 66d1bb54f629d06b2baa0bc756eb0ca2b76bf057 Mon Sep 17 00:00:00 2001 From: Potuz Date: Sun, 3 Nov 2024 11:37:52 -0300 Subject: [PATCH 2/4] Change the signature of ProcessPayload (#14610) --- CHANGELOG.md | 1 + beacon-chain/core/blocks/payload.go | 14 +++++++------- beacon-chain/core/blocks/payload_test.go | 9 +++------ .../core/transition/transition_no_verify_sig.go | 3 +-- .../shared/common/operations/block_header.go | 4 ++-- .../shared/common/operations/execution_payload.go | 4 ++-- .../shared/common/operations/test_runner.go | 6 +++--- 7 files changed, 19 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d32badecc1..c09d0cc41f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve - Simplified `EjectedValidatorIndices`. - `engine_newPayloadV4`,`engine_getPayloadV4` are changes due to new execution request serialization decisions, [PR](https://github.com/prysmaticlabs/prysm/pull/14580) - Use ROBlock earlier in block syncing pipeline. +- Changed the signature of `ProcessPayload` ### Deprecated diff --git a/beacon-chain/core/blocks/payload.go b/beacon-chain/core/blocks/payload.go index a7ffde06aa5..e697fad104b 100644 --- a/beacon-chain/core/blocks/payload.go +++ b/beacon-chain/core/blocks/payload.go @@ -202,24 +202,24 @@ func ValidatePayload(st state.BeaconState, payload interfaces.ExecutionData) err // block_hash=payload.block_hash, // transactions_root=hash_tree_root(payload.transactions), // ) -func ProcessPayload(st state.BeaconState, body interfaces.ReadOnlyBeaconBlockBody) (state.BeaconState, error) { +func ProcessPayload(st state.BeaconState, body interfaces.ReadOnlyBeaconBlockBody) error { payload, err := body.Execution() if err != nil { - return nil, err + return err } if err := verifyBlobCommitmentCount(body); err != nil { - return nil, err + return err } if err := ValidatePayloadWhenMergeCompletes(st, payload); err != nil { - return nil, err + return err } if err := ValidatePayload(st, payload); err != nil { - return nil, err + return err } if err := st.SetLatestExecutionPayloadHeader(payload); err != nil { - return nil, err + return err } - return st, nil + return nil } func verifyBlobCommitmentCount(body interfaces.ReadOnlyBeaconBlockBody) error { diff --git a/beacon-chain/core/blocks/payload_test.go b/beacon-chain/core/blocks/payload_test.go index 20992c949aa..c56d725a95b 100644 --- a/beacon-chain/core/blocks/payload_test.go +++ b/beacon-chain/core/blocks/payload_test.go @@ -587,8 +587,7 @@ func Test_ProcessPayload(t *testing.T) { ExecutionPayload: tt.payload, }) require.NoError(t, err) - st, err := blocks.ProcessPayload(st, body) - if err != nil { + if err := blocks.ProcessPayload(st, body); err != nil { require.Equal(t, tt.err.Error(), err.Error()) } else { require.Equal(t, tt.err, err) @@ -619,8 +618,7 @@ func Test_ProcessPayloadCapella(t *testing.T) { ExecutionPayload: payload, }) require.NoError(t, err) - _, err = blocks.ProcessPayload(st, body) - require.NoError(t, err) + require.NoError(t, blocks.ProcessPayload(st, body)) } func Test_ProcessPayload_Blinded(t *testing.T) { @@ -677,8 +675,7 @@ func Test_ProcessPayload_Blinded(t *testing.T) { ExecutionPayloadHeader: p, }) require.NoError(t, err) - st, err := blocks.ProcessPayload(st, body) - if err != nil { + if err := blocks.ProcessPayload(st, body); err != nil { require.Equal(t, tt.err.Error(), err.Error()) } else { require.Equal(t, tt.err, err) diff --git a/beacon-chain/core/transition/transition_no_verify_sig.go b/beacon-chain/core/transition/transition_no_verify_sig.go index 402d607837b..e633c844d35 100644 --- a/beacon-chain/core/transition/transition_no_verify_sig.go +++ b/beacon-chain/core/transition/transition_no_verify_sig.go @@ -333,8 +333,7 @@ func ProcessBlockForStateRoot( return nil, errors.Wrap(err, "could not process withdrawals") } } - state, err = b.ProcessPayload(state, blk.Body()) - if err != nil { + if err = b.ProcessPayload(state, blk.Body()); err != nil { return nil, errors.Wrap(err, "could not process execution data") } } diff --git a/testing/spectest/shared/common/operations/block_header.go b/testing/spectest/shared/common/operations/block_header.go index d38221535af..c1f01e597ef 100644 --- a/testing/spectest/shared/common/operations/block_header.go +++ b/testing/spectest/shared/common/operations/block_header.go @@ -56,10 +56,10 @@ func RunBlockHeaderTest(t *testing.T, config string, fork string, sszToBlock SSZ bodyRoot, err := block.Block().Body().HashTreeRoot() require.NoError(t, err) pr := block.Block().ParentRoot() - beaconState, err := blocks.ProcessBlockHeaderNoVerify(context.Background(), preBeaconState, block.Block().Slot(), block.Block().ProposerIndex(), pr[:], bodyRoot[:]) + _, err = blocks.ProcessBlockHeaderNoVerify(context.Background(), preBeaconState, block.Block().Slot(), block.Block().ProposerIndex(), pr[:], bodyRoot[:]) if postSSZExists { require.NoError(t, err) - comparePostState(t, postSSZFilepath, sszToState, preBeaconState, beaconState) + comparePostState(t, postSSZFilepath, sszToState, preBeaconState) } else { // Note: This doesn't test anything worthwhile. It essentially tests // that *any* error has occurred, not any specific error. diff --git a/testing/spectest/shared/common/operations/execution_payload.go b/testing/spectest/shared/common/operations/execution_payload.go index c305efae2cd..652b500aa49 100644 --- a/testing/spectest/shared/common/operations/execution_payload.go +++ b/testing/spectest/shared/common/operations/execution_payload.go @@ -54,10 +54,10 @@ func RunExecutionPayloadTest(t *testing.T, config string, fork string, sszToBloc config := &ExecutionConfig{} require.NoError(t, utils.UnmarshalYaml(file, config), "Failed to Unmarshal") - gotState, err := blocks.ProcessPayload(preBeaconState, body) + err = blocks.ProcessPayload(preBeaconState, body) if postSSZExists { require.NoError(t, err) - comparePostState(t, postSSZFilepath, sszToState, preBeaconState, gotState) + comparePostState(t, postSSZFilepath, sszToState, preBeaconState) } else if config.Valid { // Note: This doesn't test anything worthwhile. It essentially tests // that *any* error has occurred, not any specific error. diff --git a/testing/spectest/shared/common/operations/test_runner.go b/testing/spectest/shared/common/operations/test_runner.go index e166c830f0d..1fa5087841b 100644 --- a/testing/spectest/shared/common/operations/test_runner.go +++ b/testing/spectest/shared/common/operations/test_runner.go @@ -50,10 +50,10 @@ func RunBlockOperationTest( } helpers.ClearCache() - beaconState, err := operationFn(context.Background(), preState, wsb) + _, err = operationFn(context.Background(), preState, wsb) if postSSZExists { require.NoError(t, err) - comparePostState(t, postSSZFilepath, sszToState, preState, beaconState) + comparePostState(t, postSSZFilepath, sszToState, preState) } else { // Note: This doesn't test anything worthwhile. It essentially tests // that *any* error has occurred, not any specific error. @@ -65,7 +65,7 @@ func RunBlockOperationTest( } } -func comparePostState(t *testing.T, postSSZFilepath string, sszToState SSZToState, want state.BeaconState, got state.BeaconState) { +func comparePostState(t *testing.T, postSSZFilepath string, sszToState SSZToState, want state.BeaconState) { postBeaconStateFile, err := os.ReadFile(postSSZFilepath) // #nosec G304 require.NoError(t, err) postBeaconStateSSZ, err := snappy.Decode(nil /* dst */, postBeaconStateFile) From 9ae97786c5229beabf5d5e400866c5fe755c5c6f Mon Sep 17 00:00:00 2001 From: Nishant Das Date: Mon, 4 Nov 2024 17:21:34 +0800 Subject: [PATCH 3/4] Build Proto State Object Once (#14612) * Call it Once * Changelog --- CHANGELOG.md | 1 + beacon-chain/db/kv/state.go | 15 ++++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c09d0cc41f3..810e86d5d10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve - `engine_newPayloadV4`,`engine_getPayloadV4` are changes due to new execution request serialization decisions, [PR](https://github.com/prysmaticlabs/prysm/pull/14580) - Use ROBlock earlier in block syncing pipeline. - Changed the signature of `ProcessPayload` +- Only Build the Protobuf state once during serialization ### Deprecated diff --git a/beacon-chain/db/kv/state.go b/beacon-chain/db/kv/state.go index b78e45ab25b..8f840448d45 100644 --- a/beacon-chain/db/kv/state.go +++ b/beacon-chain/db/kv/state.go @@ -18,6 +18,7 @@ import ( "github.com/prysmaticlabs/prysm/v5/monitoring/tracing" "github.com/prysmaticlabs/prysm/v5/monitoring/tracing/trace" ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1" + "github.com/prysmaticlabs/prysm/v5/runtime/version" "github.com/prysmaticlabs/prysm/v5/time" "github.com/prysmaticlabs/prysm/v5/time/slots" bolt "go.etcd.io/bbolt" @@ -603,14 +604,14 @@ func (s *Store) unmarshalState(_ context.Context, enc []byte, validatorEntries [ // marshal versioned state from struct type down to bytes. func marshalState(ctx context.Context, st state.ReadOnlyBeaconState) ([]byte, error) { - switch st.ToProtoUnsafe().(type) { - case *ethpb.BeaconState: + switch st.Version() { + case version.Phase0: rState, ok := st.ToProtoUnsafe().(*ethpb.BeaconState) if !ok { return nil, errors.New("non valid inner state") } return encode(ctx, rState) - case *ethpb.BeaconStateAltair: + case version.Altair: rState, ok := st.ToProtoUnsafe().(*ethpb.BeaconStateAltair) if !ok { return nil, errors.New("non valid inner state") @@ -623,7 +624,7 @@ func marshalState(ctx context.Context, st state.ReadOnlyBeaconState) ([]byte, er return nil, err } return snappy.Encode(nil, append(altairKey, rawObj...)), nil - case *ethpb.BeaconStateBellatrix: + case version.Bellatrix: rState, ok := st.ToProtoUnsafe().(*ethpb.BeaconStateBellatrix) if !ok { return nil, errors.New("non valid inner state") @@ -636,7 +637,7 @@ func marshalState(ctx context.Context, st state.ReadOnlyBeaconState) ([]byte, er return nil, err } return snappy.Encode(nil, append(bellatrixKey, rawObj...)), nil - case *ethpb.BeaconStateCapella: + case version.Capella: rState, ok := st.ToProtoUnsafe().(*ethpb.BeaconStateCapella) if !ok { return nil, errors.New("non valid inner state") @@ -649,7 +650,7 @@ func marshalState(ctx context.Context, st state.ReadOnlyBeaconState) ([]byte, er return nil, err } return snappy.Encode(nil, append(capellaKey, rawObj...)), nil - case *ethpb.BeaconStateDeneb: + case version.Deneb: rState, ok := st.ToProtoUnsafe().(*ethpb.BeaconStateDeneb) if !ok { return nil, errors.New("non valid inner state") @@ -662,7 +663,7 @@ func marshalState(ctx context.Context, st state.ReadOnlyBeaconState) ([]byte, er return nil, err } return snappy.Encode(nil, append(denebKey, rawObj...)), nil - case *ethpb.BeaconStateElectra: + case version.Electra: rState, ok := st.ToProtoUnsafe().(*ethpb.BeaconStateElectra) if !ok { return nil, errors.New("non valid inner state") From 5ef5b65ffe2be087ba0478c1d0d1b36279cd0e58 Mon Sep 17 00:00:00 2001 From: Potuz Date: Mon, 4 Nov 2024 10:59:41 -0300 Subject: [PATCH 4/4] Blocks after capella are execution (#14614) * Blocks after capella are execution * fix test --- CHANGELOG.md | 5 +++-- beacon-chain/core/blocks/payload.go | 3 +++ beacon-chain/core/blocks/payload_test.go | 3 ++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 810e86d5d10..cf7e02dcec7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,8 +40,9 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve - Simplified `EjectedValidatorIndices`. - `engine_newPayloadV4`,`engine_getPayloadV4` are changes due to new execution request serialization decisions, [PR](https://github.com/prysmaticlabs/prysm/pull/14580) - Use ROBlock earlier in block syncing pipeline. -- Changed the signature of `ProcessPayload` -- Only Build the Protobuf state once during serialization +- Changed the signature of `ProcessPayload`. +- Only Build the Protobuf state once during serialization. +- Capella blocks are execution. ### Deprecated diff --git a/beacon-chain/core/blocks/payload.go b/beacon-chain/core/blocks/payload.go index e697fad104b..6d52931b141 100644 --- a/beacon-chain/core/blocks/payload.go +++ b/beacon-chain/core/blocks/payload.go @@ -61,6 +61,9 @@ func IsExecutionBlock(body interfaces.ReadOnlyBeaconBlockBody) (bool, error) { if body == nil { return false, errors.New("nil block body") } + if body.Version() >= version.Capella { + return true, nil + } payload, err := body.Execution() switch { case errors.Is(err, consensus_types.ErrUnsupportedField): diff --git a/beacon-chain/core/blocks/payload_test.go b/beacon-chain/core/blocks/payload_test.go index c56d725a95b..38ce31eaa2d 100644 --- a/beacon-chain/core/blocks/payload_test.go +++ b/beacon-chain/core/blocks/payload_test.go @@ -253,7 +253,8 @@ func Test_IsExecutionBlockCapella(t *testing.T) { require.NoError(t, err) got, err := blocks.IsExecutionBlock(wrappedBlock.Body()) require.NoError(t, err) - require.Equal(t, false, got) + // #14614 + require.Equal(t, true, got) } func Test_IsExecutionEnabled(t *testing.T) {