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

feat: make eip2718 & eip1559 configurable in txpool #206

Merged
merged 9 commits into from
Feb 3, 2023

Conversation

lispc
Copy link

@lispc lispc commented Jan 31, 2023

background:

circuit only support legacy tx but cannot support Typed Transaction Envelope tx. We plan to support non legacy tx within months. So here i disable non-legacy tx at txpool level but not consensus level. all the sequencers within months will be "non-evil", they will not deliberately produce blocks with Typed Transaction Envelope. So as long as we support non-legacy tx before scroll network becomes decentralied (i believe this), this PR is a good solution.

alternative solutions:
(1) disable London/Berlin. We will miss a lot of new features
(2) disable some related EIPs. I find it is not easy.. Forks and eips are coupled tightly in codes, and i think using this solution we need to modify much more codes and much more error-prone.

@colinlyguo
Copy link
Member

colinlyguo commented Jan 31, 2023

Add two configs in ChainConfig:
Only enable legacy (type 0x01 && 0x02 disabled):

"enableEIP2718" : false,

Only disable EIP-1559 (type 0x02 disabled):

"enableEIP2718" : true,
"enableEIP1559" : false,

HAOYUatHZ
HAOYUatHZ previously approved these changes Feb 1, 2023
@HAOYUatHZ HAOYUatHZ changed the title txpool only supports legacy tx feat: make eip2718 & eip1559 configurable in txpool Feb 1, 2023
@mask-pp
Copy link

mask-pp commented Feb 1, 2023

Is it ok to disable params/config.go:IsBerlin() and params/config.go:IsLondon()?

@colinlyguo
Copy link
Member

colinlyguo commented Feb 1, 2023

Is it ok to disable params/config.go:IsBerlin() and params/config.go:IsLondon()?

This will disable Berlin Hard Fork and London Hard Fork, which would disable some other EIPs (may be useful).

@colinlyguo
Copy link
Member

colinlyguo commented Feb 1, 2023

Incompatibility issue: MetaMask chooses eip1559 when latest block header contains baseFeePerGas, so we let MetaMask send legacy tx by not setting baseFeePerGas in block headers.
https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/controllers/network/network-controller.js#L165-L166
Tested with MetaMask by switching the configs and restarting geth.

@mask-pp
Copy link

mask-pp commented Feb 2, 2023

Ok, add --IsEIP2718Enabled --IsEIP1559Enabled in cmd line can be more convenient.

@colinlyguo colinlyguo marked this pull request as draft February 2, 2023 11:34
@colinlyguo colinlyguo force-pushed the feat/legacy-tx branch 2 times, most recently from d8d5ed0 to e1071df Compare February 2, 2023 12:46
@colinlyguo colinlyguo marked this pull request as ready for review February 2, 2023 12:56
internal/ethapi/api.go Outdated Show resolved Hide resolved
Copy link

@mask-pp mask-pp left a comment

Choose a reason for hiding this comment

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

LGTM

miner/worker.go Show resolved Hide resolved
miner/worker.go Show resolved Hide resolved
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.

None yet

4 participants