feat: add 4844 header fields and consensus checks by Rjected · Pull Request #3972 · paradigmxyz/reth

Rjected · 2023-07-27T17:17:50Z

This adds EIP-4844 header fields:

blob_gas_used
excess_blob_gas

And their according consensus header validation checks. The method calculate_excess_blob_gas is added which is specified in the EIP-4844 header extension section.

RLP encoding and decoding are updated in a similar way to the withdrawals root.

Fixes #3941

Rjected · 2023-07-27T17:32:23Z

failing proptest cases, needs an encoding / decoding fix - working on it

codecov · 2023-07-27T17:34:11Z

Codecov Report

Merging #3972 (1385c79) into main (717bad8) will decrease coverage by 0.30%.
Report is 19 commits behind head on main.
The diff coverage is 70.86%.

Files Changed	Coverage Δ
crates/consensus/auto-seal/src/lib.rs	`0.00% <0.00%> (ø)`
crates/interfaces/src/consensus.rs	`100.00% <ø> (ø)`
crates/payload/basic/src/lib.rs	`0.00% <0.00%> (ø)`
crates/primitives/src/blobfee.rs	`0.00% <0.00%> (ø)`
crates/primitives/src/lib.rs	`100.00% <ø> (ø)`
crates/rpc/rpc-types/src/eth/block.rs	`38.84% <0.00%> (-0.46%)`	⬇️
crates/rpc/rpc/src/eth/api/pending_block.rs	`0.00% <0.00%> (ø)`
crates/consensus/common/src/validation.rs	`72.52% <13.84%> (-6.94%)`	⬇️
crates/primitives/src/header.rs	`95.55% <98.15%> (+0.95%)`	⬆️
crates/net/eth-wire/src/types/blocks.rs	`97.25% <100.00%> (+0.05%)`	⬆️
... and 1 more

... and 44 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`16.32% <12.59%> (+0.77%)`	⬆️
unit-tests	`64.26% <70.07%> (-0.28%)`	⬇️

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

Components	Coverage Δ
reth binary	`26.18% <ø> (-1.01%)`	⬇️
blockchain tree	`83.04% <ø> (ø)`
pipeline	`89.82% <ø> (ø)`
storage (db)	`74.30% <ø> (ø)`
trie	`94.70% <ø> (ø)`
txpool	`46.00% <ø> (+<0.01%)`	⬆️
networking	`77.66% <100.00%> (-0.04%)`	⬇️
rpc	`58.47% <37.50%> (-0.29%)`	⬇️
consensus	`63.51% <13.43%> (-0.96%)`	⬇️
revm	`33.08% <ø> (-0.61%)`	⬇️
payload builder	`6.58% <0.00%> (-0.04%)`	⬇️
primitives	`87.93% <95.80%> (+0.10%)`	⬆️

Rjected · 2023-07-27T18:50:17Z

crates/primitives/src/header.rs

        // but withdrawals root is present.
        if let Some(ref base_fee) = self.base_fee_per_gas {
            U256::from(*base_fee).encode(out);
-        } else if self.withdrawals_root.is_some() {
+        } else if self.withdrawals_root.is_some() ||
+            self.blob_gas_used.is_some() ||
+            self.excess_blob_gas.is_some()
+        {
            out.put_u8(EMPTY_STRING_CODE);
        }

+        // Encode withdrawals root. Put empty string if withdrawals root is missing,
+        // but blob gas used is present.
        if let Some(ref root) = self.withdrawals_root {
            root.encode(out);
+        } else if self.blob_gas_used.is_some() || self.excess_blob_gas.is_some() {
+            out.put_u8(EMPTY_STRING_CODE);
+        }
+
+        // Encode blob gas used. Put empty string if blob gas used is missing,
+        // but excess blob gas is present.
+        if let Some(ref blob_gas_used) = self.blob_gas_used {
+            U256::from(*blob_gas_used).encode(out);
+        } else if self.excess_blob_gas.is_some() {
+            out.put_u8(EMPTY_STRING_CODE);
+        }
+
+        // Encode excess blob gas. If new fields are added, the above pattern will need to be
+        // repeated and placeholders added. Otherwise, it's impossible to tell _which_ fields
+        // are missing. This is mainly relevant for contrived cases where a header is created
+        // at random, for example:
+        //  * A header is created with a withdrawals root, but no base fee. Shanghai blocks are


we should think of how to make this less horrible, since it's possible there will be even more header extensions in the future

maybe different header types? like we do with transactions?

ah, this wouldn't work because we don't have a tx type byte like we do for transactions - we'd need to know in advance (before decoding) which type of header it is

mattsse

this looks pretty good already, less horrible than expected actually.

we should add a few checks:

consensus sanity checks
rlp checks with some reference rlp data

mattsse · 2023-07-28T11:56:40Z

crates/consensus/common/src/validation.rs

+    if chain_spec.fork(Hardfork::Cancun).active_at_timestamp(header.timestamp) {
+        let blob_gas_used = header.blob_gas_used.ok_or(ConsensusError::BlobGasUsedMissing)?;


I'd like to extract these into a standalone function

we also need to check the full block downloaders whether we need to update the checks there

just realized our full block client enforces that the header has a withdrawals root, which is not true for pre-shanghai. but we can only perform these checks properly if we have the Arc<ChainSpec>, so it will need to be added

so yes they need to be updated

^ nevermind, I misread the code and this is completely wrong.

We currently only validate the body w.r.t the header, and do no other checks:

reth/crates/interfaces/src/p2p/full_block.rs

Lines 316 to 331 in be8797d

let withdrawals = block.withdrawals.as_deref().unwrap_or(&[]);

if let Some(header_withdrawals_root) = header.withdrawals_root {

let withdrawals_root = reth_primitives::proofs::calculate_withdrawals_root(withdrawals);

if withdrawals_root != header_withdrawals_root {

return Err(ConsensusError::BodyWithdrawalsRootDiff {

got: withdrawals_root,

expected: header_withdrawals_root,

})

}

return Ok(())

}

if !withdrawals.is_empty() {

return Err(ConsensusError::WithdrawalsRootUnexpected)

}

so, if there is no withdrawals root, but withdrawals exist in the body, an error is returned. And vice versa for non-empty withdrawals roots.

I'm not sure what we would check here, we perform header validation in the tree, so maybe we can keep the checks as-is.

mattsse · 2023-07-28T11:58:27Z

crates/primitives/src/blobfee.rs

+    if excess_blob_gas < TARGET_DATA_GAS_PER_BLOCK {
+        return 0
+    }
+
+    excess_blob_gas - TARGET_DATA_GAS_PER_BLOCK


this is just saturating_sub, right?

right, not sure how i missed this

mattsse · 2023-07-28T12:00:41Z

crates/primitives/src/header.rs

+        // Encode excess blob gas. If new fields are added, the above pattern will need to be
+        // repeated and placeholders added. Otherwise, it's impossible to tell _which_ fields
+        // are missing. This is mainly relevant for contrived cases where a header is created
+        // at random, for example:
+        //  * A header is created with a withdrawals root, but no base fee. Shanghai blocks are


yeah this is a bit horrible, but still managable

mattsse · 2023-07-28T12:01:39Z

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

+                    // TODO: add header fields to the rpc header
+                    blob_gas_used: _,
+                    excess_blob_gas: _,


will add an issue for this

Rjected · 2023-07-28T17:16:47Z

will find some reference RLP data, may have to generate it using geth code

Rjected · 2023-07-28T20:16:36Z

Added RLP tests from devnet-7 and EF tests

mattsse

lgtm

Rjected added 2 commits July 27, 2023 13:11

feat: add 4844 header fields and consensus checks

cd6ba5a

add 4844 fields to size heuristic

55196fd

Rjected added C-enhancement New feature or request M-changelog This change should be included in the changelog A-consensus Related to the consensus engine E-cancun Related to the Cancun network upgrade labels Jul 27, 2023

Rjected requested a review from gakonst as a code owner July 27, 2023 17:17

Rjected requested a review from mattsse July 27, 2023 17:18

Rjected added 2 commits July 27, 2023 13:26

fix missing blob fields

f78f5c9

fix proptest case with mixed blob field Some / None

89d1851

add cursed optional field encoding and decoding

1cddf52

Rjected commented Jul 27, 2023

View reviewed changes

mattsse requested changes Jul 28, 2023

View reviewed changes

mattsse mentioned this pull request Jul 28, 2023

Add cancun header fields to rpc types #3981

Closed

extract 4844 checks into separate method

be8797d

add rlp tests for header

1385c79

Rjected requested a review from mattsse July 28, 2023 20:16

mattsse approved these changes Jul 29, 2023

View reviewed changes

mattsse added this pull request to the merge queue Jul 29, 2023

Merged via the queue into main with commit 334d606 Jul 29, 2023

mattsse deleted the dan/blob-header-field-validation branch July 29, 2023 13:00

		if chain_spec.fork(Hardfork::Cancun).active_at_timestamp(header.timestamp) {
		let blob_gas_used = header.blob_gas_used.ok_or(ConsensusError::BlobGasUsedMissing)?;


	let withdrawals = block.withdrawals.as_deref().unwrap_or(&[]);
	if let Some(header_withdrawals_root) = header.withdrawals_root {
	let withdrawals_root = reth_primitives::proofs::calculate_withdrawals_root(withdrawals);
	if withdrawals_root != header_withdrawals_root {
	return Err(ConsensusError::BodyWithdrawalsRootDiff {
	got: withdrawals_root,
	expected: header_withdrawals_root,
	})
	}
	return Ok(())
	}

	if !withdrawals.is_empty() {
	return Err(ConsensusError::WithdrawalsRootUnexpected)
	}

Conversation

Rjected commented Jul 27, 2023

Uh oh!

Rjected commented Jul 27, 2023

Uh oh!

codecov bot commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rjected Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rjected Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rjected commented Jul 28, 2023

Uh oh!

Rjected commented Jul 28, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jul 27, 2023 •

edited

Loading

Rjected Jul 27, 2023 •

edited

Loading

Rjected Jul 28, 2023 •

edited

Loading