Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ private RemoteBid createRemoteBid(
final SignedExecutionPayloadBid bid, final BuilderEntry builderEntry) {
final UInt64 valueInGwei =
getBidValueInGwei(bid, builderEntry.getMaxExecutionPayment(), builderEntry.getUrl());
return new RemoteBid(bid, valueInGwei, Optional.of(builderEntry.getUrl()));
return new RemoteBid(bid, valueInGwei, Optional.of(builderEntry));
}

// For bids received via the builder API, the total bid
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@
import tech.pegasys.teku.infrastructure.async.SafeFuture;
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
import tech.pegasys.teku.spec.datastructures.builder.versions.gloas.BuilderConfig;
import tech.pegasys.teku.spec.datastructures.builder.versions.gloas.BuilderEntry;
import tech.pegasys.teku.spec.datastructures.epbs.versions.gloas.SignedExecutionPayloadBid;
import tech.pegasys.teku.spec.datastructures.execution.GetPayloadResponse;
import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
import tech.pegasys.teku.statetransition.OperationAddedSubscriber;
import tech.pegasys.teku.statetransition.execution.ExecutionPayloadBidManager.BidForBlock;
import tech.pegasys.teku.statetransition.validation.InternalValidationResult;

public interface ExecutionPayloadBidManager {
Expand Down Expand Up @@ -76,11 +76,23 @@ record LocalBid(
SignedExecutionPayloadBid bid, UInt256 valueInWei, boolean shouldOverrideBuilder) {}

// can represent both P2P bids and Builder API bids
record RemoteBid(SignedExecutionPayloadBid bid, UInt64 valueInGwei, Optional<String> builderUrl) {
record RemoteBid(
SignedExecutionPayloadBid bid,
UInt64 valueInGwei,
// if Builder API was used for retrieving the bid, this value would be present
Optional<BuilderEntry> builderEntry) {

public UInt64 builderIndex() {
return bid.getMessage().getBuilderIndex();
}

public UInt64 builderBoostFactor(final BuilderConfig builderConfig) {
// if Builder API is used, use the configured builder_boost_factor
return builderEntry
.map(BuilderEntry::getBuilderBoostFactor)
// fallback to top-level builder_boost_factor (applies to P2P bids)
.orElse(builderConfig.getBuilderBoostFactor());
}
}

// The best bid determined for the block proposal after evaluating local bids, P2P bids, and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,19 @@
import static tech.pegasys.teku.infrastructure.logging.LogFormatter.formatAbbreviatedHashRoot;
import static tech.pegasys.teku.spec.constants.EthConstants.GWEI_TO_WEI;

import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.tuweni.bytes.Bytes32;
import org.apache.tuweni.units.bigints.UInt256;
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
import tech.pegasys.teku.spec.datastructures.builder.versions.gloas.BuilderConfig;
import tech.pegasys.teku.spec.datastructures.builder.versions.gloas.BuilderEntry;
import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
import tech.pegasys.teku.spec.executionlayer.BuilderBoostFactorEvaluator;
import tech.pegasys.teku.spec.executionlayer.BuilderBoostFactorFormatter;
Expand All @@ -41,9 +44,6 @@ public class ExecutionPayloadBidSelector {
private final boolean useShouldOverrideBuilderFlag;
private final ExecutionPayloadBidCircuitBreaker executionPayloadBidCircuitBreaker;

private static final Comparator<RemoteBid> REMOTE_BID_BY_VALUE_ASCENDING =
Comparator.comparing(RemoteBid::valueInGwei);

public ExecutionPayloadBidSelector(
final boolean useShouldOverrideBuilderFlag,
final ExecutionPayloadBidCircuitBreaker executionPayloadBidCircuitBreaker) {
Expand All @@ -53,9 +53,9 @@ public ExecutionPayloadBidSelector(

/**
* Selects the highest-value bid from p2p and builder bids. P2P bids are filtered by parent root,
* parent block hash, min bid, and {@code isBuilderAllowed}; builder bids are filtered by {@code
* isBuilderAllowed} only because all validation is done during fetching the bids. On equal value,
* the builder bid is preferred.
* parent block hash, min bid, and {@code isBuilderAllowed}; builder bids are filtered by min bid
* and {@code isBuilderAllowed} only because all validation is done during fetching the bids. On
* equal value, the builder bid is preferred.
*/
public Optional<RemoteBid> selectBestRemoteBid(
final Set<RemoteBid> p2pBids,
Expand All @@ -64,34 +64,42 @@ public Optional<RemoteBid> selectBestRemoteBid(
final Bytes32 parentBlockHash,
final BeaconState state,
final BuilderConfig builderConfig) {
final Optional<RemoteBid> bestP2PBid =
p2pBids.stream()
.filter(bid -> bid.bid().getMessage().getParentBlockRoot().equals(parentRoot))
.filter(bid -> bid.bid().getMessage().getParentBlockHash().equals(parentBlockHash))
.filter(
bid ->
executionPayloadBidCircuitBreaker.isBuilderAllowed(bid.builderIndex(), state))
// A bid is eligible only if `bid_score >= min_bid
.filter(bid -> bid.valueInGwei().isGreaterThanOrEqualTo(builderConfig.getMinBid()))
.max(REMOTE_BID_BY_VALUE_ASCENDING);

final Optional<RemoteBid> bestBuilderBid =
builderBids.stream()
.filter(
bid ->
executionPayloadBidCircuitBreaker.isBuilderAllowed(bid.builderIndex(), state))
.max(REMOTE_BID_BY_VALUE_ASCENDING);

if (bestBuilderBid.isEmpty()) {
return bestP2PBid;
}
if (bestP2PBid.isEmpty()) {
return bestBuilderBid;
}
// on equal value, prefer the builder bid
return bestBuilderBid.get().valueInGwei().isGreaterThanOrEqualTo(bestP2PBid.get().valueInGwei())
? bestBuilderBid
: bestP2PBid;
final Predicate<RemoteBid> circuitBreakerPredicate =
bid -> executionPayloadBidCircuitBreaker.isBuilderAllowed(bid.builderIndex(), state);
// A remote bid is eligible only if `bid_score >= min_bid
final Predicate<RemoteBid> minBidPredicate =
bid -> {
final UInt64 minBid =
bid.builderEntry()
// if Builder API is used, use the configured min_bid
.map(BuilderEntry::getMinBid)
// fallback to top-level min_bid (applies to P2P bids)
.orElse(builderConfig.getMinBid());
return bid.valueInGwei().isGreaterThanOrEqualTo(minBid);
};
final List<RemoteBid> eligibleRemoteBids = new ArrayList<>();
// Add eligible builder bids
builderBids.stream()
.filter(circuitBreakerPredicate)
.filter(minBidPredicate)
.forEach(eligibleRemoteBids::add);
// Add eligible p2p bids
p2pBids.stream()
.filter(bid -> bid.bid().getMessage().getParentBlockRoot().equals(parentRoot))
.filter(bid -> bid.bid().getMessage().getParentBlockHash().equals(parentBlockHash))
.filter(circuitBreakerPredicate)
.filter(minBidPredicate)
.forEach(eligibleRemoteBids::add);
// selecting the highest bid value based on their boosted values
final Comparator<RemoteBid> remoteBidByBoostedValueAscending =
Comparator.comparing(
bid -> {
final UInt64 builderBoostFactor = bid.builderBoostFactor(builderConfig);
return bid.valueInGwei()
.bigIntegerValue()
.multiply(builderBoostFactor.bigIntegerValue());
});
return eligibleRemoteBids.stream().max(remoteBidByBoostedValueAscending);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Zero boost factor collapses remote ranking

Medium Severity

selectBestRemoteBid ranks remotes only by valueInGwei times builderBoostFactor. PREFER_EXECUTION is 0, so every such bid scores 0 and max returns an arbitrary winner. When the local payload is unavailable, that remote is used unconditionally, so a lower-value bid can be proposed.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b92f08b. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tbenr I think this whole class needs a different approach, we can pass all remote bids to selectBestBidForBlock and do the boosted ranking there, however this makes the change slightly larger in that case

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ye that's an edgy thing. maybe we can return a pair of bids (best "raw" and best "boosted") so we can can consider the raw only during local -> remote fallback.

anyway latest changes seems to capture well the most common configuration. If that idea adds complexity (assuming is a good direction) we can add it in a followup PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah in both cases, it's bit edgy, we can go with this for now, at least for testnets and readjust later

}

/**
Expand Down Expand Up @@ -141,7 +149,8 @@ public BidForBlock selectBestBidForBlock(
return selectLocalBid(localBid, slot);
}

final UInt64 builderBoostFactor = builderConfig.getBuilderBoostFactor();
final UInt64 builderBoostFactor = remoteBid.builderBoostFactor(builderConfig);

final boolean localValueWins =
BuilderBoostFactorEvaluator.isLocalValueWinning(
localBid.valueInWei(), remoteValueInWei, builderBoostFactor);
Expand Down Expand Up @@ -169,7 +178,8 @@ private BidForBlock selectRemoteBid(
remoteBid.builderIndex(),
formatAbbreviatedHashRoot(remoteBid.bid().getMessage().getBlockHash()),
slot);
return new BidForBlock(remoteBid.bid(), remoteValueInWei, remoteBid.builderUrl());
return new BidForBlock(
remoteBid.bid(), remoteValueInWei, remoteBid.builderEntry().map(BuilderEntry::getUrl));
}

private void logValueComparison(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ void includesBidsFromConfiguredBuilders() {

assertThat(result).map(RemoteBid::bid).containsExactly(firstBid, secondBid);
assertThat(result)
.map(RemoteBid::builderUrl)
.map(RemoteBid::builderEntry)
.containsExactly(
Optional.of(builderConfig.getBuilders().get(0).getUrl()),
Optional.of(builderConfig.getBuilders().get(1).getUrl()));
Optional.of(builderConfig.getBuilders().get(0)),
Optional.of(builderConfig.getBuilders().get(1)));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@
import static tech.pegasys.teku.spec.executionlayer.BuilderBoostFactorEvaluator.BUILDER_BOOST_FACTOR_PREFER_BUILDER;
import static tech.pegasys.teku.spec.executionlayer.BuilderBoostFactorEvaluator.BUILDER_BOOST_FACTOR_PREFER_EXECUTION;
import static tech.pegasys.teku.spec.schemas.ApiSchemas.BUILDER_CONFIG_SCHEMA;
import static tech.pegasys.teku.spec.schemas.ApiSchemas.BUILDER_ENTRY_SCHEMA;

import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import org.apache.tuweni.bytes.Bytes;
import org.apache.tuweni.bytes.Bytes32;
import org.apache.tuweni.units.bigints.UInt256;
import org.junit.jupiter.api.Test;
Expand All @@ -34,6 +37,7 @@
import tech.pegasys.teku.spec.Spec;
import tech.pegasys.teku.spec.TestSpecFactory;
import tech.pegasys.teku.spec.datastructures.builder.versions.gloas.BuilderConfig;
import tech.pegasys.teku.spec.datastructures.builder.versions.gloas.BuilderEntry;
import tech.pegasys.teku.spec.datastructures.epbs.versions.gloas.ExecutionPayloadBid;
import tech.pegasys.teku.spec.datastructures.epbs.versions.gloas.ExecutionPayloadBidSchema;
import tech.pegasys.teku.spec.datastructures.epbs.versions.gloas.SignedExecutionPayloadBid;
Expand Down Expand Up @@ -452,6 +456,65 @@ void logsBuilderBoostFactorsWithExpectedFormatting() {
}
}

@Test
void selectBestRemoteBidFiltersBuilderApiBidsBelowEntryMinBid() {
final UInt64 slot = UInt64.valueOf(10);
final Bytes32 parentRoot = dataStructureUtil.randomBytes32();
final Bytes32 parentBlockHash = dataStructureUtil.randomBytes32();
// top-level minBid is 50 but the builder entry's minBid is 100
final BuilderConfig builderConfig =
BUILDER_CONFIG_SCHEMA.create(UInt64.valueOf(50), UInt64.valueOf(100), List.of());
final BuilderEntry builderEntry = createBuilderEntry(UInt64.valueOf(100), UInt64.valueOf(100));
// bid value (75) is above top-level min but below entry-level min
final SignedExecutionPayloadBid belowEntryMinBid =
createBid(slot, parentRoot, parentBlockHash, UInt64.valueOf(75));
final SignedExecutionPayloadBid atEntryMinBid =
createBid(slot, parentRoot, parentBlockHash, UInt64.valueOf(100));
when(circuitBreaker.isBuilderAllowed(any(), any())).thenReturn(true);

assertThat(
selector.selectBestRemoteBid(
Set.of(),
List.of(toBuilderApiBid(belowEntryMinBid, builderEntry)),
parentRoot,
parentBlockHash,
state,
builderConfig))
.isEmpty();
assertThat(
selector.selectBestRemoteBid(
Set.of(),
List.of(toBuilderApiBid(atEntryMinBid, builderEntry)),
parentRoot,
parentBlockHash,
state,
builderConfig))
.contains(toBuilderApiBid(atEntryMinBid, builderEntry));
}

@Test
void builderBoostFactorFromBuilderEntryOverridesTopLevelConfig() {
final UInt64 slot = UInt64.valueOf(10);
final SignedExecutionPayloadBid remoteBid =
createBid(
slot, dataStructureUtil.randomBytes32(), dataStructureUtil.randomBytes32(), UInt64.ONE);
// top-level config would prefer execution, but entry-level should prefer builder
final BuilderEntry builderEntry =
createBuilderEntry(UInt64.ZERO, BUILDER_BOOST_FACTOR_PREFER_BUILDER);
final BuilderConfig builderConfig =
BuilderConfig.withBuilderBoostFactor(BUILDER_BOOST_FACTOR_PREFER_EXECUTION);

final LocalBid localBid = new LocalBid(randomLocalSelfBuiltBid(slot), UInt256.MAX_VALUE, false);
final BidForBlock selectedBid =
selector.selectBestBidForBlock(
Optional.of(localBid),
Optional.of(toBuilderApiBid(remoteBid, builderEntry)),
builderConfig,
slot);

assertThat(selectedBid.bid()).isEqualTo(remoteBid);
}

private BidForBlock selectBestBidForBlock(
final SignedExecutionPayloadBid remoteBid,
final UInt256 localValue,
Expand All @@ -468,6 +531,21 @@ private RemoteBid toRemoteBid(final SignedExecutionPayloadBid bid) {
return new RemoteBid(bid, bid.getMessage().getValue(), Optional.empty());
}

private RemoteBid toBuilderApiBid(
final SignedExecutionPayloadBid bid, final BuilderEntry builderEntry) {
return new RemoteBid(bid, bid.getMessage().getValue(), Optional.of(builderEntry));
}

private BuilderEntry createBuilderEntry(final UInt64 minBid, final UInt64 builderBoostFactor) {
return BUILDER_ENTRY_SCHEMA.create(
Bytes.of("https://builder.example.com".getBytes(StandardCharsets.UTF_8)),
dataStructureUtil.randomSignedBuilderRequestAuth(),
List.of(),
UInt64.MAX_VALUE,
minBid,
builderBoostFactor);
}

private SignedExecutionPayloadBid randomLocalSelfBuiltBid(final UInt64 slot) {
final SchemaDefinitionsGloas schemaDefinitions =
SchemaDefinitionsGloas.required(spec.atSlot(slot).getSchemaDefinitions());
Expand Down
Loading