Skip to content

Commit

Permalink
deprecate always_prefer_builder_payload, builder-profit-threshold, ig…
Browse files Browse the repository at this point in the history 
…nore_builder_override_suggestion_threshold and builder_comparison_factor flags
  • Loading branch information
@eserilev
eserilev committed Jan 5, 2024
1 parent 68e76dd commit f167ffd
Show file tree
Hide file tree
Showing 12 changed files with 4 additions and 404 deletions.
2 changes: 0 additions & 2 deletions beacon_node/execution_layer/src/engine_api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,8 +408,6 @@ pub struct GetPayloadResponse<T: EthSpec> {
pub block_value: Uint256,
#[superstruct(only(Deneb))]
pub blobs_bundle: BlobsBundle<T>,
#[superstruct(only(Deneb), partial_getter(copy))]
pub should_override_builder: bool,
}

impl<E: EthSpec> GetPayloadResponse<E> {
Expand Down
3 changes: 0 additions & 3 deletions beacon_node/execution_layer/src/engine_api/json_structures.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,6 @@ pub struct JsonGetPayloadResponse<T: EthSpec> {
pub block_value: Uint256,
#[superstruct(only(V3))]
pub blobs_bundle: JsonBlobsBundleV1<T>,
#[superstruct(only(V3))]
pub should_override_builder: bool,
}

impl<T: EthSpec> From<JsonGetPayloadResponse<T>> for GetPayloadResponse<T> {
Expand All @@ -322,7 +320,6 @@ impl<T: EthSpec> From<JsonGetPayloadResponse<T>> for GetPayloadResponse<T> {
execution_payload: response.execution_payload.into(),
block_value: response.block_value,
blobs_bundle: response.blobs_bundle.into(),
should_override_builder: response.should_override_builder,
})
}
}
Expand Down
126 changes: 0 additions & 126 deletions beacon_node/execution_layer/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,10 +338,7 @@ struct Inner<E: EthSpec> {
proposers: RwLock<HashMap<ProposerKey, Proposer>>,
executor: TaskExecutor,
payload_cache: PayloadCache<E>,
builder_profit_threshold: Uint256,
log: Logger,
always_prefer_builder_payload: bool,
ignore_builder_override_suggestion_threshold: f32,
/// Track whether the last `newPayload` call errored.
///
/// This is used *only* in the informational sync status endpoint, so that a VC using this
Expand All @@ -368,11 +365,7 @@ pub struct Config {
pub jwt_version: Option<String>,
/// Default directory for the jwt secret if not provided through cli.
pub default_datadir: PathBuf,
/// The minimum value of an external payload for it to be considered in a proposal.
pub builder_profit_threshold: u128,
pub execution_timeout_multiplier: Option<u32>,
pub always_prefer_builder_payload: bool,
pub ignore_builder_override_suggestion_threshold: f32,
}

/// Provides access to one execution engine and provides a neat interface for consumption by the
Expand All @@ -382,40 +375,6 @@ pub struct ExecutionLayer<T: EthSpec> {
inner: Arc<Inner<T>>,
}

/// This function will return the percentage difference between 2 U256 values, using `base_value`
/// as the denominator. It is accurate to 7 decimal places which is about the precision of
/// an f32.
///
/// If some error is encountered in the calculation, None will be returned.
fn percentage_difference_u256(base_value: Uint256, comparison_value: Uint256) -> Option<f32> {
if base_value == Uint256::zero() {
return None;
}
// this is the total supply of ETH in WEI
let max_value = Uint256::from(12u8) * Uint256::exp10(25);
if base_value > max_value || comparison_value > max_value {
return None;
}

// Now we should be able to calculate the difference without division by zero or overflow
const PRECISION: usize = 7;
let precision_factor = Uint256::exp10(PRECISION);
let scaled_difference = if base_value <= comparison_value {
(comparison_value - base_value) * precision_factor
} else {
(base_value - comparison_value) * precision_factor
};
let scaled_proportion = scaled_difference / base_value;
// max value of scaled difference is 1.2 * 10^33, well below the max value of a u128 / f64 / f32
let percentage =
100.0f64 * scaled_proportion.low_u128() as f64 / precision_factor.low_u128() as f64;
if base_value <= comparison_value {
Some(percentage as f32)
} else {
Some(-percentage as f32)
}
}

impl<T: EthSpec> ExecutionLayer<T> {
/// Instantiate `Self` with an Execution engine specified in `Config`, using JSON-RPC via HTTP.
pub fn from_config(config: Config, executor: TaskExecutor, log: Logger) -> Result<Self, Error> {
Expand All @@ -428,10 +387,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
jwt_id,
jwt_version,
default_datadir,
builder_profit_threshold,
execution_timeout_multiplier,
always_prefer_builder_payload,
ignore_builder_override_suggestion_threshold,
} = config;

if urls.len() > 1 {
Expand Down Expand Up @@ -492,10 +448,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
execution_blocks: Mutex::new(LruCache::new(EXECUTION_BLOCKS_LRU_CACHE_SIZE)),
executor,
payload_cache: PayloadCache::default(),
builder_profit_threshold: Uint256::from(builder_profit_threshold),
log,
always_prefer_builder_payload,
ignore_builder_override_suggestion_threshold,
last_new_payload_errored: RwLock::new(false),
};

Expand Down Expand Up @@ -533,7 +486,6 @@ impl<T: EthSpec> ExecutionLayer<T> {
self.log(),
"Using external block builder";
"builder_url" => ?builder_url,
"builder_profit_threshold" => self.inner.builder_profit_threshold.as_u128(),
"local_user_agent" => builder_client.get_user_agent(),
);
self.inner.builder.swap(Some(Arc::new(builder_client)));
Expand Down Expand Up @@ -1156,10 +1108,6 @@ impl<T: EthSpec> ExecutionLayer<T> {
)));
}

