From 65b6b7850f91aed21f1631808e20fe0b30945ada Mon Sep 17 00:00:00 2001 From: jonny rhea Date: Tue, 3 Feb 2026 15:08:32 -0600 Subject: [PATCH] address review feedback --- core/blockchain.go | 8 +++++--- core/state_processor.go | 45 ++++++++++++++++++++++++++--------------- eth/catalyst/api.go | 43 +++++++++++++-------------------------- eth/catalyst/witness.go | 19 +++++------------ 4 files changed, 53 insertions(+), 62 deletions(-) diff --git a/core/blockchain.go b/core/blockchain.go index a2bf7f41cb..3c69f52f13 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -2651,14 +2651,16 @@ func (bc *BlockChain) reorg(oldHead *types.Header, newHead *types.Header) error // The key difference between the InsertChain is it won't do the canonical chain // updating. It relies on the additional SetCanonical call to finalize the entire // procedure. -func (bc *BlockChain) InsertBlockWithoutSetHead(ctx context.Context, block *types.Block, makeWitness bool) (*stateless.Witness, error) { +func (bc *BlockChain) InsertBlockWithoutSetHead(ctx context.Context, block *types.Block, makeWitness bool) (witness *stateless.Witness, err error) { + _, _, spanEnd := telemetry.StartSpan(ctx, "core.blockchain.InsertBlockWithoutSetHead") + defer spanEnd(err) if !bc.chainmu.TryLock() { return nil, errChainStopped } defer bc.chainmu.Unlock() - witness, _, err := bc.insertChain(ctx, types.Blocks{block}, false, makeWitness) - return witness, err + witness, _, err = bc.insertChain(ctx, types.Blocks{block}, false, makeWitness) + return } // SetCanonical rewinds the chain to set the new head block as the specified diff --git a/core/state_processor.go b/core/state_processor.go index d831f1ff8e..50f4d303f5 100644 --- a/core/state_processor.go +++ b/core/state_processor.go @@ -115,22 +115,9 @@ func (p *StateProcessor) Process(ctx context.Context, block *types.Block, stated receipts = append(receipts, receipt) allLogs = append(allLogs, receipt.Logs...) } - // Read requests if Prague is enabled. - var requests [][]byte - if config.IsPrague(block.Number(), block.Time()) { - requests = [][]byte{} - // EIP-6110 - if err := ParseDepositLogs(&requests, allLogs, config); err != nil { - return nil, fmt.Errorf("failed to parse deposit logs: %w", err) - } - // EIP-7002 - if err := ProcessWithdrawalQueue(&requests, evm); err != nil { - return nil, fmt.Errorf("failed to process withdrawal queue: %w", err) - } - // EIP-7251 - if err := ProcessConsolidationQueue(&requests, evm); err != nil { - return nil, fmt.Errorf("failed to process consolidation queue: %w", err) - } + requests, err := postExecution(ctx, config, block, allLogs, evm) + if err != nil { + return nil, err } // Finalize the block, applying any consensus engine specific extras (e.g. block rewards) @@ -144,6 +131,32 @@ func (p *StateProcessor) Process(ctx context.Context, block *types.Block, stated }, nil } +// postExecution processes the post-execution system calls if Prague is enabled. +func postExecution(ctx context.Context, config *params.ChainConfig, block *types.Block, allLogs []*types.Log, evm *vm.EVM) (requests [][]byte, err error) { + _, _, spanEnd := telemetry.StartSpan(ctx, "core.postExecution") + defer spanEnd(err) + // Read requests if Prague is enabled. + if config.IsPrague(block.Number(), block.Time()) { + requests = [][]byte{} + // EIP-6110 + if err = ParseDepositLogs(&requests, allLogs, config); err != nil { + err = fmt.Errorf("failed to parse deposit logs: %w", err) + return + } + // EIP-7002 + if err = ProcessWithdrawalQueue(&requests, evm); err != nil { + err = fmt.Errorf("failed to process withdrawal queue: %w", err) + return + } + // EIP-7251 + if err = ProcessConsolidationQueue(&requests, evm); err != nil { + err = fmt.Errorf("failed to process consolidation queue: %w", err) + return + } + } + return +} + // ApplyTransactionWithEVM attempts to apply a transaction to the given state database // and uses the input parameters for its environment similar to ApplyTransaction. However, // this method takes an already created EVM instance as input. diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 5fa0cf8b80..1711c54c2b 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -626,19 +626,8 @@ func (api *ConsensusAPI) getBlobs(hashes []common.Hash, v2 bool) ([]*engine.Blob // Helper for NewPayload* methods. var invalidStatus = engine.PayloadStatusV1{Status: engine.INVALID} -// executableDataAttrs returns telemetry attributes for an ExecutableData payload. -func executableDataAttrs(params engine.ExecutableData) []telemetry.Attribute { - return []telemetry.Attribute{ - telemetry.Int64Attribute("block.number", int64(params.Number)), - telemetry.StringAttribute("block.hash", params.BlockHash.Hex()), - telemetry.Int64Attribute("tx.count", int64(len(params.Transactions))), - } -} - // NewPayloadV1 creates an Eth1 block, inserts it in the chain, and returns the status of the chain. -func (api *ConsensusAPI) NewPayloadV1(ctx context.Context, params engine.ExecutableData) (result engine.PayloadStatusV1, err error) { - ctx, _, spanEnd := telemetry.StartSpan(ctx, "engine.newPayloadV1", executableDataAttrs(params)...) - defer spanEnd(err) +func (api *ConsensusAPI) NewPayloadV1(ctx context.Context, params engine.ExecutableData) (engine.PayloadStatusV1, error) { if params.Withdrawals != nil { return invalidStatus, paramsErr("withdrawals not supported in V1") } @@ -646,9 +635,7 @@ func (api *ConsensusAPI) NewPayloadV1(ctx context.Context, params engine.Executa } // NewPayloadV2 creates an Eth1 block, inserts it in the chain, and returns the status of the chain. -func (api *ConsensusAPI) NewPayloadV2(ctx context.Context, params engine.ExecutableData) (result engine.PayloadStatusV1, err error) { - ctx, _, spanEnd := telemetry.StartSpan(ctx, "engine.newPayloadV2", executableDataAttrs(params)...) - defer spanEnd(err) +func (api *ConsensusAPI) NewPayloadV2(ctx context.Context, params engine.ExecutableData) (engine.PayloadStatusV1, error) { var ( cancun = api.config().IsCancun(api.config().LondonBlock, params.Timestamp) shanghai = api.config().IsShanghai(api.config().LondonBlock, params.Timestamp) @@ -669,9 +656,7 @@ func (api *ConsensusAPI) NewPayloadV2(ctx context.Context, params engine.Executa } // NewPayloadV3 creates an Eth1 block, inserts it in the chain, and returns the status of the chain. -func (api *ConsensusAPI) NewPayloadV3(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash) (result engine.PayloadStatusV1, err error) { - ctx, _, spanEnd := telemetry.StartSpan(ctx, "engine.newPayloadV3", executableDataAttrs(params)...) - defer spanEnd(err) +func (api *ConsensusAPI) NewPayloadV3(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash) (engine.PayloadStatusV1, error) { switch { case params.Withdrawals == nil: return invalidStatus, paramsErr("nil withdrawals post-shanghai") @@ -690,9 +675,7 @@ func (api *ConsensusAPI) NewPayloadV3(ctx context.Context, params engine.Executa } // NewPayloadV4 creates an Eth1 block, inserts it in the chain, and returns the status of the chain. -func (api *ConsensusAPI) NewPayloadV4(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, executionRequests []hexutil.Bytes) (result engine.PayloadStatusV1, err error) { - ctx, _, spanEnd := telemetry.StartSpan(ctx, "engine.newPayloadV4", executableDataAttrs(params)...) - defer spanEnd(err) +func (api *ConsensusAPI) NewPayloadV4(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, executionRequests []hexutil.Bytes) (engine.PayloadStatusV1, error) { switch { case params.Withdrawals == nil: return invalidStatus, paramsErr("nil withdrawals post-shanghai") @@ -710,13 +693,13 @@ func (api *ConsensusAPI) NewPayloadV4(ctx context.Context, params engine.Executa return invalidStatus, unsupportedForkErr("newPayloadV4 must only be called for prague/osaka payloads") } requests := convertRequests(executionRequests) - if err = validateRequests(requests); err != nil { + if err := validateRequests(requests); err != nil { return engine.PayloadStatusV1{Status: engine.INVALID}, engine.InvalidParams.With(err) } return api.newPayload(ctx, params, versionedHashes, beaconRoot, requests, false) } -func (api *ConsensusAPI) newPayload(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, requests [][]byte, witness bool) (engine.PayloadStatusV1, error) { +func (api *ConsensusAPI) newPayload(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, requests [][]byte, witness bool) (result engine.PayloadStatusV1, err error) { // The locking here is, strictly, not required. Without these locks, this can happen: // // 1. NewPayload( execdata-N ) is invoked from the CL. It goes all the way down to @@ -730,14 +713,18 @@ func (api *ConsensusAPI) newPayload(ctx context.Context, params engine.Executabl // sequentially. // Hence, we use a lock here, to be sure that the previous call has finished before we // check whether we already have the block locally. + var attrs = []telemetry.Attribute{ + telemetry.Int64Attribute("block.number", int64(params.Number)), + telemetry.StringAttribute("block.hash", params.BlockHash.Hex()), + telemetry.Int64Attribute("tx.count", int64(len(params.Transactions))), + } + ctx, _, spanEnd := telemetry.StartSpan(ctx, "engine.newPayload", attrs...) + defer spanEnd(err) api.newPayloadLock.Lock() defer api.newPayloadLock.Unlock() log.Trace("Engine API request received", "method", "NewPayload", "number", params.Number, "hash", params.BlockHash) - attrs := executableDataAttrs(params) - _, _, spanEnd := telemetry.StartSpan(ctx, "engine.ExecutableDataToBlock", attrs...) block, err := engine.ExecutableDataToBlock(params, versionedHashes, beaconRoot, requests) - spanEnd(err) if err != nil { bgu := "nil" if params.BlobGasUsed != nil { @@ -810,11 +797,9 @@ func (api *ConsensusAPI) newPayload(ctx context.Context, params engine.Executabl return engine.PayloadStatusV1{Status: engine.ACCEPTED}, nil } log.Trace("Inserting block without sethead", "hash", block.Hash(), "number", block.Number()) - sctx, _, spanEnd := telemetry.StartSpan(ctx, "api.eth.Blockchain().InsertBlockWithoutSetHead", attrs...) start := time.Now() - proofs, err := api.eth.BlockChain().InsertBlockWithoutSetHead(sctx, block, witness) + proofs, err := api.eth.BlockChain().InsertBlockWithoutSetHead(ctx, block, witness) processingTime := time.Since(start) - spanEnd(err) if err != nil { log.Warn("NewPayload: inserting block failed", "error", err) diff --git a/eth/catalyst/witness.go b/eth/catalyst/witness.go index 44fd8dec04..14ca29e079 100644 --- a/eth/catalyst/witness.go +++ b/eth/catalyst/witness.go @@ -28,7 +28,6 @@ import ( "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/core/stateless" "github.com/ethereum/go-ethereum/core/vm" - "github.com/ethereum/go-ethereum/internal/telemetry" "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/params/forks" "github.com/ethereum/go-ethereum/rlp" @@ -88,9 +87,7 @@ func (api *ConsensusAPI) ForkchoiceUpdatedWithWitnessV3(update engine.Forkchoice // NewPayloadWithWitnessV1 is analogous to NewPayloadV1, only it also generates // and returns a stateless witness after running the payload. -func (api *ConsensusAPI) NewPayloadWithWitnessV1(ctx context.Context, params engine.ExecutableData) (result engine.PayloadStatusV1, err error) { - ctx, _, spanEnd := telemetry.StartSpan(ctx, "engine.newPayloadWithWitnessV1", executableDataAttrs(params)...) - defer spanEnd(err) +func (api *ConsensusAPI) NewPayloadWithWitnessV1(ctx context.Context, params engine.ExecutableData) (engine.PayloadStatusV1, error) { if params.Withdrawals != nil { return engine.PayloadStatusV1{Status: engine.INVALID}, engine.InvalidParams.With(errors.New("withdrawals not supported in V1")) } @@ -99,9 +96,7 @@ func (api *ConsensusAPI) NewPayloadWithWitnessV1(ctx context.Context, params eng // NewPayloadWithWitnessV2 is analogous to NewPayloadV2, only it also generates // and returns a stateless witness after running the payload. -func (api *ConsensusAPI) NewPayloadWithWitnessV2(ctx context.Context, params engine.ExecutableData) (result engine.PayloadStatusV1, err error) { - ctx, _, spanEnd := telemetry.StartSpan(ctx, "engine.newPayloadWithWitnessV2", executableDataAttrs(params)...) - defer spanEnd(err) +func (api *ConsensusAPI) NewPayloadWithWitnessV2(ctx context.Context, params engine.ExecutableData) (engine.PayloadStatusV1, error) { var ( cancun = api.config().IsCancun(api.config().LondonBlock, params.Timestamp) shanghai = api.config().IsShanghai(api.config().LondonBlock, params.Timestamp) @@ -123,9 +118,7 @@ func (api *ConsensusAPI) NewPayloadWithWitnessV2(ctx context.Context, params eng // NewPayloadWithWitnessV3 is analogous to NewPayloadV3, only it also generates // and returns a stateless witness after running the payload. -func (api *ConsensusAPI) NewPayloadWithWitnessV3(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash) (result engine.PayloadStatusV1, err error) { - ctx, _, spanEnd := telemetry.StartSpan(ctx, "engine.newPayloadWithWitnessV3", executableDataAttrs(params)...) - defer spanEnd(err) +func (api *ConsensusAPI) NewPayloadWithWitnessV3(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash) (engine.PayloadStatusV1, error) { switch { case params.Withdrawals == nil: return invalidStatus, paramsErr("nil withdrawals post-shanghai") @@ -145,9 +138,7 @@ func (api *ConsensusAPI) NewPayloadWithWitnessV3(ctx context.Context, params eng // NewPayloadWithWitnessV4 is analogous to NewPayloadV4, only it also generates // and returns a stateless witness after running the payload. -func (api *ConsensusAPI) NewPayloadWithWitnessV4(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, executionRequests []hexutil.Bytes) (result engine.PayloadStatusV1, err error) { - ctx, _, spanEnd := telemetry.StartSpan(ctx, "engine.newPayloadWithWitnessV4", executableDataAttrs(params)...) - defer spanEnd(err) +func (api *ConsensusAPI) NewPayloadWithWitnessV4(ctx context.Context, params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, executionRequests []hexutil.Bytes) (engine.PayloadStatusV1, error) { switch { case params.Withdrawals == nil: return invalidStatus, paramsErr("nil withdrawals post-shanghai") @@ -165,7 +156,7 @@ func (api *ConsensusAPI) NewPayloadWithWitnessV4(ctx context.Context, params eng return invalidStatus, unsupportedForkErr("newPayloadV4 must only be called for prague/osaka payloads") } requests := convertRequests(executionRequests) - if err = validateRequests(requests); err != nil { + if err := validateRequests(requests); err != nil { return engine.PayloadStatusV1{Status: engine.INVALID}, engine.InvalidParams.With(err) } return api.newPayload(ctx, params, versionedHashes, beaconRoot, requests, true)