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: implement EIP-7685 #8053

Merged
merged 19 commits into from May 13, 2024
Merged

feat: implement EIP-7685 #8053

merged 19 commits into from May 13, 2024

Conversation

onbjerg
Copy link
Member

@onbjerg onbjerg commented May 2, 2024

Adds types for EIP-7685, adds a new Requests table, and adjusts the code where appropriate.

This still lacks:

  • Blockchain tree integration
  • Writing the Requests in the bodies stage
  • Validating the requests root in the body downloader
  • Full engine API integration

Will do these in separate PRs to unblock EIPs 6110 and 7002

@onbjerg onbjerg added C-enhancement New feature or request E-prague Related to the prague network upgrade labels May 2, 2024
@@ -325,6 +326,8 @@ impl StorageInner {
Some(calculate_excess_blob_gas(parent_excess_blob_gas, parent_blob_gas_used))
}

// todo: do we support requests root in auto-seal
Copy link
Member

Choose a reason for hiding this comment

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

Just leave it empty

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.

smol nit

crates/primitives/src/proofs.rs Outdated Show resolved Hide resolved
Comment on lines +9 to +10
#[derive(Debug, Clone, PartialEq, Eq, Default, Hash, RlpEncodableWrapper, RlpDecodableWrapper)]
pub struct Requests(pub Vec<Request>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we're missing a few helper impls but this is ok

Copy link
Member Author

Choose a reason for hiding this comment

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

will add before merge to main

@gakonst gakonst mentioned this pull request May 13, 2024
10 tasks
@gakonst gakonst mentioned this pull request May 13, 2024
@@ -727,6 +736,7 @@ impl<TX: DbTxMut + DbTx> DatabaseProvider<TX> {
let block_ommers = self.get_or_take::<tables::BlockOmmers, TAKE>(range.clone())?;
let block_withdrawals =
self.get_or_take::<tables::BlockWithdrawals, TAKE>(range.clone())?;
let block_requests = self.get_or_take::<tables::BlockRequests, TAKE>(range.clone())?;
Copy link
Member

Choose a reason for hiding this comment

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

still dont love this get/take function we should investigate fixing later

Copy link
Member Author

Choose a reason for hiding this comment

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

agree

Comment on lines +248 to +256
// note(onbjerg): the rpc spec has not been changed to include requests, so for now we just
// set these to empty
let (requests, requests_root) =
if chain_spec.is_prague_active_at_timestamp(block_env.timestamp.to::<u64>()) {
(Some(Requests::default()), Some(EMPTY_ROOT_HASH))
} else {
(None, None)
};

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@onbjerg onbjerg May 13, 2024

Choose a reason for hiding this comment

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

the pr is for the engine api, this code snippet is for regular rpc which does not have any changes currently

@@ -84,7 +84,7 @@ impl BlockBatchRecord {
}

/// Returns all recorded requests.
pub fn take_requests(&mut self) -> Requests {
pub fn take_requests(&mut self) -> Vec<Requests> {
Copy link
Member

Choose a reason for hiding this comment

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

I think we start having a lot of these Vec<Vec<_>> outer / inner vecs so maybe we should do a generic Batch<T> to encapsulate that

Comment on lines +16 to +18
// HACK(onbjerg): we need this to always set `requests` to `None` since we might otherwise generate
// a block with `None` withdrawals and `Some` requests, in which case we end up trying to decode the
// requests as withdrawals
Copy link
Member

Choose a reason for hiding this comment

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

this seems fine, not a hack?

@onbjerg onbjerg merged commit b60225f into devnet-0 May 13, 2024
22 of 23 checks passed
@onbjerg onbjerg deleted the onbjerg/eip-7685 branch May 13, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request E-prague Related to the prague network upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants