feat: integrate blobs into the payload builder #4305

Rjected · 2023-08-22T01:41:04Z

This prompts the payload builder to include EIP-4844 transactions if the fork is active, and fill in the required header fields if necessary.

ref #4205

codecov · 2023-08-22T01:49:21Z

Codecov Report

Attention: Patch coverage is 1.17647% with 84 lines in your changes missing coverage. Please review.

Project coverage is 68.32%. Comparing base (0c7a937) to head (9f4daa4).
Report is 4517 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/payload/basic/src/lib.rs	0.00%	39 Missing ⚠️
crates/payload/builder/src/payload.rs	0.00%	21 Missing ⚠️
crates/rpc/rpc-engine-api/src/engine_api.rs	7.69%	12 Missing ⚠️
crates/rpc/rpc-types/src/eth/engine/payload.rs	0.00%	12 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
crates/payload/builder/src/error.rs	`0.00% <ø> (ø)`
crates/transaction-pool/src/lib.rs	`40.21% <ø> (ø)`
crates/rpc/rpc-engine-api/src/engine_api.rs	`81.37% <7.69%> (-1.97%)`	⬇️
crates/rpc/rpc-types/src/eth/engine/payload.rs	`81.75% <0.00%> (-3.60%)`	⬇️
crates/payload/builder/src/payload.rs	`0.00% <0.00%> (ø)`
crates/payload/basic/src/lib.rs	`0.00% <0.00%> (ø)`

