Add option pattern for Optimistic Relay mode #323
Conversation
ferranbt
requested review from
dvush,
ZanCorDX and
liamaharon
as code owners
Benchmark results for
|
| Date (UTC) | 2025-01-03T15:22:09+00:00 |
| Commit | df6ea7f04fd03007f9effef1f8bebceef1da4bb9 |
| Base SHA | 39ac5f760f8f0fd187f224de60b15121188338cb |
Significant changes
| Benchmark | Mean | Status |
|---|---|---|
| gather_nodes_shared_cache | -2.25% | Performance has improved. |
| gather_nodes_account_trie | 7.39% | Performance has degraded. |
| gather_nodes_big_changes_account | 11.73% | Performance has degraded. |
| MEV-Boost SubmitBlock serialization/JSON encoding | -36.62% | Performance has improved. |
| pub optimisitic_config: Option<OptimisticConfig>, | ||
| pub bid_observer: Box<dyn BidObserver + Send + Sync>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct OptimisticConfig { | ||
| pub signer: BLSBlockSigner, | ||
| pub max_bid_value: U256, | ||
| pub prevalidate_optimistic_blocks: bool, | ||
| } |
There was a problem hiding this comment.
Some doc comments on what the optimistic config and its behavior would be nice.
| pub optimistic_enabled: bool, | ||
| pub optimistic_signer: BLSBlockSigner, | ||
| pub optimistic_max_bid_value: U256, | ||
| pub optimistic_prevalidate_optimistic_blocks: bool, | ||
|
|
||
| pub optimisitic_config: Option<OptimisticConfig>, |
There was a problem hiding this comment.
Previously optimistic config fields were mandatory, what's the rationale making them optional?
There was a problem hiding this comment.
The relay optimistic functionality is optional, only enabled if optimistic_enabled is set to true. When it is set to false, those fields have default null values. That to me is kind of the same as an Option config.
| if submission_optimistic { | ||
| let can_submit = if config.optimistic_prevalidate_optimistic_blocks { | ||
| if let Some(optimistic_signed_submission) = &optimistic_signed_submission { | ||
| let optimistic_config = optimistic_config.expect("optimistic config is not None"); |
There was a problem hiding this comment.
expect always makes me a bit nervous. it might hold true for now, but it's not guaranteed to hold in the future.
maybe ln 257 can return Some((res, optimistic_config)), which will bring the safely unwrapped config into scope here?
There was a problem hiding this comment.
That is a nice pattern :) Fixed!
📝 Summary
This PR introduces a new
OptimisitcConfigfor the relayer with an option pattern for its fields. This replaces the currentoptimistic_enabledboolean to signal whether to use the subset of Optimistic configs or not.💡 Motivation and Context
✅ I have completed the following steps:
make lintmake test