Builder flow for Deneb & Blobs

[jimmygchen]

Issue Addressed

Addresses #3689

Builder spec: ethereum/builder-specs#61
4844 Builder flow: https://hackmd.io/@jimmygchen/B1dLR74Io

Proposed Changes

• Add new Sidecar and BlockProposal traits that allows parameterizing sidecar types in BlockContents, SignedBlockContents and to reduce code duplication required for blinded blob flow.

Beacon Node

• produceBlindedBlock endpoint updates
  • Handle new Deneb BuilderBid response from builder endpoint (new BlindedBlobsBundle type)
  • Build BlockContents response (containing kzg_commitments, proof and blinded_blob_sidecars)
• submitBlindedBlock endpoint updates
  • Handle SignedBlockContents request and forward to builder
  • Handle unblinded ExecutionPayloadAndBlobsBundle response from builder endpoint, and publish block
• Caching blobs from local EE in the blinded flow

Validator Client

• Handle produceBlindedBlock response from Beacon API
• Individual signing of blinded_blob_sidecars
• Build SignedBlockContents and submit to Beacon API (submitBlindedBlock)

[jimmygchen]

Hey @realbigsean @pawanjay176 @ethDreamer

I've introduced an AbstractSidecar trait to support blinded blobs, similar to how the existing AbstractExecPayload works for blinded payloads.

This PR is not complete, but it would be great to get some feedback on the generic parts of it for now, as it would have quite a bit of impact on the implementation.

I think it would be good to look at these types:

lighthouse/consensus/types/src/blob_sidecar.rs

Lines 21 to 29 in 59fb1af

	pub trait AbstractSidecar<E: EthSpec>:
	serde::Serialize
	+ DeserializeOwned
	+ Encode
	+ Decode
	+ Hash
	+ TreeHash
	+ TestRandom
	+ for<'a> arbitrary::Arbitrary<'a>

lighthouse/consensus/types/src/signed_blob.rs

Lines 32 to 40 in 59fb1af

	pub struct SignedSidecar<T: EthSpec, Sidecar: AbstractSidecar<T>> {
	pub message: Arc<Sidecar>,
	pub signature: Signature,
	#[ssz(skip_serializing, skip_deserializing)]
	#[tree_hash(skip_hashing)]
	#[serde(skip)]
	#[arbitrary(default)]
	pub _phantom: PhantomData<T>,
	}

lighthouse/common/eth2/src/types.rs

Lines 1280 to 1284 in 59fb1af

	pub enum BlockContents<T: EthSpec, Payload: AbstractExecPayload<T>, Sidecar: AbstractSidecar<T>> {
	BlockAndBlobSidecars(BeaconBlockAndBlobSidecars<T, Payload, Sidecar>),
	BlindedBlockAndBlobSidecars(BlindedBeaconBlockAndBlobSidecars<T, Payload, Sidecar>),
	Block(BeaconBlock<T, Payload>),
	}

lighthouse/common/eth2/src/types.rs

Lines 1358 to 1366 in 59fb1af

	pub enum SignedBlockContents<
	T: EthSpec,
	Payload: AbstractExecPayload<T> = FullPayload<T>,
	Sidecar: AbstractSidecar<T> = BlobSidecar<T>,
	> {
	BlockAndBlobSidecars(SignedBeaconBlockAndBlobSidecars<T, Payload, Sidecar>),
	BlindedBlockAndBlobSidecars(SignedBlindedBeaconBlockAndBlobSidecars<T, Payload, Sidecar>),
	Block(SignedBeaconBlock<T, Payload>),
	}

I've also maintained some aliases to existing types for readability, and it also allows me to make less changes to existing code:

lighthouse/consensus/types/src/signed_blob.rs

Lines 43 to 46 in 59fb1af

	pub type SignedSidecarList<T, Sidecar> =
	VariableList<SignedSidecar<T, Sidecar>, <T as EthSpec>::MaxBlobsPerBlock>;
	pub type SignedBlobSidecar<T> = SignedSidecar<T, BlobSidecar<T>>;

Let me know your thoughts 🙏

[jimmygchen]

I'm going to do the remaining tasks in a separate PR to keep the PR size manageable (already quite big!):

• blob publishing (submitBlindedBlock endpoint updates)
• blob signing

[ethDreamer]

Nice work Jimmy!

it would be great to get some feedback on the generic parts of it for now, as it would have quite a bit of impact on the implementation

Well, in my mind, you may have opened up a can of worms here. I think it's worth pausing and considering the best way forward for achieving polymorphism in this case. There are 2 options:

1. Traits (as you've done here)
2. Enums (via superstruct)

I found some background from this article on this subject. The author tends to lead towards enums, and I suspect they would even more if they used superstruct. The main reason I want to pause and consider this is that I'm not sure if traits are the best choice in the case of blinded objects.

Originally @realbigsean created the ExecPayload trait in order to abstract across blinded and unblinded payloads. That worked fine, but it got much more complicated when Capella hit and we had to abstract across the ExecutionPayload fork variants and the blinded/unblinded payloads at the same time. @michaelsproul eventually came up with AbstractExecPayload to abstract across both of these dimensions. Doing this required him to be quite clever and use generic associated types, a feature which was (luckily) being mainlined into rust at exactly the time we were working on Capella. While we eventually got it to work, I do find that code somewhat difficult to read/understand, and I remember at the time wondering if we had painted ourselves into a corner by choosing generics for abstracting across blinded/unblinded payloads.

Now it could be the case that either implementation leads to complex code once you have to abstract across two dimensions. But the experiment hasn't been run, and this is perhaps an opportunity to consider if we want to run that experiment. I suspect if we tried the enum route then maybe some of the enums you've created to encapsulate the dual behavior might no longer be necessary/be written in a cleaner way.

If we do try it out with enums, and then in some future fork there is a change to the BlobSidecar, then we'll get to find out which path is simpler across two dimensions. If it turns out that enums are easier to work with/cleaner, we might consider converting ExecutionPayloads in that direction as well.

If we decide to continue with traits for blinded sidecars, then I suggest the AbstractSidcar trait be renamed to just Sidecar, and AbstractSidcar be reserved for the case where a future fork changes the BlobSidecar so that we keep the same convention as ExecPayload / AbstractExecPayload.

I'm interested to hear if other people have thoughts on this :)

[jimmygchen]

Hey Mark, thanks a lot for the insights! 🙏

Very useful to understand how we got here and your experience with ExecPayload. I agree with your points and definitely keen to hear from you and others and consider options before continuing - and I think you've raised some great points that I'd like to do some further experiment on. I've aleady done a few experiments on different approaches and I was also avoiding generics earlier because:

• I also found it a bit difficult to understand, looking at how Payloads are handled
• Given we don't foresee adding fork versions for blob sidecars yet, it would be nice to keep it simple without using traits / generics

My last attempt before generics was using enums without superstruct (I was thinking we reserve the superstruct option if we need to add the fork variants) but that didn't go so well and I ran into issues with SSZ decoding:

Use an enum to wrap Blob & BlobRoot, and #[serde(flatten)] it so it uses different field name for each variant, but this doesn't work for SSZ decoding because Decode cannot be derived for enum_behaviour "transparent"- I'm thinking perhaps implementing Decode would work?

@sproul pointed out the above won't work because there's no signifier in the blob data that defines whether the sidecar should be full/blinded). So I scrapped that PR and started again with generics. Now your comments made me think again - maybe there's a way that could work using enums if I structure the data differently to the above. I'm keen to give it another try!

I'm still pretty new to Rust, and especially with generics & traits, so this is definitely an area worth exploring and I don't mind doing a few iterations.

Here's some of my learnings from this current PR using traits:

Advantages:

1. It's similar to what we already have with Payload, so it's a bit more comfortable and consistent, without having to touch ExecutionPayload - however like you said, if we find a better approach we could also convert ExecutionPayload
2. It's not as complicated as Payload because we don't need forked versions yet.
3. It's more readable and concise than I thought it would be (but this is always biased coming from the author..)

Disadvantages

• The Rust generics doesn't work the same way as most other languages, and the type needs to be known at compile time, so we can't have a function return multiple variants or an list/map that stores different variants, which we can do with Rust enums. (we also need a separate proposal cache for blinded blobs because of this)
• I had to add Payload and Sidecar generic type params to BlindedBeaconBlockAndBlobSidecars, which I thought are unnecessary given the types here should always be BlindedPayload and BlindedSidecar, but this is required if want to allow BlockContents functions to return variants of Payloads and Sidecar - I think using enum may potentially improve this:

lighthouse/common/eth2/src/types.rs

Lines 1495 to 1502 in 59fb1af

	pub struct BlindedBeaconBlockAndBlobSidecars<
	T: EthSpec,
	Payload: AbstractExecPayload<T>,
	Sidecar: AbstractSidecar<T>,
	> {
	pub blinded_block: BeaconBlock<T, Payload>,
	pub blinded_blob_sidecars: SidecarList<T, Sidecar>,
	}

lighthouse/common/eth2/src/types.rs

Lines 1289 to 1311 in 59fb1af

	pub fn block(&self) -> &BeaconBlock<T, Payload> {
	match self {
	BlockContents::BlockAndBlobSidecars(block_and_sidecars) => &block_and_sidecars.block,
	BlockContents::BlindedBlockAndBlobSidecars(block_and_sidecars) => {
	&block_and_sidecars.blinded_block
	}
	BlockContents::Block(block) => block,
	}
	}
	pub fn deconstruct(self) -> (BeaconBlock<T, Payload>, Option<SidecarList<T, Sidecar>>) {
	match self {
	BlockContents::BlockAndBlobSidecars(block_and_sidecars) => (
	block_and_sidecars.block,
	Some(block_and_sidecars.blob_sidecars),
	),
	BlockContents::BlindedBlockAndBlobSidecars(block_and_sidecars) => (
	block_and_sidecars.blinded_block,
	Some(block_and_sidecars.blinded_blob_sidecars),
	),
	BlockContents::Block(block) => (block, None),
	}
	}

