Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove --xblock-v3-enabled and useBlindedBlock variable #8511

Merged
merged 23 commits into from
Aug 21, 2024

Conversation

gfukushima
Copy link
Contributor

@gfukushima gfukushima commented Aug 13, 2024

PR Description

This PR removes the experimental flag that allows users to enable or disable block production using the produceBlockV3 API. V3 is now the default behaviour and V2 deprecated implementations have been removed.

Fixed Issue(s)

Fixes #7779

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@@ -287,8 +281,7 @@ private SafeFuture<Void> setPayloadOrPayloadHeader(
builderBidOrFallbackData -> {
// we should try to return unblinded content only if no explicit "blindness" has been
// requested and builder flow fallbacks to local
if (requestedBlinded.isEmpty()
&& builderBidOrFallbackData.getFallbackData().isPresent()) {
if (builderBidOrFallbackData.getFallbackData().isPresent()) {
Copy link
Contributor

@StefanBratanov StefanBratanov Aug 13, 2024

Choose a reason for hiding this comment

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

This whole if statement should be simplified. After this change, we have only one condition, if fallback data is present, we return unblinded executionPayload, otherwise we have the builderBid and return the blinded header.

Essentially, we can skip this, since it will never happen:

final ExecutionPayload executionPayload =builderBidOrFallbackData
    .getFallbackDataRequired()
    .getExecutionPayload();
 return schemaDefinitions
     .getExecutionPayloadHeaderSchema()
     .createFromExecutionPayload(executionPayload);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well that's interesting, I understand the logic behind this clean up but wouldn't that be somehow a weird enforcement we would do? Every CL must have a EL in order to be fully functional in the network as a proposer, no? If so the first condition will always be true and even if you configure a builderBid we will default to use the fallback always isn't that so?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, the way it works is that either the builder bid is present or the fallback is present. The two won't be present at the same time.

Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
@@ -291,16 +288,7 @@ private SafeFuture<Void> setPayloadOrPayloadHeader(
// from the builder bid
Copy link
Contributor

Choose a reason for hiding this comment

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

We can create similar method to getFallbackDataRequired() like getBuilderBidRequired() to make it bit cleaner


// pre-Merge Execution Payload / Execution Payload Header
if (executionPayloadContext.isEmpty()) {
if (shouldTryBuilderFlow) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm why did we remove the if statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if executionPayloadContext is empty then shouldTryBuilderFlow will always be false

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes, got it. In this case, we can just leave the .executionPayload

Copy link
Contributor

Choose a reason for hiding this comment

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

And move shouldTryBuilderFlow initialization afterwards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

leaving just the .executionPayload break one of the bellatrix unit tests (shouldIncludeExecutionPayloadHeaderOfDefaultPayload). which in my opinion should probably be fine to be removed but I'd like a second opinion here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we can just remove that test, it is no longer applicable. We will always use the unblinded flow in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think leaving only .executionPayload() will fix the acceptance test failure

Signed-off-by: Gabriel Fukushima <[email protected]>
@gfukushima gfukushima marked this pull request as ready for review August 14, 2024 14:15
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request removes the '--xblock-v3-enabled' flag and 'useBlindedBlock' variable from the Teku codebase, simplifying the block creation process. Key changes include:

  • Removed 'requestedBlinded' parameter from block creation methods across multiple classes
  • Simplified 'createUnsignedBlock' method signatures in various components
  • Eliminated fallback mechanisms for blinded block production
  • Updated test cases to reflect the removal of blinded block options
  • Removed deprecated methods related to older block versions

These changes streamline the block production process, potentially improving code maintainability and reducing complexity in the Teku Ethereum client.

30 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

Comment on lines 106 to +107
return validatorApiChannel.createUnsignedBlock(
slot, randao, graffiti, Optional.empty(), requestedBuilderBoostFactor);
slot, randao, graffiti, requestedBuilderBoostFactor);
Copy link

Choose a reason for hiding this comment

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

logic: The Optional.empty() parameter for isBlinded has been removed. Verify that this doesn't break any existing functionality.

Copy link
Contributor

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

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

LGTM with one comment

@StefanBratanov
Copy link
Contributor

I think we should remove this in ProduceBlockRequest

.withHandler(SC_NOT_FOUND,
                (__, ___) -> {
                  throw new BlockProductionV3FailedException();
                });

So that endpoint call from VC correctly returns 404. We don't have any fallback, so better to return correct response code to VC.

@gfukushima gfukushima enabled auto-merge (squash) August 21, 2024 00:23
@gfukushima gfukushima merged commit 3f73ba0 into Consensys:master Aug 21, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the block production workflow to use the block V3 API by default
2 participants