if self.inner.always_prefer_builder_payload {
return ProvenancedPayload::try_from(relay.data.message);
}

let builder_boost_factor =
builder_boost_factor.unwrap_or(DEFAULT_BUILDER_BOOST_FACTOR);

Expand All @@ -1182,42 +1130,6 @@ impl<T: EthSpec> ExecutionLayer<T> {
)));
}

if boosted_relay_value < self.inner.builder_profit_threshold {
info!(
self.log(),
"Builder payload ignored";
"info" => "using local payload",
"reason" => format!("payload value of {} does not meet user-configured profit-threshold of {}", relay_value, self.inner.builder_profit_threshold),
"relay_block_hash" => ?header.block_hash(),
"parent_hash" => ?parent_hash,
"boosted_relay_value" => %boosted_relay_value,
"builder_boost_factor" => %builder_boost_factor,
);
return Ok(ProvenancedPayload::Local(BlockProposalContentsType::Full(
local.try_into()?,
)));
}

if local.should_override_builder().unwrap_or(false) {
let percentage_difference =
percentage_difference_u256(local_value, boosted_relay_value);
if percentage_difference.map_or(false, |percentage| {
percentage < self.inner.ignore_builder_override_suggestion_threshold
}) {
info!(
self.log(),
"Using local payload because execution engine suggested we ignore builder payload";
"local_block_value" => %local_value,
"relay_value" => %relay_value,
"boosted_relay_value" => %boosted_relay_value,
"builder_boost_factor" => %builder_boost_factor
);
return Ok(ProvenancedPayload::Local(BlockProposalContentsType::Full(
local.try_into()?,
)));
}
}

info!(
self.log(),
"Relay block is more profitable than local block";
Expand Down Expand Up @@ -2395,42 +2307,4 @@ mod test {
})
.await;
}