Agree with re-naming if we continue with the trait approach (definitely came across my mind.. 😅)

[ethDreamer]

@sproul pointed out the above won't work because there's no signifier in the blob data that defines whether the sidecar should be full/blinded)

Yeah I definitely thought Decode might be where you would run into issues. However correct me if I'm wrong but blinded blobs are only ever used during block production right? Thus they:

1. never get stored in the DB
2. are never received from the network

If those are true then the only time you Decode a blob is when you receive it from gossip as a full blob. In which case you can implement a simple Decode that always expects a full blob.

[realbigsean]

However correct me if I'm wrong but blinded blobs are only ever used during block production right

At some point we will probably use SSZ in the block production APIs (#3701) so we probably still want Decode for blinded blobs even if we aren't using it today. Although since blinded blobs will be small maybe it's OK if it was uniquely non-SSZ but that'd be a little weird Edit: I forgot blocks + blobs are combined in the API so I think we are gonna want to use SSZ for blocks + blobs there eventually

[realbigsean]

I am generally in support of exploring the enum approach because the payload types are pretty confusing. I think Decode generally forces us to put all the type information in the outermost type we're decoding, which is why we end up using traits or top-level enums (superstruct). I can only think of this working by making our top-level enum include blinded and non-blinded variants for each fork. But this would be a big pain because right now we do a lot per-fork logic by matching on the top-level enum. I wonder if this sort of thing could work ? BeaconBlock::Capella(BeaconBlockCapella::Blinded(BeaconBlockCapellaBlinded {..})) generated using superduperstruct?

[jimmygchen]

@ethDreamer @realbigsean

I did a small experiment with enums using superstruct on this repo and documented some of my learnings on the README.md:
https://github.com/jimmygchen/superstruct-enum-experiment

I think the enum approach above does seem simpler to understand but potentially comes at a cost of increased code duplication (with my implementation anyway), and could get worse if we introduce a new type for EIP-6110 (in-protocol deposits)! There might be a nicer way of doing this. IMO the traits + generics approach doesn't seem too bad - at least it hides the complexity in one place 😅 .

I don't see a clear win here between the two approaches (enum vs traits) to justify a switch. I do agree enum is generally the nicer way to do polymorphism in Rust, but this becomes less clear adding an additional dimension (block type) into the mix. The third approach with superduperstruct seems to require more work and could be more complicated than needed. At this point, I think it might be worth taking the 6110 changes into account before we switch to a different approach.

@michaelsproul raised a few interesting points:

• we store finalized blocks without their execution payload by default (unless user adds the --prune-payloads false flag)
• the in-protocol deposit feature may require a third block type (sparse), although I don't understand it enough to make a good judgement based on this, but I think given there are still quite a bit unknowns there, it might be worth considering to postpone making big changes until we understand the 6110 changes a bit more, and deliver Deneb first using the existing familiar approach.

More from @michaelsproul re 6110 changes:

another thing to consider (which might give weight to the trait approach) is which abstraction fits best with making some types of blocks non-executable

atm both blinded and full blocks can go through the state transitions. But with 6110 only sparse and full will actually be executable

so like fn process_block<T: ExecutableBlock>(block: T) might be better than fn process_block(block: SignedBeaconBlock) where SignedBeaconBlock includes variants that we have to error on

In summary, I think it's definitely an experiment worth doing, but at present I don't have a better way that justifies the switch, and given 6110 might make it in the next fork after Deneb, we might want some more time to think about this.

Keen to hear your thoughts!

[sproul]

hey guys, I have been included in this conversation by accident and will unsubscribe now. Looks like I've been confused for @michaelsproul

[realbigsean]

Great job Jimmy! Gave it a more thorough review today

[jimmygchen]

Thanks for the review @realbigsean ! I've addressed your comments, please take another look when you have time. Diffs here.

[jimmygchen]

ah looks like I'm getting some weird test failures! On windows it's a state access violation, and I'm getting invalid memory reference on MacOS locally, and in linux the tests just seem to be weird comparisons. Will look into this tomorrow.

[realbigsean]

Ok the issue is with the changes I made to BuidlerBid. The type we're embedding now is BlindedPayloadFork rather than ExecutionPayloadHeaderFork. So we are failing deserialization because we have this:

{
  "message": {
    "header": {
       "execution_payload_header": {
         ..
       },
       ..
    },
    ..
  }
}

Investigating this made me realize we don't need BuilderBid to be generic over blinded/non-blinded at all.. it should always be blinded. I'm not sure why I originally wrote it like that. I made some updates to remove its generic here, and it fixed this deserialization issue: d00383c

[jimmygchen]

Nice, make sense! It was an oversight on my part, didn't notice that, but the release tests look happy now 🎉