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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5c0fad7
remove experimental flag and useBlindedBlock
gfukushima Aug 13, 2024
30854e4
remove outdated comment
gfukushima Aug 14, 2024
8fdd44d
remove redundant code
gfukushima Aug 14, 2024
52d6c38
Merge branch 'refs/heads/master' into remove-Xblock-v3-enabled
gfukushima Aug 14, 2024
ae20b68
fix tests
gfukushima Aug 14, 2024
73974e4
fix tests
gfukushima Aug 14, 2024
3534683
spotless
gfukushima Aug 14, 2024
0e2c4ae
remove old test used as base of v2 and blindedblock
gfukushima Aug 14, 2024
d18b280
spotless
gfukushima Aug 14, 2024
f47afa5
remove redundant check
gfukushima Aug 14, 2024
996085c
fix tests
gfukushima Aug 14, 2024
a59953e
fix tests
gfukushima Aug 14, 2024
46356e2
spotless
gfukushima Aug 14, 2024
6a6a685
remove redundant ITs
gfukushima Aug 14, 2024
1853058
spotless
gfukushima Aug 14, 2024
2ec44e8
removed unused method
gfukushima Aug 15, 2024
8ab3f75
PR review suggestion
gfukushima Aug 16, 2024
fbe3836
clean up deprecated log warn
gfukushima Aug 19, 2024
d389a88
Merge branch 'master' into remove-Xblock-v3-enabled
gfukushima Aug 19, 2024
713f7e0
fix AT
gfukushima Aug 20, 2024
c469051
Merge branch 'master' into remove-Xblock-v3-enabled
gfukushima Aug 20, 2024
c98b937
remove fallback catch and fix IT
gfukushima Aug 21, 2024
f7b999f
Merge branch 'refs/heads/master' into remove-Xblock-v3-enabled
gfukushima Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ SafeFuture<BlockContainerAndMetaData> createUnsignedBlock(
UInt64 proposalSlot,
BLSSignature randaoReveal,
Optional<Bytes32> optionalGraffiti,
Optional<Boolean> requestedBlinded,
Optional<UInt64> requestedBuilderBoostFactor,
BlockProductionPerformance blockProductionPerformance);

Expand Down
2 changes: 0 additions & 2 deletions ...on/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockFactoryDeneb.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,13 @@ public SafeFuture<BlockContainerAndMetaData> createUnsignedBlock(
final UInt64 proposalSlot,
final BLSSignature randaoReveal,
final Optional<Bytes32> optionalGraffiti,
final Optional<Boolean> requestedBlinded,
final Optional<UInt64> requestedBuilderBoostFactor,
final BlockProductionPerformance blockProductionPerformance) {
return super.createUnsignedBlock(
blockSlotState,
proposalSlot,
randaoReveal,
optionalGraffiti,
requestedBlinded,
requestedBuilderBoostFactor,
blockProductionPerformance)
.thenCompose(
Expand Down
2 changes: 0 additions & 2 deletions ...n/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockFactoryPhase0.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ public SafeFuture<BlockContainerAndMetaData> createUnsignedBlock(
final UInt64 proposalSlot,
final BLSSignature randaoReveal,
final Optional<Bytes32> optionalGraffiti,
final Optional<Boolean> requestedBlinded,
final Optional<UInt64> requestedBuilderBoostFactor,
final BlockProductionPerformance blockProductionPerformance) {
checkArgument(
Expand All @@ -76,7 +75,6 @@ public SafeFuture<BlockContainerAndMetaData> createUnsignedBlock(
blockSlotState,
randaoReveal,
optionalGraffiti,
requestedBlinded,
requestedBuilderBoostFactor,
blockProductionPerformance),
blockProductionPerformance)
Expand Down
49 changes: 15 additions & 34 deletions .../src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ public Function<BeaconBlockBodyBuilder, SafeFuture<Void>> createSelector(
final BeaconState blockSlotState,
final BLSSignature randaoReveal,
final Optional<Bytes32> optionalGraffiti,
final Optional<Boolean> requestedBlinded,
final Optional<UInt64> requestedBuilderBoostFactor,
final BlockProductionPerformance blockProductionPerformance) {

Expand Down Expand Up @@ -190,7 +189,6 @@ public Function<BeaconBlockBodyBuilder, SafeFuture<Void>> createSelector(
setExecutionData(
executionPayloadContext,
bodyBuilder,
requestedBlinded,
requestedBuilderBoostFactor,
SchemaDefinitionsBellatrix.required(schemaDefinitions),
blockSlotState,
Expand All @@ -208,7 +206,6 @@ public Function<BeaconBlockBodyBuilder, SafeFuture<Void>> createSelector(
private SafeFuture<Void> setExecutionData(
final Optional<ExecutionPayloadContext> executionPayloadContext,
final BeaconBlockBodyBuilder bodyBuilder,
final Optional<Boolean> requestedBlinded,
final Optional<UInt64> requestedBuilderBoostFactor,
final SchemaDefinitionsBellatrix schemaDefinitions,
final BeaconState blockSlotState,
Expand All @@ -221,26 +218,23 @@ private SafeFuture<Void> setExecutionData(
blockSlotState.getSlot()));
}

// if requestedBlinded has been specified, we strictly follow it, otherwise we should run
// Builder flow (blinded) only if we have a validator registration
final boolean shouldTryBuilderFlow =
requestedBlinded.orElseGet(
() ->
executionPayloadContext
.map(ExecutionPayloadContext::isValidatorRegistrationPresent)
.orElse(false));

// 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

bodyBuilder.executionPayloadHeader(
schemaDefinitions.getExecutionPayloadHeaderSchema().getHeaderOfDefaultPayload());
} else {
bodyBuilder.executionPayload(schemaDefinitions.getExecutionPayloadSchema().getDefault());
}

bodyBuilder.executionPayloadHeader(
schemaDefinitions.getExecutionPayloadHeaderSchema().getHeaderOfDefaultPayload());

bodyBuilder.executionPayload(schemaDefinitions.getExecutionPayloadSchema().getDefault());

return SafeFuture.COMPLETE;
}

// We should run Builder flow (blinded) only if we have a validator registration
final boolean shouldTryBuilderFlow =
executionPayloadContext
.map(ExecutionPayloadContext::isValidatorRegistrationPresent)
.orElse(false);

final ExecutionPayloadResult executionPayloadResult =
executionLayerBlockProductionManager.initiateBlockProduction(
executionPayloadContext.orElseThrow(),
Expand All @@ -251,8 +245,7 @@ private SafeFuture<Void> setExecutionData(

return SafeFuture.allOf(
cacheExecutionPayloadValue(executionPayloadResult, blockSlotState),
setPayloadOrPayloadHeader(
bodyBuilder, schemaDefinitions, requestedBlinded, executionPayloadResult),
setPayloadOrPayloadHeader(bodyBuilder, executionPayloadResult),
setKzgCommitments(bodyBuilder, schemaDefinitions, executionPayloadResult));
}

Expand All @@ -268,8 +261,6 @@ private SafeFuture<Void> cacheExecutionPayloadValue(

private SafeFuture<Void> setPayloadOrPayloadHeader(
final BeaconBlockBodyBuilder bodyBuilder,
final SchemaDefinitionsBellatrix schemaDefinitions,
final Optional<Boolean> requestedBlinded,
final ExecutionPayloadResult executionPayloadResult) {

if (executionPayloadResult.isFromLocalFlow()) {
Expand All @@ -287,8 +278,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 •
edited
Loading

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.

bodyBuilder.executionPayload(
builderBidOrFallbackData.getFallbackDataRequired().getExecutionPayload());
} else {
Expand All @@ -298,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

.map(BuilderBid::getHeader)
// from the local fallback
.orElseGet(
() -> {
final ExecutionPayload executionPayload =
builderBidOrFallbackData
.getFallbackDataRequired()
.getExecutionPayload();
return schemaDefinitions
.getExecutionPayloadHeaderSchema()
.createFromExecutionPayload(executionPayload);
});
.orElseThrow();
bodyBuilder.executionPayloadHeader(executionPayloadHeader);
}
});
Expand Down
2 changes: 0 additions & 2 deletions ...tor/src/main/java/tech/pegasys/teku/validator/coordinator/MilestoneBasedBlockFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ public SafeFuture<BlockContainerAndMetaData> createUnsignedBlock(
final UInt64 proposalSlot,
final BLSSignature randaoReveal,
final Optional<Bytes32> optionalGraffiti,
final Optional<Boolean> requestedBlinded,
final Optional<UInt64> requestedBuilderBoostFactor,
final BlockProductionPerformance blockProductionPerformance) {
final SpecMilestone milestone = getMilestone(proposalSlot);
Expand All @@ -78,7 +77,6 @@ public SafeFuture<BlockContainerAndMetaData> createUnsignedBlock(
proposalSlot,
randaoReveal,
optionalGraffiti,
requestedBlinded,
requestedBuilderBoostFactor,
blockProductionPerformance);
}
Expand Down
4 changes: 0 additions & 4 deletions .../validator/src/main/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,6 @@ public SafeFuture<Optional<BlockContainerAndMetaData>> createUnsignedBlock(
final UInt64 slot,
final BLSSignature randaoReveal,
final Optional<Bytes32> graffiti,
final Optional<Boolean> requestedBlinded,
final Optional<UInt64> requestedBuilderBoostFactor) {
LOG.info("Creating unsigned block for slot {}", slot);
performanceTracker.reportBlockProductionAttempt(spec.computeEpochAtSlot(slot));
Expand All @@ -357,7 +356,6 @@ public SafeFuture<Optional<BlockContainerAndMetaData>> createUnsignedBlock(
slot,
randaoReveal,
graffiti,
requestedBlinded,
requestedBuilderBoostFactor,
blockSlotState,
blockProductionPerformance))
Expand All @@ -368,7 +366,6 @@ private SafeFuture<Optional<BlockContainerAndMetaData>> createBlock(
final UInt64 slot,
final BLSSignature randaoReveal,
final Optional<Bytes32> graffiti,
final Optional<Boolean> requestedBlinded,
final Optional<UInt64> requestedBuilderBoostFactor,
final Optional<BeaconState> maybeBlockSlotState,
final BlockProductionPerformance blockProductionPerformance) {
Expand All @@ -390,7 +387,6 @@ private SafeFuture<Optional<BlockContainerAndMetaData>> createBlock(
slot,
randaoReveal,
graffiti,
requestedBlinded,
requestedBuilderBoostFactor,
blockProductionPerformance)
.thenApply(
Expand Down
16 changes: 11 additions & 5 deletions ...dator/src/test/java/tech/pegasys/teku/validator/coordinator/AbstractBlockFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,10 +196,17 @@ protected BlockContainerAndMetaData assertBlockCreated(
when(voluntaryExitPool.getItemsForBlock(any(), any(), any())).thenReturn(voluntaryExits);
when(blsToExecutionChangePool.getItemsForBlock(any())).thenReturn(blsToExecutionChanges);
when(eth1DataCache.getEth1Vote(any())).thenReturn(ETH1_DATA);
when(forkChoiceNotifier.getPayloadId(any(), any()))
.thenReturn(
SafeFuture.completedFuture(
Optional.of(dataStructureUtil.randomPayloadExecutionContext(false))));
if (blinded) {
when(forkChoiceNotifier.getPayloadId(any(), any()))
.thenReturn(
SafeFuture.completedFuture(
Optional.of(dataStructureUtil.randomPayloadExecutionContext(false, true))));
} else {
when(forkChoiceNotifier.getPayloadId(any(), any()))
.thenReturn(
SafeFuture.completedFuture(
Optional.of(dataStructureUtil.randomPayloadExecutionContext(false))));
}

final BLSSignature randaoReveal = dataStructureUtil.randomSignature();
final Bytes32 bestBlockRoot = recentChainData.getBestBlockRoot().orElseThrow();
Expand Down Expand Up @@ -237,7 +244,6 @@ protected BlockContainerAndMetaData assertBlockCreated(
newSlot,
randaoReveal,
Optional.empty(),
Optional.of(blinded),
Optional.empty(),
BlockProductionPerformance.NOOP));

Expand Down
32 changes: 16 additions & 16 deletions .../test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,6 @@ void shouldNotSelectOperationsWhenNoneAreAvailable() {
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));

Expand Down Expand Up @@ -303,7 +302,6 @@ void shouldIncludeValidOperations() {
randaoReveal,
Optional.of(defaultGraffiti),
Optional.empty(),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));

Expand Down Expand Up @@ -391,7 +389,6 @@ void shouldNotIncludeInvalidOperations() {
randaoReveal,
Optional.of(defaultGraffiti),
Optional.empty(),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));

Expand Down Expand Up @@ -422,7 +419,6 @@ void shouldIncludeDefaultExecutionPayload() {
blockSlotState,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(false),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));
Expand All @@ -443,7 +439,6 @@ void shouldIncludeExecutionPayloadHeaderOfDefaultPayload() {
blockSlotState,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(true),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));
Expand Down Expand Up @@ -471,7 +466,6 @@ void shouldIncludeNonDefaultExecutionPayload() {
blockSlotState,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(false),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));
Expand All @@ -490,9 +484,16 @@ void shouldIncludeExecutionPayloadHeaderIfBlindedBlockRequested() {
dataStructureUtil.randomExecutionPayloadHeader();
final UInt256 blockExecutionValue = dataStructureUtil.randomUInt256();

final ExecutionPayloadContext executionPayloadContextWithValidatorRegistration =
dataStructureUtil.randomPayloadExecutionContext(false, true);
when(forkChoiceNotifier.getPayloadId(any(), any()))
.thenReturn(
SafeFuture.completedFuture(
Optional.of(executionPayloadContextWithValidatorRegistration)));

prepareBlockProductionWithPayloadHeader(
randomExecutionPayloadHeader,
executionPayloadContext,
executionPayloadContextWithValidatorRegistration,
blockSlotState,
Optional.of(blockExecutionValue));

Expand All @@ -503,7 +504,6 @@ void shouldIncludeExecutionPayloadHeaderIfBlindedBlockRequested() {
blockSlotState,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(true),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));
Expand Down Expand Up @@ -534,7 +534,6 @@ void shouldIncludeExecutionPayloadIfUnblindedBlockRequested() {
blockSlotState,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(false),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));
Expand Down Expand Up @@ -566,7 +565,6 @@ void shouldIncludeExecutionPayloadIfRequestedBlindedIsEmpty() {
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));