#[tokio::test]
async fn percentage_difference_u256_tests() {
// ensure function returns `None` when base value is zero
assert_eq!(percentage_difference_u256(0.into(), 1.into()), None);
// ensure function returns `None` when either value is greater than 120 Million ETH
let max_value = Uint256::from(12u8) * Uint256::exp10(25);
assert_eq!(
percentage_difference_u256(1u8.into(), max_value + Uint256::from(1u8)),
None
);
assert_eq!(
percentage_difference_u256(max_value + Uint256::from(1u8), 1u8.into()),
None
);
// it should work up to max value
assert_eq!(
percentage_difference_u256(max_value, max_value / Uint256::from(2u8)),
Some(-50f32)
);
// should work when base value is greater than comparison value
assert_eq!(
percentage_difference_u256(4u8.into(), 3u8.into()),
Some(-25f32)
);
// should work when comparison value is greater than base value
assert_eq!(
percentage_difference_u256(4u8.into(), 5u8.into()),
Some(25f32)
);
// should be accurate to 7 decimal places
let result =
percentage_difference_u256(Uint256::from(31415926u64), Uint256::from(13371337u64))
.expect("should get percentage");
// result = -57.4377116
assert!(result > -57.43772);
assert!(result <= -57.43771);
}
}
1 change: 0 additions & 1 deletion beacon_node/execution_layer/src/test_utils/handle_rpc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,6 @@ pub async fn handle_rpc<T: EthSpec>(
GENERIC_ERROR_CODE,
))?
.into(),
should_override_builder: false,
})
.unwrap()
}
Expand Down
1 change: 0 additions & 1 deletion beacon_node/execution_layer/src/test_utils/mock_execution_layer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ impl<T: EthSpec> MockExecutionLayer<T> {
execution_endpoints: vec![url],
secret_files: vec![path],
suggested_fee_recipient: Some(Address::repeat_byte(42)),
builder_profit_threshold: builder_threshold.unwrap_or(DEFAULT_BUILDER_THRESHOLD_WEI),
..Default::default()
};
let el =
Expand Down
80 changes: 0 additions & 80 deletions beacon_node/http_api/tests/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4636,70 +4636,6 @@ impl ApiTester {
self
}

pub async fn test_payload_rejects_inadequate_builder_threshold(self) -> Self {
// Mutate value.
self.mock_builder
.as_ref()
.unwrap()
.add_operation(Operation::Value(Uint256::from(
DEFAULT_BUILDER_THRESHOLD_WEI - 1,
)));

let slot = self.chain.slot().unwrap();
let epoch = self.chain.epoch().unwrap();

let (_, randao_reveal) = self.get_test_randao(slot, epoch).await;

let payload: BlindedPayload<E> = self
.client
.get_validator_blinded_blocks::<E>(slot, &randao_reveal, None)
.await
.unwrap()
.data
.body()
.execution_payload()
.unwrap()
.into();

// If this cache is populated, it indicates fallback to the local EE was correctly used.
assert!(self
.chain
.execution_layer
.as_ref()
.unwrap()
.get_payload_by_root(&payload.tree_hash_root())
.is_some());
self
}

pub async fn test_payload_v3_rejects_inadequate_builder_threshold(self) -> Self {
// Mutate value.
self.mock_builder
.as_ref()
.unwrap()
.add_operation(Operation::Value(Uint256::from(
DEFAULT_BUILDER_THRESHOLD_WEI - 1,
)));

let slot = self.chain.slot().unwrap();
let epoch = self.chain.epoch().unwrap();

let (_, randao_reveal) = self.get_test_randao(slot, epoch).await;

let (payload_type, _) = self
.client
.get_validator_blocks_v3::<E>(slot, &randao_reveal, None, None)
.await
.unwrap();

match payload_type.data {
ProduceBlockV3Response::Full(_) => (),
ProduceBlockV3Response::Blinded(_) => panic!("Expecting a full payload"),
};

self
}

