corrected upfront gas deduction for type-2 transactions #1849

rachit77 · 2023-08-24T17:23:53Z

Description

These changes will be implemented as hard fork.

This PR resolves the bug reported in EVM-803 and EVM-804

EVM-803: As of now upfront amount deducted for dynamic transaction was gas*maxGasFeeCap which is wrong.
upfront amount should be gas*gasPrice and for dynamic transaction gasPrice = Min((gasTipCap + baseFee), gasFeeCap)

EVM-804: Txpool didn't handle the legacy transactions properly when London fork is enabled.

Underpriced legacy transaction were not discarded in txpool.
Coinbase tip was wrong in case of Legacy transactions.

Changes include

Bugfix (non-breaking change that solves an issue)
Hotfix (change that solves an urgent issue, and requires immediate attention)
New feature (non-breaking change that adds functionality)
Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

I have assigned this PR to myself
I have added at least 1 reviewer
I have added the relevant labels
I have updated the official documentation
I have added sufficient documentation in code

Testing

I have tested this code with the official test suite
I have tested this code manually

Manual tests

Executed a transaction and compared the initial and final account balance of burn contract, validator, transaction sender and transaction receiver

jp-imx · 2023-08-25T05:40:15Z

state/executor.go

@@ -450,7 +450,7 @@ func (t *Transition) subGasLimitPrice(msg *types.Transaction) error {
 	factor := new(big.Int)
 	if msg.GasFeeCap != nil && msg.GasFeeCap.BitLen() > 0 {


Hi team, just trying to better understand the implementation of the executor.go and I would like to ask a question.

Why check msg.GasFeeCap instead of msg.Type? The txpool.validateTx enforces that all DynamicFee transactions (i.e., msg.Type == DynamicFeeTx) have a gasFeeCap and a gasTipCap, and gasFeeCap >= baseFee. Is there a better guarantee in checking the gasFeeCap here? 🤔

There is no strict reason why we should go this way rather than checking tx type. The idea was to be more explicit and avoid potential nil pointer panic issue there.

We should be consistent through code-base. I vote for type check everywhere. If we have dynamic tx without those fields than we have much bigger problem

jp-imx · 2023-08-25T06:04:58Z

state/executor.go

@@ -450,7 +450,7 @@ func (t *Transition) subGasLimitPrice(msg *types.Transaction) error {
 	factor := new(big.Int)
 	if msg.GasFeeCap != nil && msg.GasFeeCap.BitLen() > 0 {
 		// Apply EIP-1559 tx cost calculation factor
-		factor = factor.Set(msg.GasFeeCap)
+		factor = factor.Set(msg.GetGasPrice(t.ctx.BaseFee.Uint64()))


Yesterday I sent this transaction to Polygon Edge and it was mined:

{ "jsonrpc": "2.0", "id": 1, "result": { "nonce": "0x0", "gasPrice": "0x6b", "maxPriorityFeePerGas": "0x64", "maxFeePerGas": "0x19a45965", "gas": "0x5208", "to": "0x1064c5ddD8635bA3D8FEe5cd8DEf8d502721EcEa", "value": "0x3b9aca00", "input": "0x", "v": "0x1", "r": "0x88e8f015ad334bc048c60b5561791a31ced4f8e87237eea00794c080eb13db5c", "s": "0x30758b64ea1472a298b0f8f85d0f9fd15c1e2b7f9eae26744dc4a597bb9f52d8", "hash": "0x209cefcd3bf2c6713195e4732575824827e13b7977d48002e5f039fdf15838bc", "from": "0xB23650A2b25aB51590e3b4e66bFc4AE426Eb1cA5", "blockHash": "0xe7bfc47dac8bd7c1fe1390cc4d55fbf1bfcc7d158a457326d425cf4ffd29541f", "blockNumber": "0x139", "transactionIndex": "0x0", "chainId": "0x343c", "type": "0x2" } }

Notice that it has all gasPrice, maxFeePerGas and maxPriorityFeePerGas. Also, gasPrice is a lot lower than maxFeePerGas. baseFee for the block was 0x7, as you can see here:

{ "jsonrpc": "2.0", "id": 1, "result": { "parentHash": "0xa3b9b3c72a5bcfe37e4259026033f0ead7e4d950160d509c177c74426b6f4513", "sha3Uncles": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347", "miner": "0x46bc0fc20e0c71bd74bef78095fb68be8d0f9f58", "stateRoot": "0xaf4c8dcc798a862af50dadd16718ee80e56ba7d48e2d6ca528e7f672e9d7c463", "transactionsRoot": "0x9d248b1717270cb57407026f4e5bfa3ea060d9e4768b056a8e40ede234532dec", "receiptsRoot": "0x056b23fbba480696b65fe5a59b8f2148a1299103c4f57df839233af2cf4ca2d2", "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", "difficulty": "0x1", "totalDifficulty": "0x1", "size": "0x345", "number": "0x139", "gasLimit": "0xb2d05e00", "gasUsed": "0x5208", "timestamp": "0x64e68fa1", "extraData": "0x0000000000000000000000000000000000000000000000000000000000000000f8b0c0f843b84029d32f595b6b76fa23111138ab770da9698418f6caba9a3427b08e3a38d44ee003cb04b88acfea9c0a85cda63b156979daaa3f28a534ab90937c494cca1d7cdf07c28080f8658002a00166805ec67e416c4d360ecea2d51bab58cbfbd1dab4b612180dfc5aabf48aafa00166805ec67e416c4d360ecea2d51bab58cbfbd1dab4b612180dfc5aabf48aafa00000000000000000000000000000000000000000000000000000000000000000", "mixHash": "0xadce6e5230abe012342a44e4e9b6d05997d6f015387ae0e59be924afc7ec70c1", "nonce": "0x0000000000000000", "hash": "0xe7bfc47dac8bd7c1fe1390cc4d55fbf1bfcc7d158a457326d425cf4ffd29541f", "transactions": [ "0x209cefcd3bf2c6713195e4732575824827e13b7977d48002e5f039fdf15838bc" ], "uncles": [], "baseFeePerGas": "0x7" } }

Does the implementation of GetGasPrice(baseFee uint64) mean that I can force a lower price for DynamicFeeTx because gasPrice will be picked over baseFee + gasTipCap / gasFeeCap? 🤔

@jp-imx can you please share the transaction object which you used as well as the tool/library employed for the purpose of transaction signing?

@rachit77 I used cast from foundry. I believe I ran this command:

cast send <RECIPIENT_ADDRESS> --value 1gwei --rpc-url http://127.0.0.1:40002/ --gas-price 430201189wei --priority-gas-price=1wei --private-key <PRIVATE_KEY> --nonce 11

@jp-imx Answering your original question:

You signed a type-2(dynamic tx) with cast with maxFeePerGas = 430201189 wei and maxPriorityFee= 100 wei, the transaction was included in block when baseFee was 7 wei.
Effective gas price that will be charged according to EIP-1559 specs, min( maxFeePerGas, baseFee+maxPriorityFee).
With the above formula effective gasPrice for your above transaction should be 107 wei and it is the same.

Notice that it has all gasPrice, maxFeePerGas and maxPriorityFeePerGas. Also, gasPrice is a lot lower than maxFeePerGas. baseFee for the block was 0x7, as you can see here:

The receipt of dynamic tx has all three fields, maxFeePerGas and maxPriorityFeePerGas will be there since they are provided with the transaction. gasPrice is the effective gas price that is deduced and charged according to EIP-1559 specifications.

@rachit77, thanks for getting back to me. I agree with your analysis. But taking it one step further, given the implementation of GetGasPrice starts like this:

func (t *Transaction) GetGasPrice(baseFee uint64) *big.Int { if t.GasPrice != nil && t.GasPrice.BitLen() > 0 { return new(big.Int).Set(t.GasPrice) } else if baseFee == 0 { return big.NewInt(0) } //... }

If I create a custom Tx object that sets gasFeeCap at a very high number but gasPrice to 1, could I underpay for my transaction?

@jp-imx

cast send <RECIPIENT_ADDRESS> --value 1gwei --rpc-url http://127.0.0.1:40002/ --gas-price 430201189wei --priority-gas-price=1wei --private-key <PRIVATE_KEY> --nonce 11

The above transaction will be treated as Type-2 transaction by cast, although you have provided gas-price value in the transaction but signer will treat this as maxFeePerGas.

Foundry Docs here:

--gas-price price
Gas price for the transaction, or max fee per gas for EIP1559 transactions.

When edge client receives this signed transaction, the transaction won't have gasPrice field but rather it will have GasFeeCap(maxFeePerGas) and GasTipCap(maxPriorityFee). So there is no possibility of underpaying for a transaction.

@rachit77 I don't think using cast is relevant. I can roll custom code for sending a raw transaction and include all three gasPrice, gasFeeCap, and gasTipCap. The issue is mute though because on further inspection I found that rlp_unmarshal.go ignores gasPrice and if all three are included the Tx is rejected with incorrect number of transaction elements, expected 12 but found 13. So the transaction would never reach GetGasPrice

yeah that is right

The GetGasPrice function looks strange to me as well and it seems it would be more natural to check transaction type if DynamicFeeTx should not have a gas price set.

txpool/txpool.go

vcastellm

LGTM

Stefan-Ethernal

Generally looks good, just address pending comments.

rachit77 · 2023-09-05T12:34:42Z

The changes from this PR are included in 1857

* corrected upfront gas deduction for type-2 transactions * solve the bug reported in EVM-804 * code optimization * testCase fix * added logic * fix linting error * refactored code * consensus should also have baseFee and gasPrice/gasFeeCap check for legacy and dynamic tx respecitively * linter fix * cr fix * don't build executable heap with baseFee * fixed txpool e2e tests * fixed e2e migration test * logic for fork handler * fix linting * fix unit test * added hard fork logic for PR #1849 * fixed TestE2E_TxPool_Transfer_Linear * added e2e test case for EIP-1559 spec * fix Test_Transition_checkDynamicFees

corrected upfront gas deduction for type-2 transactions

5bcedac

rachit77 added the bug fix Functionality that fixes a bug label Aug 24, 2023

rachit77 self-assigned this Aug 24, 2023

rachit77 requested review from begmaroman, igorcrevar and vcastellm August 24, 2023 17:37

rachit77 added don't merge Please don't merge this functionality temporarily breaking change Functionality that contains breaking changes labels Aug 24, 2023

rachit77 added 2 commits August 25, 2023 01:28

solve the bug reported in EVM-804

d37eace

resolved merge conflicts

54c33d6

jp-imx reviewed Aug 25, 2023

View reviewed changes

rachit77 added 2 commits August 25, 2023 21:57

code optimization

779ccbd

testCase fix

727e277

begmaroman reviewed Aug 30, 2023

View reviewed changes

txpool/txpool.go Show resolved Hide resolved

vcastellm approved these changes Aug 31, 2023

View reviewed changes

Stefan-Ethernal approved these changes Sep 1, 2023

View reviewed changes

jelacamarko approved these changes Sep 1, 2023

View reviewed changes

rachit77 added a commit to rachit77/polygon-edge that referenced this pull request Sep 3, 2023

added hard fork logic for PR 0xPolygon#1849

8b1d992

rachit77 closed this Sep 5, 2023

github-actions bot locked and limited conversation to collaborators Sep 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

corrected upfront gas deduction for type-2 transactions #1849

corrected upfront gas deduction for type-2 transactions #1849

rachit77 commented Aug 24, 2023 •

edited

Loading

jp-imx Aug 25, 2023

begmaroman Aug 28, 2023

igorcrevar Aug 29, 2023

jp-imx Aug 25, 2023

rachit77 Aug 25, 2023

jp-imx Aug 27, 2023

rachit77 Aug 28, 2023

jp-imx Aug 28, 2023

rachit77 Aug 28, 2023

jp-imx Aug 29, 2023 •

edited

Loading

rachit77 Aug 29, 2023

Stefan-Ethernal Sep 1, 2023 •

edited

Loading

vcastellm left a comment

Stefan-Ethernal left a comment

rachit77 commented Sep 5, 2023

		@@ -450,7 +450,7 @@ func (t Transition) subGasLimitPrice(msg types.Transaction) error {
		factor := new(big.Int)
		if msg.GasFeeCap != nil && msg.GasFeeCap.BitLen() > 0 {

corrected upfront gas deduction for type-2 transactions #1849

corrected upfront gas deduction for type-2 transactions #1849

Conversation

rachit77 commented Aug 24, 2023 • edited Loading

Description

Changes include

Checklist

Testing

Manual tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jp-imx Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stefan-Ethernal Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

vcastellm left a comment

Choose a reason for hiding this comment

Stefan-Ethernal left a comment

Choose a reason for hiding this comment

rachit77 commented Sep 5, 2023

rachit77 commented Aug 24, 2023 •

edited

Loading

jp-imx Aug 29, 2023 •

edited

Loading

Stefan-Ethernal Sep 1, 2023 •

edited

Loading