Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove gas_target from block structure #3552

Merged
merged 4 commits into from
May 7, 2021

Conversation

lightclient
Copy link
Member

@lightclient lightclient commented May 5, 2021

Per @karalabe's document and proposal, this PR will remove the notion of gasTarget in the block structure. It will revert to the current status quo of gasLimit. The gasTarget will continue to be an integral part of EIP-1559, but it will be computed each block with the formula gasLimit // ELASTICITY_MULTIPLIER. On the fork block, the gas target will be interpreted as the parent block's gas limit to avoid unduly halving the gas target.

@holiman
Copy link
Contributor

holiman commented May 5, 2021

I also started on something similar. This is what I had so far:

diff --git a/EIPS/eip-1559.md b/EIPS/eip-1559.md
index e8559ef..34a1c53 100644
--- a/EIPS/eip-1559.md
+++ b/EIPS/eip-1559.md
@@ -16,7 +16,7 @@ A transaction pricing mechanism that includes fixed-per-block network fee that i
 ## Abstract
 We introduce a new [EIP-2718](./eip-2718.md) transaction type, with the format `0x02 || rlp([chainId, nonce, maxPriorityFeePerGas, maxFeePerGas, gasLimit, to, value, data, access_list, signatureYParity, signatureR, signatureS])`.
 
-There is a base fee per gas in protocol, which can move up or down each block according to a formula which is a function of gas used in parent block and gas target (formerly known as gas limit) of parent block.
+There is a base fee per gas in protocol, which can move up or down each block according to a formula which is a function of gas used in parent block and gas target (half the gas limit) of parent block.
 The algorithm results in the base fee per gas increasing when blocks are above the gas target, and decreasing when blocks are below the gas target.
 The base fee per gas is burned.
 Transactions specify the maximum fee per gas they are willing to give to miners to incentivize them to include their transaction (aka: priority fee).
@@ -133,7 +133,7 @@ class Block:
        logs_bloom: int = 0
        difficulty: int = 0
        number: int = 0
-       gas_target: int = 0 # note the name change to gas_target from gas_limit
+       gas_limit: int = 0 
        gas_used: int = 0
        timestamp: int = 0
        extra_data: bytes = bytes()
@@ -157,15 +157,16 @@ class World(ABC):
        def validate_block(self, block: Block) -> None:
                parent_base_fee_per_gas = self.parent(block).base_fee_per_gas
                parent_gas_used = self.parent(block).gas_used
-               parent_gas_target = self.parent(block).gas_target
+               parent_gas_target = self.parent(block).gas_limit // ELASTICITY_MULTIPLIER
                transactions = self.transactions(block)
+               gas_target = block.gas_limit // ELASTICITY_MULTIPLIER
 
                # check if the block used too much gas
-               assert block.gas_used <= block.gas_target * ELASTICITY_MULTIPLIER, 'invalid block: too much gas used'
+               assert block.gas_used <= block.gas_limit, 'invalid block: too much gas used'
 
                # check if the block changed the gas target too much
-               assert block.gas_target <= parent_gas_target + parent_gas_target // 1024, 'invalid block: gas target increased too much'
-               assert block.gas_target >= parent_gas_target - parent_gas_target // 1024, 'invalid block: gas target decreased too much'
+               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'
 
                # check if the base fee is correct
                if parent_gas_used == parent_gas_target:

EIPS/eip-1559.md Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

holiman commented May 5, 2021

It looks like the changes are fairly small. One thing I just noticed, though, is that the reference implementation doesn't mention how to act on the switch-over between non-1559 and 1559, where the gas_target did not exist. And with this change, unless we explicitly tell the gas_limit to bump, it'll effectively be halved at the fork block.

@alita-moore
Copy link
Contributor

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
Copy link
Contributor

holiman commented May 5, 2021

For future reference though; I posted the idea in ACD, but I think it was @fjl or @karalabe who had the idea during review call today, so don't credit me for it

@lightclient
Copy link
Member Author

It looks like the changes are fairly small. One thing I just noticed, though, is that the reference implementation doesn't mention how to act on the switch-over between non-1559 and 1559, where the gas_target did not exist. And with this change, unless we explicitly tell the gas_limit to bump, it'll effectively be halved at the fork block.

This is a good point. I've updated to avoid this.

@vbuterin
Copy link
Contributor

vbuterin commented May 5, 2021

Would this reduce capacity by 2x once the EIP turns on and before miners get around to voting the gaslimit up to 25m? Is that an issue?

@lightclient
Copy link
Member Author

@vbuterin: @holiman pointed this out, so I updated the spec as follows:

# On the fork block, don't account for the ELASTICITY_MULTIPLIER to avoid
# unduly halving the gas_target. 
if INITIAL_FORK_BLOCK_NUMBER == block.number:
    parent_gas_target = self.parent(block).gas_limit 

@karalabe
Copy link
Member

karalabe commented May 6, 2021

For future reference though; I posted the idea in ACD, but I think it was @fjl or @karalabe who had the idea during review call today, so don't credit me for it

'twas mine 🥰

@karalabe
Copy link
Member

karalabe commented May 6, 2021

Before merging this into the spec, one open question would be how to do the miner side gas control of things - if in any way - so they don't start pushing the limit down once the fork passes. Currently Geth has two flags to control the miner's gas behavior:

  --miner.gastarget value             Target gas floor for mined blocks (default: 8000000)
  --miner.gaslimit value              Target gas ceiling for mined blocks (default: 8000000)

Essentially --miner.gastarget is the baseline size of empty / not full blocks below which the miner won't go (or will increment upwards to). And --miner.gaslimit is the size above which the miner will not go even if the blocks are full.

On mainnet, Geth miners I'd assume are running with --miner.gaslimit=15M (and perhaps also --miner.gastarget=15M, but that's less relevant as blocks are always full).

Come the 1559 fork, with this gasLimit proposal we'll see a jump from 15M gas to 30M gas on the fork block. This will make all Geth miners realize they're over their allowance and start voting the limit back down to 15M (or the semantic gasTarget to 7.5M). I'm unsure if we should try to be smart here and define some temporary mining hack to circumvent this, or just ask miners to bump their targets right after the fork?

@vbuterin vbuterin merged commit 67d0d75 into ethereum:master May 7, 2021
PhABC pushed a commit to PhABC/EIPs that referenced this pull request Jan 25, 2022
* remove gas_target from block structure

* missed block.gas_target

* avoid undue gas_target halving on 1559 fork block

* fix spelling error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants