From 2b1686a6c5bd6d08c21c2f7bcadea45863a0c5dd Mon Sep 17 00:00:00 2001 From: Mateusz Morusiewicz <11313015+Ruteri@users.noreply.github.com> Date: Mon, 10 Jun 2024 16:57:06 +0200 Subject: [PATCH 1/6] Adjusts how coinbase diff-based payment validation is performed to avoid any possibly underflowing substractions --- core/blockchain.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/core/blockchain.go b/core/blockchain.go index 12639a34d..024e17705 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -2494,17 +2494,20 @@ func (bc *BlockChain) ValidatePayload(block *types.Block, feeRecipient common.Ad return err } - feeRecipientBalanceDelta := new(uint256.Int).Set(statedb.GetBalance(feeRecipient)) - feeRecipientBalanceDelta.Sub(feeRecipientBalanceDelta, feeRecipientBalanceBefore) + feeRecipientBalanceAfter := new(uint256.Int).Set(statedb.GetBalance(feeRecipient)) + + amtWithdrawn := new(uin256.Int) if excludeWithdrawals { for _, w := range block.Withdrawals() { if w.Address == feeRecipient { amount := new(uint256.Int).Mul(new(uint256.Int).SetUint64(w.Amount), uint256.NewInt(params.GWei)) - feeRecipientBalanceDelta.Sub(feeRecipientBalanceDelta, amount) + amtWithdrawn.Add(amtWithdrawn, amount) } } } + amtBeforeOrWithdrawn := feeRecipientBalanceBefore.Add(new(uint256).Set(feeRecipientBalanceBefore), amtWithdrawn) + if bc.Config().IsShanghai(header.Number, header.Time) { if header.WithdrawalsHash == nil { return fmt.Errorf("withdrawals hash is missing") @@ -2529,12 +2532,16 @@ func (bc *BlockChain) ValidatePayload(block *types.Block, feeRecipient common.Ad // Validate proposer payment - if useBalanceDiffProfit { + if useBalanceDiffProfit && feeRecipientBalanceAfter.Cmp(amtBeforeOrWithdrawn) >= 0 { + feeRecipientBalanceDelta := new(uin256.Int).Set(feeRecipientBalanceAfter) + feeRecipientBalanceDelta = feeRecipientBalanceDelta.Sub(amtBeforeOrWithdrawn) + uint256ExpectedProfit, ok := uint256.FromBig(expectedProfit) if !ok { if feeRecipientBalanceDelta.Cmp(uint256ExpectedProfit) >= 0 { if feeRecipientBalanceDelta.Cmp(uint256ExpectedProfit) > 0 { log.Warn("builder claimed profit is lower than calculated profit", "expected", expectedProfit, "actual", feeRecipientBalanceDelta) + return errors.New("builder claimed profit is lower than calculated profit") } return nil } From c204d3ea6c647bf8d912bb3c9e45d0c4d6b6a306 Mon Sep 17 00:00:00 2001 From: Mateusz Morusiewicz <11313015+Ruteri@users.noreply.github.com> Date: Mon, 10 Jun 2024 17:02:35 +0200 Subject: [PATCH 2/6] Fixes package names --- core/blockchain.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/blockchain.go b/core/blockchain.go index 024e17705..c9f09721c 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -2496,7 +2496,7 @@ func (bc *BlockChain) ValidatePayload(block *types.Block, feeRecipient common.Ad feeRecipientBalanceAfter := new(uint256.Int).Set(statedb.GetBalance(feeRecipient)) - amtWithdrawn := new(uin256.Int) + amtWithdrawn := new(uint256.Int) if excludeWithdrawals { for _, w := range block.Withdrawals() { if w.Address == feeRecipient { @@ -2506,7 +2506,7 @@ func (bc *BlockChain) ValidatePayload(block *types.Block, feeRecipient common.Ad } } - amtBeforeOrWithdrawn := feeRecipientBalanceBefore.Add(new(uint256).Set(feeRecipientBalanceBefore), amtWithdrawn) + amtBeforeOrWithdrawn := feeRecipientBalanceBefore.Add(new(uint256.Int).Set(feeRecipientBalanceBefore), amtWithdrawn) if bc.Config().IsShanghai(header.Number, header.Time) { if header.WithdrawalsHash == nil { @@ -2533,8 +2533,8 @@ func (bc *BlockChain) ValidatePayload(block *types.Block, feeRecipient common.Ad // Validate proposer payment if useBalanceDiffProfit && feeRecipientBalanceAfter.Cmp(amtBeforeOrWithdrawn) >= 0 { - feeRecipientBalanceDelta := new(uin256.Int).Set(feeRecipientBalanceAfter) - feeRecipientBalanceDelta = feeRecipientBalanceDelta.Sub(amtBeforeOrWithdrawn) + feeRecipientBalanceDelta := new(uint256.Int).Set(feeRecipientBalanceAfter) + feeRecipientBalanceDelta = feeRecipientBalanceDelta.Sub(feeRecipientBalanceDelta, amtBeforeOrWithdrawn) uint256ExpectedProfit, ok := uint256.FromBig(expectedProfit) if !ok { From 2874dadde1c971b6767a131b1917430eadce7bae Mon Sep 17 00:00:00 2001 From: Mateusz Morusiewicz <11313015+Ruteri@users.noreply.github.com> Date: Mon, 10 Jun 2024 17:09:05 +0200 Subject: [PATCH 3/6] Reverts erroring out on overpaying bids --- core/blockchain.go | 1 - 1 file changed, 1 deletion(-) diff --git a/core/blockchain.go b/core/blockchain.go index c9f09721c..0c584cef6 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -2541,7 +2541,6 @@ func (bc *BlockChain) ValidatePayload(block *types.Block, feeRecipient common.Ad if feeRecipientBalanceDelta.Cmp(uint256ExpectedProfit) >= 0 { if feeRecipientBalanceDelta.Cmp(uint256ExpectedProfit) > 0 { log.Warn("builder claimed profit is lower than calculated profit", "expected", expectedProfit, "actual", feeRecipientBalanceDelta) - return errors.New("builder claimed profit is lower than calculated profit") } return nil } From 0d92e09625eef2ab38fcf3ccc28f44fe0a0a0c2e Mon Sep 17 00:00:00 2001 From: Mateusz Morusiewicz <11313015+Ruteri@users.noreply.github.com> Date: Mon, 10 Jun 2024 17:20:46 +0200 Subject: [PATCH 4/6] Simplify withdrawals calculation --- core/blockchain.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/blockchain.go b/core/blockchain.go index 0c584cef6..e1b1ea1bc 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -2496,18 +2496,16 @@ func (bc *BlockChain) ValidatePayload(block *types.Block, feeRecipient common.Ad feeRecipientBalanceAfter := new(uint256.Int).Set(statedb.GetBalance(feeRecipient)) - amtWithdrawn := new(uint256.Int) + amtBeforeOrWithdrawn := new(uint256.Int).Set(feeRecipientBalanceBefore) if excludeWithdrawals { for _, w := range block.Withdrawals() { if w.Address == feeRecipient { amount := new(uint256.Int).Mul(new(uint256.Int).SetUint64(w.Amount), uint256.NewInt(params.GWei)) - amtWithdrawn.Add(amtWithdrawn, amount) + amtBeforeOrWithdrawn = amtBeforeOrWithdrawn.Add(amtBeforeOrWithdrawn, amount) } } } - amtBeforeOrWithdrawn := feeRecipientBalanceBefore.Add(new(uint256.Int).Set(feeRecipientBalanceBefore), amtWithdrawn) - if bc.Config().IsShanghai(header.Number, header.Time) { if header.WithdrawalsHash == nil { return fmt.Errorf("withdrawals hash is missing") From 0a1290b4e959439a4c12f16763d6569987cacdeb Mon Sep 17 00:00:00 2001 From: Mateusz Morusiewicz <11313015+Ruteri@users.noreply.github.com> Date: Wed, 26 Jun 2024 16:26:34 +0200 Subject: [PATCH 5/6] Adds regression test --- eth/block-validation/api_test.go | 93 ++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/eth/block-validation/api_test.go b/eth/block-validation/api_test.go index 4340e99b3..6a06cf806 100644 --- a/eth/block-validation/api_test.go +++ b/eth/block-validation/api_test.go @@ -845,6 +845,99 @@ func executableDataToBlockValidationRequest(execData *engine.ExecutableData, pro return blockRequest, nil } +func TestValidateBuilderSubmissionV2_CoinbasePaymentUnderflow(t *testing.T) { + genesis, preMergeBlocks := generatePreMergeChain(20) + lastBlock := preMergeBlocks[len(preMergeBlocks)-1] + time := lastBlock.Time() + 5 + genesis.Config.ShanghaiTime = &time + n, ethservice := startEthService(t, genesis, preMergeBlocks) + ethservice.Merger().ReachTTD() + defer n.Close() + + api := NewBlockValidationAPI(ethservice, nil, true, true) + + baseFee := eip1559.CalcBaseFee(ethservice.BlockChain().Config(), lastBlock.Header()) + txs := make(types.Transactions, 0) + + statedb, _ := ethservice.BlockChain().StateAt(lastBlock.Root()) + nonce := statedb.GetNonce(testAddr) + validatorNonce := statedb.GetNonce(testValidatorAddr) + signer := types.LatestSigner(ethservice.BlockChain().Config()) + + expectedProfit := uint64(0) + + tx1, _ := types.SignTx(types.NewTransaction(nonce, common.Address{0x16}, big.NewInt(10), 21000, big.NewInt(2*baseFee.Int64()), nil), signer, testKey) + txs = append(txs, tx1) + expectedProfit += 21000 * baseFee.Uint64() + + // this tx will use 56996 gas + tx2, _ := types.SignTx(types.NewContractCreation(nonce+1, new(big.Int), 1000000, big.NewInt(2*baseFee.Int64()), logCode), signer, testKey) + txs = append(txs, tx2) + expectedProfit += 56996 * baseFee.Uint64() + + tx3, _ := types.SignTx(types.NewTransaction(nonce+2, testAddr, big.NewInt(10), 21000, baseFee, nil), signer, testKey) + txs = append(txs, tx3) + + // Test transfering out more than the profit + toTransferOut := 2*expectedProfit - 21000*baseFee.Uint64() + tx4, _ := types.SignTx(types.NewTransaction(validatorNonce, testAddr, big.NewInt(int64(toTransferOut)), 21000, baseFee, nil), signer, testValidatorKey) + txs = append(txs, tx4) + expectedProfit += 7 + + withdrawals := []*types.Withdrawal{ + { + Index: 0, + Validator: 1, + Amount: 100, + Address: testAddr, + }, + { + Index: 1, + Validator: 1, + Amount: 100, + Address: testAddr, + }, + } + withdrawalsRoot := types.DeriveSha(types.Withdrawals(withdrawals), trie.NewStackTrie(nil)) + + buildBlockArgs := buildBlockArgs{ + parentHash: lastBlock.Hash(), + parentRoot: lastBlock.Root(), + feeRecipient: testValidatorAddr, + txs: txs, + random: common.Hash{}, + number: lastBlock.NumberU64() + 1, + gasLimit: lastBlock.GasLimit(), + timestamp: lastBlock.Time() + 5, + extraData: nil, + baseFeePerGas: baseFee, + withdrawals: withdrawals, + } + + execData, err := buildBlock(buildBlockArgs, ethservice.BlockChain()) + require.NoError(t, err) + + value := big.NewInt(int64(expectedProfit)) + + req, err := executableDataToBlockValidationRequest(execData, testValidatorAddr, value, withdrawalsRoot) + require.NoError(t, err) + require.NoError(t, api.ValidateBuilderSubmissionV2(req)) + + // 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)) + + // 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") +} + // This tests payment when the proposer fee recipient is the same as the coinbase func TestValidateBuilderSubmissionV2_CoinbasePaymentDefault(t *testing.T) { genesis, preMergeBlocks := generatePreMergeChain(20) From 6690ba58aa61fec1ef3b7c595522214975f1eadb Mon Sep 17 00:00:00 2001 From: Mateusz Morusiewicz <11313015+Ruteri@users.noreply.github.com> Date: Wed, 26 Jun 2024 16:44:19 +0200 Subject: [PATCH 6/6] Fix lint and reverse the unit test flow --- eth/block-validation/api_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/eth/block-validation/api_test.go b/eth/block-validation/api_test.go index 6a06cf806..4d8afc6ff 100644 --- a/eth/block-validation/api_test.go +++ b/eth/block-validation/api_test.go @@ -878,7 +878,7 @@ func TestValidateBuilderSubmissionV2_CoinbasePaymentUnderflow(t *testing.T) { tx3, _ := types.SignTx(types.NewTransaction(nonce+2, testAddr, big.NewInt(10), 21000, baseFee, nil), signer, testKey) txs = append(txs, tx3) - // Test transfering out more than the profit + // Test transferring out more than the profit toTransferOut := 2*expectedProfit - 21000*baseFee.Uint64() tx4, _ := types.SignTx(types.NewTransaction(validatorNonce, testAddr, big.NewInt(int64(toTransferOut)), 21000, baseFee, nil), signer, testValidatorKey) txs = append(txs, tx4) @@ -921,14 +921,14 @@ func TestValidateBuilderSubmissionV2_CoinbasePaymentUnderflow(t *testing.T) { req, err := executableDataToBlockValidationRequest(execData, testValidatorAddr, value, withdrawalsRoot) require.NoError(t, err) - require.NoError(t, api.ValidateBuilderSubmissionV2(req)) + require.ErrorContains(t, api.ValidateBuilderSubmissionV2(req), "payment tx not to the proposers fee recipient") // 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)) + require.ErrorContains(t, api.ValidateBuilderSubmissionV2(req), "payment tx not to the proposers fee recipient") // try to claim more profit than expected, should fail value.SetUint64(expectedProfit + 1)