From 51bce0227c969c7906d9e4b858a7edf85a7f5543 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 6 Sep 2023 10:18:13 -0700 Subject: [PATCH 1/4] feature: validate block endpoint returns gas and hash overrides --- builder/builder.go | 6 +++- core/blockchain.go | 21 ++++++++++++ eth/block-validation/api.go | 57 ++++++++++++++++++++++++-------- eth/block-validation/api_test.go | 38 ++++++++++++++------- 4 files changed, 95 insertions(+), 27 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index 44201bcdeb..d3f29e77f9 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -319,10 +319,14 @@ func (b *Builder) submitCapellaBlock(block *types.Block, blockValue *big.Int, or } if b.dryRun { - err = b.validator.ValidateBuilderSubmissionV2(&blockvalidation.BuilderBlockValidationRequestV2{SubmitBlockRequest: blockSubmitReq, RegisteredGasLimit: vd.GasLimit}) + resp, err := b.validator.ValidateBuilderSubmissionV2(&blockvalidation.BuilderBlockValidationRequestV2{SubmitBlockRequest: blockSubmitReq, RegisteredGasLimit: vd.GasLimit}) if err != nil { log.Error("could not validate block for capella", "err", err) } + if resp.NewGasLimit != payload.GasLimit { + log.Error("gas limit adjusted needed for capella", "gasLimit", payload.GasLimit, "newGasLimit", resp.NewGasLimit) + } + } else { go b.ds.ConsumeBuiltBlock(block, blockValue, ordersClosedAt, sealedAt, commitedBundles, allBundles, usedSbundles, &blockBidMsg) err = b.relay.SubmitBlockCapella(&blockSubmitReq, vd) diff --git a/core/blockchain.go b/core/blockchain.go index 8e5372c905..d8b831cc06 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -2494,6 +2494,27 @@ func (bc *BlockChain) SetBlockValidatorAndProcessorForTesting(v Validator, p Pro bc.processor = p } +// +func (bc *BlockChain) AdjustedGasLimit(block *types.Block, registeredGasLimit uint64) (uint64, error) { + header := block.Header() + if err := bc.engine.VerifyHeader(bc, header, true); err != nil { + return 0, err + } + + current := bc.CurrentBlock() + reorg, err := bc.forker.ReorgNeeded(current, header) + if err == nil && reorg { + return 0, errors.New("block requires a reorg") + } + + parent := bc.GetHeader(block.ParentHash(), block.NumberU64()-1) + if parent == nil { + return 0, errors.New("parent not found") + } + + return utils.CalcGasLimit(parent.GasLimit, registeredGasLimit), nil +} + // ValidatePayload validates the payload of the block. // It returns nil if the payload is valid, otherwise it returns an error. // - `useBalanceDiffProfit` if set to false, proposer payment is assumed to be in the last transaction of the block diff --git a/eth/block-validation/api.go b/eth/block-validation/api.go index 58ad8dc93d..6c36a65e4d 100644 --- a/eth/block-validation/api.go +++ b/eth/block-validation/api.go @@ -228,32 +228,38 @@ func (r *BuilderBlockValidationRequestV2) UnmarshalJSON(data []byte) error { return nil } -func (api *BlockValidationAPI) ValidateBuilderSubmissionV2(params *BuilderBlockValidationRequestV2) error { +type BuilderBlockValidationResponseV2 struct { + NewGasLimit uint64 `json:"new_gas_limit,string"` + NewBlockHash string `json:"new_block_hash"` +} + +func (api *BlockValidationAPI) ValidateBuilderSubmissionV2(params *BuilderBlockValidationRequestV2) (*BuilderBlockValidationResponseV2, error) { // TODO: fuzztest, make sure the validation is sound // TODO: handle context! if params.ExecutionPayload == nil { - return errors.New("nil execution payload") + return nil, errors.New("nil execution payload") } + payload := params.ExecutionPayload block, err := engine.ExecutionPayloadV2ToBlock(payload) if err != nil { - return err + return nil, err } if params.Message.ParentHash != phase0.Hash32(block.ParentHash()) { - return fmt.Errorf("incorrect ParentHash %s, expected %s", params.Message.ParentHash.String(), block.ParentHash().String()) + return nil, fmt.Errorf("incorrect ParentHash %s, expected %s", params.Message.ParentHash.String(), block.ParentHash().String()) } if params.Message.BlockHash != phase0.Hash32(block.Hash()) { - return fmt.Errorf("incorrect BlockHash %s, expected %s", params.Message.BlockHash.String(), block.Hash().String()) + return nil, fmt.Errorf("incorrect BlockHash %s, expected %s", params.Message.BlockHash.String(), block.Hash().String()) } if params.Message.GasLimit != block.GasLimit() { - return fmt.Errorf("incorrect GasLimit %d, expected %d", params.Message.GasLimit, block.GasLimit()) + return nil, fmt.Errorf("incorrect GasLimit %d, expected %d", params.Message.GasLimit, block.GasLimit()) } if params.Message.GasUsed != block.GasUsed() { - return fmt.Errorf("incorrect GasUsed %d, expected %d", params.Message.GasUsed, block.GasUsed()) + return nil, fmt.Errorf("incorrect GasUsed %d, expected %d", params.Message.GasUsed, block.GasUsed()) } feeRecipient := common.BytesToAddress(params.Message.ProposerFeeRecipient[:]) @@ -263,13 +269,13 @@ func (api *BlockValidationAPI) ValidateBuilderSubmissionV2(params *BuilderBlockV var tracer *logger.AccessListTracer = nil if api.accessVerifier != nil { if err := api.accessVerifier.isBlacklisted(block.Coinbase()); err != nil { - return err + return nil, err } if err := api.accessVerifier.isBlacklisted(feeRecipient); err != nil { - return err + return nil, err } if err := api.accessVerifier.verifyTransactions(types.LatestSigner(api.eth.BlockChain().Config()), block.Transactions()); err != nil { - return err + return nil, err } isPostMerge := true // the call is PoS-native precompiles := vm.ActivePrecompiles(api.eth.APIBackend.ChainConfig().Rules(new(big.Int).SetUint64(params.ExecutionPayload.BlockNumber), isPostMerge, params.ExecutionPayload.Timestamp)) @@ -277,18 +283,41 @@ func (api *BlockValidationAPI) ValidateBuilderSubmissionV2(params *BuilderBlockV vmconfig = vm.Config{Tracer: tracer, Debug: true} } - err = api.eth.BlockChain().ValidatePayload(block, feeRecipient, expectedProfit, params.RegisteredGasLimit, vmconfig, api.useBalanceDiffProfit) + bc := api.eth.BlockChain() + + // After validation of the message, we modify the block's gas limit + adjustedGasLimit, err := bc.AdjustedGasLimit(block, params.RegisteredGasLimit) + if err != nil { + return nil, err + } + // If the gas limit was adjusted, we need to regenerate the block + // as mutating them is generally unsupported + if payload.GasLimit != adjustedGasLimit { + payload.GasLimit = adjustedGasLimit + block, err = engine.ExecutionPayloadV2ToBlock(payload) + if err != nil { + return nil, err + } + } + + + err = bc.ValidatePayload(block, feeRecipient, expectedProfit, params.RegisteredGasLimit, vmconfig, api.useBalanceDiffProfit) if err != nil { log.Error("invalid payload", "hash", payload.BlockHash.String(), "number", payload.BlockNumber, "parentHash", payload.ParentHash.String(), "err", err) - return err + return nil, err } if api.accessVerifier != nil && tracer != nil { if err := api.accessVerifier.verifyTraces(tracer); err != nil { - return err + return nil, err } } log.Info("validated block", "hash", block.Hash(), "number", block.NumberU64(), "parentHash", block.ParentHash()) - return nil + + return &BuilderBlockValidationResponseV2{ + NewGasLimit: block.GasLimit(), + NewBlockHash: block.Hash().String(), + }, nil } + diff --git a/eth/block-validation/api_test.go b/eth/block-validation/api_test.go index 5a925f4316..5f44f02e77 100644 --- a/eth/block-validation/api_test.go +++ b/eth/block-validation/api_test.go @@ -246,15 +246,18 @@ func TestValidateBuilderSubmissionV2(t *testing.T) { WithdrawalsRoot: withdrawalsRoot, } - require.ErrorContains(t, api.ValidateBuilderSubmissionV2(blockRequest), "inaccurate payment") + _, err = api.ValidateBuilderSubmissionV2(blockRequest) + require.ErrorContains(t, err, "inaccurate payment") blockRequest.Message.Value = uint256.NewInt(149842511727212) - require.NoError(t, api.ValidateBuilderSubmissionV2(blockRequest)) + _, err = api.ValidateBuilderSubmissionV2(blockRequest) + require.NoError(t, err) blockRequest.Message.GasLimit += 1 blockRequest.ExecutionPayload.GasLimit += 1 updatePayloadHashV2(t, blockRequest) - require.ErrorContains(t, api.ValidateBuilderSubmissionV2(blockRequest), "incorrect gas limit set") + _, err = api.ValidateBuilderSubmissionV2(blockRequest) + require.ErrorContains(t, err, "incorrect gas limit set") blockRequest.Message.GasLimit -= 1 blockRequest.ExecutionPayload.GasLimit -= 1 @@ -267,7 +270,9 @@ func TestValidateBuilderSubmissionV2(t *testing.T) { testAddr: {}, }, } - require.ErrorContains(t, api.ValidateBuilderSubmissionV2(blockRequest), "transaction from blacklisted address 0x71562b71999873DB5b286dF957af199Ec94617F7") + + _, err = api.ValidateBuilderSubmissionV2(blockRequest) + require.ErrorContains(t, err, "transaction from blacklisted address 0x71562b71999873DB5b286dF957af199Ec94617F7") // Test tx to blacklisted address api.accessVerifier = &AccessVerifier{ @@ -275,12 +280,15 @@ func TestValidateBuilderSubmissionV2(t *testing.T) { {0x16}: {}, }, } - require.ErrorContains(t, api.ValidateBuilderSubmissionV2(blockRequest), "transaction to blacklisted address 0x1600000000000000000000000000000000000000") + + _, err = api.ValidateBuilderSubmissionV2(blockRequest) + require.ErrorContains(t, err, "transaction to blacklisted address 0x1600000000000000000000000000000000000000") api.accessVerifier = nil blockRequest.Message.GasUsed = 10 - require.ErrorContains(t, api.ValidateBuilderSubmissionV2(blockRequest), "incorrect GasUsed 10, expected 119996") + _, err = api.ValidateBuilderSubmissionV2(blockRequest) + require.ErrorContains(t, err, "incorrect GasUsed 10, expected 119996") blockRequest.Message.GasUsed = execData.GasUsed newTestKey, _ := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f290") @@ -297,7 +305,8 @@ func TestValidateBuilderSubmissionV2(t *testing.T) { copy(invalidPayload.ReceiptsRoot[:], hexutil.MustDecode("0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421")[:32]) blockRequest.ExecutionPayload = invalidPayload updatePayloadHashV2(t, blockRequest) - require.ErrorContains(t, api.ValidateBuilderSubmissionV2(blockRequest), "could not apply tx 4", "insufficient funds for gas * price + value") + _, err = api.ValidateBuilderSubmissionV2(blockRequest) + require.ErrorContains(t, err, "could not apply tx 4", "insufficient funds for gas * price + value") } func updatePayloadHash(t *testing.T, blockRequest *BuilderBlockValidationRequest) { @@ -703,21 +712,24 @@ func TestValidateBuilderSubmissionV2_CoinbasePaymentDefault(t *testing.T) { req, err := executableDataToBlockValidationRequest(execData, testValidatorAddr, value, withdrawalsRoot) require.NoError(t, err) - require.NoError(t, api.ValidateBuilderSubmissionV2(req)) + _, err = api.ValidateBuilderSubmissionV2(req) + require.NoError(t, err) // try to claim less profit than expected, should work value.SetUint64(expectedProfit - 1) req, err = executableDataToBlockValidationRequest(execData, testValidatorAddr, value, withdrawalsRoot) require.NoError(t, err) - require.NoError(t, api.ValidateBuilderSubmissionV2(req)) + _, err = api.ValidateBuilderSubmissionV2(req) + require.NoError(t, err) // try to claim more profit than expected, should fail value.SetUint64(expectedProfit + 1) req, err = executableDataToBlockValidationRequest(execData, testValidatorAddr, value, withdrawalsRoot) require.NoError(t, err) - require.ErrorContains(t, api.ValidateBuilderSubmissionV2(req), "payment") + _, err = api.ValidateBuilderSubmissionV2(req) + require.ErrorContains(t, err, "payment") } func TestValidateBuilderSubmissionV2_Blocklist(t *testing.T) { @@ -777,8 +789,10 @@ func TestValidateBuilderSubmissionV2_Blocklist(t *testing.T) { req, err := executableDataToBlockValidationRequest(execData, testValidatorAddr, common.Big0, withdrawalsRoot) require.NoError(t, err) - require.NoError(t, apiNoBlock.ValidateBuilderSubmissionV2(req)) - require.ErrorContains(t, apiWithBlock.ValidateBuilderSubmissionV2(req), "blacklisted") + _, err = apiNoBlock.ValidateBuilderSubmissionV2(req) + require.NoError(t, err) + _, err = apiWithBlock.ValidateBuilderSubmissionV2(req) + require.ErrorContains(t, err, "blacklisted") }) } } From 8bb0248e57c6efdfa8e841d8803f647115549c48 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 6 Sep 2023 10:41:38 -0700 Subject: [PATCH 2/4] fix: remove test --- eth/block-validation/api.go | 1 - eth/block-validation/api_test.go | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/eth/block-validation/api.go b/eth/block-validation/api.go index 6c36a65e4d..ee2745cf8e 100644 --- a/eth/block-validation/api.go +++ b/eth/block-validation/api.go @@ -300,7 +300,6 @@ func (api *BlockValidationAPI) ValidateBuilderSubmissionV2(params *BuilderBlockV } } - err = bc.ValidatePayload(block, feeRecipient, expectedProfit, params.RegisteredGasLimit, vmconfig, api.useBalanceDiffProfit) if err != nil { log.Error("invalid payload", "hash", payload.BlockHash.String(), "number", payload.BlockNumber, "parentHash", payload.ParentHash.String(), "err", err) diff --git a/eth/block-validation/api_test.go b/eth/block-validation/api_test.go index 5f44f02e77..c92ad15a14 100644 --- a/eth/block-validation/api_test.go +++ b/eth/block-validation/api_test.go @@ -256,8 +256,9 @@ func TestValidateBuilderSubmissionV2(t *testing.T) { blockRequest.ExecutionPayload.GasLimit += 1 updatePayloadHashV2(t, blockRequest) - _, err = api.ValidateBuilderSubmissionV2(blockRequest) - require.ErrorContains(t, err, "incorrect gas limit set") + // // Test removed as the API now overwrites the gas limit + // _, err = api.ValidateBuilderSubmissionV2(blockRequest) + // require.ErrorContains(t, err, "incorrect gas limit set") blockRequest.Message.GasLimit -= 1 blockRequest.ExecutionPayload.GasLimit -= 1 From a7a9219a87b4dac5920018131f0ecf98bb349139 Mon Sep 17 00:00:00 2001 From: James Date: Wed, 6 Sep 2023 11:01:29 -0700 Subject: [PATCH 3/4] chore: lint and gofmt --- builder/builder.go | 1 - core/blockchain.go | 3 ++- eth/block-validation/api.go | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index d3f29e77f9..61dcf404c8 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -326,7 +326,6 @@ func (b *Builder) submitCapellaBlock(block *types.Block, blockValue *big.Int, or if resp.NewGasLimit != payload.GasLimit { log.Error("gas limit adjusted needed for capella", "gasLimit", payload.GasLimit, "newGasLimit", resp.NewGasLimit) } - } else { go b.ds.ConsumeBuiltBlock(block, blockValue, ordersClosedAt, sealedAt, commitedBundles, allBundles, usedSbundles, &blockBidMsg) err = b.relay.SubmitBlockCapella(&blockSubmitReq, vd) diff --git a/core/blockchain.go b/core/blockchain.go index d8b831cc06..67fa0b2bed 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -2494,7 +2494,8 @@ func (bc *BlockChain) SetBlockValidatorAndProcessorForTesting(v Validator, p Pro bc.processor = p } -// +// AdjustedGasLimit calculates the gas limit for the given block and returns +// the calculated limit. func (bc *BlockChain) AdjustedGasLimit(block *types.Block, registeredGasLimit uint64) (uint64, error) { header := block.Header() if err := bc.engine.VerifyHeader(bc, header, true); err != nil { diff --git a/eth/block-validation/api.go b/eth/block-validation/api.go index ee2745cf8e..c683d57e6d 100644 --- a/eth/block-validation/api.go +++ b/eth/block-validation/api.go @@ -229,7 +229,7 @@ func (r *BuilderBlockValidationRequestV2) UnmarshalJSON(data []byte) error { } type BuilderBlockValidationResponseV2 struct { - NewGasLimit uint64 `json:"new_gas_limit,string"` + NewGasLimit uint64 `json:"new_gas_limit,string"` NewBlockHash string `json:"new_block_hash"` } @@ -315,8 +315,7 @@ func (api *BlockValidationAPI) ValidateBuilderSubmissionV2(params *BuilderBlockV log.Info("validated block", "hash", block.Hash(), "number", block.NumberU64(), "parentHash", block.ParentHash()) return &BuilderBlockValidationResponseV2{ - NewGasLimit: block.GasLimit(), + NewGasLimit: block.GasLimit(), NewBlockHash: block.Hash().String(), }, nil } - From 690d5de87ae47baffd19f4feea8f80d1ae1039d2 Mon Sep 17 00:00:00 2001 From: James Date: Mon, 11 Sep 2023 08:56:45 -0700 Subject: [PATCH 4/4] cleanup: better comment and delete test --- eth/block-validation/api.go | 4 ++++ eth/block-validation/api_test.go | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/eth/block-validation/api.go b/eth/block-validation/api.go index c683d57e6d..41ed786254 100644 --- a/eth/block-validation/api.go +++ b/eth/block-validation/api.go @@ -228,6 +228,10 @@ func (r *BuilderBlockValidationRequestV2) UnmarshalJSON(data []byte) error { return nil } +// Response object for the block validation v2 request. Contains a gas limit +// and a block hash that are valid for the `RegisteredGasLimit` property of the +// request. This is to allow post-building adjustment of the gas limit in the +// built block. type BuilderBlockValidationResponseV2 struct { NewGasLimit uint64 `json:"new_gas_limit,string"` NewBlockHash string `json:"new_block_hash"` diff --git a/eth/block-validation/api_test.go b/eth/block-validation/api_test.go index c92ad15a14..9cfa004457 100644 --- a/eth/block-validation/api_test.go +++ b/eth/block-validation/api_test.go @@ -256,10 +256,6 @@ func TestValidateBuilderSubmissionV2(t *testing.T) { blockRequest.ExecutionPayload.GasLimit += 1 updatePayloadHashV2(t, blockRequest) - // // Test removed as the API now overwrites the gas limit - // _, err = api.ValidateBuilderSubmissionV2(blockRequest) - // require.ErrorContains(t, err, "incorrect gas limit set") - blockRequest.Message.GasLimit -= 1 blockRequest.ExecutionPayload.GasLimit -= 1 updatePayloadHashV2(t, blockRequest)