Changes for `devnet-8` #4518

ethDreamer · 2023-07-18T20:05:23Z

Issue Addressed

Reviewing this PR is probably easiest by reviewing the individual commits as they are self-contained.

beacon_node/beacon_chain/src/execution_payload.rs

jimmygchen · 2023-07-19T06:39:38Z

beacon_node/execution_layer/src/engine_api.rs

+            ExecutionPayload::Capella(payload) => Ok(Self::Capella(NewPayloadRequestCapella {
+                execution_payload: payload,
+            })),
+            ExecutionPayload::Deneb(_) => Err(Self::Error::IncorrectStateVariant),


Is there a reason why this returns an error?

For deneb, you can't convert only an ExecutionPayload into a NewPayloadRequest, you also need the parent_beacon_block_root. This conversion is only used for tests thus far. Lighthouse is always using the code above to convert from a BeaconBlockRef to a NewPayloadRequest.

Related to this: ethereum/consensus-specs#3454
it's a pain point for lodestar I guess, has it impacted our tests?

This is just here to make some testing code (which currently doesn't bother to test post-merge forks) more concise. It only impacts some merge fork tests (thus the error condition isn't triggered). There's TODO's in the code to consider making those tests fork-aware / test multiple forks. I we decide to do that, this code will be revisited.

If it's only used in tests, I wonder if it's worth adding #[cfg(test)] to make it more explicit and ensure we don't later use this in production code?

Unfortunately we don't have our code structured that way. It's used in the mock_execution_engine as well as the execution engine integration tests. I believe the later isn't run with cfg(test) enabled. Fixing up these tests is beyond the scope of this PR. I'd be happy to look into doing this but I'm not sure it should be done before we get deneb-free-blobs merged into unstable as it would only make the review even worse.

As it stands it's not really the worst thing in the world. I would be worried if it were a panic but an error isn't bad.

ah yeah that wouldn't work for integration tests, got it thanks :)

jimmygchen · 2023-07-19T07:02:40Z

I think we may also need to add the engine_forkchoiceUpdatedV3 method calls to support the new PayloadAttributes

lighthouse/beacon_node/execution_layer/src/engine_api/http.rs

Lines 1144 to 1156 in 6ef151a

    
           pub async fn forkchoice_updated( 
        
               &self, 
        
               forkchoice_state: ForkchoiceState, 
        
               payload_attributes: Option<PayloadAttributes>, 
        
           ) -> Result<ForkchoiceUpdatedResponse, Error> { 
        
               let engine_capabilities = self.get_engine_capabilities(None).await?; 
        
               if engine_capabilities.forkchoice_updated_v2 { 
        
                   self.forkchoice_updated_v2(forkchoice_state, payload_attributes) 
        
                       .await 
        
               } else if engine_capabilities.forkchoice_updated_v1 { 
        
                   self.forkchoice_updated_v1(forkchoice_state, payload_attributes) 
        
                       .await 
        
               } else {

realbigsean

Nice! A few things:

I think we should bump the EF tests in the PR to get CI working Add Deneb spec tests for v1.4.0-beta.0 #4519
I don't see these p2p changes in attestation verification: https://github.com/ethereum/consensus-specs/pull/3360/files#diff-21cd1dde2a4815e714ef42d0cefa1bbd2997a4358ecf6e9e51025332c1d642fd
- I think we need to adjust verify_propagation_slot_range by ditching ATTESTATION_PROPAGATION_SLOT_RANGE for deneb
After implementing 4778, do you have any thoughts about this? I think they are trying to get more CL feedback about whether to merge it. Whatever is easiest, preferable on our end Move parent_beacon_block_root in execution payload and update verification ethereum/consensus-specs#3454
We need to update voluntary exit signing in the account manager as well: https://github.com/sigp/lighthouse/blob/stable/account_manager/src/validator/exit.rs

beacon_node/execution_layer/src/engine_api.rs

realbigsean · 2023-07-19T13:18:52Z

beacon_node/execution_layer/src/lib.rs

                                } else {
+                                    // TODO: determine an actual heuristic for when we take the


If we just make this BUILDER_PROFIT_THRESHOLD configurable (we should rename it though because we already have a --builder-profit-threshold flag) and expose it with a flag, this seems like enough for now. We could also just make it a binary option to listen to the EL suggestion or not

I'm not sure if this is something we necessarily want to treat in the same way as the --builder-profit-threshold setting. It's a difficult question of when do we listen to the execution layer for overriding the builder. I put this trivial mechanism in purely as a safety measure so users won't lose a super valuable block. I think there needs to be more discussion around this involving both EL & CL before we decide how to properly deal with this.

I think we should just ignore should_override_builder for now then, and maybe just log a warning until we have a better idea of how we should interpret it.

okay I've updated this to have a new flag:

ignore-builder-override-suggestion-threshold which is a percentage difference to be exceeded by the builder payload in order to override this. I gave it a default value of 10%

consensus/state_processing/src/per_block_processing/verify_attestation.rs

jimmygchen · 2023-07-20T00:34:55Z

We need to update voluntary exit signing in the account manager as well: https://github.com/sigp/lighthouse/blob/stable/account_manager/src/validator/exit.rs

Oh good point, we also need to update voluntary exit signing in the VC API:

lighthouse/validator_client/src/validator_store.rs

Lines 663 to 670 in 6ef151a

    
           pub async fn sign_voluntary_exit( 
        
               &self, 
        
               validator_pubkey: PublicKeyBytes, 
        
               voluntary_exit: VoluntaryExit, 
        
           ) -> Result<SignedVoluntaryExit, Error> { 
        
               let signing_epoch = voluntary_exit.epoch; 
        
               let signing_context = self.signing_context(Domain::VoluntaryExit, signing_epoch); 
        
               let signing_method = self.doppelganger_bypassed_signing_method(validator_pubkey)?;

realbigsean

So the consensus changes for EIP-7045 are very minor but end up creating a decent amount of code duplication. I'm wondering if we think it's worth making an effort to try to reduce the duplication to make it easier to review on the deneb merge to unstable. If not I think we should probably make it a bit easier to understand what exactly about these methods should be different, maybe by pointing out specifics in method docs. What are your thoughts?

realbigsean · 2023-08-02T15:03:03Z

validator_client/src/validator_store.rs

@@ -666,7 +666,19 @@ impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore<T, E> {
        voluntary_exit: VoluntaryExit,
    ) -> Result<SignedVoluntaryExit, Error> {
        let signing_epoch = voluntary_exit.epoch;
-        let signing_context = self.signing_context(Domain::VoluntaryExit, signing_epoch);
+        let mut signing_context = self.signing_context(Domain::VoluntaryExit, signing_epoch);
+        match self.spec.fork_name_at_epoch(signing_epoch) {


do you think it's worth moving this logic to the self.signing_context() method? in case it's ever used elsewhere with Domain::VoluntaryExit

yes i can do that

link to fix

realbigsean · 2023-08-02T15:13:58Z

consensus/state_processing/src/per_block_processing/signature_sets.rs

+        BeaconState::Base(_)
+        | BeaconState::Altair(_)
+        | BeaconState::Merge(_)
+        | BeaconState::Capella(_) => spec.get_domain(


Similar to the other comment - should we move fork specific logic into spec.get_domain so that other potential usages with the voluntary exit domain don't break? Since we pass in fork I think we have enough info

Not in this case because get_domain is a spec-defined function & changes of this type are done via bypassing this function to instead go straight to compute_domain. Compare process_voluntary_exit in:

phase0

deneb

this puts the change in the spec in approximately the same place as the change in our code and makes our code more spec-conformant overall

Right! Makes sense

beacon_node/execution_layer/src/engine_api/http.rs

realbigsean · 2023-08-02T15:52:12Z

beacon_node/src/cli.rs

+                    EE may make this suggestion depend on the EE's implementation, with the \
+                    primary intent being to safeguard against potential censorship attacks \
+                    from builders. Setting this flag to 0 will cause Lighthouse to always \
+                    ignore the EE's suggestion. Default: 10 (equivalent to 10%).")


since this parses to an f32, this would also accept decimals right? might be worth making that explict

good idea :)

link to fix

ethDreamer · 2023-08-03T19:46:14Z

So the consensus changes for EIP-7045 are very minor but end up creating a decent amount of code duplication. I'm wondering if we think it's worth making an effort to try to reduce the duplication to make it easier to review on the deneb merge to unstable.

I was also kind of annoyed by the code duplication, however I think the code is significantly more readable. If attestation processing has changed in deneb, I think it would be much more confusing to be calling methods like compute_beacon_block_attestation_reward_altair, new_for_altair, altair::process_attestations, within the deneb fork.

As far as removing some duplication, I suppose you could make the argument for making get_attestation_participation_flag_indices universal (fork-agnostic). But doing this would make the code for compute_beacon_block_attestation_reward_altair identical to compute_beacon_block_attestation_reward_deneb which is a problem because the fork subscript is required here to differentiate it from compute_beacon_block_attestation_reward_base. Thus we would be calling compute_beacon_block_attestation_reward_altair from within deneb and hiding that it actually behaves differently.

If not I think we should probably make it a bit easier to understand what exactly about these methods should be different, maybe by pointing out specifics in method docs. What are your thoughts?

I can add comments to the functions if that's what you mean

realbigsean

LGTM

realbigsean · 2023-08-04T16:24:36Z

We're testing some testing networking stuff on devnet 7 still so I think we should wait to merge this for a bit

realbigsean · 2023-08-08T14:43:51Z

There's a couple merge conflicts, but I can make a devnet-7 branch and we can merge this once that's resolved

Add override threshold flag Added tests for Override Threshold Flag Override default shown in decimal

Addressed Jimmy's Comments No need for matches Fix Mock Execution Engine Tests Fix clippy fix fcuv3 bug

Attestation Verification Post-Deneb Fix Gossip Attestation Verification Test

Fix Exit Signing for EIP-7044 Fix cross exit test Move 7044 Logic to signing_context()

ethDreamer · 2023-08-09T18:50:04Z

alright I believe I've gotten rid of the duplicated code :)

realbigsean

LGTM

ethDreamer requested a review from realbigsean July 18, 2023 20:05

jimmygchen mentioned this pull request Jul 19, 2023

Add Deneb spec tests for v1.4.0-beta.0 #4519

Closed

jimmygchen reviewed Jul 19, 2023

View reviewed changes

beacon_node/beacon_chain/src/execution_payload.rs Outdated Show resolved Hide resolved

jimmygchen reviewed Jul 19, 2023

View reviewed changes

realbigsean requested changes Jul 19, 2023

View reviewed changes

jimmygchen added deneb deneb-devnet-8 consensus An issue/PR that touches consensus code, such as state_processing or block verification. labels Jul 20, 2023

ethDreamer force-pushed the devnet-8 branch 8 times, most recently from 9fa5e27 to 4b47258 Compare July 30, 2023 21:26

ethDreamer force-pushed the devnet-8 branch 2 times, most recently from 4ebd3db to 9585d41 Compare August 1, 2023 21:56

realbigsean reviewed Aug 2, 2023

View reviewed changes

ethDreamer force-pushed the devnet-8 branch 2 times, most recently from e0106ff to 82b3ed4 Compare August 4, 2023 15:29

realbigsean approved these changes Aug 4, 2023

View reviewed changes

ethDreamer force-pushed the devnet-8 branch from fd77b79 to b0bc5ca Compare August 7, 2023 21:55

ethDreamer added 2 commits August 9, 2023 10:16

Addressed sigp#4487

0cfceba

Add override threshold flag Added tests for Override Threshold Flag Override default shown in decimal

Addressed sigp#4445

819d26f

Addressed Jimmy's Comments No need for matches Fix Mock Execution Engine Tests Fix clippy fix fcuv3 bug

ethDreamer added 6 commits August 9, 2023 10:21

Fix Block Root Calculation post-Deneb

d088470

Addressed sigp#4444

8c9679a

Attestation Verification Post-Deneb Fix Gossip Attestation Verification Test

Addressed sigp#4443

0c7f590

Fix Exit Signing for EIP-7044 Fix cross exit test Move 7044 Logic to signing_context()

Update EF Tests

4f588da

Addressed sigp#4560

6ab9a26

Added Comments around EIP7045

38731be

ethDreamer force-pushed the devnet-8 branch from b0bc5ca to 38731be Compare August 9, 2023 15:28

Combine Altair Deneb to Eliminate Duplicated Code

e7c0ff8

realbigsean approved these changes Aug 9, 2023

View reviewed changes

realbigsean merged commit 2b5385f into sigp:deneb-free-blobs Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for `devnet-8` #4518

Changes for `devnet-8` #4518

ethDreamer commented Jul 18, 2023 •

edited

Loading

jimmygchen Jul 19, 2023

ethDreamer Jul 19, 2023

realbigsean Jul 19, 2023

ethDreamer Jul 23, 2023

jimmygchen Jul 24, 2023

ethDreamer Jul 24, 2023

ethDreamer Jul 24, 2023

jimmygchen Jul 24, 2023

jimmygchen commented Jul 19, 2023 •

edited

Loading

realbigsean left a comment

realbigsean Jul 19, 2023

ethDreamer Jul 23, 2023

realbigsean Jul 25, 2023

ethDreamer Aug 1, 2023

jimmygchen commented Jul 20, 2023

realbigsean left a comment

realbigsean Aug 2, 2023

ethDreamer Aug 3, 2023

ethDreamer Aug 4, 2023

realbigsean Aug 2, 2023

ethDreamer Aug 3, 2023 •

edited

Loading

realbigsean Aug 4, 2023

realbigsean Aug 2, 2023

ethDreamer Aug 3, 2023

ethDreamer Aug 4, 2023 •

edited

Loading

ethDreamer commented Aug 3, 2023 •

edited

Loading

realbigsean left a comment

realbigsean commented Aug 4, 2023 •

edited

Loading

realbigsean commented Aug 8, 2023

ethDreamer commented Aug 9, 2023

realbigsean left a comment

		} else {
		// TODO: determine an actual heuristic for when we take the

Changes for devnet-8 #4518

Changes for devnet-8 #4518

Conversation

ethDreamer commented Jul 18, 2023 • edited Loading

Issue Addressed

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

Choose a reason for hiding this comment

jimmygchen commented Jul 19, 2023 • edited Loading

realbigsean 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

jimmygchen commented Jul 20, 2023

realbigsean 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

ethDreamer Aug 3, 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

ethDreamer Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

ethDreamer commented Aug 3, 2023 • edited Loading

realbigsean left a comment

Choose a reason for hiding this comment

realbigsean commented Aug 4, 2023 • edited Loading

realbigsean commented Aug 8, 2023

ethDreamer commented Aug 9, 2023

realbigsean left a comment

Choose a reason for hiding this comment

Changes for `devnet-8` #4518

Changes for `devnet-8` #4518

ethDreamer commented Jul 18, 2023 •

edited

Loading

jimmygchen commented Jul 19, 2023 •

edited

Loading

ethDreamer Aug 3, 2023 •

edited

Loading

ethDreamer Aug 4, 2023 •

edited

Loading

ethDreamer commented Aug 3, 2023 •

edited

Loading

realbigsean commented Aug 4, 2023 •

edited

Loading