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

Fix: Base Fee Off By One #2015

Merged
merged 17 commits into from
Jan 8, 2025
Merged

Fix: Base Fee Off By One #2015

merged 17 commits into from
Jan 8, 2025

Conversation

jewei1997
Copy link
Contributor

@jewei1997 jewei1997 commented Dec 30, 2024

Describe your changes and provide context

This PR fixes the issue where the block base fee was the base fee after the execution of the current block's txs, but it should be the base fee before the block's execution. A separate PrevBlockBaseFeePerGasPrefix is introduced to store the previous block's base fee per gas to store on the current block.

Having base fee on the wrong block impacted tracing. Tracing would fail like this

{
    "jsonrpc": "2.0",
    "id": 1,
    "error": {
        "code": -32000,
        "message": "transaction 0xd31bfeb29244add3d26a073b48084fe8c5e92c13a38b1de81b39489a8bb3089f failed: max fee per gas less than block base fee: address 0xd61Fa8312C1825f29605dEBDbC9822ccA31A2038, maxFeePerGas: 1000000010, baseFee: 1003515001"
    }
}

Testing performed to validate your change

integration test + tested upgrade.

Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 70.45455% with 13 lines in your changes missing coverage. Please review.

Project coverage is 61.49%. Comparing base (0586ad8) to head (2dc370a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
x/evm/keeper/fee.go 66.66% 7 Missing and 3 partials ⚠️
x/evm/module.go 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2015      +/-   ##
==========================================
+ Coverage   61.29%   61.49%   +0.20%     
==========================================
  Files         263      264       +1     
  Lines       24521    24550      +29     
==========================================
+ Hits        15030    15097      +67     
+ Misses       8369     8324      -45     
- Partials     1122     1129       +7     
Files with missing lines Coverage Δ
evmrpc/block.go 76.24% <100.00%> (ø)
evmrpc/info.go 71.01% <100.00%> (ø)
evmrpc/simulate.go 65.21% <100.00%> (ø)
x/evm/ante/fee.go 65.38% <100.00%> (ø)
x/evm/keeper/keeper.go 48.55% <100.00%> (ø)
x/evm/migrations/migrate_base_fee_off_by_one.go 100.00% <100.00%> (ø)
x/evm/types/keys.go 18.86% <ø> (ø)
x/evm/module.go 38.53% <25.00%> (-0.58%) ⬇️
x/evm/keeper/fee.go 76.47% <66.66%> (-7.66%) ⬇️

... and 3 files with indirect coverage changes

x/evm/keeper/fee.go Fixed Show fixed Hide fixed
d := sdk.Dec{}
err := d.UnmarshalJSON(bz)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
@jewei1997 jewei1997 changed the title [wip] Base Fee Off By One Fix: Base Fee Off By One Jan 7, 2025

const testCases = [
["No truncation from max priority fee", gp, gp],
["With truncation from max priority fee", gp, highgp],
["With complete truncation from max priority fee", zero, highgp]
];

it("Check base fee on the right block", async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 👍

store := ctx.KVStore(k.storeKey)
bz, err := baseFeePerGas.MarshalJSON()
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

where does this panic end up getting thrown up to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would get thrown in EndBlock which would halt the chain if thrown, but technically shouldn't get thrown unless something has gone really wrong.

store := ctx.KVStore(k.storeKey)
bz, err := baseFeePerGas.MarshalJSON()
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
Copy link
Collaborator

@codchen codchen left a comment

Choose a reason for hiding this comment

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

nice testing

@jewei1997 jewei1997 enabled auto-merge (squash) January 8, 2025 14:16
@jewei1997 jewei1997 merged commit 8b679d7 into main Jan 8, 2025
49 checks passed
@jewei1997 jewei1997 deleted the base-fee-off-by-one branch January 8, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants