Skip to content

Conversation

@ryanschneider
Copy link
Contributor

@ryanschneider ryanschneider commented Feb 26, 2025

Issue Addressed

This change makes the total_difficulty field in ExecutionBlock an Option<Uint256> since newer clients are no longer including the totalDifficulty field.

I think this will fix #6937 but I was actually more focused on the builder registration case described below.

In our builder-playground we setup a local devnet using lighthouse, reth, and mev-boost-relay. After upgrading to reth 1.2.0 and lighthouse v7.0.0.beta.0 for Pectra, we noticed that the validator registration process was sometimes failing with:

Feb 25 23:35:25.038 ERRO Unable to publish proposer preparation to all beacon nodes, error: Some endpoints failed, num_failed: 1 http://localhost:3500/ => RequestFailed(ServerMessage(ErrorMessage { code: 400, message: "BAD_REQUEST: error updating proposer preparations: ForkchoiceUpdate(EngineError(Api { error: Json(Error(\"missing field `totalDifficulty`\", line: 0, column: 0)) }))", stacktraces: [] })), service: preparation
Feb 25 23:35:25.099 WARN Unable to publish validator registrations to the builder network, error: Some endpoints failed, num_failed: 1 http://localhost:3500/ => RequestFailed(ServerMessage(ErrorMessage { code: 400, message: "BAD_REQUEST: error updating proposer preparations: ForkchoiceUpdate(EngineError(Api { error: Json(Error(\"missing field `totalDifficulty`\", line: 0, column: 0)) }))", stacktraces: [] })), service: preparation

What was even more confusing, was that it was sometimes working, which actually led to a wild goose chase thinking it was a networking issue. However, when tracing through the LH code, I came across this comment:

/// This function will result in a call to `forkchoiceUpdated` on the EL if we're in the
/// tail-end of the slot (as defined by `self.config.prepare_payload_lookahead`).

This explained why it sometimes worked, in our playground we run lighthouse with --prepare-payload-lookahead 8000 thus there was always a 4-second window where the call wasn't made.

But, if the call was made, then this code would 100% fail with updated reth:

https://github.com/sigp/lighthouse/blob/unstable/beacon_node/execution_layer/src/lib.rs#L1688-L1692

Which would then mapped to a Error::ForkchoiceUpdate in update_execution_engine_forkchoice.

Proposed Changes

Anyways, the fix was to make total_difficulty Optional, and then to update any code paths where it was used. In doing so, I assume that if the EL doesn't include total difficulty then the chain is already post-merge.

Additional Info

I left execution_layer::test_utils::PoWBlock::total_difficulty non-optional since its used for testing the PoW transition, and total difficulty was an expected field at that time.

@CLAassistant
Copy link

CLAassistant commented Feb 26, 2025

CLA assistant check
All committers have signed the CLA.

@ryanschneider ryanschneider marked this pull request as ready for review February 26, 2025 21:07
@realbigsean realbigsean added bug Something isn't working ready-for-review The code is ready for review v7.0.0-beta.clean Clean release post Holesky rescue labels Feb 27, 2025
@realbigsean realbigsean changed the base branch from unstable to release-v7.0.0 February 27, 2025 14:46
@realbigsean realbigsean force-pushed the optional-total-diffculty branch from 84e6de8 to 015d454 Compare February 27, 2025 14:52
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Eventually I'd like to delete all this merge-related/difficulty-related stuff too.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Mar 5, 2025
mergify bot added a commit that referenced this pull request Mar 5, 2025
@mergify mergify bot merged commit efa6ba3 into sigp:release-v7.0.0 Mar 5, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready-for-merge This PR is ready to merge. v7.0.0-beta.clean Clean release post Holesky rescue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants