Make the block builder pool a block building algorithm #141
Conversation
ferranbt
added
enhancement
Benchmark results for
|
| Date (UTC) | 2024-08-23T09:46:58+00:00 |
| Commit | e64e6829eeb92363fae5dc5ea277a5ceed473894 |
| Base SHA | b84a0d1940dd3392f3ea5d069e1dc54d828b9c45 |
Significant changes
None
| tokio::spawn(multiplex_job(simulations_for_block.orders, broadcast_input)); | ||
|
|
||
| if let Some(builder) = self.builder.as_ref() { | ||
| builder.build_blocks(input); |
There was a problem hiding this comment.
Even though you know that BlockBuildingPool is not blocking, builders are expected to be blocking. We should tokio::task::spawn_blocking this.
There was a problem hiding this comment.
Which one do you want as spawn_blocking?
There was a problem hiding this comment.
builder.build_blocks(input);
There was a problem hiding this comment.
Check again, I think this is resolved now.
| // This was done before in block building pool | ||
| { | ||
| let max_time_to_build = time_until_slot_end.try_into().unwrap_or_default(); | ||
| let block_cancellation = self.global_cancellation.clone().child_token(); | ||
|
|
||
| let cancel = block_cancellation.clone(); | ||
| tokio::spawn(async move { | ||
| tokio::time::sleep(max_time_to_build).await; | ||
| cancel.cancel(); | ||
| }); | ||
|
|
||
| let (orders_for_block, sink) = OrdersForBlock::new_with_sink(); | ||
| // add OrderReplacementManager to manage replacements and cancellations | ||
| let order_replacement_manager = OrderReplacementManager::new(Box::new(sink)); | ||
| // sink removal is automatic via OrderSink::is_alive false | ||
| let _block_sub = orderpool_subscriber.add_sink( | ||
| block_ctx.block_env.number.to(), | ||
| Box::new(order_replacement_manager), | ||
| ); | ||
|
|
||
| let simulations_for_block = order_simulation_pool.spawn_simulation_job( | ||
| block_ctx.clone(), | ||
| orders_for_block, | ||
| block_cancellation.clone(), | ||
| ); | ||
|
|
||
| let (broadcast_input, _) = broadcast::channel(10_000); | ||
| let builder_sink = sink_factory.create_sink(payload, block_cancellation.clone()); | ||
|
|
||
| let input = BlockBuildingAlgorithmInput::<DB> { | ||
| provider_factory: self.provider_factory.provider_factory_unchecked(), | ||
| ctx: block_ctx, | ||
| sink: builder_sink, | ||
| input: broadcast_input.subscribe(), | ||
| cancel: block_cancellation, | ||
| }; | ||
|
|
||
| tokio::spawn(multiplex_job(simulations_for_block.orders, broadcast_input)); | ||
|
|
||
| if let Some(builder) = self.builder.as_ref() { | ||
| builder.build_blocks(input); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you put this into a func so the code is more readable like before?
There was a problem hiding this comment.
Due to type restrictions we didn't have time to polish (BlockBuildingAlgorithmInput having a very specific communication method: broadcast::Receiver) you were forced to add an unnecesary background task on LiveBuilder::run():
tokio::spawn(multiplex_job(simulations_for_block.orders, broadcast_input));
I think this can be avoided by using a trait since the only functions called are try_recv and recv, both implemented on broadcast::Receiver and mpsc::Receiver.
You could either add wrapper objects or extend mpsc::Receiver/broadcast/Receiver (this is cheaper).
Any hints on how to do this? |
|
Shot out to @liamaharon for the help on the fixes. |
| @@ -74,7 +79,7 @@ pub struct LiveBuilder<DB, BlocksSourceType: SlotSource> { | |||
| pub global_cancellation: CancellationToken, | |||
|
|
|||
| pub sink_factory: Box<dyn UnfinishedBlockBuildingSinkFactory>, | |||
| pub builders: Vec<Arc<dyn BlockBuildingAlgorithm<DB>>>, | |||
| pub builder: Arc<dyn BlockBuildingAlgorithm<DB>>, // doing the Option because there is a fuunction that creates the live_builder without the builder. | |||
There was a problem hiding this comment.
Any reason dyn was chosen over making the builder algorithm generic?
📝 Summary
Right now there is an ad-hoc
BlockBuilderPoolentity that lives inside thelive_builderand manages a list ofBlockBuildingAlgorithmtrait implementations. The pool takes the input to build a block and spawns different algorithms with the same sink destination.The
live_builderuses a concrete reference of theBlockBuilderPooland initialises it with a list of builders.This PR converts the
BlockBuilderPoolin an implementation of theBlockBuildingAlgorithmtrait. Then, thelive_builderdoes not need to have a concrete reference of theBlockBuilderPoolbut just adyn BlockBuildingAlgorithmreference which can be a single block building algorithm or a pool of them.💡 Motivation and Context
✅ I have completed the following steps:
make lintmake test