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: add blobs to eth history #6469

Merged
merged 9 commits into from
Feb 8, 2024

Conversation

allnil
Copy link
Contributor

@allnil allnil commented Feb 7, 2024

draft for: #6330

@allnil allnil marked this pull request as ready for review February 7, 2024 18:49
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice,

we're missing one thing:

title: baseFeePerBlobGasArray
description: An array of block base fees per blob gas. This includes the next block after the newest of the returned range, because this value can be derived from the newest block. Zeroes are returned for pre-EIP-4844 blocks.

Same goes for base_fee_per_gas which we include for the next block:

https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/eth/api/fees.rs#L149-L154

https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/eth/api/fees.rs#L203-L208

Comment on lines +351 to +353
base_fee_per_blob_gas: block.blob_fee(),
blob_gas_used_ratio: block.blob_gas_used() as f64 /
reth_primitives::constants::eip4844::MAX_DATA_GAS_PER_BLOCK as f64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks correct to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattsse mattsse added A-rpc Related to the RPC implementation E-cancun Related to the Cancun network upgrade labels Feb 7, 2024
@mattsse
Copy link
Collaborator

mattsse commented Feb 7, 2024

fyi

pub fn next_block_blob_fee(&self) -> Option<u128> {

@allnil allnil requested a review from mattsse February 7, 2024 20:24
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

awesome,
I realize that there's one thing missing here, the base_fee_per_blob_gas for the next block if we're having cached entries.

The FeeEntry currently doesn't have the required data to derive that for the next block

what we can do instead is storing the excess blob gas and the blob gas used in the cache entry

https://github.com/paradigmxyz/reth/blob/main/crates/primitives/src/header.rs#L258-L263

and compute the relevant fields via functions on the cache entry
https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/eth/api/fee_history.rs#L351-L351

this way we can also derive the values for the next block
https://github.com/paradigmxyz/reth/blob/main/crates/primitives/src/header.rs#L261-L262

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

@mattsse mattsse added this pull request to the merge queue Feb 8, 2024
Merged via the queue into paradigmxyz:main with commit 787c9b1 Feb 8, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Related to the RPC implementation E-cancun Related to the Cancun network upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants