-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(engine): batch multiproof messages #19603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@0xSooki looks like e2e tests are failing, can you take a look at why that might be? You should be able to run them locally, no need to wait for CI to find out if they're fixed or not. |
|
Absolutely, working on it. |
|
@mediocregopher done |
mediocregopher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @yongkangc
| num_batched += 1; | ||
| } | ||
| Ok(other_msg) => { | ||
| let _ = self.tx.send(other_msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this message is a MultiProofMessage::StateUpdate, then I believe this will result in state updates being consumed in the incorrect order. Currently they must be consumed in the order they were produced, as that ultimately determines their sequence number and therefore the order they get sent to the state root task.
If there is a way to push to the front of a crossbeam channel then some kind of buffer local to function will need to be used.
fixes #19168
Batching additions:
PrefetchProofsmessagesStateUpdatemessagesTesting enhancements:
test_prefetch_proofs_batchingto verify that multiplePrefetchProofsmessages are correctly batched and processed as a single unit.test_state_update_batchingto ensure that multipleStateUpdatemessages are merged and handled together as intended.