chore(store): move write SQL off diesel - #2505
Conversation
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
I didn't thoroughly inspect everything under the assumption that a lot of the code is just shuffling about.
Its looking good; some questions around testing, and then I think we should prefer wrapping the tx types.
| /// # Errors | ||
| /// | ||
| /// Returns an error if the requested invalidation or insertion fails. | ||
| pub(super) fn insert_account_storage_map_value_inner( |
There was a problem hiding this comment.
I assume this gets used somewhere else as well.
There was a problem hiding this comment.
I think perhaps we can shorten these (and similar) by removing account?
| let storage_header = match storage_header { | ||
| Some(header) => header, | ||
| None => AccountStorageHeader::new(Vec::new())?, | ||
| }; | ||
| let storage_commitment = storage_header.to_commitment(); |
There was a problem hiding this comment.
| let storage_header = match storage_header { | |
| Some(header) => header, | |
| None => AccountStorageHeader::new(Vec::new())?, | |
| }; | |
| let storage_commitment = storage_header.to_commitment(); | |
| let storage_header = storage_header.unwrap_or_default(); | |
| let storage_commitment = storage_header.to_commitment(); |
| use miden_protocol::note::Nullifier; | ||
|
|
||
| /// Returns the high 16 bits of the provided nullifier. | ||
| pub fn get_nullifier_prefix(nullifier: &Nullifier) -> u16 { |
There was a problem hiding this comment.
I wonder if this could be a method added in protocol. Though perhaps its doesn't really have any utility outside of the node, and we'll want it to be a flexible number of bits eventually. @PhilippGackstatter thoughts?
| pub(crate) struct TestDb { | ||
| // Held as `Option` so [`Drop`] can drop the pools inside the runtime's context: their pooled | ||
| // connections are closed on a blocking task, which panics without a runtime to spawn it on. | ||
| writer: Option<DbWriter>, | ||
| reader: Option<DbReader>, | ||
| runtime: Runtime, | ||
| path: PathBuf, | ||
| } |
There was a problem hiding this comment.
Why do we need a specialized type for this? Could we instead just have Db::for_tests() or Db::temp()?
| pub(crate) fn apply_block( | ||
| tx: &WriteTx<'_>, | ||
| block: &SignedBlock, | ||
| notes: &[(NoteRecord, Option<Nullifier>)], | ||
| precomputed_public_states: &PrecomputedPublicAccountStates, | ||
| ) -> Result<usize, DatabaseError> { |
There was a problem hiding this comment.
I think we would benefit from creating transaction wrappers.
i.e. tx.apply_block(...) instead of free functions. This would also simplify our exports (no need to export each function).
There was a problem hiding this comment.
This would also hide raw sql functionality from the caller i.e. they can only use tx.apply_block(...).
There was a problem hiding this comment.
If we go the tx wrapper route, then I would reorganise this to something like:
crates/store/src/db
/writer
/mod.rs -- contains the wrapper definition(s)
/insert_block_header/
/...
| /// Returns the subset of `account_ids` whose latest committed state is a network account. | ||
| /// | ||
| /// Unknown ids and non-network accounts are silently omitted. | ||
| pub(crate) fn select_network_accounts_subset( |
There was a problem hiding this comment.
Perhaps filter is a better word?
2 participants
Summary
First step of migrating the store off diesel to the
miden-node-dbSQLite framework, splitting #2473 into reviewable chunks as suggested in this comment.This PR moves the write path:
bootstrapandapply_block(all inserts, upsert_accounts, prune_history) now run on the framework's single writer connection, while reads stay on the diesel pool over the same database file.Tests drive writes through a new
TestDbframework handle; each remaining diesel read sits behind a test wrapper, so follow-up PRs flip one wrapper per query. A final PR ports the remaining ~16 reads and deletes the diesel pool, models/, and the diesel dependencies.Changelog