feat: user batch support#1846
feat: user batch support#1846Mirko-von-Leipzig wants to merge 9 commits intomirko/mempool-tx-revertingfrom
Conversation
Mirko-von-Leipzig
force-pushed
the
mirko/mempool-user-batches
branch
from
bf86aec to
51e74e4
Compare
Mirko-von-Leipzig
force-pushed
the
mirko/mempool-user-batches
branch
from
51e74e4 to
6dd5f53
Compare
| // Encoded using [winter_utils::Serializable] implementation for | ||
| // [miden_protocol::transaction::proven_tx::ProvenTransaction]. | ||
| bytes encoded = 1; | ||
| message TransactionBatch { |
There was a problem hiding this comment.
The suggestion here I think was to re-add the proven_batch property, and have the others be optional so we can drop them at some point.
Mirko-von-Leipzig
requested review from
PhilippGackstatter,
bobbinth,
igamigo and
sergerad
There was a problem hiding this comment.
@PhilippGackstatter if you could throw an eye on the process here to ensure I'm checking the correct things.
The state itself is checked in the mempool, so here we really just want to ensure that the batch and its transactions are valid and the reference block is correct iiuc.
| let reference_commitment: Word = reference_header | ||
| .chain_commitment | ||
| .expect("store should always fill block header") | ||
| .try_into() | ||
| .expect("store Word should be okay"); | ||
| if reference_commitment != proof.reference_block_commitment() { |
There was a problem hiding this comment.
Is this correct?
| &mut self, | ||
| txs: &[Arc<AuthenticatedTransaction>], | ||
| ) -> Result<BlockNumber, MempoolSubmissionError> { | ||
| assert!(!txs.is_empty(), "Cannot have a batch with no transactions"); |
There was a problem hiding this comment.
Just checking we want to crash here instead of return error
There was a problem hiding this comment.
I assumed that one cannot build a ProvenBatch without one, so this would indicate an internal bug somewhere. But maybe that's a poor assumption.
| } | ||
|
|
||
| pub fn select_batch(&mut self, budget: BatchBudget) -> Option<SelectedBatch> { | ||
| self.select_user_batch().or_else(|| self.select_conventional_batch(budget)) |
There was a problem hiding this comment.
Might want some doc comments to make it clear that budget is intended to only relevant for conventional batches.
There was a problem hiding this comment.
Also are we OK with user batches always taking priority over conventional here?
There was a problem hiding this comment.
Also wondering if we need to prevent user batches of size 1 (or some other limit). Unsure if that is relevant to this PR just a general thought.
There was a problem hiding this comment.
Also are we OK with user batches always taking priority over conventional here?
I'm unsure, but at the moment it doesn't matter much. If its a concern we can make it random -- I was thinking maybe that's best.
Also wondering if we need to prevent user batches of size 1 (or some other limit). Unsure if that is relevant to this PR just a general thought.
Good question. I'm unsure 😬 I wonder if that makes some user loop more difficult i.e. they always submit user batches, but sometimes they don't have many transactions to bundle..
Probably we would want some limit even in the future? cc @bobbinth
| .store | ||
| .get_tx_inputs(tx) | ||
| .await | ||
| .map_err(MempoolSubmissionError::StoreConnectionFailed)?; |
There was a problem hiding this comment.
Would it be worth to do these queries concurrently with something like futures::future::try_join_all?
There was a problem hiding this comment.
Or a batch endpoint. Probably not worth it based on the number of txns in a batch?
There was a problem hiding this comment.
Good point, tbh I didn't even consider it. I think simple is better for now, there's also the thought that this makes it equivalent to normal transactions i.e. submitting a batch of N isn't unfairly faster, or consumes more resources, than N single ones.
I don't feel strongly about this.
2 participants
This PR is the third and final part of the mempool refactoring PR stack. Part 1 (#1820) performs the broad mempool refactoring to simplify this PR. Builds on part 2 (#1832).
Batch submissions must include their transaction inputs since we currently require this for the validator to verify them before inclusion in a block. This PR abuses this by treating the batch as a set of normal transactions at the mempool level. This simplifies the mempool implementation, which is currently built around a DAG of transactions - so having to insert a batch directly would be more complex. This will need to change once we stop requiring transaction inputs as part of the validator; but it won't be too bad.
The way this is implemented here, is that the transaction DAG tracks user batches and ensures that when a batch is selected, that transactions from user batches are not mixed with conventional transactions. That is,
select_batchoutputs either a user batch, or a conventional batch.Effectively, the transaction DAG internally ensures that the user batch's transactions remain coherent even though the batch has been deconstructed into individual transactions. The benefit is that this doesn't require any major structural changes to the mempool. The rest of the mempool then treats the user batch as per normal.
Closes #1112