feat: add EIP-4788 parent_beacon_block_root to Header #4299

Rjected · 2023-08-21T18:49:32Z

Adds an optional parent beacon block root to the header, as well as the engine newPayload methods arguments. Consensus checks are also added, making sure that the field only exists past Cancun.

Because adding new header fields has the possibility of breaking the DB, here is the previous output of cargo expand for HeaderFlags on main:

use bytes::Buf;
use modular_bitfield::prelude::*;
///Fieldset that facilitates compacting the parent type. Used bytes: 4 | Unused bits: 2
#[allow(clippy::identity_op)]
pub struct HeaderFlags {
    bytes: [::core::primitive::u8; {
        ((({
            0usize + <B1 as ::modular_bitfield::Specifier>::BITS
                + <B6 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B1 as ::modular_bitfield::Specifier>::BITS
                + <B1 as ::modular_bitfield::Specifier>::BITS
                + <B1 as ::modular_bitfield::Specifier>::BITS
                + <B2 as ::modular_bitfield::Specifier>::BITS
        } - 1) / 8) + 1) * 8
    } / 8usize],
}

and here is the new output of cargo expand for HeaderFlags:

use bytes::Buf;
use modular_bitfield::prelude::*;
///Fieldset that facilitates compacting the parent type. Used bytes: 4 | Unused bits: 1
#[allow(clippy::identity_op)]
pub struct HeaderFlags {
    bytes: [::core::primitive::u8; {
        ((({
            0usize + <B1 as ::modular_bitfield::Specifier>::BITS
                + <B6 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B4 as ::modular_bitfield::Specifier>::BITS
                + <B1 as ::modular_bitfield::Specifier>::BITS
                + <B1 as ::modular_bitfield::Specifier>::BITS
                + <B1 as ::modular_bitfield::Specifier>::BITS
                + <B1 as ::modular_bitfield::Specifier>::BITS
                + <B1 as ::modular_bitfield::Specifier>::BITS
        } - 1) / 8) + 1) * 8
    } / 8usize],
}

We used up one of our unused bits adding the optional field, so this shouldn't break the DB.

codecov · 2023-08-21T19:21:57Z

Codecov Report

Merging #4299 (ab0ee67) into main (d8b1609) will increase coverage by 0.01%.
The diff coverage is 90.08%.

Files Changed	Coverage Δ
crates/consensus/auto-seal/src/lib.rs	`0.00% <0.00%> (ø)`
crates/consensus/beacon/src/engine/message.rs	`38.46% <ø> (ø)`
crates/interfaces/src/consensus.rs	`93.33% <ø> (ø)`
crates/payload/basic/src/lib.rs	`0.00% <0.00%> (ø)`
crates/rpc/rpc/src/eth/api/pending_block.rs	`0.00% <0.00%> (ø)`
crates/consensus/common/src/validation.rs	`70.75% <28.57%> (-0.47%)`	⬇️
crates/rpc/rpc-types/src/eth/block.rs	`57.44% <50.00%> (-0.17%)`	⬇️
crates/rpc/rpc-engine-api/src/engine_api.rs	`83.33% <66.66%> (ø)`
crates/consensus/beacon/src/engine/handle.rs	`89.79% <100.00%> (+1.15%)`	⬆️
crates/consensus/beacon/src/engine/mod.rs	`75.25% <100.00%> (+0.39%)`	⬆️
... and 4 more

... and 8 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`16.70% <51.23%> (+0.01%)`	⬆️
unit-tests	`63.66% <86.77%> (+0.01%)`	⬆️

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

Components	Coverage Δ
reth binary	`26.01% <ø> (ø)`
blockchain tree	`82.56% <ø> (ø)`
pipeline	`90.07% <ø> (ø)`
storage (db)	`74.72% <ø> (ø)`
trie	`94.88% <ø> (ø)`
txpool	`47.23% <ø> (ø)`
networking	`77.48% <100.00%> (-0.01%)`	⬇️
rpc	`57.95% <89.18%> (-0.02%)`	⬇️
consensus	`63.54% <81.81%> (+0.15%)`	⬆️
revm	`31.89% <ø> (ø)`
payload builder	`6.77% <0.00%> (-0.02%)`	⬇️
primitives	`86.34% <100.00%> (+0.03%)`	⬆️

mattsse

lgtm,
pending @joshieDo

mattsse · 2023-08-21T20:30:51Z

crates/consensus/beacon/src/engine/message.rs

        payload: ExecutionPayload,
+        /// The parent beacon block root, if any.
+        parent_beacon_block_root: Option<H256>,


I thought that we could introduce a new wrapper type here, but I actually prefer having a Some|None argument in all the on_new_payload functions so it is evident when we're using it

joshieDo · 2023-08-23T09:41:00Z

lgtm! Only one bit left..

gakonst

I wonder if this is something where we'd want feature flags, I don't love it but the potentially unintended DB breakage is a bit scary

gakonst · 2023-08-23T17:50:59Z

