EIP1559: import original min gas limit

[jochem-brouwer]

Current spec seems to indicate we remove the min gas limit (see Yellow Paper reference):

This PR fixes this as removing the min gas limit is not desired.

[alita-moore]

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

 - eip-1559.md requires approval from one of (vbuterin, econoar, afdudley, mslipper, i-norden, abdelhamidbakhta)

@vbuterin @econoar @AFDudley @mslipper @i-norden @abdelhamidbakhta

[holiman]

Another thing to note: The original rules (which can be seen in the screenshot above), used the <, not <=.
Therefore, if we just translate between limit and target, the corresponding would be:

	assert gas_target < parent_gas_target + parent_gas_target // 1024, 'invalid block: gas target increased too much'
	assert gas_target > parent_gas_target - parent_gas_target // 1024, 'invalid block: gas target decreased too much'

Whereas EIP-1559, as currently writen, uses:

	assert gas_target <= parent_gas_target + parent_gas_target // 1024, 'invalid block: gas target increased too much'
	assert gas_target => parent_gas_target - parent_gas_target // 1024, 'invalid block: gas target decreased too much'

I didn't notice this until now, and I suspect that the same oversight may be present in other implementations.

cc @RatanRSur @timbeiko @karalabe @tkstanczak

[jochem-brouwer]

Ouch that's a good spot, I didn't notice this myself either! 😄

[jochem-brouwer]

Should we change this so it matches the YP again?

[holiman]

Should we change this so it matches the YP again?

I have no strong opinion in that matter. However, I do have a strong opinion on getting clients to agree. Here are some test vectors for how geth is currently implemented (afaict according to the spec as written):

https://github.com/ethereum/go-ethereum/blob/c9116a7c19301d6906c1aff87085c2fa345a84bf/consensus/misc/eip1559_test.go#L62

Would be neat if other implementors checked those same exact cases. They test both the exact limit, and exceeding the limit, up aswell as down.

[holiman]

If it turns out that most clients have it the other (original) way, I think I'd prefer to use that. It's slightly simpler, because the flooring doesn't matter. As it is now, the lower limit is not symmetrical with the upper limit.

EDIT: The fact that we divide the gaslimit by 2 to arrive at the gasTarget, and then do the calculations off of the gasTarget instead of the gasLimit is what causes the non-symmetry

[jochem-brouwer]

I think we should go for the original way, because this change somewhat increases code complexity.

[holiman]

Same testcases, but in table-form:

parent.GasLimit	isLondon(parent)	header.GasLimit	Valid?
10000000	false	20000000	true
10000000	false	20019531	true
10000000	false	20019532	false
10000000	false	19980470	true
10000000	false	19980469	false

parent.GasLimit	isLondon(parent)	header.GasLimit	Valid?
20000000	true	20000000	true
20000000	true	20019531	true
20000000	true	20019532	false
20000000	true	19980470	true
20000000	true	19980469	false

parent.GasLimit	isLondon(parent)	header.GasLimit	Valid?
40000000	true	40039063	true
40000000	true	40039064	false
40000000	true	39960938	true
40000000	true	39960937	false

[matkt]

Same testcases, but in table-form:
parent.GasLimit isLondon(parent) header.GasLimit Valid?
10000000 false 20000000 true
10000000 false 20019531 true
10000000 false 20019532 false
10000000 false 19980470 true
10000000 false 19980469 false
parent.GasLimit isLondon(parent) header.GasLimit Valid?
20000000 true 20000000 true
20000000 true 20019531 true
20000000 true 20019532 false
20000000 true 19980470 true
20000000 true 19980469 false
parent.GasLimit isLondon(parent) header.GasLimit Valid?
40000000 true 40039063 true
40000000 true 40039064 false
40000000 true 39960938 true
40000000 true 39960937 false

thanks for that. Do you have some testcases when it it is the INITIAL_FORK_BLOCK_NUMBER ?

[holiman]

thanks for that. Do you have some testcases when it it is the INITIAL_FORK_BLOCK_NUMBER ?

In the first table, isLondon(parent) == false, which means that the header itself is the first london block == INITIAL_FORK_BLOCK_NUMBER

[sunce86]

I think we should go for the original way, because this change somewhat increases code complexity.

For OE original way also has lower code complexity.

[karalabe]

So, I think operating on the gasTarget at this point is a leftover. The spec did it because the header exposed gasTarget instead of gasLimit, but once we reverted to using the limit, we should revert everywhere. It keeps things simple and consistent.

[holiman]

I agree. I'll open a new PR

[holiman]

Suggestion:#3566

[holiman]

@vbuterin please approve this one too, my PR which got merged was intended to be in conjunction with this one.

[holiman]

but @jochem-brouwer perhaps you need to rebase it now (sorry!)

[jochem-brouwer]

@holiman No problem, rebased upon master 😄

[AbdelStark]

Looks good to me