Conversation
akundaz
requested review from
0x416e746f6e,
SozinM and
avalonche
as code owners
akundaz
force-pushed
the
ash-ursmuxwvvpvt
branch
2 times, most recently
from
9658148 to
3d97dbe
Compare
akundaz
force-pushed
the
ash-ursmuxwvvpvt
branch
from
3d97dbe to
85520f1
Compare
akundaz
changed the title
akundaz
force-pushed
the
ash-ursmuxwvvpvt
branch
from
9877893 to
9d4cf05
Compare
There was a problem hiding this comment.
I have a slight feeling that this whole module adds some unnecessary complexity. In the current form it would be better to keep validation + response at the rpc level and only spawn this "pool presim" worker as a post-hook after pool insertion. If the goal is to have an invariant every bundle need to be simulated before being added to the pool, then it would be better to first implement changes to make that possible.
| long = "builder.presim-max-concurrency", | ||
| env = "PRESIM_MAX_CONCURRENCY" | ||
| )] | ||
| pub presim_max_concurrency: Option<usize>, |
There was a problem hiding this comment.
Need to validate >0 as channel(0) would panic.
| }; | ||
|
|
||
| match &bounded_sender { | ||
| Some(sender) => sender.send(fut).await.unwrap(), |
There was a problem hiding this comment.
When presim queue is full the send(fut).await blocks the single insertion worker so new RPC requests would pile up in the unbounded pool_rx. Consider using try_send and handle the TrySendError::Full with atleast a metric or log.
| let bounded_sender = max_concurrency.map(|max| { | ||
| let (sender, receiver) = mpsc::channel(max); | ||
| runtime.spawn_task(async move { | ||
| let _ = ReceiverStream::new(receiver) |
There was a problem hiding this comment.
Maybe a PresimJob struct would be clearer instead of sending future through the channel, and you create the future here.
|
Closing this PR as there's too much feature creep going on |
3 participants
1st commit: refactoring pool insertion to happen after sending on a channel. This will make it easier later on to put the presim at the right spot (before bundles get into the reth pool).
2nd commit: Added a flag to constrain the number of simultaneously running presim tasks