feat(relay): ExecutionRequestsV4 with eip7685::Requests conversion by ryanschneider · Pull Request #1787 · alloy-rs/alloy

ryanschneider · 2024-12-12T20:38:50Z

This is a further cleaned up version of #1786 which also includes the necessary ExecutionRequestsV4 <-> Requests conversions. However, the Requests -> ExecutionRequestsV4 conversion requires use of the ssz_types crate, which was added as a feature.

Fixes #1763.

Motivation

As reviewing the builder-specs (and unsuccessfully attempting to change them with ethereum/builder-specs#107) I determined that the V4 relay submission actually needs to include the requests in their JSON "object" form rather than the encoded EIP-7865 Vec<Bytes>.

Solution

So, this restores some of the structs removed in #1515 and adds a new ExecutionRequestsV4 struct to rpc-types-beacon.

A couple things to note:

In restoring the eip structs I removed the "EL-specific" encoding helpers in favor of the CL flavors, adding a new alloy_serde::ssz::json::uint serde helper for encoing integers in SSZ "canonical JSON" format. I think this makes sense since these structs are no longer used by the EL/engine API, but I could be convinced to either move these structs in rpc-types-beacon or restore them with "EL-encoding" and add wrapper/custom serde functionality in rpc-types-beacon.
I also replaced the quantity encoding on SignedBidSubmissionV4::target_blobs_per_block with DisplayFromStr as I'm pretty sure this should be "decimal" encoded. NOTE: With 7742 pulled from PEctra I think this can be fully removed instead.
I also removed (or didn't readd) RLP encoding/decoding from these types since that should never be necessary.
The example JSON in crates/rpc-types-beacon/src/examples/relay_builder_block_validation_request_v4.json is a bit of frankenstein, it's a payload I captured from my locally running rbuilder inside our builder-playground with execution_requests and target_blobs_per_block manually edited, but it's the best I could find at the moment.
This should give us everything we need to round trip the requests created by the system contracts into ExecutionRequestsV4 objects that can be included in relay submissions, for example to close Pectra devnet-5: execution_requests needs latest alloy and should have the request_type byte prefixed to each flashbots/rbuilder#267.

PR Checklist

Added Tests
Added Documentation
Breaking changes

ryanschneider · 2024-12-13T01:18:20Z

    /// The EIP-7742 blobs per block for this bid.
-    #[serde(with = "alloy_serde::quantity")]
+    #[serde_as(as = "DisplayFromStr")]
    pub target_blobs_per_block: u64,


Aside: I saw today that EIP-7742 was pulled from Pectra, my PR might conflict with removing that since I changed SignedBidSubmissionV4::target_blobs_per_block to DisplayFromStr as well.

https://ethereum-magicians.org/t/all-core-devs-consensus-acdc-147-december-12-2024/22161/2

ryanschneider · 2024-12-13T01:19:17Z

+      }
+    ]
+  },
+  "target_blobs_per_block": "6",


EIP7742 will need to be removed here as well.

mattsse

some nits and qs

mattsse · 2024-12-13T16:47:14Z

+#[cfg(feature = "serde")]
+use cfg_eval::cfg_eval;


why do we need this?
can we do without? not familiar with this

iiuc we need this because of how serde_as works. we could solve this with a helper like serde(with = "from_str") I guess

similarly to https://github.com/foundry-rs/foundry/blob/b4a47336038cd4d1342eedb3f9555772585e745f/crates/config/src/lib.rs#L2514-L2534 but without lowercase

Its necessary to make #[serde_as] conditional on the serde feature: https://docs.rs/serde_with/latest/serde_with/guide/serde_as/index.html#gating-serde_as-on-features

Without it we get compilation failures running e.g. cargo +nightly check --no-default-features --target riscv32imac-unknown-none-elf --manifest-path crates/consensus/Cargo.toml in the no_std GHA.

FWIW @klkvr mentioned in telegram that we could move from serde_as to serde_with to remove the dep but IMO thats outside the scope of this PR.

Ah we cross-posted so I didn't see the foundry link, I'll take a stab at this later this afternoon, thanks!

Ok, I added a new alloy_serde::ssz::json::uint (de)serializer that if we're happy with I can switch other places to start using as well (in a follow-on PR). This let me get rid of cfg_eval and stop using serde_with in alloy-eips.

use alloy_serde; use serde::{Deserialize, Serialize}; #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] pub struct Container { #[serde(with = "alloy_serde::ssz::json::uint")] value: u64, } let val = Container { value: 18112749083033600 }; let s = serde_json::to_string(&val).unwrap(); assert_eq!(s, "{\"value\":\"18112749083033600\"}"); let deserialized: Container = serde_json::from_str(&s).unwrap(); assert_eq!(val, deserialized);

klkvr · 2024-12-13T17:02:04Z

+#[cfg(feature = "serde")]
+use cfg_eval::cfg_eval;


iiuc we need this because of how serde_as works. we could solve this with a helper like serde(with = "from_str") I guess

similarly to https://github.com/foundry-rs/foundry/blob/b4a47336038cd4d1342eedb3f9555772585e745f/crates/config/src/lib.rs#L2514-L2534 but without lowercase

ryanschneider · 2024-12-13T18:00:59Z

Thanks for the latest feedback, I got caught in some other tasks but will address this round later this afternoon.

Re-add {Deposit,Withdrawal,Consolidation}Request structs to relevant EIPs.

…_types.

…::json::uint

ryanschneider · 2024-12-13T21:27:14Z

Ok, I think all new feedback has been addressed, let me know what you think, especially around #1787 (comment).

edit: forgot to say rebased against latest master, so using Requests::with_capacity() now as well. Also let me know if I should squash my fixup commits once we're ready to merge, I've been trying to avoid too many force pushes to keep it easier to track the code evolution but am happy to squash down to a handful of commits once everything is signed off on.

ryanschneider · 2024-12-13T21:52:03Z

Also Claude recommended a pretty good refactor on the try_from implementation, if you think this is worth doing let me know and I'll try it out: https://claude.site/artifacts/e420d91a-8c2d-41ef-a40a-51666a8fa0e6

edit: after some more back-and-forth with it I was also able to remove some of the vector copying (e.g. calling extend): https://claude.site/artifacts/d58c9609-a1f4-4c5e-b0a4-278e2f8f051c

ryanschneider · 2024-12-13T22:07:01Z

^ Actually I went ahead and nerd sniped myself into integrating it. 😊

mattsse

last pedantic nits

mattsse · 2024-12-14T08:38:06Z

+    /// let deserialized: Container = serde_json::from_str(&s).unwrap();
+    /// assert_eq!(val, deserialized);
+    /// ```
+    pub mod uint {


this isn't limited to uint

mattsse · 2024-12-14T08:41:02Z

can we rename this entire module to displayfromstr ?

Ya will do, personally I have a slight preference for the current naming since it says why the encoding is used instead instead of how the encoding works, but I'll defer to you all.

Renamed in daf7703

mattsse · 2024-12-14T08:44:01Z

+    impl ExecutionRequestsV4 {
+        /// Convert the [ExecutionRequestsV4] into a [Requests].
+        pub fn to_requests(&self) -> Requests {
+            self.into()
+        }
+    }


can we move this top of mod?

Yup, moved in daf7703. Required some addiitonal #[cfg(feature = "ssz") pragmas though.

mattsse

lgtm

ryanschneider force-pushed the feature/re-add-CL-requests-from-requests branch 2 times, most recently from a3e9bb9 to c930f1d Compare December 12, 2024 23:14

ryanschneider changed the title ~~feat(relay): ExecutionRequestsV4 with from Requests conversion~~ feat(relay): ExecutionRequestsV4 with eip7685::Requests conversion Dec 12, 2024

ryanschneider force-pushed the feature/re-add-CL-requests-from-requests branch from c930f1d to 4e5eae5 Compare December 12, 2024 23:23

ryanschneider mentioned this pull request Dec 12, 2024

feat(relay): Re-add {Deposit,Withdrawal,Consolidation}Request objects for relay submissions #1786

Closed

3 tasks

ryanschneider force-pushed the feature/re-add-CL-requests-from-requests branch 6 times, most recently from a56d2e9 to 4e8d5bd Compare December 13, 2024 01:08

ryanschneider marked this pull request as ready for review December 13, 2024 01:12

ryanschneider requested review from DaniPopes, gakonst, mattsse and onbjerg as code owners December 13, 2024 01:12

ryanschneider commented Dec 13, 2024

View reviewed changes

klkvr reviewed Dec 13, 2024

View reviewed changes

Comment thread crates/rpc-types-beacon/src/requests.rs Outdated

Comment thread crates/rpc-types-beacon/src/requests.rs Outdated

ryanschneider requested a review from klkvr December 13, 2024 16:42

mattsse requested changes Dec 13, 2024

View reviewed changes

klkvr requested changes Dec 13, 2024

View reviewed changes

mattsse mentioned this pull request Dec 13, 2024

feat: add requests with capacity #1794

Merged

ryanschneider added 6 commits December 13, 2024 13:17

feat(relay): Add ExecutionRequestsV4 for relay submissions.

ff65983

Re-add {Deposit,Withdrawal,Consolidation}Request structs to relevant EIPs.

feat(requests): Add ExecutionRequestsV4::TryFrom<&Requests> using ssz…

679f0ed

…_types.

fixup(serde): Fix serde conditionals on eip6110/7002 using cfg_eval

616de8b

fixup: feature ssz_types should require ssz.

a0cded7

fixup: deps cleanup - remove ssz_types, cfg_eval to workspace

7842dcc

feat(serde): replace serde_with::DisplayFromStr with alloy_serde::ssz…

b321248

…::json::uint

ryanschneider added 2 commits December 13, 2024 13:17

fixup: doc comment typos

bab4a85

fixup: derive thiserror::Error on TryFromRequestsError

03ff69e

ryanschneider force-pushed the feature/re-add-CL-requests-from-requests branch from 01d222f to 03ff69e Compare December 13, 2024 21:17

fixup: use Requests::with_capacity().

5bf7dc8

ryanschneider requested review from klkvr and mattsse December 13, 2024 21:26

ryanschneider commented Dec 13, 2024

View reviewed changes

Comment thread Cargo.toml Outdated

ryanschneider commented Dec 13, 2024

View reviewed changes

Comment thread crates/serde/src/ssz.rs Outdated

fixup: cleanup cargo changes

14cbaad

fixup: More efficient/readable try_from implementation.

eb43d13

mattsse requested changes Dec 14, 2024

View reviewed changes

fixup: rename to serde::displayfromstr, other nits.

daf7703

ryanschneider requested a review from mattsse December 14, 2024 21:43

mattsse approved these changes Dec 16, 2024

View reviewed changes

Merge branch 'main' into feature/re-add-CL-requests-from-requests

35c7ae7

mattsse merged commit bbae675 into alloy-rs:main Dec 16, 2024

rkrasiuk mentioned this pull request Dec 23, 2024

[Feature] EIP-7251: update MAX_CONSOLIDATIONS #1831

Closed

Conversation

ryanschneider commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Solution

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanschneider commented Dec 13, 2024

Uh oh!

ryanschneider commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanschneider commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanschneider commented Dec 13, 2024

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ryanschneider commented Dec 12, 2024 •

edited

Loading

ryanschneider commented Dec 13, 2024 •

edited

Loading

ryanschneider commented Dec 13, 2024 •

edited

Loading