feat: Duplicate Withdrawal and move try from impls to rpc-compat #4186

supernovahs · 2023-08-14T11:25:20Z

codecov · 2023-08-14T11:59:05Z

Codecov Report

Merging #4186 (d9a5306) into main (55339d7) will increase coverage by 0.19%.
Report is 5 commits behind head on main.
The diff coverage is 47.11%.

Files Changed	Coverage Δ
crates/payload/builder/src/payload.rs	`0.00% <0.00%> (ø)`
crates/rpc/rpc-types/src/eth/engine/payload.rs	`74.56% <ø> (+9.51%)`	⬆️
crates/rpc/rpc-types-compat/src/engine/payload.rs	`44.23% <44.23%> (ø)`
crates/rpc/rpc-types/src/eth/withdrawal.rs	`75.00% <75.00%> (ø)`
crates/consensus/beacon/src/engine/mod.rs	`74.58% <100.00%> (+0.15%)`	⬆️
crates/rpc/rpc-engine-api/src/engine_api.rs	`79.44% <100.00%> (+0.10%)`	⬆️
crates/rpc/rpc-engine-api/src/payload.rs	`75.00% <100.00%> (ø)`

... and 25 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`16.80% <37.50%> (-0.15%)`	⬇️
unit-tests	`63.40% <40.70%> (+0.22%)`	⬆️

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

Components	Coverage Δ
reth binary	`32.17% <ø> (+0.20%)`	⬆️
blockchain tree	`83.59% <ø> (ø)`
pipeline	`88.54% <ø> (ø)`
storage (db)	`73.47% <ø> (ø)`
trie	`94.73% <ø> (-0.04%)`	⬇️
txpool	`49.91% <ø> (+0.46%)`	⬆️
networking	`77.43% <ø> (+0.20%)`	⬆️
rpc	`57.44% <47.70%> (+0.09%)`	⬆️
consensus	`62.66% <100.00%> (+0.07%)`	⬆️
revm	`28.04% <ø> (+8.38%)`	⬆️
payload builder	`8.45% <0.00%> (-0.61%)`	⬇️
primitives	`86.54% <ø> (+0.10%)`	⬆️

supernovahs · 2023-08-14T15:33:12Z

Since SealedBlock is also using Withdrawal under the hood, I think we would also have to move it, for now I have not moved it.

mattsse

great start,

this requires some cleaning up

crates/rpc/rpc-engine-api/tests/it/payload.rs

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

crates/rpc/rpc-types/Cargo.toml

mattsse · 2023-08-15T11:36:04Z

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

+impl From<&Withdrawal> for StandaloneWithdraw {
+    fn from(withdrawal: &Withdrawal) -> Self {
+        StandaloneWithdraw {
+            index: withdrawal.index,
+            validator_index: withdrawal.validator_index,
+            address: withdrawal.address,
+            amount: withdrawal.amount,
+        }
+    }
+}
+
+impl From<StandaloneWithdraw> for Withdrawal {
+    fn from(standalone: StandaloneWithdraw) -> Self {
+        Withdrawal {
+            index: standalone.index,
+            validator_index: standalone.validator_index,
+            address: standalone.address,
+            amount: standalone.amount,
+        }
+    }
+}


if we include the conversion here then we cannot get rid of it which defeats the point of moving the conversions

My bad . Will fix

If I implement the conversion in block.rs in primitives, then it leads to cyclic dependency error as I would have to import StandaloneWithdraw from rpc-types.
The. only way I can think is to duplicate block.rs as well in rpc-types?

cc @mattsse

right, we should convert this tryfrom into a function instead, like the other things we moved to compat

mattsse · 2023-08-15T20:35:37Z

crates/payload/builder/src/payload.rs

@@ -98,13 +98,19 @@ impl PayloadBuilderAttributes {
    /// Derives the unique [PayloadId] for the given parent and attributes
    pub fn new(parent: H256, attributes: PayloadAttributes) -> Self {
        let id = payload_id(&parent, &attributes);
+        let withdraw = attributes.withdrawals.as_ref().map(|withdrawals| {


Suggested change

let withdraw = attributes.withdrawals.as_ref().map(|withdrawals| {

let withdrawals = attributes.withdrawals.as_ref().map(|withdrawals| {

mattsse · 2023-08-17T15:00:11Z

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

+impl From<&Withdrawal> for StandaloneWithdraw {
+    fn from(withdrawal: &Withdrawal) -> Self {
+        StandaloneWithdraw {
+            index: withdrawal.index,
+            validator_index: withdrawal.validator_index,
+            address: withdrawal.address,
+            amount: withdrawal.amount,
+        }
+    }
+}
+
+impl From<StandaloneWithdraw> for Withdrawal {
+    fn from(standalone: StandaloneWithdraw) -> Self {
+        Withdrawal {
+            index: standalone.index,
+            validator_index: standalone.validator_index,
+            address: standalone.address,
+            amount: standalone.amount,
+        }
+    }
+}


right, we should convert this tryfrom into a function instead, like the other things we moved to compat

supernovahs · 2023-08-18T12:33:10Z

4 tests are failing in https://github.com/paradigmxyz/reth/blob/main/crates/consensus/beacon/src/engine/mod.rs

Investigating why it happened
`failures:

engine::tests::new_payload::new_payload_before_forkchoice

engine::tests::new_payload::payload_known

engine::tests::new_payload::payload_parent_unknown

engine::tests::new_payload::payload_pre_merge

`

failures:

---- engine::tests::new_payload::new_payload_before_forkchoice stdout ----
thread 'engine::tests::new_payload::new_payload_before_forkchoice' panicked at 'assertion failed: `(left == right)`
  left: `PayloadStatus { status: Invalid { validation_error: "blockhash mismatch, want 0xb4c1…c075, got 0x5053…ad13" }, latest_valid_hash: None }`,
 right: `PayloadStatus { status: Syncing, latest_valid_hash: None }`', crates/consensus/beacon/src/engine/mod.rs:2261:48
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- engine::tests::new_payload::payload_parent_unknown stdout ----
thread 'engine::tests::new_payload::payload_parent_unknown' panicked at 'assertion failed: `(left == right)`
  left: `PayloadStatus { status: Invalid { validation_error: "blockhash mismatch, want 0x6d5a…81a7, got 0x03e0…9bf6" }, latest_valid_hash: None }`,
 right: `PayloadStatus { status: Syncing, latest_valid_hash: None }`', crates/consensus/beacon/src/engine/mod.rs:2409:48

---- engine::tests::new_payload::payload_known stdout ----
thread 'engine::tests::new_payload::payload_known' panicked at 'assertion failed: `(left == right)`
  left: `PayloadStatus { status: Invalid { validation_error: "blockhash mismatch, want 0x9429…222a, got 0x853a…eb91" }, latest_valid_hash: None }`,
 right: `PayloadStatus { status: Valid, latest_valid_hash: Some(0x94292372ffb7a28138c3f2dcdacdc3f09f64203f641868af37c6e75fcdfe222a) }`', crates/consensus/beacon/src/engine/mod.rs:2312:13

---- engine::tests::new_payload::payload_pre_merge stdout ----
thread 'engine::tests::new_payload::payload_pre_merge' panicked at 'assertion failed: `(left == right)`
  left: `PayloadStatus { status: Invalid { validation_error: "blockhash mismatch, want 0x19ac…db7a, got 0x9895…3ff9" }, latest_valid_hash: None }`,
 right: `PayloadStatus { status: Invalid { validation_error: "Block 0x19acdf9d012fd0453a12b0c302f107c2f9836b2fcfc28f753b1399d68e97db7a is pre merge" }, latest_valid_hash: Some(0x0000000000000000000000000000000000000000000000000000000000000000) }`', crates/consensus/beacon/src/engine/mod.rs:2477:13

mattsse · 2023-08-29T16:53:23Z

any updates here @supernovahs?

this will become a blocker for us shortly,
if you don't have the bandwidth, lmk, happy to help getting this over the line

supernovahs · 2023-08-29T17:02:13Z

Yeah actually I could not find why are the 4 tests failing . Any pointers would help

supernovahs · 2023-09-05T14:36:37Z

@mattsse Can u take a look ? I'm not able to figure out why they are failing?Thx

supernovahs · 2023-09-16T18:47:31Z

@mattsse Can u review ? thx
Also clippy wants changes in pruner.

supernovahs · 2023-09-18T10:25:22Z

Ready i think

mattsse · 2023-09-18T21:57:00Z

cool, checking shortly

supernovahs · 2023-09-18T22:00:19Z

Yeah , got errors due to a recent update . Will update it tomorrow

supernovahs · 2023-09-19T09:04:38Z

Fixed

mattsse

great, a few nits.

wdyt @Rjected

crates/rpc/rpc-engine-api/src/payload.rs

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

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

Rjected

This looks very close! Just one question and a few nits, otherwise lgtm

crates/rpc/rpc-types-compat/Cargo.toml

Rjected · 2023-09-19T15:23:38Z

crates/rpc/rpc-types-compat/src/engine/mod.rs

@@ -0,0 +1,6 @@
+//! Standalone functions for engine specific


nit:

Suggested change

//! Standalone functions for engine specific

//! Standalone functions for engine specific rpc type conversions

Rjected · 2023-09-19T15:24:46Z

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

+pub mod payload;
 mod transition;
-
 pub use self::{cancun::*, forkchoice::*, payload::*, transition::*};
+pub use payload::{
+    ExecutionPayload, ExecutionPayloadBodyV1, ExecutionPayloadFieldV2, ExecutionPayloadV1,
+    ExecutionPayloadV2, ExecutionPayloadV3, PayloadError, Withdrawal,
+};


why wasn't the previous pub use self::payload::* sufficient here?

Rjected

LGTM

mattsse

lgtm

supernovahs added 9 commits August 14, 2023 16:49

duplicate withdrawal and move try_from impls to compat

755ae30

delete unwanted comments

c40b84a

fmt

b82163c

no need for pub withdrawal

270960e

add proptest as dep

01c354a

remove unused dep

179a3fd

add back deps

4ffdeba

add decodable

734a9c4

remove unused dep

e7e94cc

clippy

0c2227a

supernovahs marked this pull request as ready for review August 14, 2023 12:17

supernovahs requested review from mattsse, Rjected and gakonst as code owners August 14, 2023 12:17

lint

ce1d3a2

mattsse requested changes Aug 15, 2023

View reviewed changes

mattsse added the C-debt Refactor of code section that is hard to understand or maintain label Aug 15, 2023

mattsse mentioned this pull request Aug 15, 2023

Extend ExecutionPayload type with cancun fields #4206

Closed

recommended changes excluding conversion

d736a41

mattsse requested changes Aug 17, 2023

View reviewed changes

wip , moved impls to compat , 4 tests failing

3738f86

supernovahs requested a review from rkrasiuk as a code owner August 18, 2023 12:30

mattsse mentioned this pull request Aug 30, 2023

chore: introduce versioned ExecutionPayload #4400

Merged

mattsse force-pushed the main branch from 60edc6c to b32562f Compare September 4, 2023 13:34

supernovahs added 7 commits September 16, 2023 19:03

doc for module

a8280b5

payload docs

c5f9406

remove unused dep

e539396

clean code and fix clippy

0efb110

fix errors

9440d5e

clippy

1011361

clippy ..

76499e9

supernovahs requested review from shekhirin and joshieDo as code owners September 16, 2023 18:44

Merge branch 'main' into standalone_rpc_types

ed47088

supernovahs and others added 2 commits September 18, 2023 15:32

Merge branch 'paradigmxyz:main' into standalone_rpc_types

6c0d707

doc

39aaeeb

Merge branch 'main' into standalone_rpc_types

e49d5a5

fix new conflicts

5fc02f7

mattsse requested changes Sep 19, 2023

View reviewed changes

nits

534a4dd

Rjected requested changes Sep 19, 2023

View reviewed changes

nits more

d9a5306

Rjected approved these changes Sep 19, 2023

View reviewed changes

mattsse approved these changes Sep 19, 2023

View reviewed changes

mattsse added this pull request to the merge queue Sep 19, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 19, 2023

Rjected added this pull request to the merge queue Sep 19, 2023

Merged via the queue into paradigmxyz:main with commit 8012942 Sep 19, 2023
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Duplicate Withdrawal and move try from impls to rpc-compat #4186

feat: Duplicate Withdrawal and move try from impls to rpc-compat #4186

supernovahs commented Aug 14, 2023 •

edited

Loading

codecov bot commented Aug 14, 2023 •

edited

Loading

supernovahs commented Aug 14, 2023

mattsse left a comment

mattsse Aug 15, 2023

supernovahs Aug 15, 2023

supernovahs Aug 16, 2023 •

edited

Loading

supernovahs Aug 16, 2023

mattsse Aug 17, 2023

mattsse Aug 15, 2023

mattsse Aug 17, 2023

supernovahs commented Aug 18, 2023

mattsse commented Aug 29, 2023

supernovahs commented Aug 29, 2023

supernovahs commented Sep 5, 2023

supernovahs commented Sep 16, 2023 •

edited

Loading

supernovahs commented Sep 18, 2023

mattsse commented Sep 18, 2023

supernovahs commented Sep 18, 2023

supernovahs commented Sep 19, 2023

mattsse left a comment

Rjected left a comment

Rjected Sep 19, 2023

supernovahs Sep 19, 2023

Rjected Sep 19, 2023

supernovahs Sep 19, 2023

Rjected left a comment

mattsse left a comment

	let withdraw = attributes.withdrawals.as_ref().map(\|withdrawals\| {
	let withdrawals = attributes.withdrawals.as_ref().map(\|withdrawals\| {

	//! Standalone functions for engine specific
	//! Standalone functions for engine specific rpc type conversions

feat: Duplicate Withdrawal and move try from impls to rpc-compat #4186

feat: Duplicate Withdrawal and move try from impls to rpc-compat #4186

Conversation

supernovahs commented Aug 14, 2023 • edited Loading

codecov bot commented Aug 14, 2023 • edited Loading

Codecov Report

supernovahs commented Aug 14, 2023

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

supernovahs Aug 16, 2023 • edited Loading

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

supernovahs commented Aug 18, 2023

mattsse commented Aug 29, 2023

supernovahs commented Aug 29, 2023

supernovahs commented Sep 5, 2023

supernovahs commented Sep 16, 2023 • edited Loading

supernovahs commented Sep 18, 2023

mattsse commented Sep 18, 2023

supernovahs commented Sep 18, 2023

supernovahs commented Sep 19, 2023

mattsse left a comment

Choose a reason for hiding this comment

Rjected 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

Rjected left a comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

supernovahs commented Aug 14, 2023 •

edited

Loading

codecov bot commented Aug 14, 2023 •

edited

Loading

supernovahs Aug 16, 2023 •

edited

Loading

supernovahs commented Sep 16, 2023 •

edited

Loading