crates/rpc/rpc-types/src/eth/engine/payload.rs

+    pub fn try_into_sealed_block(
+        self,
+        parent_beacon_block_root: Option<H256>,
+    ) -> Result<SealedBlock, PayloadError> {


meh - can we/should we capture this in executionpayload? i g not?

The API defines it as a separate argument, so if we were to put it into the execution payload we'd have the field duplicated across the execution payload and RPC args, which isn't ideal:

method: engine_newPayloadV3

params:

executionPayload: ExecutionPayloadV3.

expectedBlobVersionedHashes: Array of DATA, 32 Bytes - Array of expected blob versioned hashes to validate.

parentBeaconBlockRoot: DATA, 32 Bytes - Root of the parent beacon block.

@mattsse was referencing a wrapper type for this

I thought that we could introduce a new wrapper type here, but I actually prefer having a Some|None argument in all the on_new_payload functions so it is evident when we're using it

which would wrap the execution payload and be used as the type in the newpayload message, and arguments for new_payload, ensure_well_formed_payload, etc

gakonst · 2023-08-23T17:52:09Z

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

@@ -131,6 +131,9 @@ pub struct Header {
    /// Excess blob gas
    #[serde(rename = "excessBlobGas", skip_serializing_if = "Option::is_none")]
    pub excess_blob_gas: Option<U64>,
+    /// Parent beacon block root
+    #[serde(rename = "parentBeaconBlockRoot", skip_serializing_if = "Option::is_none")]


are tehse renames needed? doesn't the serde name camel case cut it?

they aren't needed, missed the camel case rename. the only one that needs the rename is the uncles hash

Rjected · 2023-08-23T18:20:47Z

I wonder if this is something where we'd want feature flags, I don't love it but the potentially unintended DB breakage is a bit scary

If future forks add 2 or more fields, then we might want to have a feature flag while we're not activated, but once the fork is active it looks like DB breakage is unavoidable

mattsse

good to go @Rjected ?

Rjected · 2023-08-29T17:55:09Z

good to go @Rjected ?

yep

Rjected added C-enhancement New feature or request M-eip This change relates to the implementation of an EIP E-cancun Related to the Cancun network upgrade labels Aug 21, 2023

Rjected requested review from rkrasiuk, mattsse and gakonst as code owners August 21, 2023 18:49

Rjected force-pushed the dan/eip-4788-header-fields branch from e310978 to c64c49a Compare August 21, 2023 18:55

mattsse requested a review from joshieDo August 21, 2023 20:23

mattsse approved these changes Aug 21, 2023

View reviewed changes

Rjected mentioned this pull request Aug 21, 2023

Validate versioned hashes on engine_newPayloadV3 #4300

Closed

joshieDo approved these changes Aug 23, 2023

View reviewed changes

gakonst reviewed Aug 23, 2023

View reviewed changes

Rjected added 2 commits August 25, 2023 12:40

feat: add EIP-4788 parent_beacon_block_root to Header

6d00449

add parent beacon block root arg to new_payload_v3 call

ab0ee67

Rjected force-pushed the dan/eip-4788-header-fields branch from 0d2fec4 to ab0ee67 Compare August 25, 2023 16:40

mattsse approved these changes Aug 29, 2023

View reviewed changes

Rjected added this pull request to the merge queue Aug 29, 2023

Merged via the queue into main with commit 0c7a937 Aug 29, 2023
24 checks passed

Rjected deleted the dan/eip-4788-header-fields branch August 29, 2023 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add EIP-4788 parent_beacon_block_root to Header #4299

feat: add EIP-4788 parent_beacon_block_root to Header #4299

Rjected commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 21, 2023 •

edited

Loading

mattsse left a comment

mattsse Aug 21, 2023

joshieDo commented Aug 23, 2023

gakonst left a comment

gakonst Aug 23, 2023

Rjected Aug 23, 2023

gakonst Aug 23, 2023

Rjected Aug 23, 2023

Rjected commented Aug 23, 2023

mattsse left a comment

Rjected commented Aug 29, 2023

feat: add EIP-4788 parent_beacon_block_root to Header #4299

feat: add EIP-4788 parent_beacon_block_root to Header #4299

Conversation

Rjected commented Aug 21, 2023 • edited Loading

codecov bot commented Aug 21, 2023 • edited Loading

Codecov Report

mattsse left a comment

Choose a reason for hiding this comment

mattsse Aug 21, 2023

Choose a reason for hiding this comment

joshieDo commented Aug 23, 2023

gakonst left a comment

Choose a reason for hiding this comment

gakonst Aug 23, 2023

Choose a reason for hiding this comment

Rjected Aug 23, 2023

Choose a reason for hiding this comment

gakonst Aug 23, 2023

Choose a reason for hiding this comment

Rjected Aug 23, 2023

Choose a reason for hiding this comment

Rjected commented Aug 23, 2023

mattsse left a comment

Choose a reason for hiding this comment

Rjected commented Aug 29, 2023

Rjected commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 21, 2023 •

edited

Loading