From 295fa55473e95a70c4a4c6ff9cd1eb618c566f40 Mon Sep 17 00:00:00 2001 From: Gilad Chase Date: Wed, 8 Jan 2025 17:42:35 +0200 Subject: [PATCH] feat(starknet_l1_provider): add `SoftDeleteIndexMap` Previous implementation inside transaction manager didn't account for validate, the current implementation is more general, and can in the future perhaps be reused elsewhere after some changes are made. --- crates/blockifier/cairo_native | 2 +- .../src/l1_provider_tests.rs | 2 +- crates/starknet_l1_provider/src/lib.rs | 2 + .../src/soft_delete_index_map.rs | 135 ++++++++++++++++++ crates/starknet_l1_provider/src/test_utils.rs | 34 +++-- .../src/transaction_manager.rs | 38 ++--- crates/starknet_l1_provider_types/src/lib.rs | 1 + 7 files changed, 180 insertions(+), 34 deletions(-) create mode 100644 crates/starknet_l1_provider/src/soft_delete_index_map.rs diff --git a/crates/blockifier/cairo_native b/crates/blockifier/cairo_native index 185e94bce3..76e83965d3 160000 --- a/crates/blockifier/cairo_native +++ b/crates/blockifier/cairo_native @@ -1 +1 @@ -Subproject commit 185e94bce3e380db988f98d96b4ddcb3e6c044bc +Subproject commit 76e83965d3bf1252eb6c68200a3accd5fd1ec004 diff --git a/crates/starknet_l1_provider/src/l1_provider_tests.rs b/crates/starknet_l1_provider/src/l1_provider_tests.rs index 9122f01c30..3523e65446 100644 --- a/crates/starknet_l1_provider/src/l1_provider_tests.rs +++ b/crates/starknet_l1_provider/src/l1_provider_tests.rs @@ -62,7 +62,7 @@ fn validate_happy_flow() { // Transaction wasn't deleted after the validation. assert_eq!( l1_provider.validate(tx_hash!(1), BlockNumber(1)).unwrap(), - ValidationStatus::Validated + ValidationStatus::AlreadyIncludedInPropsedBlock ); } diff --git a/crates/starknet_l1_provider/src/lib.rs b/crates/starknet_l1_provider/src/lib.rs index 6c5fca52cd..8dbbb8c3ad 100644 --- a/crates/starknet_l1_provider/src/lib.rs +++ b/crates/starknet_l1_provider/src/lib.rs @@ -4,6 +4,8 @@ pub mod l1_provider; pub mod l1_scraper; pub(crate) mod transaction_manager; +mod soft_delete_index_map; + #[cfg(test)] pub mod test_utils; diff --git a/crates/starknet_l1_provider/src/soft_delete_index_map.rs b/crates/starknet_l1_provider/src/soft_delete_index_map.rs new file mode 100644 index 0000000000..f369fb4a65 --- /dev/null +++ b/crates/starknet_l1_provider/src/soft_delete_index_map.rs @@ -0,0 +1,135 @@ +use std::collections::HashSet; + +use indexmap::map::Entry; +use indexmap::IndexMap; +use starknet_api::executable_transaction::L1HandlerTransaction; +use starknet_api::transaction::TransactionHash; + +/// An IndexMap that supports soft deletion of entries. +/// Entries marked as deleted remain hidden in the map, allowing for potential recovery, +/// selective permanent deletion, or rollback before being purged. +// TODO: replace with a fully generic struct if there's a need for it. +// TODO: replace with a BTreeIndexMap if commit performance becomes an issue, see note in commit. +#[derive(Clone, Debug, Default)] +pub struct SoftDeleteIndexMap { + txs: IndexMap, + staged_txs: HashSet, +} + +impl SoftDeleteIndexMap { + pub fn _new() -> Self { + Self::default() + } + + /// Inserts a transaction into the map, returning the previous transaction if it existed. + pub fn _insert(&mut self, tx: L1HandlerTransaction) -> bool { + let tx_hash = tx.tx_hash; + match self.txs.entry(tx_hash) { + Entry::Occupied(entry) => { + assert_eq!(entry.get().transaction, tx); + false + } + Entry::Vacant(entry) => { + entry.insert(TransactionEntry::new(tx)); + true + } + } + } + + /// Soft delete and return a reference to the first unstaged transaction, by insertion order. + pub fn soft_pop_front(&mut self) -> Option<&L1HandlerTransaction> { + let entry = self.txs.iter().find(|(_, tx)| tx.is_available()); + let (&tx_hash, _) = entry?; + self.soft_remove(tx_hash) + } + + /// Stages the given transaction with the given hash if it exists and is not already staged, and + /// returns a reference to it. + pub fn soft_remove(&mut self, tx_hash: TransactionHash) -> Option<&L1HandlerTransaction> { + let entry = self.txs.get_mut(&tx_hash)?; + + if !entry.is_available() { + return None; + } + + assert_eq!(self.staged_txs.get(&tx_hash), None); + entry.set_state(TxState::Staged); + self.staged_txs.insert(tx_hash); + + Some(&entry.transaction) + } + + /// Commits given transactions by removing them entirely and returning the removed transactions. + /// Uncommitted staged transactions are rolled back to unstaged first. + // Performance note: This operation is linear time with both the number + // of known transactions and the number of committed transactions. This is assumed to be + // good enough while l1-handler numbers remain low, but if this changes and we need log(n) + // removals (amortized), replace indexmap with this (basically a BTreeIndexMap): + // BTreeMap, Hashmap and a counter: u32, such that + // every new tx is inserted to the map with key counter++ and the counter is not reduced + // when removing entries. Once the counter reaches u32::MAX/2 we recreate the DS in Theta(n). + pub fn _commit(&mut self, tx_hashes: &[TransactionHash]) -> Vec { + self._rollback_staging(); + let tx_hashes: HashSet<_> = tx_hashes.iter().copied().collect(); + if tx_hashes.is_empty() { + return Vec::new(); + } + + // NOTE: this takes Theta(|self.txs|), see docstring. + let (committed, not_committed): (Vec<_>, Vec<_>) = + self.txs.drain(..).partition(|(hash, _)| tx_hashes.contains(hash)); + self.txs.extend(not_committed); + + committed.into_iter().map(|(_, entry)| entry.transaction).collect() + } + + /// Rolls back all staged transactions, converting them to unstaged. + pub fn _rollback_staging(&mut self) { + for tx_hash in self.staged_txs.drain() { + self.txs.entry(tx_hash).and_modify(|entry| entry.set_state(TxState::Unstaged)); + } + } + + pub fn is_staged(&self, tx_hash: &TransactionHash) -> bool { + self.staged_txs.contains(tx_hash) + } +} + +impl From> for SoftDeleteIndexMap { + fn from(txs: Vec) -> Self { + let txs = txs.into_iter().map(|tx| (tx.tx_hash, TransactionEntry::new(tx))).collect(); + SoftDeleteIndexMap { txs, ..Default::default() } + } +} + +/// Indicates whether a transaction is unstaged or staged. +#[derive(Debug, Clone)] +enum TxState { + Unstaged, + Staged, +} + +/// Wraps an L1HandlerTransaction along with its current TxState, +/// and provides convenience methods for stage/unstage. +#[derive(Debug, Clone)] +struct TransactionEntry { + pub transaction: L1HandlerTransaction, + pub state: TxState, +} + +impl TransactionEntry { + pub fn new(transaction: L1HandlerTransaction) -> Self { + Self { transaction, state: TxState::Unstaged } + } + + pub fn set_state(&mut self, state: TxState) { + self.state = state + } + + pub fn is_available(&self) -> bool { + match self.state { + TxState::Unstaged => true, + TxState::Staged => false, + } + } +} diff --git a/crates/starknet_l1_provider/src/test_utils.rs b/crates/starknet_l1_provider/src/test_utils.rs index 8646235d8c..2fe2b50297 100644 --- a/crates/starknet_l1_provider/src/test_utils.rs +++ b/crates/starknet_l1_provider/src/test_utils.rs @@ -1,8 +1,8 @@ +use std::collections::HashSet; use std::mem; use std::sync::Mutex; use async_trait::async_trait; -use indexmap::{IndexMap, IndexSet}; use pretty_assertions::assert_eq; use starknet_api::block::BlockNumber; use starknet_api::executable_transaction::{ @@ -18,8 +18,10 @@ use starknet_l1_provider_types::{ }; use crate::l1_provider::L1Provider; +use crate::soft_delete_index_map::SoftDeleteIndexMap; use crate::transaction_manager::TransactionManager; use crate::ProviderState; + // Represents the internal content of the L1 provider for testing. // Enables customized (and potentially inconsistent) creation for unit testing. #[derive(Debug, Default)] @@ -32,10 +34,7 @@ pub struct L1ProviderContent { impl From for L1Provider { fn from(content: L1ProviderContent) -> L1Provider { L1Provider { - tx_manager: content - .tx_manager_content - .map(|tm_content| tm_content.complete_to_tx_manager()) - .unwrap_or_default(), + tx_manager: content.tx_manager_content.map(Into::into).unwrap_or_default(), state: content.state.unwrap_or_default(), current_height: content.current_height, } @@ -85,29 +84,34 @@ impl L1ProviderContentBuilder { // Enables customized (and potentially inconsistent) creation for unit testing. #[derive(Debug, Default)] struct TransactionManagerContent { - txs: Option>, - committed: Option>, + txs: Option>, + committed: Option>, } -impl TransactionManagerContent { - fn complete_to_tx_manager(self) -> TransactionManager { +impl From for TransactionManager { + fn from(mut content: TransactionManagerContent) -> TransactionManager { + let txs: Vec<_> = mem::take(&mut content.txs).unwrap(); TransactionManager { - txs: self.txs.unwrap_or_default(), - committed: self.committed.unwrap_or_default(), - ..Default::default() + txs: SoftDeleteIndexMap::from(txs), + committed: content + .committed + .unwrap_or_default() + .into_iter() + .map(|tx_hash| (tx_hash, None)) + .collect(), } } } #[derive(Debug, Default)] struct TransactionManagerContentBuilder { - txs: Option>, - committed: Option>, + txs: Option>, + committed: Option>, } impl TransactionManagerContentBuilder { fn with_txs(mut self, txs: impl IntoIterator) -> Self { - self.txs = Some(txs.into_iter().map(|tx| (tx.tx_hash, tx)).collect()); + self.txs = Some(txs.into_iter().collect()); self } diff --git a/crates/starknet_l1_provider/src/transaction_manager.rs b/crates/starknet_l1_provider/src/transaction_manager.rs index 565c286020..33c92635f1 100644 --- a/crates/starknet_l1_provider/src/transaction_manager.rs +++ b/crates/starknet_l1_provider/src/transaction_manager.rs @@ -1,34 +1,38 @@ -use indexmap::{IndexMap, IndexSet}; +use indexmap::IndexMap; use starknet_api::executable_transaction::L1HandlerTransaction; use starknet_api::transaction::TransactionHash; use starknet_l1_provider_types::ValidationStatus; +use crate::soft_delete_index_map::SoftDeleteIndexMap; + #[derive(Debug, Default)] pub struct TransactionManager { - pub txs: IndexMap, - pub staged: IndexSet, - pub committed: IndexSet, + pub txs: SoftDeleteIndexMap, + pub committed: IndexMap>, } impl TransactionManager { pub fn get_txs(&mut self, n_txs: usize) -> Vec { - let (tx_hashes, txs): (Vec<_>, Vec<_>) = self - .txs - .iter() - .skip(self.staged.len()) // Transactions are proposed FIFO. - .take(n_txs) - .map(|(&hash, tx)| (hash, tx.clone())) - .unzip(); - - self.staged.extend(tx_hashes); + let mut txs = Vec::with_capacity(n_txs); + + for _ in 0..n_txs { + match self.txs.soft_pop_front().cloned() { + Some(tx) => txs.push(tx), + None => break, + } + } txs } - pub fn validate_tx(&self, tx_hash: TransactionHash) -> ValidationStatus { - if self.txs.contains_key(&tx_hash) { + pub fn validate_tx(&mut self, tx_hash: TransactionHash) -> ValidationStatus { + if self.committed.contains_key(&tx_hash) { + return ValidationStatus::AlreadyIncludedOnL2; + } + + if self.txs.soft_remove(tx_hash).is_some() { ValidationStatus::Validated - } else if self.committed.contains(&tx_hash) { - ValidationStatus::AlreadyIncludedOnL2 + } else if self.txs.is_staged(&tx_hash) { + ValidationStatus::AlreadyIncludedInPropsedBlock } else { ValidationStatus::ConsumedOnL1OrUnknown } diff --git a/crates/starknet_l1_provider_types/src/lib.rs b/crates/starknet_l1_provider_types/src/lib.rs index 4d27dd07c0..a20cb4cbdf 100644 --- a/crates/starknet_l1_provider_types/src/lib.rs +++ b/crates/starknet_l1_provider_types/src/lib.rs @@ -23,6 +23,7 @@ pub type SharedL1ProviderClient = Arc; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum ValidationStatus { + AlreadyIncludedInPropsedBlock, AlreadyIncludedOnL2, ConsumedOnL1OrUnknown, Validated,