Conversation
PR Review: Add optimistic collateral
Working on it... |
|
Claude finished @ZanCorDX's task in 2m 18s —— View job PR Review: Add optimistic collateral
The PR adds a new Issues raised inline:
Additional notes:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if relay.optimistic_v3() | ||
| && relay | ||
| .optimistic_collateral() | ||
| .is_none_or(|cap| bid_value <= cap) | ||
| { |
There was a problem hiding this comment.
Consider adding a trace/debug log when optimistic v3 is disabled due to the collateral cap. Silent downgrades from optimistic v3 to regular submission will be difficult to diagnose operationally.
Something like:
if relay.optimistic_v3() {
if relay.optimistic_collateral().is_some_and(|cap| bid_value > cap) {
trace!(
bid_value = %bid_value,
cap = %relay.optimistic_collateral().unwrap(),
"Optimistic V3 disabled: bid exceeds collateral cap"
);
} else if let Some(config) = optimistic_v3_config {
// ... existing logic
}
}
crates/rbuilder/src/mev_boost/mod.rs
Outdated
| /// Do not use optimistic V3 when bid value is above this (ETH). None -> no cap. | ||
| #[serde(default)] | ||
| pub optimistic_collateral_eth: Option<String>, |
There was a problem hiding this comment.
Naming nit: optimistic_collateral_eth suggests it applies to all optimistic modes, but it only gates optimistic_v3. Consider a more specific name like optimistic_v3_collateral_eth or optimistic_v3_max_bid_eth to avoid confusion with the existing optimistic flag.
|
Claude finished @ZanCorDX's task in 2m 38s —— View job PR Review: Add optimistic collateral (re-review after update)
The updated PR addresses the naming feedback from the first review — Issues raised inline:
|
| } else { | ||
| if let Some(config) = optimistic_v3_config { | ||
| optimistic_v3 = create_optimistic_v3_request( | ||
| &config.builder_url, | ||
| request.as_ref(), | ||
| maybe_adjustment_data, | ||
| relay.optimistic_v3_bid_adjustment_required(), | ||
| ) | ||
| .map(|request| (config.clone(), SubmitHeaderRequestWithMetadata { | ||
| submission: request, | ||
| metadata: bid_metadata.clone() | ||
| })) | ||
| .inspect_err(|error| { | ||
| error!(parent: submission_span, ?error, "Unable to create optimistic V3 request"); | ||
| }) | ||
| .ok(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: else { if ... } can be collapsed into else if ... to reduce nesting. This is also a clippy::collapsible_else_if lint.
| } else { | |
| if let Some(config) = optimistic_v3_config { | |
| optimistic_v3 = create_optimistic_v3_request( | |
| &config.builder_url, | |
| request.as_ref(), | |
| maybe_adjustment_data, | |
| relay.optimistic_v3_bid_adjustment_required(), | |
| ) | |
| .map(|request| (config.clone(), SubmitHeaderRequestWithMetadata { | |
| submission: request, | |
| metadata: bid_metadata.clone() | |
| })) | |
| .inspect_err(|error| { | |
| error!(parent: submission_span, ?error, "Unable to create optimistic V3 request"); | |
| }) | |
| .ok(); | |
| } | |
| } | |
| } else if let Some(config) = optimistic_v3_config { | |
| optimistic_v3 = create_optimistic_v3_request( | |
| &config.builder_url, | |
| request.as_ref(), | |
| maybe_adjustment_data, | |
| relay.optimistic_v3_bid_adjustment_required(), | |
| ) | |
| .map(|request| (config.clone(), SubmitHeaderRequestWithMetadata { | |
| submission: request, | |
| metadata: bid_metadata.clone() | |
| })) | |
| .inspect_err(|error| { | |
| error!(parent: submission_span, ?error, "Unable to create optimistic V3 request"); | |
| }) | |
| .ok(); | |
| } |
| .optimistic_v3_max_bid() | ||
| .is_some_and(|cap| bid_value > cap) | ||
| { | ||
| info!( |
There was a problem hiding this comment.
info! will fire for every bid above the cap on every block submission cycle. Given that high-value bids may be common in practice, this could produce significant log volume. Consider using trace! or debug! instead, matching the silent approach used by the existing max_bid check on line 411.
| /// 0. We divide relays into optimistic and non-optimistic (defined in config file) | ||
| /// 1. We schedule submissions with non-optimistic key for all non-optimistic relays. | ||
| /// 1.1 If "optimistic_enabled" is false or bid_value >= "optimistic_max_bid_value" we schedule submissions with non-optimistic key | ||
| /// 1.1 If "optimistic_enabled" is false or bid_value >= "optimistic_v3_max_bid_eth" we schedule submissions with non-optimistic key |
There was a problem hiding this comment.
Minor: this doc comment says >= but the actual code on line 432 uses > (strictly greater). Since this PR touched this line to rename the field, it would be a good opportunity to fix the inconsistency.
| /// 1.1 If "optimistic_enabled" is false or bid_value >= "optimistic_v3_max_bid_eth" we schedule submissions with non-optimistic key | |
| /// 1.1 If "optimistic_enabled" is false or bid_value > "optimistic_v3_max_bid_eth" we schedule submissions with non-optimistic key |
|
Claude finished @ZanCorDX's task in 1m 40s —— View job PR Review: Add optimistic collateral (re-review after latest push)
The latest push addresses the Two outstanding issues from previous review, raised inline again:
Both are minor but worth addressing before merge. |
| /// 0. We divide relays into optimistic and non-optimistic (defined in config file) | ||
| /// 1. We schedule submissions with non-optimistic key for all non-optimistic relays. | ||
| /// 1.1 If "optimistic_enabled" is false or bid_value >= "optimistic_max_bid_value" we schedule submissions with non-optimistic key | ||
| /// 1.1 If "optimistic_enabled" is false or bid_value >= "optimistic_v3_max_bid_eth" we schedule submissions with non-optimistic key |
There was a problem hiding this comment.
The doc comment still says >= but the code on line 432 uses > (strictly greater than). This was flagged in the previous review. Please either update the doc to match the code or vice versa.
| /// 1.1 If "optimistic_enabled" is false or bid_value >= "optimistic_v3_max_bid_eth" we schedule submissions with non-optimistic key | |
| /// 1.1 If "optimistic_enabled" is false or bid_value > "optimistic_v3_max_bid_eth" we schedule submissions with non-optimistic key |
| if relay | ||
| .optimistic_v3_max_bid() | ||
| .is_some_and(|cap| bid_value > cap) | ||
| { | ||
| info!( | ||
| bid_value = format_ether(bid_value), | ||
| cap = format_ether(relay.optimistic_v3_max_bid().unwrap()), | ||
| "Optimistic V3 disabled: bid exceeds max bid cap" | ||
| ); |
There was a problem hiding this comment.
Two issues here:
-
Log level:
info!will fire on every bid above the cap per submission cycle. This was flagged in the previous review —debug!ortrace!would be more appropriate, consistent with the silentmax_bidcheck on line 411. -
Redundant getter +
unwrap:relay.optimistic_v3_max_bid()is called twice — once inis_some_andand again with.unwrap(). Usingif let Some(cap)avoids both the duplicate call and theunwrap:
if let Some(cap) = relay.optimistic_v3_max_bid() {
if bid_value > cap {
debug!(
bid_value = format_ether(bid_value),
cap = format_ether(cap),
"Optimistic V3 disabled: bid exceeds max bid cap"
);
} else if let Some(config) = optimistic_v3_config {
// ... existing logic
}
} else if let Some(config) = optimistic_v3_config {
// ... existing logic
}Or keep the current structure but bind the cap:
if let Some(cap) = relay.optimistic_v3_max_bid().filter(|&cap| bid_value > cap) {
debug!(
bid_value = format_ether(bid_value),
cap = format_ether(cap),
"Optimistic V3 disabled: bid exceeds max bid cap"
);
} else if let Some(config) = optimistic_v3_config {1 participant
📝 Summary
Added a new cfg to the relays: optimistic_collateral_eth.
When set, if the bid is > optimistic_collateral_eth it will disable optimistic v3.
This will allow the relays to treat the bid as a non optimistic one instead of droping it.
✅ I have completed the following steps:
make lintmake test