-
Notifications
You must be signed in to change notification settings - Fork 105
feat: user batch support #1846
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: mirko/mempool-tx-reverting
Are you sure you want to change the base?
feat: user batch support #1846
Changes from all commits
5e84013
6691da9
a1f2066
9786578
ba58888
d656f18
4b264df
6dd5f53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ use std::sync::Arc; | |||||
|
|
||||||
| use miden_protocol::Word; | ||||||
| use miden_protocol::account::AccountId; | ||||||
| use miden_protocol::batch::BatchId; | ||||||
| use miden_protocol::block::BlockNumber; | ||||||
| use miden_protocol::note::Nullifier; | ||||||
| use miden_protocol::transaction::TransactionId; | ||||||
|
|
@@ -75,6 +76,9 @@ pub struct TransactionGraph { | |||||
| /// These are batch or block proving errors in which the transaction was a part of. This is | ||||||
| /// used to identify potentially buggy transactions that should be evicted. | ||||||
| failures: HashMap<TransactionId, u32>, | ||||||
|
|
||||||
| user_batch_txs: HashMap<BatchId, Vec<TransactionId>>, | ||||||
| txs_user_batch: HashMap<TransactionId, BatchId>, | ||||||
| } | ||||||
|
|
||||||
| impl TransactionGraph { | ||||||
|
|
@@ -85,16 +89,100 @@ impl TransactionGraph { | |||||
| self.inner.append(tx) | ||||||
| } | ||||||
|
|
||||||
| pub fn select_batch(&mut self, mut budget: BatchBudget) -> Option<SelectedBatch> { | ||||||
| pub fn append_user_batch( | ||||||
| &mut self, | ||||||
| batch: &[Arc<AuthenticatedTransaction>], | ||||||
| ) -> Result<(), StateConflict> { | ||||||
| let batch_id = | ||||||
| BatchId::from_transactions(batch.iter().map(|tx| tx.raw_proven_transaction())); | ||||||
|
|
||||||
| // Append each transaction, but revert atomically on error. | ||||||
| for (idx, tx) in batch.iter().enumerate() { | ||||||
| if let Err(err) = self.append(Arc::clone(tx)) { | ||||||
| // We revert in reverse order because inner.revert panics if the node doesn't exist. | ||||||
| for tx in batch.iter().take(idx).rev() { | ||||||
| let reverted = self.inner.revert_node_and_descendants(tx.id()); | ||||||
| assert_eq!(reverted.len(), 1); | ||||||
| assert_eq!(&reverted[0], tx); | ||||||
| } | ||||||
|
|
||||||
| return Err(err); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| let txs = batch.iter().map(GraphNode::id).collect::<Vec<_>>(); | ||||||
| for tx in &txs { | ||||||
| self.txs_user_batch.insert(*tx, batch_id); | ||||||
| } | ||||||
| self.user_batch_txs.insert(batch_id, txs); | ||||||
|
|
||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| pub fn select_batch(&mut self, budget: BatchBudget) -> Option<SelectedBatch> { | ||||||
| self.select_user_batch().or_else(|| self.select_conventional_batch(budget)) | ||||||
| } | ||||||
|
|
||||||
| fn select_user_batch(&mut self) -> Option<SelectedBatch> { | ||||||
| // Comb through all user batch candidates. | ||||||
| let candidate_batches = self | ||||||
| .inner | ||||||
| .selection_candidates() | ||||||
| .values() | ||||||
| .filter_map(|tx| self.txs_user_batch.get(&tx.id())) | ||||||
| .copied() | ||||||
| .collect::<HashSet<_>>(); | ||||||
|
|
||||||
| 'outer: for candidate in candidate_batches { | ||||||
| let mut selected = SelectedBatch::builder(); | ||||||
|
|
||||||
| let txs = self | ||||||
| .user_batch_txs | ||||||
| .get(&candidate) | ||||||
| .cloned() | ||||||
| .expect("bi-directional mapping should be coherent"); | ||||||
|
|
||||||
| for tx in txs { | ||||||
| let Some(tx) = self.inner.selection_candidates().get(&tx).copied() else { | ||||||
| // Rollback this batch selection since it cannot complete. | ||||||
| for tx in selected.txs.into_iter().rev() { | ||||||
| self.inner.deselect(tx.id()); | ||||||
| } | ||||||
|
|
||||||
| continue 'outer; | ||||||
| }; | ||||||
| let tx = Arc::clone(tx); | ||||||
|
|
||||||
| self.inner.select_candidate(tx.id()); | ||||||
| selected.push(tx); | ||||||
| } | ||||||
|
|
||||||
| assert!(!selected.is_empty(), "User batch should not be empty"); | ||||||
| return Some(selected.build()); | ||||||
| } | ||||||
|
|
||||||
| None | ||||||
| } | ||||||
|
|
||||||
| fn select_conventional_batch(&mut self, mut budget: BatchBudget) -> Option<SelectedBatch> { | ||||||
| let mut selected = SelectedBatch::builder(); | ||||||
|
|
||||||
| while let Some((id, tx)) = self.inner.selection_candidates().pop_first() { | ||||||
| if budget.check_then_subtract(tx) == BudgetStatus::Exceeded { | ||||||
| loop { | ||||||
| // Select arbitrary candidate which is _not_ part of a user batch. | ||||||
| let candidates = self.inner.selection_candidates(); | ||||||
| let Some(candidate) = | ||||||
| candidates.values().find(|tx| !self.txs_user_batch.contains_key(&tx.id())) | ||||||
| else { | ||||||
| break; | ||||||
| }; | ||||||
|
|
||||||
| if budget.check_then_subtract(candidate) == BudgetStatus::Exceeded { | ||||||
| break; | ||||||
| } | ||||||
|
|
||||||
| selected.push(Arc::clone(tx)); | ||||||
| self.inner.select_candidate(*id); | ||||||
| let candidate = Arc::clone(candidate); | ||||||
| self.inner.select_candidate(candidate.id()); | ||||||
| selected.push(candidate); | ||||||
| } | ||||||
|
|
||||||
| if selected.is_empty() { | ||||||
|
|
@@ -127,22 +215,40 @@ impl TransactionGraph { | |||||
| /// | ||||||
| /// This includes batches that have been marked as proven. | ||||||
| /// | ||||||
| /// Returns the reverted batches in the _reverse_ chronological order they were appended in. | ||||||
| /// Returns the reverted transactions in the _reverse_ chronological order they were appended | ||||||
| /// in. | ||||||
| pub fn revert_tx_and_descendants(&mut self, transaction: TransactionId) -> Vec<TransactionId> { | ||||||
| // We need this check because `inner.revert..` panics if the node is unknown. | ||||||
| if !self.inner.contains(&transaction) { | ||||||
| return Vec::default(); | ||||||
| } | ||||||
| // This is a bit more involved because we also need to atomically revert user batches. | ||||||
| let mut to_revert = vec![transaction]; | ||||||
| let mut reverted = Vec::new(); | ||||||
|
|
||||||
| while let Some(revert) = to_revert.pop() { | ||||||
| // We need this check because `inner.revert..` panics if the node is unknown. | ||||||
| // | ||||||
| // And this transaction might already have been reverted as part of descendents in a | ||||||
| // prior loop. | ||||||
| if !self.inner.contains(&revert) { | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| let reverted = self | ||||||
| .inner | ||||||
| .revert_node_and_descendants(transaction) | ||||||
| .into_iter() | ||||||
| .map(|tx| tx.id()) | ||||||
| .collect(); | ||||||
| let x = self.inner.revert_node_and_descendants(transaction); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Should this be revert instead of transaction?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, thank you! |
||||||
|
|
||||||
| // Clean up book keeping and also revert transactions from the same user batch, if any. | ||||||
| for tx in &x { | ||||||
| self.failures.remove(&tx.id()); | ||||||
|
|
||||||
| // Note that this is a pretty rough shod approach. We just dump the entire batch of | ||||||
| // transactions in, which will result in at least the current | ||||||
| // transaction being duplicated in `to_revert`. This isn't a concern | ||||||
| // though since we skip already processed transactions at the top of the loop. | ||||||
| if let Some(batch) = self.txs_user_batch.remove(&tx.id()) { | ||||||
| if let Some(batch) = self.user_batch_txs.remove(&batch) { | ||||||
| to_revert.extend(batch); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| for tx in &reverted { | ||||||
| self.failures.remove(tx); | ||||||
| reverted.extend(x.into_iter().map(|tx| tx.id())); | ||||||
| } | ||||||
|
|
||||||
| reverted | ||||||
|
|
@@ -187,15 +293,19 @@ impl TransactionGraph { | |||||
| reverted | ||||||
| } | ||||||
|
|
||||||
| /// Prunes the given transaction. | ||||||
| /// Prunes the given given batch's transactions. | ||||||
| /// | ||||||
| /// # Panics | ||||||
| /// | ||||||
| /// Panics if the transaction does not exist, or has existing ancestors in the transaction | ||||||
| /// Panics if the transactions do not exist, or has existing ancestors in the transaction | ||||||
| /// graph. | ||||||
| pub fn prune(&mut self, transaction: TransactionId) { | ||||||
| self.inner.prune(transaction); | ||||||
| self.failures.remove(&transaction); | ||||||
| pub fn prune(&mut self, batch: &SelectedBatch) { | ||||||
| for tx in batch.transactions() { | ||||||
| self.inner.prune(tx.id()); | ||||||
| self.failures.remove(&tx.id()); | ||||||
| self.txs_user_batch.remove(&tx.id()); | ||||||
| } | ||||||
| self.user_batch_txs.remove(&batch.id()); | ||||||
| } | ||||||
|
|
||||||
| /// Number of transactions which have not been selected for inclusion in a batch. | ||||||
|
|
||||||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also are we OK with user batches always taking priority over conventional here?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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