feat(block-producer): improve mempool config#543
feat(block-producer): improve mempool config#543Mirko-von-Leipzig merged 7 commits intonext-block-producerfrom
Conversation
Mirko-von-Leipzig
added
the
no changelog
polydez
left a comment
polydez
left a comment
There was a problem hiding this comment.
Looks good to me, thank you! I've left a couple of nit comments.
| while let Some(batch_id) = self.inner.roots().first().copied() { | ||
| // SAFETY: Since it was a root batch, it must definitely have a processed batch | ||
| // associated with it. | ||
| let batch = self.inner.get(&batch_id).unwrap().clone(); |
There was a problem hiding this comment.
We have no such limitations in our coding style yet, but I think, we should avoid using unwraps in favor of expect. We can put safety description in panic message here. Even if it panics, the user/developer will have info why it panicked.
There was a problem hiding this comment.
I've gone back and forth on unwrap vs expect a few times. expect should indicate what you expected to happen, and will mostly be shorter than the SAFETY comment I would write instead.
To me the two aren't quite the same i.e. will contain different strings. Or put differently, I prefer having a lengthy SAFETY paragraph instead of a one-line expect message.
Though maybe I should just be doing both.
What I will say is that with expect I often find myself writing/copying the same message in various places which sort of negates identifying the actual point-of-failure. In which case we should be running with BACKTRACE=1 always so we get actual line and panic info. There isn't really a good reason not to do this.
cc @bobbinth for his style guide input :D
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of small comments inline.
In general, I think this mechanism should work well. We may need to make some adjustments based on where we land with 0xMiden/protocol#973, but the overall approach will probably remain the same.
Mirko-von-Leipzig
force-pushed
the
mirko-cleanup-mempool
branch
from
1a35a9e to
451c460
Compare
Mirko-von-Leipzig
force-pushed
the
mirko-cleanup-mempool
branch
from
451c460 to
75053be
Compare
|
@polydez I've added some |
Mirko-von-Leipzig
force-pushed
the
mirko-cleanup-mempool
branch
from
01300fe to
5eb9b24
Compare
Minor cleanup
Address more review comments Minor cleanup
Mirko-von-Leipzig
force-pushed
the
mirko-cleanup-mempool
branch
from
5eb9b24 to
d2163ba
Compare
polydez
left a comment
polydez
left a comment
There was a problem hiding this comment.
It looks good to me, thanks for addressing review comments!
4 participants
This PR improves mempool configuration and simplifies the block-producer errors exposed during batch and block building.
Mempool configuration is now separated out into a builder, and constraint support is added as a sort of batch/block budget that gets exhausted during the batch/block selection. In other words, batches/blocks produced by the mempool are guaranteed to adhere to the limits placed on them, removing the need for asserting these errors in the building process.
I've removed the transaction data passing in the error variant, which was used by the previous FIFO implementation to return the failed transactions into the pool. This is no longer required as the mempool retains a copy of all inflight transactions.
We may still want to perform these checks in the builders regardless; though I'd prefer only doing this in a single place.
This completes some tasks in #519