Expand Down Expand Up @@ -598,7 +596,6 @@ void shouldIncludeExecutionPayloadIfRequestedBlindedIsEmptyAndBuilderFlowFallsBa
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));

Expand Down Expand Up @@ -640,7 +637,6 @@ void shouldIncludeExecutionPayloadIfRequestedBlindedIsEmptyAndBuilderFlowFallsBa
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));

Expand Down Expand Up @@ -704,7 +700,6 @@ void shouldIncludeKzgCommitmentsInBlock() {
blockSlotState,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(false),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));
Expand All @@ -729,9 +724,16 @@ void shouldIncludeKzgCommitmentsInBlindedBlock() {
final SszList<SszKZGCommitment> blobKzgCommitments =
dataStructureUtil.randomBlobKzgCommitments();

final ExecutionPayloadContext executionPayloadContextWithValidatorRegistration =
dataStructureUtil.randomPayloadExecutionContext(false, true);
when(forkChoiceNotifier.getPayloadId(any(), any()))
.thenReturn(
SafeFuture.completedFuture(
Optional.of(executionPayloadContextWithValidatorRegistration)));

prepareBlindedBlockAndBlobsProduction(
randomExecutionPayloadHeader,
executionPayloadContext,
executionPayloadContextWithValidatorRegistration,
blockSlotState,
blobKzgCommitments,
blockExecutionValue);
Expand All @@ -745,7 +747,6 @@ void shouldIncludeKzgCommitmentsInBlindedBlock() {
blockSlotState,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(true),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));
Expand Down Expand Up @@ -979,7 +980,6 @@ void shouldThrowWhenExecutionPayloadContextNotProvided() {
blockSlotState,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(false),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder))
Expand Down
Loading