... and 11 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`16.71% <1.17%> (-0.03%)`	⬇️
unit-tests	`63.57% <0.00%> (-0.05%)`	⬇️

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.84% <ø> (-0.04%)`	⬇️
txpool	`47.97% <ø> (+0.49%)`	⬆️
networking	`77.49% <ø> (-0.05%)`	⬇️
rpc	`57.83% <4.00%> (-0.13%)`	⬇️
consensus	`63.54% <ø> (ø)`
revm	`31.74% <ø> (ø)`
payload builder	`6.39% <0.00%> (-0.38%)`	⬇️
primitives	`86.18% <ø> (-0.02%)`	⬇️

Rjected · 2023-08-22T20:58:55Z

crates/payload/basic/src/lib.rs

+            if chain_spec.is_cancun_activated_at_timestamp(attributes.timestamp) {
+                // mark tx and all dependent txs as invalid because we can't process blob txs
+                // until cancun is active
+                best_txs.mark_invalid(&pool_tx);
+                continue
+            }


@mattsse do fork checks in the txpool make this redundant?

yeah, they're only accepted when the timestamp has been reached, this also does not affect reorgs, so we can omit the check here.

cool, removed

mattsse · 2023-08-27T20:34:27Z

crates/payload/basic/src/lib.rs

+            if chain_spec.is_cancun_activated_at_timestamp(attributes.timestamp) {
+                // mark tx and all dependent txs as invalid because we can't process blob txs
+                // until cancun is active
+                best_txs.mark_invalid(&pool_tx);
+                continue
+            }


yeah, they're only accepted when the timestamp has been reached, this also does not affect reorgs, so we can omit the check here.

crates/payload/basic/src/lib.rs

mattsse · 2023-08-27T20:35:28Z

crates/payload/builder/src/payload.rs

+// TODO(rjected): we could improve this by wrapping envelope / payload types by version, so we can
+// have explicitly versioned return types for getPayload. Then BuiltPayload could essentially be a
+// builder for those types, and it would not be possible to e.g. return cancun fields for a
+// pre-cancun endpoint.


yes I like this

mattsse · 2023-08-27T20:36:52Z

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

+        // TODO: either upstream a conversion or upstream QUANTITY serde impls
+        let (commitments, proofs, blobs) = sidecars.into_iter().fold(
+            (Vec::new(), Vec::new(), Vec::new()),
+            |(mut commitments, mut proofs, mut blobs), sidecar| {
+                commitments
+                    .extend(sidecar.commitments.iter().map(|sidecar| Bytes::from(sidecar.deref())));
+                proofs.extend(sidecar.proofs.iter().map(|proof| Bytes::from(proof.deref())));
+                blobs.extend(sidecar.blobs.iter().map(|blob| Bytes::from(blob.deref())));
+                (commitments, proofs, blobs)
+            },


yeah this is a bit bad because this effectively clones everything, right?

ideally there's a into_bytes or to_vec impl

Ended up using the kzg types now that the serde traits are upstreamed

mattsse

last two nits

mattsse · 2023-08-29T17:48:47Z

crates/payload/basic/src/lib.rs

@@ -679,6 +682,20 @@ where
        // convert tx to a signed transaction
        let tx = pool_tx.to_recovered_transaction();

+        if let Transaction::Eip4844(blob_tx) = &tx.transaction {


nit, we now have as_eip4844

right, that makes this cleaner

mattsse · 2023-08-29T17:50:10Z

crates/payload/basic/src/lib.rs

+    // only determine cancun fields when active
+    if chain_spec.is_cancun_activated_at_timestamp(attributes.timestamp) {
+        // grab the blob sidecars from the executed txs
+        let blobs = pool.get_all_blobs(executed_txs.iter().map(|tx| tx.hash).collect())?;


this also includes non eip4844 transactions?

should filter before (is_eip4844), otherwise we wast blobstore lookups

makes sense, added

mattsse

lgtm, pending conflict

This reverts commit c690fce.

Rjected added A-block-building Related to block building M-eip This change relates to the implementation of an EIP E-cancun Related to the Cancun network upgrade labels Aug 22, 2023

Rjected force-pushed the dan/payload-builder-v3 branch 2 times, most recently from 6f8acb2 to 98754b7 Compare August 22, 2023 20:57

Rjected commented Aug 22, 2023

View reviewed changes

Rjected marked this pull request as ready for review August 22, 2023 21:01

Rjected requested review from mattsse and gakonst as code owners August 22, 2023 21:01

Rjected force-pushed the dan/payload-builder-v3 branch from d792126 to 1fd3ad2 Compare August 25, 2023 16:42

mattsse requested changes Aug 27, 2023

View reviewed changes

Rjected force-pushed the dan/payload-builder-v3 branch from 1fd3ad2 to c0f74c1 Compare August 29, 2023 16:59

Rjected requested a review from mattsse August 29, 2023 17:29

Rjected mentioned this pull request Aug 29, 2023

chore: introduce versioned ExecutionPayload #4400

Merged

mattsse requested changes Aug 29, 2023

View reviewed changes

Rjected force-pushed the dan/payload-builder-v3 branch from 9b3c9aa to f4704d0 Compare August 29, 2023 18:00

Rjected requested a review from mattsse August 29, 2023 18:07

Rjected added 2 commits August 29, 2023 11:21

add blobs bundle to execution payload envelope

064bacb

impl From<Vec<BlobTransactionSidecar>> for BlobsBundleV1

b0db6b5

mattsse approved these changes Aug 29, 2023

View reviewed changes

Rjected added 9 commits August 29, 2023 11:24

extract blobs during payload building

26c6709

add docs to blobs_bundle

7d11b6f

add into_v3_payload

bd2b21e

set should_override_builder to false by default

58b78b1

finish comment

a06e133

use BlobTransactionSidecar type for BlobsBundleV1

761aced

Revert "use BlobTransactionSidecar type for BlobsBundleV1"

a09506e

This reverts commit c690fce.

remove redundant 4844 check in builder

1925c77

use upstream kzg types for BlobsBundleV1

c12216b

Rjected added 3 commits August 29, 2023 11:24

remove comment

379a9c1

remove into_iter

f4d95a4

use as_eip4844 and filter before fetching blobs

9f4daa4

Rjected force-pushed the dan/payload-builder-v3 branch from f4704d0 to 9f4daa4 Compare August 29, 2023 18:24

Rjected enabled auto-merge August 29, 2023 18:32

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

Merged via the queue into main with commit 82fb0ee Aug 29, 2023
25 checks passed

Rjected deleted the dan/payload-builder-v3 branch August 29, 2023 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate blobs into the payload builder #4305

feat: integrate blobs into the payload builder #4305

Rjected commented Aug 22, 2023

codecov bot commented Aug 22, 2023 •

edited

Loading

Rjected Aug 22, 2023

mattsse Aug 27, 2023

Rjected Aug 29, 2023

mattsse Aug 27, 2023

mattsse Aug 27, 2023

mattsse Aug 27, 2023

Rjected Aug 29, 2023

mattsse left a comment

mattsse Aug 29, 2023

Rjected Aug 29, 2023

mattsse Aug 29, 2023

Rjected Aug 29, 2023

mattsse left a comment •

edited

Loading

feat: integrate blobs into the payload builder #4305

feat: integrate blobs into the payload builder #4305

Conversation

Rjected commented Aug 22, 2023

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2023 •

edited

Loading

mattsse left a comment •

edited

Loading