Implementation of 'eth_gasPrice' #2396
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
This looks great so far (left two specific comments on things I think need to be revised).
Also, can you add a test that runs 20 blocks and computes the gasPrice? Maybe run 21 blocks and validate the computation produces an average of only the last 20 (maybe put some astronomical gas fee in the first block that moves the average up and then make the gas fee just like 20 or something in the following 20) and then validate that the price returned by the RPC call is 20 and not 20 + some large number divided/# txns?
| gasPrice = baseFee + priorityFee | ||
| } else { | ||
| // For chains that don't support EIP-1559 we iterate over the last 20 | ||
| // blocks to get an average gas price. We cap the tx lookup to 500. |
There was a problem hiding this comment.
| // blocks to get an average gas price. We cap the tx lookup to 500. | |
| // blocks to get an average gas price. We cap the tx lookup per block to 500. |
Not sure if you're actually intending to cap the transaction look up in the block or total transactions examined. If you're intending to do it as looking at a total number of transactions, you'll need to move the break in line 1030 after 1031. Otherwise, it will just break from the inner loop and not the outer one.
There was a problem hiding this comment.
The total tx lookup is 500 in total not per block. So I added the break in the inner and outer for loop so it doesn't exceed the transaction count.
There was a problem hiding this comment.
@acolytec3 capping it per block makes no sense (unless I'm in the wrong here), we can get the entire block body from the LevelDB immediately so just getting the avg gas price should be super fast once we got the block from the DB. (This would ofc. be different if we would have to get tx-by-hash each time)
|
Added the tests for 21 blocks ! |
| const txGasPrice = (tx as Transaction).gasPrice | ||
| gasPrice += txGasPrice | ||
| txCount++ | ||
| if (txCount >= 500) break |
There was a problem hiding this comment.
I really do not think we should do a per-block check, and I also do not think we need a tx cap limit. I might be wrong here, but the heaviest operation here is pulling the block body from the DB. Iterating over the txs should be much faster than that.
There was a problem hiding this comment.
Yes this makes sense. I removed the tx cap.
| */ | ||
| async gasPrice() { | ||
| let gasPrice = BigInt(0) | ||
| const latest = await this._chain.getCanonicalHeadHeader() |
There was a problem hiding this comment.
I generally wonder if we want to make this dependent from the synchronized status of the client this.client.config.synchronized? 🤔 it likely doesn't make sense to return anything (respectively: is not useful) if the client is still syncing and we are not calculating upon the latest blocks?
Not sure, maybe to test this in Geth what is responded in such a case?
Also, then: any reason to not also take the chain common object here for the 1559 check (below)?
There was a problem hiding this comment.
- Also, then: any reason to not also take the chain common object here for the 1559 check (below)?
->Made a change where the min gas price from the chain common object is now checked on both (1559 and non 1559) chains.
There was a problem hiding this comment.
Not sure, maybe to test this in Geth what is responded in such a case?
I checked this in geth and it does return a gas price even when syncing. Not sure where that gas price comes from but it's producing something. The spec doesn't give any specifics about how to calculate current gas price so I guess I would just we go with compute best gas price available based on the current view of the chain that we have (i.e. what is implemented here) and leave it to the consumer to validate the sync status of the client via the eth_syncing when evaluating gas price.
There was a problem hiding this comment.
Ok, this looks good now! 🙂 Thanks a lot @rodrigoherrerai for the additional iterations and the contribution in general! 💯 🙏
Will merge.
Co-authored-by: acolytec3 <[email protected]>
|
Rebased this via UI. |
Great ! Is there any other are that you guys need help ? Would love to contribute more if possible :) |
Definitely! If you're looking for another RPC endpoint to implement, the |
Perf! Can you provide the link to the discord ? |
Hi!
This implements 'eth_gasPrice'.
As discussed in issue #1114 , the implementation consists of the following:
If the chain is not 1559 compatible, it iterates over the last 20 blocks with a limit of 500 transactions. It then gets the average price of those transactions.
If the chain is 1559 compatible, it iterates over the last block to get the average priority fee and adds it to the next base fee.