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.

Sign up for GitHub

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

Jump to bottom

feat: re-enable EIP-1559 in Banach hard fork #634

Merged
merged 31 commits into from
Mar 11, 2024
Merged

feat: re-enable EIP-1559 in Banach hard fork #634

merged 31 commits into from
Mar 11, 2024

Conversation

Thegaram
Copy link

@Thegaram Thegaram commented Feb 18, 2024 •
edited

1. Purpose or design rationale of this PR

Re-enable EIP-2718 and and EIP-1559 transactions.

Main changes:

  • Accept 2718 and 1559 txs in txpool.
  • Set baseFee in worker.
  • Update execution and verification logic.
  • Enable BASEFEE opcode.
  • Update max and target gas limit.
  • Do not burn ETH but send to fee vault.
  • Update traces to support new tx fields.

Test plan:

  • Test compatibility (sync with Scroll and Scroll Sepolia)
  • Test APIs (eth_sendRawTransaction, eth_getBlockByNumber, eth_feeHistory).
  • Test transition: Run local testnet with Banach block N > 0.

Local testnet test notes: https://gist.github.com/Thegaram/131e368f1faad4c8e23d556d92d1e463

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • feat: A new feature

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

@Thegaram Thegaram mentioned this pull request Feb 18, 2024
Closed
13 tasks
colinlyguo
colinlyguo reviewed Feb 29, 2024
View reviewed changes
Copy link
Member

@colinlyguo colinlyguo left a comment

Choose a reason for hiding this comment

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

Awesome changes! And leaving some comments.

consensus/misc/eip1559.go Outdated Show resolved Hide resolved
consensus/misc/eip1559.go Outdated Show resolved Hide resolved
core/genesis.go Outdated Show resolved Hide resolved
consensus/misc/eip1559.go Show resolved Hide resolved
miner/worker.go Show resolved Hide resolved
internal/ethapi/api.go Show resolved Hide resolved
consensus/misc/eip1559.go Show resolved Hide resolved
consensus/misc/eip1559.go Show resolved Hide resolved
Thegaram
Thegaram commented Mar 4, 2024
View reviewed changes
accounts/abi/bind/backends/simulated.go Show resolved Hide resolved
consensus/clique/clique.go Show resolved Hide resolved
consensus/ethash/consensus.go Show resolved Hide resolved
core/state_transition.go Outdated Show resolved Hide resolved
internal/ethapi/transaction_args.go Show resolved Hide resolved
miner/worker.go Show resolved Hide resolved
HAOYUatHZ
HAOYUatHZ reviewed Mar 4, 2024
View reviewed changes
internal/ethapi/api.go Outdated Show resolved Hide resolved
HAOYUatHZ
HAOYUatHZ previously approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@HAOYUatHZ HAOYUatHZ left a comment

Choose a reason for hiding this comment

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

LGTM!

@Thegaram Thegaram requested review from vyzo and omerfirmak March 5, 2024 09:06
@Thegaram
Copy link
Author

Thegaram commented Mar 5, 2024

Looks great! maybe I will approved again once other types of txs are tested

Updated, any also updated the traces part.

@Thegaram Thegaram marked this pull request as ready for review March 5, 2024 09:52
HAOYUatHZ
HAOYUatHZ previously approved these changes Mar 5, 2024
View reviewed changes
colinlyguo
colinlyguo previously approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@colinlyguo colinlyguo left a comment

Choose a reason for hiding this comment

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

LGTM.

internal/ethapi/transaction_args.go Show resolved Hide resolved
omerfirmak
omerfirmak reviewed Mar 5, 2024
View reviewed changes
eth/catalyst/api.go Outdated Show resolved Hide resolved
@Thegaram Thegaram dismissed stale reviews from colinlyguo and HAOYUatHZ via af385f6 March 5, 2024 17:40
@lispc
Copy link

lispc commented Mar 6, 2024

this branch works well with my EVM official testing cases.

@lispc
Copy link

lispc commented Mar 6, 2024

btw i can safely assume blobk.coinbase == fee_vault always?

lispc
lispc previously approved these changes Mar 6, 2024
View reviewed changes
colinlyguo
colinlyguo previously approved these changes Mar 11, 2024
View reviewed changes
@Thegaram Thegaram dismissed stale reviews from colinlyguo and lispc via 30a5fec March 11, 2024 13:47
@Thegaram
Copy link
Author

btw i can safely assume blobk.coinbase == fee_vault always?

Yes, the fee recipient (for base fee + tips) is always the fee vault. (But header.coinbase/header.miner is used by Clique for other purposes).

@Thegaram Thegaram merged commit ccec84c into develop Mar 11, 2024
6 checks passed
@Thegaram Thegaram deleted the banach-enable-1559 branch March 11, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lispc lispc lispc left review comments

@omerfirmak omerfirmak omerfirmak left review comments

@HAOYUatHZ HAOYUatHZ HAOYUatHZ approved these changes

@colinlyguo colinlyguo colinlyguo approved these changes

@vyzo vyzo Awaiting requested review from vyzo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Thegaram @HAOYUatHZ @lispc @omerfirmak @colinlyguo