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

EIP-1559 implementation #22617

Closed
wants to merge 44 commits into from
Closed

EIP-1559 implementation #22617

wants to merge 44 commits into from

Conversation

lightclient
Copy link
Member

@lightclient lightclient commented Apr 4, 2021

I'm opening this PR to track the consensus changes for EIP-1559. Updates to non-consensus code has been started and will be opened in another PR. This is a WIP.

@lightclient lightclient force-pushed the eip1559 branch 2 times, most recently from 7489ab6 to 37a7845 Compare April 16, 2021 20:12
@lightclient lightclient marked this pull request as ready for review April 16, 2021 22:40
@lightclient
Copy link
Member Author

lightclient commented Apr 16, 2021

The EIP-1559 and EIP-3198 changes are ready for review. They've been running on Aleut for about a week now. Additionally, I've added --aleut to the cli here. Feel free to cherry-pick that commit onto this if you wish.
cc: @karalabe @holiman @fjl

func (tx *DynamicFeeTx) gas() uint64 { return tx.Gas }
func (tx *DynamicFeeTx) feeCap() *big.Int { return tx.FeeCap }
func (tx *DynamicFeeTx) tip() *big.Int { return tx.Tip }
func (tx *DynamicFeeTx) gasPrice() *big.Int { return tx.Tip }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the best way to handle gasPrice in DynamicFeeTx. It seems like there are 3 options:

  • use tx.Tip as it still conveys the "urgency" like `gasPrice
  • use a sentinel value (e.g. 0)
  • refactor tx interface to be better "typed" (e.g. DynamicFeeTx simply does not have gas price as a method)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed it in triage call and it should return tx.FeeCap

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor nitpicks. Looks pretty good, thank you!
I'll do some more in-depth testing soonish

consensus/misc/eip1559.go Outdated Show resolved Hide resolved
consensus/misc/eip1559.go Show resolved Hide resolved
@@ -92,6 +173,7 @@ type headerMarshaling struct {
GasUsed hexutil.Uint64
Time hexutil.Uint64
Extra hexutil.Bytes
BaseFee *hexutil.Big
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to regenerate the headerMarshalling in core/types/gen_header_json.go

consensus/misc/eip1559.go Show resolved Hide resolved
consensus/misc/eip1559_test.go Show resolved Hide resolved
core/types/dynamic_fee_tx.go Outdated Show resolved Hide resolved
core/types/transaction.go Outdated Show resolved Hide resolved
core/vm/eips.go Show resolved Hide resolved
params/config.go Outdated Show resolved Hide resolved
params/config.go Outdated Show resolved Hide resolved
consensus/misc/eip1559.go Outdated Show resolved Hide resolved
consensus/misc/eip1559.go Outdated Show resolved Hide resolved
consensus/misc/eip1559.go Outdated Show resolved Hide resolved
consensus/misc/eip1559.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

holiman commented Apr 28, 2021

See https://github.com/ledgerwatch/turbo-geth/pull/1834/files -- we need something similar applied here too, right?

@holiman
Copy link
Contributor

holiman commented Apr 28, 2021

state_processor.go:

func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg vm.Config) (types.Receipts, []*types.Log, uint64, error) {
	var (
		receipts types.Receipts
		usedGas  = new(uint64)
		header   = block.Header()
		allLogs  []*types.Log
		gp       = new(GasPool).AddGas(block.GasLimit())

^ here

@holiman
Copy link
Contributor

holiman commented Apr 28, 2021

And for the miner-part (another PR?), worker.go

	if w.current.gasPool == nil {
		w.current.gasPool = new(core.GasPool).AddGas(w.current.header.GasLimit)
	}

exceution.go

	gaspool.AddGas(pre.Env.GasLimit)

Copy link
Contributor

@zsfelfoldi zsfelfoldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (see my comments for minor nitpicks)

core/state_transition.go Outdated Show resolved Hide resolved
}
case isAleut && st.evm.Context.BaseFee == nil:
return fmt.Errorf("baseFee missing")
case !isAleut && st.evm.Context.BaseFee != nil:
Copy link
Member Author

@lightclient lightclient Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking back at this, do people think it makes sense to check that if EIP-1559 is not enable then the basefee should be nil? Can't seem to find it now, but I recall someone mentioning that they weren't fond of the BASEFEE op potentially dereferencing nil if somehow it made it past the fork checks and included the op in the jump table.

The reason I'm coming back to this is it seems t8n circumvents some of the code in state_processor so it can create blocks with no nil basefee even if Aleut is configured.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the errors here are named errors, like ErrNonceTooHigh, so the outer calller can do errors.Is(err, ErrNonceTooHigh). I think we should define these errors too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See core/state_processor_test.go: TestStateProcessorErrors. We should extend those tests for these cases.

if err := misc.VerifyEip1559Header(chain.Config(), parent, header); err != nil {
return err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to also have an else-block to verify that the header does not have a basefee. And I don't quite get why there's no corresponding if header.GasUsed > header.GasLimit for clique (I mean previously) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'll add this.

Not sure why that check wasn't here previously though. Should I add it?

@lightclient lightclient force-pushed the eip1559 branch 2 times, most recently from 24346e7 to f6a9d64 Compare May 6, 2021 22:00
@fjl fjl added the london label May 11, 2021
@fjl fjl added this to the 1.10.4 milestone May 11, 2021
@karalabe
Copy link
Member

Closing in favor of #22837

@karalabe karalabe closed this May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants