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 12 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
36 changes: 12 additions & 24 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 @@ -218,21 +218,20 @@ 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
// We should run Builder flow (blinded) only if we have a validator registration
final boolean shouldTryBuilderFlow =
executionPayloadContext
.map(ExecutionPayloadContext::isValidatorRegistrationPresent)
.orElse(false);
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;
}

Expand All @@ -246,8 +245,7 @@ private SafeFuture<Void> setExecutionData(

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

Expand All @@ -263,7 +261,6 @@ private SafeFuture<Void> cacheExecutionPayloadValue(

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

if (executionPayloadResult.isFromLocalFlow()) {
Expand Down Expand Up @@ -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

.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
15 changes: 11 additions & 4 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
20 changes: 17 additions & 3 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 @@ -484,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 Down Expand Up @@ -717,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 @@ -733,7 +747,7 @@ void shouldIncludeKzgCommitmentsInBlindedBlock() {
blockSlotState,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(UInt64.ONE),
Optional.empty(),
BlockProductionPerformance.NOOP)
.apply(bodyBuilder));

Expand Down
10 changes: 2 additions & 8 deletions ...idator/src/test/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,10 +497,7 @@ public void createUnsignedBlock_shouldFailWhenNodeIsSyncing() {
nodeIsSyncing();
final SafeFuture<Optional<BlockContainerAndMetaData>> result =
validatorApiHandler.createUnsignedBlock(
ONE,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(ONE));
ONE, dataStructureUtil.randomSignature(), Optional.empty(), Optional.of(ONE));

assertThat(result).isCompletedExceptionally();
assertThatThrownBy(result::get).hasRootCauseInstanceOf(NodeSyncingException.class);
Expand All @@ -517,10 +514,7 @@ public void createUnsignedBlock_shouldFailWhenParentBlockIsOptimistic() {

final SafeFuture<Optional<BlockContainerAndMetaData>> result =
validatorApiHandler.createUnsignedBlock(
newSlot,
dataStructureUtil.randomSignature(),
Optional.empty(),
Optional.of(ONE));
newSlot, dataStructureUtil.randomSignature(), Optional.empty(), Optional.of(ONE));

assertThat(result).isCompletedExceptionally();
assertThatThrownBy(result::get).hasRootCauseInstanceOf(NodeSyncingException.class);
Expand Down
21 changes: 7 additions & 14 deletions ...ntegration-test/java/tech/pegasys/teku/beaconrestapi/v3/GetNewBlockV3IntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ void shouldGetUnBlindedBeaconBlockAsJson() throws Exception {
dataStructureUtil.randomBlockContainerAndMetaData(ONE);
final BLSSignature signature =
blockContainerAndMetaData.blockContainer().getBlock().getBody().getRandaoReveal();
when(validatorApiChannel.createUnsignedBlock(
eq(UInt64.ONE), eq(signature), any(), any()))
when(validatorApiChannel.createUnsignedBlock(eq(UInt64.ONE), eq(signature), any(), any()))
.thenReturn(SafeFuture.completedFuture(Optional.of(blockContainerAndMetaData)));
Response response = get(signature, ContentTypes.JSON);
assertResponseWithHeaders(
Expand All @@ -100,8 +99,7 @@ void shouldGetUnblindedBeaconBlockAsSsz() throws IOException {
dataStructureUtil.randomBlockContainerAndMetaData(ONE);
final BLSSignature signature =
blockContainerAndMetaData.blockContainer().getBlock().getBody().getRandaoReveal();
when(validatorApiChannel.createUnsignedBlock(
eq(UInt64.ONE), eq(signature), any(), any()))
when(validatorApiChannel.createUnsignedBlock(eq(UInt64.ONE), eq(signature), any(), any()))
.thenReturn(SafeFuture.completedFuture(Optional.of(blockContainerAndMetaData)));
Response response = get(signature, ContentTypes.OCTET_STREAM);
assertResponseWithHeaders(
Expand All @@ -123,8 +121,7 @@ void shouldGetBlindedBeaconBlockAsJson() throws Exception {
dataStructureUtil.randomBlindedBlockContainerAndMetaData(ONE);
final BLSSignature signature =
blockContainerAndMetaData.blockContainer().getBlock().getBody().getRandaoReveal();
when(validatorApiChannel.createUnsignedBlock(
eq(UInt64.ONE), eq(signature), any(), any()))
when(validatorApiChannel.createUnsignedBlock(eq(UInt64.ONE), eq(signature), any(), any()))
.thenReturn(SafeFuture.completedFuture(Optional.of(blockContainerAndMetaData)));
Response response = get(signature, ContentTypes.JSON);
assertResponseWithHeaders(
Expand All @@ -148,8 +145,7 @@ void shouldGetBlindedBeaconBlockAsSsz() throws IOException {
final BeaconBlock blindedBeaconBlock = blockContainerAndMetaData.blockContainer().getBlock();
final BLSSignature signature =
blockContainerAndMetaData.blockContainer().getBlock().getBody().getRandaoReveal();
when(validatorApiChannel.createUnsignedBlock(
eq(UInt64.ONE), eq(signature), any(), any()))
when(validatorApiChannel.createUnsignedBlock(eq(UInt64.ONE), eq(signature), any(), any()))
.thenReturn(SafeFuture.completedFuture(Optional.of(blockContainerAndMetaData)));
Response response = get(signature, ContentTypes.OCTET_STREAM);
assertResponseWithHeaders(
Expand All @@ -172,8 +168,7 @@ void shouldGetUnBlindedBlockContentPostDenebAsJson() throws Exception {
dataStructureUtil.randomBlockContainerAndMetaData(blockContents, ONE);
final BLSSignature signature =
blockContainerAndMetaData.blockContainer().getBlock().getBody().getRandaoReveal();
when(validatorApiChannel.createUnsignedBlock(
eq(UInt64.ONE), eq(signature), any(), any()))
when(validatorApiChannel.createUnsignedBlock(eq(UInt64.ONE), eq(signature), any(), any()))
.thenReturn(SafeFuture.completedFuture(Optional.of(blockContainerAndMetaData)));
Response response = get(signature, ContentTypes.JSON);
assertResponseWithHeaders(
Expand All @@ -196,8 +191,7 @@ void shouldGetUnBlindedBlockContentPostDenebAsSsz() throws IOException {
final BlockContainerAndMetaData blockContainerAndMetaData =
dataStructureUtil.randomBlockContainerAndMetaData(blockContents, ONE);
final BLSSignature signature = blockContents.getBlock().getBody().getRandaoReveal();
when(validatorApiChannel.createUnsignedBlock(
eq(UInt64.ONE), eq(signature), any(), any()))
when(validatorApiChannel.createUnsignedBlock(eq(UInt64.ONE), eq(signature), any(), any()))
.thenReturn(SafeFuture.completedFuture(Optional.of(blockContainerAndMetaData)));
Response response = get(signature, ContentTypes.OCTET_STREAM);
assertResponseWithHeaders(
Expand All @@ -217,8 +211,7 @@ void shouldGetUnBlindedBlockContentPostDenebAsSsz() throws IOException {
void shouldFailWhenNoBlockProduced() throws IOException {
final BeaconBlock beaconBlock = dataStructureUtil.randomBeaconBlock(ONE);
final BLSSignature signature = beaconBlock.getBlock().getBody().getRandaoReveal();
when(validatorApiChannel.createUnsignedBlock(
eq(UInt64.ONE), eq(signature), any(), any()))
when(validatorApiChannel.createUnsignedBlock(eq(UInt64.ONE), eq(signature), any(), any()))
.thenReturn(SafeFuture.completedFuture(Optional.empty()));
Response response = get(signature, ContentTypes.JSON);
assertThat(response.code()).isEqualTo(SC_INTERNAL_SERVER_ERROR);
Expand Down
119 changes: 0 additions & 119 deletions .../beaconrestapi/src/test/java/tech/pegasys/teku/beaconrestapi/AbstractGetNewBlockTest.java
View file
Open in desktop

This file was deleted.

Loading