Conversation
frisitano
requested review from
Thegaram,
greged93,
jonastheis and
yiweichi
| @@ -90,14 +128,21 @@ impl DatabaseConnectionProvider for Database { | |||
|
|
|||
There was a problem hiding this comment.
impl DatabaseConnectionProvider for Database can we remove this? I think it's only used in test, feel free to keep it if it makes things easier.
There was a problem hiding this comment.
I'll put this behind a feature flag #[cfg(feature = "test-utils")] but leave it implemented here in this scope so we can access &self.connection from the implementation without making it public.
There was a problem hiding this comment.
It looks like it's also used in args.rs, lets leave it as is for now.
|
|
||
| // derive the attributes and attach the corresponding batch info. | ||
| let attrs = derive(batch, provider, database, l1_v2_message_queue_start_index) | ||
| let attrs = derive(batch, provider, tx, l1_v2_message_queue_start_index) |
There was a problem hiding this comment.
I'm wondering whether it is necessary to keep the read tx open for the entire duration? Do we need to make sure that the data we read here is "atomic"?
| let tx = database.tx().await?; | ||
| let (latest_safe_block, _) = | ||
| database.get_latest_safe_l2_info().await?.expect("safe block must exist"); | ||
| tx.get_latest_safe_l2_info().await?.expect("safe block must exist"); |
There was a problem hiding this comment.
How long exactly is the read lock retained here?
3 participants
Overview
This PR refactors the
Databaseimplementation to account for the fundamental properties of sqlite. Specifically, sqlite supports a single writer (across the database) and multiple concurrent writers (in WAL mode).Implementation
We introduce two transaction types,
TXandTXMut. Both theTXandTXMuttypes are created via theDatabaseinstance. To constrain the system to only allowing a singleTXMutacross all threads, we use anArc<Mutex<()>>, a lock must be acquired on this type before aTXMutcan be instantiated. TheTXtype can only perform read operations and theTXMuttype can perform both read and write operations.Fix
There is an edge case in the
prepare_on_startupdatabase operation in which we would use the next L1 block number + 1 of the previous batch to start from. This was problematic as we can have multiple batches submitted in the same L1 block, leading to skipped batches on startup. We have rectified this by starting from the L1 block number of the previous batch (removing the + 1).