pub async fn test_builder_payload_chosen_when_more_profitable(self) -> Self {
// Mutate value.
self.mock_builder
Expand Down Expand Up @@ -6300,22 +6236,6 @@ async fn builder_chain_health_optimistic_head_v3() {
.await;
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn builder_inadequate_builder_threshold() {
ApiTester::new_mev_tester()
.await
.test_payload_rejects_inadequate_builder_threshold()
.await;
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn builder_inadequate_builder_threshold_v3() {
ApiTester::new_mev_tester()
.await
.test_payload_v3_rejects_inadequate_builder_threshold()
.await;
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn builder_payload_chosen_by_profit() {
ApiTester::new_mev_tester_no_builder_threshold()
Expand Down
42 changes: 0 additions & 42 deletions beacon_node/src/cli.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1127,38 +1127,6 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
conditions.")
.takes_value(false)
)
.arg(
Arg::with_name("builder-profit-threshold")
.long("builder-profit-threshold")
.value_name("WEI_VALUE")
.help("The minimum reward in wei provided to the proposer by a block builder for \
an external payload to be considered for inclusion in a proposal. If this \
threshold is not met, the local EE's payload will be used. This is currently \
*NOT* in comparison to the value of the local EE's payload. It simply checks \
whether the total proposer reward from an external payload is equal to or \
greater than this value. In the future, a comparison to a local payload is \
likely to be added. Example: Use 250000000000000000 to set the threshold to \
0.25 ETH.")
.default_value("0")
.takes_value(true)
)
.arg(
Arg::with_name("ignore-builder-override-suggestion-threshold")
.long("ignore-builder-override-suggestion-threshold")
.value_name("PERCENTAGE")
.help("When the EE advises Lighthouse to ignore the builder payload, this flag \
specifies a percentage threshold for the difference between the reward from \
the builder payload and the local EE's payload. This threshold must be met \
for Lighthouse to consider ignoring the EE's suggestion. If the reward from \
the builder's payload doesn't exceed the local payload by at least this \
percentage, the local payload will be used. The conditions under which the \
EE may make this suggestion depend on the EE's implementation, with the \
primary intent being to safeguard against potential censorship attacks \
from builders. Setting this flag to 0 will cause Lighthouse to always \
ignore the EE's suggestion. Default: 10.0 (equivalent to 10%).")
.default_value("10.0")
.takes_value(true)
)
.arg(
Arg::with_name("builder-user-agent")
.long("builder-user-agent")
Expand Down Expand Up @@ -1205,16 +1173,6 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
This enables --http and --validator-monitor-auto and enables SSE logging.")
.takes_value(false)
)
.arg(
Arg::with_name("always-prefer-builder-payload")
.long("always-prefer-builder-payload")
.help("If set, the beacon node always uses the payload from the builder instead of the local payload.")
// The builder profit threshold flag is used to provide preference
// to local payloads, therefore it fundamentally conflicts with
// always using the builder.
.conflicts_with("builder-profit-threshold")
.conflicts_with("ignore-builder-override-suggestion-threshold")
)
.arg(
Arg::with_name("invalid-gossip-verified-blocks-path")
.long("invalid-gossip-verified-blocks-path")
Expand Down
6 changes: 0 additions & 6 deletions beacon_node/src/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,12 +317,6 @@ pub fn get_config<E: EthSpec>(
el_config.jwt_id = clap_utils::parse_optional(cli_args, "execution-jwt-id")?;
el_config.jwt_version = clap_utils::parse_optional(cli_args, "execution-jwt-version")?;
el_config.default_datadir = client_config.data_dir().clone();
el_config.builder_profit_threshold =
clap_utils::parse_required(cli_args, "builder-profit-threshold")?;
el_config.always_prefer_builder_payload =
cli_args.is_present("always-prefer-builder-payload");
el_config.ignore_builder_override_suggestion_threshold =
clap_utils::parse_required(cli_args, "ignore-builder-override-suggestion-threshold")?;
let execution_timeout_multiplier =
clap_utils::parse_required(cli_args, "execution-timeout-multiplier")?;
el_config.execution_timeout_multiplier = Some(execution_timeout_multiplier);
Expand Down
Loading

0 comments on commit f167ffd

Please sign in to comment.