Refactored ForkVersionedResponse and ForkVersionDeserialize

[PoulavBhowmick03]

Issue Addressed

Fixes #7286

Proposed Changes

• Removed the bound from ForkVersionDeserialize
• Made the version field mandatory in ForkVersionedResponse

Additional Info

Currently, only one test is failing, at beacon_node/http_api/tests/tests.rs at line 1621, all other tests pass

[CLAassistant]

All committers have signed the CLA.

[PoulavBhowmick03]

Hi @PoulavBhowmick03 thanks for working on this! Your PR does seem to make the version field in ForkVersionResponse response mandatory but this issue is a bit more involved than that.

What we're looking for is to remove the need for types that implement ForkVersionDeserialize to also implement DeserializeOwned

So I think step 1 is remove DeserializeOwned from this trait definition

pub trait ForkVersionDeserialize: Sized + DeserializeOwned

and Step 2 is go through each type that implements ForkVersionDeserialize and remove Deserialize

i.e. we'd want to remove Deserialize from this derive macro since BlockContents also implements ForkVersionDeserialize

#[derive(Debug, Clone, Serialize, Deserialize, Encode)]
#[serde(bound = "E: EthSpec")]
pub struct BlockContents<E: EthSpec> {
    pub block: BeaconBlock<E>,
    pub kzg_proofs: KzgProofs<E>,
    #[serde(with = "ssz_types::serde_utils::list_of_hex_fixed_vec")]
    pub blobs: BlobsList<E>,
}

ah okay, got it! working on this

[PoulavBhowmick03]

@eserilev once i remove the Deserialize, i get multiple errors including this, for the SsePayloadAttributes as in this image, what do you think would be the correct thing to do

the trait bound `SsePayloadAttributesV1: DeserializeOwned` is not satisfied
the following other types implement trait `Deserialize<'de>`:
  &'a Path
  &'a [u8]
  &'a serde_json::value::RawValue
  &'a str
  ()
  (T,)
  (T0, T1)
  (T0, T1, T2)
and 798 others
required for `SsePayloadAttributesV1` to implement `DeserializeOwned`rustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20[0]?0#file:///home/odinson/lighthouse/common/eth2/src/types.rs)
mod.rs(1032, 8): required by a bound in `from_value`
the trait bound `SsePayloadAttributesV1: DeserializeOwned` is not satisfied
the following other types implement trait `lighthouse::_::_serde::Deserialize<'de>`:
  &'a Path
  &'a [u8]
  &'a serde_json::value::RawValue
  &'a str
  ()
  (T,)
  (T0, T1)
  (T0, T1, T2)
and 798 others
required for `SsePayloadAttributesV1` to implement `DeserializeOwned`rustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20[1]?1#file:///home/odinson/lighthouse/common/eth2/src/types.rs)
mod.rs(1032, 8): required by a bound in `from_value`

[eserilev]

@PoulavBhowmick03 See my comment below. If it doesn't make sense you can push up what you have so far and I can put together a quick example

[eserilev]

For many of our types we use this crate called superstruct. This allows us to create variants of a type for each fork. The goal here is to remove inheriting DeserializeOwned on the parent types. The variants of the parent type can still inherit DeserializeOwned.

As a more tangible example: For the SsePayloadAttributes struct definition we derive Deserialize both for the parent struct itself and its superstruct variants

https://github.com/sigp/lighthouse/blob/bf955c7543dac8911a6f6c334b5b3ca4ef728d9c/common/eth2/src/types.rs#L1026-L1032

I think you can remove the Deserialize derive macro on line 1030

And then in deserialize_by_fork do something like

impl ForkVersionDeserialize for SsePayloadAttributes {
    fn deserialize_by_fork<'de, D: serde::Deserializer<'de>>(
        value: serde_json::value::Value,
        fork_name: ForkName,
    ) -> Result<Self, D::Error> {
        match fork_name {
            ForkName::Bellatrix => SsePayloadAttributes::V1(serde_json::from_value(value))
                .map_err(serde::de::Error::custom),
             ...
        }
    }
}

That way only the superstruct fork variants implement deserialize and not the parent struct.

[PoulavBhowmick03]

@eserilev thanks! made the changes in types.rs where ForkVersionDeserialize was implemented. PTAL One test fails currently, because of the version field in ForkVersionResponse made mandatory, looking into it

[eserilev]

Hi @PoulavBhowmick03

I've pushed up a commit that removes deserialize derive macro from Attestation and BeaconBlock (There are more that will need to be removed). Removing those caused a few issues that I needed to resolve

1. Warp Filter

We use this crate called warp to do all of our api routing. warp uses these things called "filters" to route a request to the right function and parse the request data (query params, request body etc. etc.)

We have a custom warp filter for requests that include json in the request body https://github.com/sigp/lighthouse/blob/bf955c7543dac8911a6f6c334b5b3ca4ef728d9c/common/warp_utils/src/json.rs#L19-L33

This filter requires that the type being deserialized inherits DeserializedOwned. This caused problems with the post_lighthouse_block_rewards endpoint. Because BeaconBlock is expected in the request body of that endpoint and BeaconBlock no longer implements deserialize I needed to introduce a new filter.

The new filter fork_version_json just requires that the type being deserialized inherits ForkVersionDeserialize. The filter parses the consensus version header value to figure out the correct fork.

2. Tweaks to ForkVersionDeserialize

I was able to remove the D: Deserializer type parameter from the ForkVersionDeserialize traits deserialize_by_fork function definition. I instead introduced a new error type that can be converted from generic serde error types. This gives us a bit more flexibility

Theres other types that now need to have their deserialize_by_fork tweaked a bit to handle this change. I've already made these changes for Attestation and BeaconBlock.

If you still have time to continue working on this issue, I'll leave the rest to you. Happy to work through any questions/feedback if the changes I made don't make sense. Thanks for all your work so far!

The code at the moment wont compile until we fix the ForkVersionDeserialize impls to match the trait changes I made

[PoulavBhowmick03]

Collaborator

gm. apologies for the late reply, yes i went through the changes and now it makes sense. i am working on it step by step

[PoulavBhowmick03]

@eserilev gm, so i have made changes to around 20 files in the consensus part, but there seems to be errors in the tests directory because of these changes. should I push my code for you to look at the implementation/ changes, and the next steps?

[eserilev]

@PoulavBhowmick03 yeah go ahead and push your code and i'll take a look. thanks!

[PoulavBhowmick03]

@eserilev do take a look. i have managed to fix the errors, there just seems to be one error from the first commit of mine, for the version being mandatory now, other than that, if there is anything else to fix or any changes to be made, do let me know. thanks!

[eserilev]

Noting this here:

https://github.com/ethereum/beacon-APIs/pull/428 the GET beacon_blocks v1 endpoint has been fully deprecated and removed from the beacon api spec repo. Since it doesn't have a fork versioned response, theres no reason to support it in Lighthouse anymore. I've marked this PR as backwards incompatible and removed the v1 endpoint in this commit: 3b1cc3e

[eserilev]

Another note:

We had some weird logic where a V1 endpoint would automatically return fork_name = None. I've removed that logic and refactored things a bit: a5bc81f

[eserilev]

ef_tests are failing, will take a deeper look there

[PoulavBhowmick03]

Looks great so far! I'll do a bit more of a thorough review next week and probably get another set of eyes on this PR as well. Just one small nit for now

Alrighty! Thank you

[PoulavBhowmick03]

ef_tests are failing, will take a deeper look there

Sure! I'm having a look at it aswell, in case I can figure it out

[ethDreamer]

Hi @PoulavBhowmick03 — thanks so much for diving into this and putting in the effort.

Just a heads-up: after you started working on it, I realized the underlying issue is more complex than we initially thought. It turns out we need a deeper refactor of the trait to properly unblock the EF tests for Fulu.

I’ve started working on that in a separate PR:

• #7372

Really appreciate your work here — sorry for the overlap and change in direction.

[PoulavBhowmick03]

Hi @PoulavBhowmick03 — thanks so much for diving into this and putting in the effort.

Just a heads-up: after you started working on it, I realized the underlying issue is more complex than we initially thought. It turns out we need a deeper refactor of the trait to properly unblock the EF tests for Fulu.

I’ve started working on that in a separate PR:

• #7372

Really appreciate your work here — sorry for the overlap and change in direction.

Ah I see... Thank you for the application! It was amazing being new and a first time to work and also get such insights and help from @eserilev Will this PR be discarded in this case?

[eserilev]

@PoulavBhowmick03 there might still be some useful things in this PR so I'll keep this open for now. We'll merge #7372 first and revisit this one afterwards. Thanks again for all your help!

[PoulavBhowmick03]

@PoulavBhowmick03 there might still be some useful things in this PR so I'll keep this open for now. We'll merge #7372 first and revisit this one afterwards. Thanks again for all your help!

Alright! Thank you so much, and also for your help. meanwhile will try to look for other stuffs to work on

[mergify[bot]]

This pull request has merge conflicts. Could you please resolve them @PoulavBhowmick03? 🙏

[mergify[bot]]

Hi @PoulavBhowmick03, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.