refactor: re-implement `eth_feeHistory` #3288

onbjerg · 2023-06-20T22:31:40Z

The existing implementation of eth_feeHistory had some issues:

It did not limit the request size
It did not correctly calculate percentiles, because it was comparing header.gas_used to the percentile threshold, but it should be comparing a cumulative gas usage of transactions to the percentile threshold
It did not include the base fee of the next block (even if it doesn't exist) as per the spec
It always responded with at least reward: [], but if the request does not include percentiles, then it should not respond with this at all
It always loaded and sorted transactions, even if there were no percentiles (so it would be unnecessary)
It supported block hashes when it should only support numbers or tags

I also opted to remove the fee history cache, since it adds additional overhead, and it is hard to cache this endpoint, as the response can change significantly based on the reward percentiles.

Perhaps we can use some sort of basic cache for the headers only, or the headers and a pre-sorted list of transactions, but I don't think it would really give us much benefit.

Also closes #3328

onbjerg · 2023-06-20T22:32:03Z

I am going to test this on a node and compare the results to Erigon to get a sense of if the implementation is flawed or not

codecov · 2023-06-20T22:52:08Z

Codecov Report

Merging #3288 (9f1db8a) into main (49922bf) will decrease coverage by 0.15%.
The diff coverage is 61.38%.

@@            Coverage Diff             @@
##             main    #3288      +/-   ##
==========================================
- Coverage   69.41%   69.27%   -0.15%     
==========================================
  Files         537      537              
  Lines       71991    72041      +50     
==========================================
- Hits        49973    49906      -67     
- Misses      22018    22135     +117

Flag	Coverage Δ
integration-tests	`16.33% <2.47%> (-0.01%)`	⬇️
unit-tests	`64.34% <61.38%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
crates/primitives/src/transaction/mod.rs	`85.23% <0.00%> (-1.57%)`	⬇️
crates/rpc/rpc-api/src/eth.rs	`100.00% <ø> (ø)`
crates/rpc/rpc/src/eth/api/mod.rs	`87.09% <ø> (-0.31%)`	⬇️
crates/rpc/rpc/src/eth/error.rs	`7.50% <0.00%> (ø)`
crates/rpc/rpc/src/eth/api/fees.rs	`48.41% <33.68%> (-38.55%)`	⬇️
crates/rpc/rpc/src/eth/api/server.rs	`95.41% <87.12%> (-2.87%)`	⬇️
crates/rpc/rpc-types/src/eth/fee.rs	`7.14% <100.00%> (-50.76%)`	⬇️
crates/rpc/rpc/src/eth/gas_oracle.rs	`39.59% <100.00%> (+1.24%)`	⬆️

... and 14 files with indirect coverage changes

mattsse · 2023-06-21T20:13:36Z

crates/rpc/rpc/src/eth/api/fees.rs

+            self.inner.provider.receipts_by_block(header.number.into())? else {
+            // If there are no receipts, then we do not have all info on the block
+            return Err(EthApiError::InvalidBlockRange)
+        };
+        let Some(mut transactions): Option<Vec<_>> = self
+            .inner
+            .provider
+            .transactions_by_block(header.number.into())?


these should make use of the async caching layer I think

how do I do that?

onbjerg · 2023-06-21T22:59:34Z

TL;DR: It matches except in the case of block count of 0, but this case is not defined in the spec..

Comparison between Erigon's impl and Reth's impl. Each example uses the same percentiles (0, 10, 25, 50, 100).

Block 17m, block count of 1

Erigon:

{
   "jsonrpc":"2.0",
   "id":1,
   "result":{
      "oldestBlock":"0x1036640",
      "reward":[
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0xb376620",
            "0x26df9ef958"
         ]
      ],
      "baseFeePerGas":[
         "0x4cad3abe1",
         "0x48f2114b8"
      ],
      "gasUsedRatio":[
         0.3053592666666667
      ]
   }
}

Reth:

{
   "jsonrpc":"2.0",
   "result":{
      "baseFeePerGas":[
         "0x4cad3abe1",
         "0x48f2114b8"
      ],
      "gasUsedRatio":[
         0.3053592666666667
      ],
      "oldestBlock":"0x1036640",
      "reward":[
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0xb376620",
            "0x26df9ef958"
         ]
      ]
   },
   "id":1
}

Block 17m, block count of 0

Erigon:

{
   "jsonrpc":"2.0",
   "id":1,
   "result":{
      "oldestBlock":"0x0",
      "gasUsedRatio":null
   }
}

Reth:

{
   "jsonrpc":"2.0",
   "result":{
      "baseFeePerGas":[
         
      ],
      "gasUsedRatio":[
         
      ],
      "oldestBlock":"0x0",
      "reward":null
   },
   "id":1
}

Doesn't match, but are logically similar I think

Block 17m, block count of 10

Erigon:

{
   "jsonrpc":"2.0",
   "id":1,
   "result":{
      "oldestBlock":"0x1036637",
      "reward":[
         [
            "0x0",
            "0x59a5380",
            "0x5f5e100",
            "0x11e1a300",
            "0xcb7c3cc58"
         ],
         [
            "0x0",
            "0x1f8dead",
            "0x1f8dead",
            "0x4a75410",
            "0x2cd3a56ad"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x121738e4",
            "0x77359400",
            "0x527935f3e"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0x5f5e100",
            "0x8680c0212"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x11e1a300",
            "0x3b9aca00",
            "0x2b6e7d7f4"
         ],
         [
            "0x5f5e100",
            "0xa1fa7e6",
            "0x2d8dfc0c",
            "0x2d8dfc0c",
            "0x4cc25aef1"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0x5f5e100",
            "0x42a860c240"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0x5f5e100",
            "0xdb597ddd3"
         ],
         [
            "0x0",
            "0x485ce38",
            "0x59a5380",
            "0xb376620",
            "0x419b5992a"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0xb376620",
            "0x26df9ef958"
         ]
      ],
      "baseFeePerGas":[
         "0x4475fcda8",
         "0x42ee95553",
         "0x4b4ae5842",
         "0x44d989922",
         "0x45200f7f8",
         "0x4035437f4",
         "0x4839a89b2",
         "0x4705ccab0",
         "0x4f68b16aa",
         "0x4cad3abe1",
         "0x48f2114b8"
      ],
      "gasUsedRatio":[
         0.4106688,
         0.9996475666666667,
         0.1577187,
         0.5160047666666666,
         0.2154628,
         0.9994493,
         0.43339863333333334,
         0.9723170666666666,
         0.3623692,
         0.3053592666666667
      ]
   }
}

Reth:

{
   "jsonrpc":"2.0",
   "result":{
      "baseFeePerGas":[
         "0x4475fcda8",
         "0x42ee95553",
         "0x4b4ae5842",
         "0x44d989922",
         "0x45200f7f8",
         "0x4035437f4",
         "0x4839a89b2",
         "0x4705ccab0",
         "0x4f68b16aa",
         "0x4cad3abe1",
         "0x48f2114b8"
      ],
      "gasUsedRatio":[
         0.4106688,
         0.9996475666666667,
         0.1577187,
         0.5160047666666666,
         0.2154628,
         0.9994493,
         0.43339863333333334,
         0.9723170666666666,
         0.3623692,
         0.3053592666666667
      ],
      "oldestBlock":"0x1036637",
      "reward":[
         [
            "0x0",
            "0x59a5380",
            "0x5f5e100",
            "0x11e1a300",
            "0xcb7c3cc58"
         ],
         [
            "0x0",
            "0x1f8dead",
            "0x1f8dead",
            "0x4a75410",
            "0x2cd3a56ad"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x121738e4",
            "0x77359400",
            "0x527935f3e"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0x5f5e100",
            "0x8680c0212"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x11e1a300",
            "0x3b9aca00",
            "0x2b6e7d7f4"
         ],
         [
            "0x5f5e100",
            "0xa1fa7e6",
            "0x2d8dfc0c",
            "0x2d8dfc0c",
            "0x4cc25aef1"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0x5f5e100",
            "0x42a860c240"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0x5f5e100",
            "0xdb597ddd3"
         ],
         [
            "0x0",
            "0x485ce38",
            "0x59a5380",
            "0xb376620",
            "0x419b5992a"
         ],
         [
            "0x0",
            "0x5f5e100",
            "0x5f5e100",
            "0xb376620",
            "0x26df9ef958"
         ]
      ]
   },
   "id":1
}

Pre-EIP 1559 (block 4 with a block count of 4, i.e. genesis..=4)

Erigon:

{
   "jsonrpc":"2.0",
   "id":1,
   "result":{
      "oldestBlock":"0x1",
      "reward":[
         [
            "0x0",
            "0x0",
            "0x0",
            "0x0",
            "0x0"
         ],
         [
            "0x0",
            "0x0",
            "0x0",
            "0x0",
            "0x0"
         ],
         [
            "0x0",
            "0x0",
            "0x0",
            "0x0",
            "0x0"
         ],
         [
            "0x0",
            "0x0",
            "0x0",
            "0x0",
            "0x0"
         ]
      ],
      "baseFeePerGas":[
         "0x0",
         "0x0",
         "0x0",
         "0x0",
         "0x0"
      ],
      "gasUsedRatio":[
         0,
         0,
         0,
         0
      ]
   }
}

Reth:

{
   "jsonrpc":"2.0",
   "result":{
      "baseFeePerGas":[
         "0x0",
         "0x0",
         "0x0",
         "0x0",
         "0x0"
      ],
      "gasUsedRatio":[
         0.0,
         0.0,
         0.0,
         0.0
      ],
      "oldestBlock":"0x1",
      "reward":[
         [
            "0x0",
            "0x0",
            "0x0",
            "0x0",
            "0x0"
         ],
         [
            "0x0",
            "0x0",
            "0x0",
            "0x0",
            "0x0"
         ],
         [
            "0x0",
            "0x0",
            "0x0",
            "0x0",
            "0x0"
         ],
         [
            "0x0",
            "0x0",
            "0x0",
            "0x0",
            "0x0"
         ]
      ]
   },
   "id":1
}

mattsse · 2023-06-22T02:16:50Z

reth:

"reward":null

looks like erigon/geth don't return this field if null?

but return empty

"gasUsedRatio":[
         
      ],

as null

should we mimic this as well?

mattsse

nice!

left a few suggestions to asyncify this

crates/rpc/rpc/src/eth/api/fees.rs

onbjerg · 2023-06-26T12:38:09Z

@mattsse Now uses the cache and sealed headers

onbjerg · 2023-06-26T12:41:11Z

reth:

"reward":null

looks like erigon/geth don't return this field if null?

but return empty
"gasUsedRatio":[
         
      ],
as null

should we mimic this as well?

It's annoying that it responds like that, but I implemented 1:1 compat in af48ee1

mattsse

lgtm,
pending clippy

suggestions re Option<Vec

crates/rpc/rpc-types/src/eth/fee.rs

mattsse · 2023-06-26T13:26:48Z

crates/rpc/rpc/src/eth/api/fees.rs

+            self.cache().get_receipts(header.hash.into()).await? else {
+            // If there are no receipts, then we do not have all info on the block
+            return Err(EthApiError::InvalidBlockRange)
+        };
+        let Some(mut transactions): Option<Vec<_>> = self
+            .cache()
+            .get_block_transactions(header.hash).await?


this is fine,
I'll add a new helper function that fetches both concurrently.

onbjerg · 2023-06-26T13:56:02Z

@mattsse I removed the 1024 limit, it seems I was mistaken and this is not actually part of the spec.

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

Closes #3328

mattsse · 2023-06-26T15:17:00Z

@mattsse I removed the 1024 limit, it seems I was mistaken and this is not actually part of the spec.

we definitely need to cap it,

geth caps it to 1024:

https://github.com/ethereum/go-ethereum/blob/2754b197c935ee63101cbbca2752338246384fec/eth/ethconfig/config.go#L38-L46

onbjerg · 2023-06-26T15:18:17Z

This is the full node config, i.e. it is capped because it will not have data for more than 1024 blocks at head afaik. Unsure what the best approach is to cap it

Edit: Ah, nvm, I see, this is separate from the regular full node config, instead it is a GPO config and these are just the defaults for full node. Do we have some sort of config mechanism for the RPC where we could add this (with a default of 1024)?

mattsse · 2023-06-26T15:22:35Z

it's always capped, otherwise you can DOS the endpoint

https://github.com/ethereum/go-ethereum/blob/2754b197c935ee63101cbbca2752338246384fec/eth/gasprice/feehistory.go#L222-L225

it's configurable in geth via config though

https://github.com/ethereum/go-ethereum/blob/2754b197c935ee63101cbbca2752338246384fec/eth/gasprice/gasprice.go#L42-L50

onbjerg · 2023-06-26T15:25:04Z

@mattsse used GPO config to cap the length

mattsse · 2023-06-26T15:25:05Z

Do we have some sort of config mechanism for the RPC where we could add this (with a default of 1024)?

we have in in this:

reth/crates/rpc/rpc/src/eth/gas_oracle.rs

Lines 22 to 33 in 49922bf

    
           /// Settings for the [GasPriceOracle] 
        
           #[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)] 
        
           #[serde(rename_all = "camelCase")] 
        
           pub struct GasPriceOracleConfig { 
        
               /// The number of populated blocks to produce the gas price estimate 
        
               pub blocks: u32, 
        
               /// The percentile of gas prices to use for the estimate 
        
               pub percentile: u32, 
        
               /// The maximum number of headers to keep in the cache 
        
               pub max_header_history: u64,

which we should be able to query in eth_feeHistory via .gas_oracle()

reth/crates/rpc/rpc/src/eth/api/mod.rs

Lines 287 to 288 in 49922bf

    
           /// The async gas oracle frontend for gas price suggestions 
        
           gas_oracle: GasPriceOracle<Provider>,

onbjerg added C-bug An unexpected or incorrect behavior A-rpc Related to the RPC implementation labels Jun 20, 2023

onbjerg requested review from mattsse, Rjected and gakonst as code owners June 20, 2023 22:31

mattsse mentioned this pull request Jun 20, 2023

fix: fix another out of bounds error #3287

Closed

onbjerg marked this pull request as draft June 20, 2023 22:57

onbjerg force-pushed the onbjerg/eth-fee-history branch 2 times, most recently from 714c97b to f661913 Compare June 21, 2023 01:05

onbjerg added the M-changelog This change should be included in the changelog label Jun 21, 2023

mattsse reviewed Jun 21, 2023

View reviewed changes

onbjerg marked this pull request as ready for review June 21, 2023 23:05

mattsse requested changes Jun 22, 2023

View reviewed changes

This was referenced Jun 22, 2023

Support hexOrDecimal on eth_feeHistory #3328

Closed

feat: populate fee history cache when new block is added to chain #2357

Closed

onbjerg force-pushed the onbjerg/eth-fee-history branch from eef8623 to 887eb41 Compare June 26, 2023 11:31

onbjerg requested a review from mattsse June 26, 2023 12:37

mattsse reviewed Jun 26, 2023

View reviewed changes

mattsse approved these changes Jun 26, 2023

View reviewed changes

onbjerg added 4 commits June 26, 2023 16:53

refactor: re-implement eth_feeHistory

d3e7bec

chore: rm errant todo

7a98204

fix: use block tag or number

03a8e45

fix: correctly convert receipt gas

a3515b3

onbjerg and others added 13 commits June 26, 2023 16:54

refactor: move percentile calc

79152ab

chore: rm debug

f2c1184

fix: gas calc

bf84aa8

perf: use cache and sealed headers

d955743

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

chore: 1:1 compat with erigon response

8dffb44

chore: lint

8f2c7ef

test: adjust fee history test

d4464a7

chore: add todo

d3736c5

feat: support hex in eth_feeHistory

1dda9f8

Closes #3328

refactor: simplify return types

5232415

chore: lint

9f7bbdf

fix: remove 1024 limit

9860c2d

fix: serde default

a14ae6b

onbjerg force-pushed the onbjerg/eth-fee-history branch from eae486b to a14ae6b Compare June 26, 2023 14:55

chore: fix rebase

c229d67

onbjerg added 2 commits June 26, 2023 17:24

feat: cap max request at gpo history caps

afe94e5

docs: add impl note

9f1db8a

onbjerg enabled auto-merge June 26, 2023 15:27

onbjerg added this pull request to the merge queue Jun 26, 2023

Merged via the queue into main with commit e9caf3d Jun 26, 2023

onbjerg deleted the onbjerg/eth-fee-history branch June 26, 2023 16:29

mattsse mentioned this pull request Jun 26, 2023

perf: fetch receipts and transactions concurrently #3406

Merged

ryanschneider mentioned this pull request Jul 5, 2023

eth_feeHistory not fully working in 0.1.0 #3615

Closed

1 task

pistomat mentioned this pull request Jul 10, 2023

Error attempting a cast send transaction: (code: -32602, message: Invalid block range, data: None) #1977

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: re-implement `eth_feeHistory` #3288

refactor: re-implement `eth_feeHistory` #3288

onbjerg commented Jun 20, 2023 •

edited

Loading

onbjerg commented Jun 20, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading

mattsse Jun 21, 2023

onbjerg Jun 21, 2023

onbjerg commented Jun 21, 2023 •

edited

Loading

mattsse commented Jun 22, 2023

mattsse left a comment

onbjerg commented Jun 26, 2023

onbjerg commented Jun 26, 2023

mattsse left a comment •

edited

Loading

mattsse Jun 26, 2023

onbjerg commented Jun 26, 2023

mattsse commented Jun 26, 2023

onbjerg commented Jun 26, 2023 •

edited

Loading

mattsse commented Jun 26, 2023

onbjerg commented Jun 26, 2023

mattsse commented Jun 26, 2023 •

edited

Loading

refactor: re-implement eth_feeHistory #3288

refactor: re-implement eth_feeHistory #3288

Conversation

onbjerg commented Jun 20, 2023 • edited Loading

onbjerg commented Jun 20, 2023 • edited Loading

codecov bot commented Jun 20, 2023 • edited Loading

Codecov Report

mattsse Jun 21, 2023

Choose a reason for hiding this comment

onbjerg Jun 21, 2023

Choose a reason for hiding this comment

onbjerg commented Jun 21, 2023 • edited Loading

mattsse commented Jun 22, 2023

mattsse left a comment

Choose a reason for hiding this comment

onbjerg commented Jun 26, 2023

onbjerg commented Jun 26, 2023

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

mattsse Jun 26, 2023

Choose a reason for hiding this comment

onbjerg commented Jun 26, 2023

mattsse commented Jun 26, 2023

onbjerg commented Jun 26, 2023 • edited Loading

mattsse commented Jun 26, 2023

onbjerg commented Jun 26, 2023

mattsse commented Jun 26, 2023 • edited Loading

refactor: re-implement `eth_feeHistory` #3288

refactor: re-implement `eth_feeHistory` #3288

onbjerg commented Jun 20, 2023 •

edited

Loading

onbjerg commented Jun 20, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading

onbjerg commented Jun 21, 2023 •

edited

Loading

mattsse left a comment •

edited

Loading

onbjerg commented Jun 26, 2023 •

edited

Loading

mattsse commented Jun 26, 2023 •

edited

Loading