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

Jump to bottom

Introduce option to use local payload instead of builder #6878

Merged
merged 11 commits into from
Mar 2, 2023
Merged

Introduce option to use local payload instead of builder #6878

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4f4092a
Introduce option to use local payload instead of builder if first is …
zilm13 Feb 28, 2023
9bd57ce
Merge branch 'master' into feature/payload-value-cutoff
zilm13 Feb 28, 2023
e50aa15
Add logging when we use local payload instead of builder bid
zilm13 Feb 28, 2023
f5b2e06
Add % sign
zilm13 Mar 1, 2023
851f5a5
Merge branch 'master' into feature/payload-value-cutoff
zilm13 Mar 1, 2023
5d49956
Add NEVER option to stay with Builder's bid when making block whateve…
zilm13 Mar 1, 2023
e248b16
Merge branch 'master' into feature/payload-value-cutoff
zilm13 Mar 1, 2023
68f918c
Minor winning value refactoring and descriptions cleaning
zilm13 Mar 1, 2023
966fe5d
Move feature to hidden
zilm13 Mar 1, 2023
9e775a4
Proof-reading cleanup
zilm13 Mar 2, 2023
fce617a
Merge branch 'master' into feature/payload-value-cutoff
zilm13 Mar 2, 2023
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
21 changes: 18 additions & 3 deletions ...layer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionBuilderModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
public class ExecutionBuilderModule {

private static final Logger LOG = LogManager.getLogger();
private static final int HUNDRED_PERCENTS = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldnt need to define 100%, but if we do, maybe just HUNDRED_PERCENT, as the s is confusing :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, could you explain this, please, I know you are right, but I don't understand this. Is percent always plural?


private final Spec spec;
private final AtomicBoolean latestBuilderAvailability;
Expand All @@ -58,21 +59,24 @@ public class ExecutionBuilderModule {
private final BuilderCircuitBreaker builderCircuitBreaker;
private final Optional<BuilderClient> builderClient;
private final EventLogger eventLogger;
private final int builderBidChallengePercentage;

public ExecutionBuilderModule(
final Spec spec,
final ExecutionLayerManagerImpl executionLayerManager,
final BuilderBidValidator builderBidValidator,
final BuilderCircuitBreaker builderCircuitBreaker,
final Optional<BuilderClient> builderClient,
final EventLogger eventLogger) {
final EventLogger eventLogger,
final int builderBidChallengePercentage) {
this.spec = spec;
this.latestBuilderAvailability = new AtomicBoolean(builderClient.isPresent());
this.executionLayerManager = executionLayerManager;
this.builderBidValidator = builderBidValidator;
this.builderCircuitBreaker = builderCircuitBreaker;
this.builderClient = builderClient;
this.eventLogger = eventLogger;
this.builderBidChallengePercentage = builderBidChallengePercentage;
}

private Optional<SafeFuture<HeaderWithFallbackData>> validateBuilderGetHeader(
Expand Down Expand Up @@ -164,9 +168,20 @@ public SafeFuture<HeaderWithFallbackData> builderGetHeader(
.map(ExecutionPayloadWithValue::getValue)
.orElse(UInt256.ZERO);
logReceivedBuilderBid(signedBuilderBid.getMessage());
if (signedBuilderBid.getMessage().getValue().lessOrEqualThan(localPayloadValue)) {
// 1 ETH is 10^18 wei, Uint256 max is more than 10^77
if (signedBuilderBid
.getMessage()
.getValue()
.multiply(builderBidChallengePercentage)
.lessOrEqualThan(localPayloadValue.multiply(HUNDRED_PERCENTS))) {
LOG.info(
"Using local Execution Payload instead of Builder Bid as it's challenged. "
+ "Builder bid value: {}, local block value: {}, builderBidChallengePercentage: {}",
signedBuilderBid.getMessage().getValue().toDecimalString(),
localPayloadValue.toDecimalString(),
builderBidChallengePercentage);
return getResultFromLocalExecutionPayload(
localExecutionPayload, slot, FallbackReason.LOCAL_BLOCK_VALUE_HIGHER);
localExecutionPayload, slot, FallbackReason.LOCAL_BLOCK_VALUE_WIN);
Copy link
Contributor

Choose a reason for hiding this comment

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

what about LOCAL_BLOCK_VALUE_WON ? similarly to CIRCUIT_BREAKER_ENGAGED

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was the first name. don't know why I reverted it

}
final Optional<ExecutionPayload> localPayload =
maybeLocalExecutionPayload.map(ExecutionPayloadWithValue::getExecutionPayload);
Expand Down
23 changes: 17 additions & 6 deletions ...er/src/main/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionLayerManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ public static ExecutionLayerManagerImpl create(
final MetricsSystem metricsSystem,
final BuilderBidValidator builderBidValidator,
final BuilderCircuitBreaker builderCircuitBreaker,
final BlobsBundleValidator blobsBundleValidator) {
final BlobsBundleValidator blobsBundleValidator,
final int builderBidChallengePercentage) {
final LabelledMetric<Counter> executionPayloadSourceCounter =
metricsSystem.createLabelledCounter(
TekuMetricCategory.BEACON,
Expand Down Expand Up @@ -110,7 +111,8 @@ public static ExecutionLayerManagerImpl create(
builderBidValidator,
builderCircuitBreaker,
executionPayloadSourceCounter,
blobsBundleValidator);
blobsBundleValidator,
builderBidChallengePercentage);
}

public static ExecutionLayerManagerImpl create(
Expand All @@ -121,7 +123,8 @@ public static ExecutionLayerManagerImpl create(
final MetricsSystem metricsSystem,
final BuilderBidValidator builderBidValidator,
final BuilderCircuitBreaker builderCircuitBreaker,
final BlobsBundleValidator blobsBundleValidator) {
final BlobsBundleValidator blobsBundleValidator,
final int builderBidChallengePercentage) {
final ExecutionClientHandler executionClientHandler;

if (spec.isMilestoneSupported(SpecMilestone.DENEB)) {
Expand All @@ -140,7 +143,8 @@ public static ExecutionLayerManagerImpl create(
metricsSystem,
builderBidValidator,
builderCircuitBreaker,
blobsBundleValidator);
blobsBundleValidator,
builderBidChallengePercentage);
}

public static ExecutionEngineClient createEngineClient(
Expand Down Expand Up @@ -181,14 +185,21 @@ private ExecutionLayerManagerImpl(
final BuilderBidValidator builderBidValidator,
final BuilderCircuitBreaker builderCircuitBreaker,
final LabelledMetric<Counter> executionPayloadSourceCounter,
final BlobsBundleValidator blobsBundleValidator) {
final BlobsBundleValidator blobsBundleValidator,
final int builderBidChallengePercentage) {
this.executionClientHandler = executionClientHandler;
this.spec = spec;
this.blobsBundleValidator = blobsBundleValidator;
this.executionPayloadSourceCounter = executionPayloadSourceCounter;
this.executionBuilderModule =
new ExecutionBuilderModule(
spec, this, builderBidValidator, builderCircuitBreaker, builderClient, eventLogger);
spec,
this,
builderBidValidator,
builderCircuitBreaker,
builderClient,
eventLogger,
builderBidChallengePercentage);
}

@Override
Expand Down
5 changes: 3 additions & 2 deletions ...ch/pegasys/teku/ethereum/executionlayer/ExecutionLayerBlockProductionManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ private void verifyFallbackToLocalEL(
final ExecutionPayload executionPayload = fallbackData.getExecutionPayload();
if (fallbackReason == FallbackReason.BUILDER_HEADER_NOT_AVAILABLE
|| fallbackReason == FallbackReason.BUILDER_ERROR
|| fallbackReason == FallbackReason.LOCAL_BLOCK_VALUE_HIGHER) {
|| fallbackReason == FallbackReason.LOCAL_BLOCK_VALUE_WIN) {
// we expect both builder and local engine have been called
verifyBuilderCalled(slot, executionPayloadContext);
} else {
Expand Down Expand Up @@ -469,7 +469,8 @@ private ExecutionLayerManagerImpl createExecutionLayerChannelImpl(
? new BuilderBidValidatorImpl(eventLogger)
: BuilderBidValidator.NOOP,
builderCircuitBreaker,
BlobsBundleValidator.NOOP);
BlobsBundleValidator.NOOP,
100);
}

private void updateBuilderStatus(SafeFuture<Response<Void>> builderClientResponse, UInt64 slot) {
Expand Down
84 changes: 76 additions & 8 deletions ...rc/test/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionLayerManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import tech.pegasys.teku.spec.Spec;
import tech.pegasys.teku.spec.TestSpecFactory;
import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
import tech.pegasys.teku.spec.datastructures.builder.BuilderBid;
import tech.pegasys.teku.spec.datastructures.builder.SignedBuilderBid;
import tech.pegasys.teku.spec.datastructures.execution.ExecutionPayload;
import tech.pegasys.teku.spec.datastructures.execution.ExecutionPayloadContext;
Expand Down Expand Up @@ -196,7 +197,7 @@ public void builderGetHeaderGetPayload_shouldReturnHeaderAndPayloadViaBuilder()
final BeaconState state = dataStructureUtil.randomBeaconState(slot);

final ExecutionPayloadHeader header =
prepareBuilderGetHeaderResponse(executionPayloadContext, false);
prepareBuilderGetHeaderResponse(executionPayloadContext, false).getExecutionPayloadHeader();
prepareEngineGetPayloadResponse(executionPayloadContext, UInt256.ZERO, slot);

// we expect result from the builder
Expand Down Expand Up @@ -234,9 +235,46 @@ public void builderGetHeaderGetPayload_shouldReturnHeaderAndPayloadViaEngineWhen
final UInt64 slot = executionPayloadContext.getForkChoiceState().getHeadBlockSlot();
final BeaconState state = dataStructureUtil.randomBeaconState(slot);

prepareBuilderGetHeaderResponse(executionPayloadContext, false);
final UInt256 builderValue =
prepareBuilderGetHeaderResponse(executionPayloadContext, false).getValue();
final ExecutionPayload localExecutionPayload =
prepareEngineGetPayloadResponse(executionPayloadContext, builderValue, slot);

// we expect result from the local engine
final ExecutionPayloadHeader expectedHeader =
spec.getGenesisSpec()
.getSchemaDefinitions()
.toVersionBellatrix()
.orElseThrow()
.getExecutionPayloadHeaderSchema()
.createFromExecutionPayload(localExecutionPayload);

// we expect local engine header as result
final HeaderWithFallbackData expectedResult =
HeaderWithFallbackData.create(
expectedHeader,
new FallbackData(localExecutionPayload, FallbackReason.LOCAL_BLOCK_VALUE_WIN));
assertThat(executionLayerManager.builderGetHeader(executionPayloadContext, state))
.isCompletedWithValue(expectedResult);
verifyFallbackToLocalEL(slot, executionPayloadContext, expectedResult);
}

@Test
public void builderGetHeaderGetPayload_shouldReturnEnginePayloadWhenValueLowerButChallengeWon() {
// Setup requires local payload to have at lest 50% value of builder's to win
executionLayerManager = createExecutionLayerChannelImpl(true, false, 50);
setBuilderOnline();

final ExecutionPayloadContext executionPayloadContext =
dataStructureUtil.randomPayloadExecutionContext(false, true);
final UInt64 slot = executionPayloadContext.getForkChoiceState().getHeadBlockSlot();
final BeaconState state = dataStructureUtil.randomBeaconState(slot);

final UInt256 builderValue =
prepareBuilderGetHeaderResponse(executionPayloadContext, false).getValue();
final ExecutionPayload localExecutionPayload =
prepareEngineGetPayloadResponse(executionPayloadContext, UInt256.MAX_VALUE, slot);
prepareEngineGetPayloadResponse(
executionPayloadContext, builderValue.multiply(51).divide(100), slot);
rolfyone marked this conversation as resolved.
Show resolved Hide resolved

// we expect result from the local engine
final ExecutionPayloadHeader expectedHeader =
Expand All @@ -251,12 +289,34 @@ public void builderGetHeaderGetPayload_shouldReturnHeaderAndPayloadViaEngineWhen
final HeaderWithFallbackData expectedResult =
HeaderWithFallbackData.create(
expectedHeader,
new FallbackData(localExecutionPayload, FallbackReason.LOCAL_BLOCK_VALUE_HIGHER));
new FallbackData(localExecutionPayload, FallbackReason.LOCAL_BLOCK_VALUE_WIN));
assertThat(executionLayerManager.builderGetHeader(executionPayloadContext, state))
.isCompletedWithValue(expectedResult);
verifyFallbackToLocalEL(slot, executionPayloadContext, expectedResult);
}

@Test
public void builderGetHeaderGetPayload_shouldReturnBuilderPayloadWhenBuilderWonChallenge() {
// Setup requires local payload to have at lest 50% value of builder's to win
executionLayerManager = createExecutionLayerChannelImpl(true, false, 50);
setBuilderOnline();

final ExecutionPayloadContext executionPayloadContext =
dataStructureUtil.randomPayloadExecutionContext(false, true);
final UInt64 slot = executionPayloadContext.getForkChoiceState().getHeadBlockSlot();
final BeaconState state = dataStructureUtil.randomBeaconState(slot);

final BuilderBid builderBid = prepareBuilderGetHeaderResponse(executionPayloadContext, false);
prepareEngineGetPayloadResponse(
executionPayloadContext, builderBid.getValue().multiply(49).divide(100), slot);

// we expect result from the builder
final ExecutionPayloadHeader builderHeader = builderBid.getExecutionPayloadHeader();
final HeaderWithFallbackData expectedResult = HeaderWithFallbackData.create(builderHeader);
assertThat(executionLayerManager.builderGetHeader(executionPayloadContext, state))
.isCompletedWithValue(expectedResult);
}

@Test
public void builderGetHeaderGetPayload_shouldReturnHeaderAndPayloadViaEngineOnBuilderFailure() {
setBuilderOnline();
Expand Down Expand Up @@ -577,7 +637,7 @@ void onSlot_shouldCleanUpFallbackCache() {
verifyNoMoreInteractions(executionClientHandler);
}

private ExecutionPayloadHeader prepareBuilderGetHeaderResponse(
private BuilderBid prepareBuilderGetHeaderResponse(
final ExecutionPayloadContext executionPayloadContext, final boolean prepareEmptyResponse) {
final UInt64 slot = executionPayloadContext.getForkChoiceState().getHeadBlockSlot();

Expand All @@ -599,7 +659,7 @@ private ExecutionPayloadHeader prepareBuilderGetHeaderResponse(
.orElseThrow(),
executionPayloadContext.getParentHash());

return signedBuilderBid.getMessage().getExecutionPayloadHeader();
return signedBuilderBid.getMessage();
}

private void verifyFallbackToLocalEL(
Expand All @@ -612,7 +672,7 @@ private void verifyFallbackToLocalEL(
final ExecutionPayload executionPayload = fallbackData.getExecutionPayload();
if (fallbackReason == FallbackReason.BUILDER_HEADER_NOT_AVAILABLE
|| fallbackReason == FallbackReason.BUILDER_ERROR
|| fallbackReason == FallbackReason.LOCAL_BLOCK_VALUE_HIGHER) {
|| fallbackReason == FallbackReason.LOCAL_BLOCK_VALUE_WIN) {
// we expect both builder and local engine have been called
verifyBuilderCalled(slot, executionPayloadContext);
} else {
Expand Down Expand Up @@ -683,6 +743,13 @@ private ExecutionPayload prepareEngineGetPayloadResponse(

private ExecutionLayerManagerImpl createExecutionLayerChannelImpl(
final boolean builderEnabled, final boolean builderValidatorEnabled) {
return createExecutionLayerChannelImpl(builderEnabled, builderValidatorEnabled, 100);
}

private ExecutionLayerManagerImpl createExecutionLayerChannelImpl(
final boolean builderEnabled,
final boolean builderValidatorEnabled,
final int builderBidChallengePercentaqe) {
when(builderCircuitBreaker.isEngaged(any())).thenReturn(false);
return ExecutionLayerManagerImpl.create(
eventLogger,
Expand All @@ -694,7 +761,8 @@ private ExecutionLayerManagerImpl createExecutionLayerChannelImpl(
? new BuilderBidValidatorImpl(eventLogger)
: BuilderBidValidator.NOOP,
builderCircuitBreaker,
BlobsBundleValidator.NOOP);
BlobsBundleValidator.NOOP,
builderBidChallengePercentaqe);
}

private void updateBuilderStatus(final SafeFuture<Response<Void>> builderClientResponse) {
Expand Down
2 changes: 1 addition & 1 deletion ...um/spec/src/main/java/tech/pegasys/teku/spec/datastructures/execution/FallbackReason.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public enum FallbackReason {
BUILDER_NOT_AVAILABLE("builder_not_available"),
BUILDER_NOT_CONFIGURED("builder_not_configured"),
BUILDER_HEADER_NOT_AVAILABLE("builder_header_not_available"),
LOCAL_BLOCK_VALUE_HIGHER("local_block_value_higher"),
LOCAL_BLOCK_VALUE_WIN("local_block_value_win"),
BUILDER_ERROR("builder_error"),
NONE("");

Expand Down
6 changes: 5 additions & 1 deletion ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,11 @@ public BuilderBid randomBuilderBid(final BLSPublicKey builderPublicKey) {
return SchemaDefinitionsBellatrix.required(spec.getGenesisSchemaDefinitions())
.getBuilderBidSchema()
.create(
randomExecutionPayloadHeader(spec.getGenesisSpec()), randomUInt256(), builderPublicKey);
randomExecutionPayloadHeader(spec.getGenesisSpec()),
// 1 ETH is 10^18 wei, Uint256 max is more than 10^77, so just to avoid overflows in
// computation
randomUInt256().divide(1000),
builderPublicKey);
}

public BuilderBid randomBuilderBid(final Bytes32 withdrawalsRoot) {
Expand Down
Loading