feat(block-producer): improve mempool config #543
Conversation
| 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
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 0xPolygonMiden/miden-base#973, but the overall approach will probably remain the same.
| // TODO: This is inefficient and ProvenTransaction should provide len() access. | ||
| let output_notes = tx.output_notes().count(); | ||
| let input_notes = tx.nullifiers().count(); |
There was a problem hiding this comment.
Lets create an issue for this in miden-base.
| /// Limits placed on a batch's contents. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct BatchBudget { | ||
| /// Maximum number of transactions allowed in a batch. | ||
| transactions: usize, | ||
| /// Maximum number of input notes allowed. | ||
| input_notes: usize, | ||
| /// Maximum number of output notes allowed. | ||
| output_notes: usize, | ||
| /// Maximum number of updated accounts. | ||
| accounts: usize, | ||
| } |
There was a problem hiding this comment.
Should we make this struct Copy?
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct BlockBudget { | ||
| /// Maximum number of batches allowed in a block. | ||
| batches: usize, | ||
| } |
There was a problem hiding this comment.
Same comment re making this Copy as above.
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