From 7c651998d4d5a436abb664cd5a3547405689c598 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois?= Date: Tue, 9 Apr 2024 17:25:31 +0200 Subject: [PATCH 1/3] Merge transactional_state without committing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Damir Vodenicarevic Co-authored-by: AurelienFT <32803821+AurelienFT@users.noreply.github.com> Co-authored-by: Jean-François Signed-off-by: Jean-François --- Cargo.toml | 2 + src/databases/mod.rs | 2 +- src/lib.rs | 55 ++- src/tests/merge.rs | 576 +++++++++++++++++++++++++++++++ src/tests/mod.rs | 1 + src/tests/transactional_state.rs | 167 +++++++++ src/trie/merkle_tree.rs | 6 +- 7 files changed, 799 insertions(+), 10 deletions(-) create mode 100644 src/tests/merge.rs diff --git a/Cargo.toml b/Cargo.toml index 0120d37..11afd5b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,6 +38,8 @@ rocksdb = { optional = true, version = "0.21.0", features = [ ] } [dev-dependencies] +env_logger = "0.11.3" +once_cell = "1.19.0" pprof = { version = "0.3", features = ["flamegraph"] } pathfinder-common = { git = "https://github.com/massalabs/pathfinder.git", package = "pathfinder-common", rev = "b7b6d76a76ab0e10f92e5f84ce099b5f727cb4db" } pathfinder-crypto = { git = "https://github.com/massalabs/pathfinder.git", package = "pathfinder-crypto", rev = "b7b6d76a76ab0e10f92e5f84ce099b5f727cb4db" } diff --git a/src/databases/mod.rs b/src/databases/mod.rs index e73625d..6b54e2f 100644 --- a/src/databases/mod.rs +++ b/src/databases/mod.rs @@ -6,4 +6,4 @@ pub use hashmap_db::HashMapDb; mod rocks_db; #[cfg(feature = "rocksdb")] -pub use rocks_db::{create_rocks_db, RocksDB, RocksDBBatch, RocksDBConfig}; +pub use rocks_db::{create_rocks_db, RocksDB, RocksDBBatch, RocksDBConfig, RocksDBTransaction}; diff --git a/src/lib.rs b/src/lib.rs index f75aebb..a761d0a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -87,7 +87,7 @@ #[cfg(not(feature = "std"))] extern crate alloc; -use crate::trie::merkle_tree::MerkleTree; +use crate::trie::merkle_tree::{bytes_to_bitvec, MerkleTree}; #[cfg(not(feature = "std"))] use alloc::{format, vec::Vec}; use bitvec::{order::Msb0, slice::BitSlice, vec::BitVec}; @@ -489,8 +489,55 @@ where transactional_bonsai_storage: BonsaiStorage, ) -> Result<(), BonsaiStorageError<>::DatabaseError>> { - self.tries - .db_mut() - .merge(transactional_bonsai_storage.tries.db()) + // Applying changes (all cache_leaf_modified) before calling + //self.tries.db_mut().merge() work for all test but one + //(merge_with_uncommitted_remove, it fails with RocksDB.Busy error, + //which seems to be related to use of OptimisticTransactions, but + //removing them does not solve this particular problem but causes + //others). + + //Applying changes after call to merge() would imply a big refactor as + //merge take ownership of its arguments, hence changes are not + //available anymore. + + //Hence the solution which is a tradeoff between the two solutions: + //1. memorize changes + //2. merge tries + //3. apply changes + + // memoryze changes + let MerkleTrees { db, trees, .. } = transactional_bonsai_storage.tries; + + self.tries.db_mut().merge(db)?; + + // apply changes + for (identifier, tree) in trees { + for (k, op) in tree.cache_leaf_modified { + match op { + crate::trie::merkle_tree::InsertOrRemove::Insert(v) => { + self.insert(&identifier, &bytes_to_bitvec(&k), &v) + .map_err(|e| { + BonsaiStorageError::Merge(format!( + "While merging insert({:?} {}) faced error: {}", + k, + v, + e.to_string() + )) + })?; + } + crate::trie::merkle_tree::InsertOrRemove::Remove => { + self.remove(&identifier, &bytes_to_bitvec(&k)) + .map_err(|e| { + BonsaiStorageError::Merge(format!( + "While merging remove({:?}) faced error: {}", + k, + e.to_string() + )) + })?; + } + } + } + } + Ok(()) } } diff --git a/src/tests/merge.rs b/src/tests/merge.rs new file mode 100644 index 0000000..83181a2 --- /dev/null +++ b/src/tests/merge.rs @@ -0,0 +1,576 @@ +#![cfg(feature = "std")] +use crate::{ + databases::{create_rocks_db, RocksDB, RocksDBConfig, RocksDBTransaction}, + id::{BasicId, BasicIdBuilder}, + BonsaiStorage, BonsaiStorageConfig, +}; +use bitvec::vec::BitVec; +use rocksdb::OptimisticTransactionDB; +use starknet_types_core::{felt::Felt, hash::Pedersen}; + +use once_cell::sync::Lazy; + +static PAIR1: Lazy<(BitVec, Felt)> = Lazy::new(|| { + ( + BitVec::from_vec(vec![1, 2, 2]), + Felt::from_hex("0x66342762FDD54D033c195fec3ce2568b62052e").unwrap(), + ) +}); + +static PAIR2: Lazy<(BitVec, Felt)> = Lazy::new(|| { + ( + BitVec::from_vec(vec![1, 2, 3]), + Felt::from_hex("0x66342762FD54D033c195fec3ce2568b62052e").unwrap(), + ) +}); + +static PAIR3: Lazy<(BitVec, Felt)> = Lazy::new(|| { + ( + BitVec::from_vec(vec![1, 2, 4]), + Felt::from_hex("0x66342762FD54D033c195fec3ce2568b62052e").unwrap(), + ) +}); + +/// Initializes a test environment for the BonsaiStorage data structure. +/// +/// # Arguments +/// +/// * `db` - An instance of the `OptimisticTransactionDB` struct. +/// +/// # Returns +/// +/// A tuple containing the following elements: +/// * `identifier` - A vector of bytes. +/// * `bonsai_storage` - An instance of `BonsaiStorage` with a `RocksDB` +/// backend. +/// * `bonsai_at_txn` - An instance of `BonsaiStorage` representing the +/// transactional state of `bonsai_storage` at `start_id`. +/// * `id_builder` - An instance of `BasicIdBuilder`. +/// * `start_id` - A `BasicId` representing the commit ID of the changes made in +/// `bonsai_storage`. +fn init_test<'db>( + db: &'db OptimisticTransactionDB, +) -> ( + Vec, + BonsaiStorage, Pedersen>, + BonsaiStorage, Pedersen>, + BasicIdBuilder, + BasicId, +) { + let identifier = vec![]; + + let config = BonsaiStorageConfig::default(); + let mut bonsai_storage = + BonsaiStorage::new(RocksDB::new(&db, RocksDBConfig::default()), config) + .expect("Failed to create BonsaiStorage"); + + let mut id_builder = BasicIdBuilder::new(); + + bonsai_storage + .insert(&identifier, &PAIR1.0, &PAIR1.1) + .expect("Failed to insert key-value pair"); + + let start_id = id_builder.new_id(); + bonsai_storage + .commit(start_id) + .expect("Failed to commit changes"); + + let bonsai_at_txn = bonsai_storage + .get_transactional_state(start_id, BonsaiStorageConfig::default()) + .expect("Failed to get transactional state") + .expect("Transactional state not found"); + + ( + identifier, + bonsai_storage, + bonsai_at_txn, + id_builder, + start_id, + ) +} + +#[test] +fn merge_before_simple() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + bonsai_storage.merge(bonsai_at_txn).unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + + assert_eq!( + bonsai_storage.get(&identifier, &PAIR2.0).unwrap(), + Some(PAIR2.1) + ); +} + +#[test] +fn merge_before_simple_remove() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, start_id) = + init_test(&db); + + bonsai_at_txn.remove(&identifier, &PAIR1.0).unwrap(); + bonsai_storage.merge(bonsai_at_txn).unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + + assert_eq!( + bonsai_storage.contains(&identifier, &PAIR1.0).unwrap(), + false + ); + + bonsai_storage.revert_to(start_id).unwrap(); + + assert_eq!( + bonsai_storage.get(&identifier, &PAIR1.0).unwrap(), + Some(PAIR1.1) + ); +} + +#[test] +fn merge_tx_commit_simple_remove() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, start_id) = + init_test(&db); + + bonsai_at_txn.remove(&identifier, &PAIR1.0).unwrap(); + bonsai_at_txn + .transactional_commit(id_builder.new_id()) + .unwrap(); + + bonsai_storage.merge(bonsai_at_txn).unwrap(); + + assert_eq!( + bonsai_storage.contains(&identifier, &PAIR1.0).unwrap(), + false + ); + + bonsai_storage.revert_to(start_id).unwrap(); + + assert_eq!( + bonsai_storage.get(&identifier, &PAIR1.0).unwrap(), + Some(PAIR1.1) + ); +} + +#[test] +fn merge_before_simple_revert_to() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, start_id) = + init_test(&db); + + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + bonsai_storage.merge(bonsai_at_txn).unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + bonsai_storage.revert_to(start_id).unwrap(); + + assert_eq!( + bonsai_storage.get(&identifier, &PAIR2.0).unwrap().is_none(), + true + ); +} + +#[test] +fn merge_transactional_commit_in_txn_before() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, start_id) = + init_test(&db); + + let id2 = id_builder.new_id(); + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + bonsai_at_txn.transactional_commit(id2).unwrap(); + + let id3 = id_builder.new_id(); + bonsai_at_txn + .insert(&identifier, &PAIR3.0, &PAIR3.1) + .unwrap(); + bonsai_storage.merge(bonsai_at_txn).unwrap(); + bonsai_storage.commit(id3).unwrap(); + + assert_eq!( + bonsai_storage.get(&identifier, &PAIR2.0).unwrap(), + Some(PAIR2.1) + ); + assert_eq!( + bonsai_storage.get(&identifier, &PAIR3.0).unwrap(), + Some(PAIR3.1) + ); + bonsai_storage.revert_to(id2).unwrap(); + assert_eq!( + bonsai_storage.get(&identifier, &PAIR2.0).unwrap(), + Some(PAIR2.1) + ); + assert_eq!( + bonsai_storage.get(&identifier, &PAIR3.0).unwrap().is_none(), + true + ); + bonsai_storage.revert_to(start_id).unwrap(); + assert_eq!( + bonsai_storage.get(&identifier, &PAIR2.0).unwrap().is_none(), + true + ); + assert_eq!( + bonsai_storage.get(&identifier, &PAIR3.0).unwrap().is_none(), + true + ); +} + +#[test] +fn merge_transactional_commit_in_txn_before_existing_key() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, start_id) = + init_test(&db); + + bonsai_at_txn.remove(&identifier, &PAIR1.0).unwrap(); + + let id2 = id_builder.new_id(); + bonsai_at_txn.transactional_commit(id2).unwrap(); + bonsai_storage.merge(bonsai_at_txn).unwrap(); + bonsai_storage.revert_to(id2).unwrap(); + + assert_eq!( + bonsai_storage.get(&identifier, &PAIR1.0).unwrap().is_none(), + true + ); + + bonsai_storage.revert_to(start_id).unwrap(); + + assert_eq!( + bonsai_storage.get(&identifier, &PAIR1.0.clone()).unwrap(), + Some(PAIR1.1) + ); +} + +#[test] +fn merge_get_uncommitted() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, _, _) = init_test(&db); + + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + bonsai_storage.merge(bonsai_at_txn).unwrap(); + + assert_eq!( + bonsai_storage.get(&identifier, &PAIR2.0).unwrap(), + Some(PAIR2.1) + ); +} + +#[test] +fn merge_conflict_commited_vs_commited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + let id = id_builder.new_id(); + bonsai_at_txn.transactional_commit(id).unwrap(); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + + match bonsai_storage.merge(bonsai_at_txn) { + Ok(_) => panic!("Expected merge conflict error"), + Err(err) => assert_eq!( + err.to_string(), + "Merge error: Transaction created_at BasicId(0) is lower than the last recorded id" + ), + } +} + +#[test] +fn merge_conflict_commited_vs_commited_change_order() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + bonsai_at_txn + .transactional_commit(id_builder.new_id()) + .unwrap(); + + match bonsai_storage.merge(bonsai_at_txn) { + Ok(_) => panic!("Expected merge conflict error"), + Err(err) => assert_eq!( + err.to_string(), + "Merge error: Transaction created_at BasicId(0) is lower than the last recorded id" + ), + } +} + +#[test] +fn merge_conflict_commited_vs_commited_and_noncommited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + let id = id_builder.new_id(); + bonsai_at_txn.transactional_commit(id).unwrap(); + bonsai_at_txn + .insert(&identifier, &PAIR3.0, &PAIR3.1) + .unwrap(); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + + match bonsai_storage.merge(bonsai_at_txn) { + Ok(_) => panic!("Expected merge conflict error"), + Err(err) => assert_eq!( + err.to_string(), + "Merge error: Transaction created_at BasicId(0) is lower than the last recorded id" + ), + } +} + +#[test] +fn merge_conflict_commited_and_noncommited_vs_commited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + let id = id_builder.new_id(); + bonsai_at_txn.transactional_commit(id).unwrap(); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + bonsai_storage.remove(&identifier, &PAIR2.0).unwrap(); + // .insert(&identifier, &PAIR3.0, &PAIR3.1) + // .unwrap(); + + match bonsai_storage.merge(bonsai_at_txn) { + Ok(_) => panic!("Expected merge conflict error"), + Err(err) => assert_eq!( + err.to_string(), + "Merge error: Transaction created_at BasicId(0) is lower than the last recorded id" + ), + } +} + +#[test] +fn merge_conflict_commited_and_noncommited_vs_noncommited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + bonsai_storage.remove(&identifier, &PAIR2.0).unwrap(); + // .insert(&identifier, &PAIR3.0, &PAIR3.1) + // .unwrap(); + + match bonsai_storage.merge(bonsai_at_txn) { + Ok(_) => panic!("Expected merge conflict error"), + Err(err) => assert_eq!( + err.to_string(), + "Merge error: Transaction created_at BasicId(0) is lower than the last recorded id" + ), + } +} + +#[test] +fn merge_conflict_commited_vs_noncommited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + + match bonsai_storage.merge(bonsai_at_txn) { + Ok(_) => panic!("Expected merge conflict error"), + Err(err) => assert_eq!( + err.to_string(), + "Merge error: Transaction created_at BasicId(0) is lower than the last recorded id" + ), + } +} + +#[test] +fn merge_conflict_noncommited_vs_commited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + bonsai_at_txn + .transactional_commit(id_builder.new_id()) + .unwrap(); + + bonsai_storage.merge(bonsai_at_txn).unwrap(); + + // check that changes in the transactional state overwrite the ones in the + // storage + let get = bonsai_storage.get(&identifier, &PAIR2.0).unwrap(); + assert_eq!(get, Some(PAIR2.1)); +} + +#[test] +fn merge_conflict_noncommited_vs_noncommited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, _, _) = init_test(&db); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + + bonsai_storage.merge(bonsai_at_txn).unwrap(); + + // check that changes in the transactional state overwrite the ones in the + // storage + let get = bonsai_storage.get(&identifier, &PAIR2.0).unwrap(); + assert_eq!(get, Some(PAIR2.1)); +} + +#[test] +fn merge_conflict_noncommited_vs_commited_noncommited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + bonsai_at_txn + .transactional_commit(id_builder.new_id()) + .unwrap(); + bonsai_at_txn + .insert(&identifier, &PAIR3.0, &PAIR3.1) + .unwrap(); + + bonsai_storage.merge(bonsai_at_txn).unwrap(); + + // change in the transactional state overwrites any noncommited changes in + // the storage + let get = bonsai_storage.get(&identifier, &PAIR2.0).unwrap(); + assert_eq!(get, Some(PAIR2.1)); + let get = bonsai_storage.get(&identifier, &PAIR3.0).unwrap(); + assert_eq!(get, Some(PAIR2.1)); +} + +#[test] +fn merge_conflict_commited_noncommited_vs_commited_noncommited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + // insert same key with a different value in the bonsai_storage + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR3.1) + .unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + bonsai_storage + .insert(&identifier, &PAIR3.0, &PAIR2.1) + .unwrap(); + + // insert a key in the transactional state + bonsai_at_txn + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + bonsai_at_txn + .transactional_commit(id_builder.new_id()) + .unwrap(); + bonsai_at_txn + .insert(&identifier, &PAIR3.0, &PAIR3.1) + .unwrap(); + + match bonsai_storage.merge(bonsai_at_txn) { + Ok(_) => { + panic!("Expected merge conflict error") + } + Err(err) => assert_eq!( + err.to_string(), + "Merge error: Transaction created_at BasicId(0) is lower than the last recorded id" + ), + } +} + +#[test] +fn merge_nonconflict_commited_vs_commited() { + let db = create_rocks_db(tempfile::tempdir().unwrap().path()).unwrap(); + let (identifier, mut bonsai_storage, mut bonsai_at_txn, mut id_builder, _) = init_test(&db); + + bonsai_storage + .insert(&identifier, &PAIR2.0, &PAIR2.1) + .unwrap(); + bonsai_storage.commit(id_builder.new_id()).unwrap(); + + bonsai_at_txn + .insert(&identifier, &PAIR3.0, &PAIR3.1) + .unwrap(); + bonsai_at_txn + .transactional_commit(id_builder.new_id()) + .unwrap(); + + match bonsai_storage.merge(bonsai_at_txn) { + Ok(_) => { + panic!("Expected merge conflict error") + } + Err(err) => assert_eq!( + err.to_string(), + "Merge error: Transaction created_at BasicId(0) is lower than the last recorded id" + ), + } +} diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 0292186..81223fc 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -1,4 +1,5 @@ mod madara_comparison; +mod merge; mod proof; mod simple; mod transactional_state; diff --git a/src/tests/transactional_state.rs b/src/tests/transactional_state.rs index 7271e8e..1a4d38a 100644 --- a/src/tests/transactional_state.rs +++ b/src/tests/transactional_state.rs @@ -5,6 +5,7 @@ use crate::{ BonsaiStorage, BonsaiStorageConfig, }; use bitvec::vec::BitVec; +use log::LevelFilter; use starknet_types_core::{felt::Felt, hash::Pedersen}; #[test] @@ -200,6 +201,172 @@ fn merge() { ); } +#[test] +fn merge_with_uncommitted_insert() { + let identifier = vec![]; + let tempdir = tempfile::tempdir().unwrap(); + let db = create_rocks_db(tempdir.path()).unwrap(); + let config = BonsaiStorageConfig::default(); + let mut bonsai_storage = + BonsaiStorage::new(RocksDB::new(&db, RocksDBConfig::default()), config).unwrap(); + let mut id_builder = BasicIdBuilder::new(); + + let pair1 = ( + BitVec::from_vec(vec![1, 2, 2]), + Felt::from_hex("0x66342762FDD5D033c195fec3ce2568b62052e").unwrap(), + ); + let pair2 = ( + BitVec::from_vec(vec![1, 2, 3]), + Felt::from_hex("0x66342762FDD54D033c195fec3ce2568b62052e").unwrap(), + ); + + let id1 = id_builder.new_id(); + bonsai_storage + .insert(&identifier, &pair1.0, &pair1.1) + .unwrap(); + bonsai_storage.commit(id1).unwrap(); + + let mut bonsai_at_txn: BonsaiStorage<_, _, Pedersen> = bonsai_storage + .get_transactional_state(id1, BonsaiStorageConfig::default()) + .unwrap() + .unwrap(); + bonsai_at_txn + .insert(&identifier, &pair2.0, &pair2.1) + .unwrap(); + + bonsai_storage.merge(bonsai_at_txn).unwrap(); + + // commit after merge + let revert_id = id_builder.new_id(); + bonsai_storage.commit(revert_id).unwrap(); + + // overwrite pair2 + bonsai_storage + .insert( + &identifier, + &pair2.0, + &Felt::from_hex("0x66342762FDD54D033c195fec3ce2568b62052F").unwrap(), + ) + .unwrap(); + + // revert to commit + bonsai_storage.revert_to(revert_id).unwrap(); + + assert_eq!( + bonsai_storage.get(&identifier, &pair2.0).unwrap(), + Some(pair2.1) + ); +} + +#[test] +fn merge_with_uncommitted_remove() { + let _ = env_logger::builder().is_test(true).try_init(); + log::set_max_level(LevelFilter::Trace); + + let identifier = vec![]; + let tempdir = tempfile::tempdir().unwrap(); + let db = create_rocks_db(tempdir.path()).unwrap(); + let config = BonsaiStorageConfig::default(); + let mut bonsai_storage = + BonsaiStorage::new(RocksDB::new(&db, RocksDBConfig::default()), config).unwrap(); + let mut id_builder = BasicIdBuilder::new(); + + let pair1 = ( + BitVec::from_vec(vec![1, 2, 2]), + Felt::from_hex("0x66342762FDD5D033c195fec3ce2568b62052e").unwrap(), + ); + let pair2 = ( + BitVec::from_vec(vec![1, 2, 3]), + Felt::from_hex("0x66342762FDD54D033c195fec3ce2568b62052e").unwrap(), + ); + + let id1 = id_builder.new_id(); + bonsai_storage + .insert(&identifier, &pair1.0, &pair1.1) + .unwrap(); + bonsai_storage.commit(id1).unwrap(); + + let mut bonsai_at_txn: BonsaiStorage<_, _, Pedersen> = bonsai_storage + .get_transactional_state(id1, BonsaiStorageConfig::default()) + .unwrap() + .unwrap(); + bonsai_at_txn + .insert(&identifier, &pair2.0, &pair2.1) + .unwrap(); + bonsai_at_txn + .transactional_commit(id_builder.new_id()) + .unwrap(); + + // remove pair2 but don't commit in transational state + bonsai_at_txn.remove(&identifier, &pair2.0).unwrap(); + assert_eq!( + bonsai_at_txn.contains(&identifier, &pair2.0).unwrap(), + false + ); + + let merge = bonsai_storage.merge(bonsai_at_txn); + match merge { + Ok(_) => println!("merge succeeded"), + Err(e) => { + println!("merge failed"); + panic!("{}", e); + } + }; + + // commit after merge + bonsai_storage.commit(id_builder.new_id()).unwrap(); + + assert_eq!( + bonsai_storage.contains(&identifier, &pair2.0).unwrap(), + false + ); +} + +#[test] +fn transactional_state_after_uncommitted() { + let _ = env_logger::builder().is_test(true).try_init(); + log::set_max_level(LevelFilter::Trace); + + let identifier = vec![]; + let tempdir = tempfile::tempdir().unwrap(); + let db = create_rocks_db(tempdir.path()).unwrap(); + let config = BonsaiStorageConfig::default(); + let mut bonsai_storage = + BonsaiStorage::new(RocksDB::new(&db, RocksDBConfig::default()), config).unwrap(); + let mut id_builder = BasicIdBuilder::new(); + + let pair1 = ( + BitVec::from_vec(vec![1, 2, 2]), + Felt::from_hex("0x66342762FDD5D033c195fec3ce2568b62052e").unwrap(), + ); + let pair2 = ( + BitVec::from_vec(vec![1, 2, 3]), + Felt::from_hex("0x66342762FDD54D033c195fec3ce2568b62052e").unwrap(), + ); + + let id1 = id_builder.new_id(); + bonsai_storage + .insert(&identifier, &pair1.0, &pair1.1) + .unwrap(); + bonsai_storage.commit(id1).unwrap(); + + // make a change to original tree but don't commit it + bonsai_storage + .insert(&identifier, &pair2.0, &pair2.1) + .unwrap(); + + // create a transactional state after the uncommitted change + let bonsai_at_txn: BonsaiStorage<_, _, Pedersen> = bonsai_storage + .get_transactional_state(id1, BonsaiStorageConfig::default()) + .unwrap() + .unwrap(); + + // uncommitted changes, done after the transactional state was created, + // are not included in the transactional state + let contains = bonsai_at_txn.contains(&identifier, &pair2.0).unwrap(); + assert_eq!(contains, false); +} + #[test] fn merge_override() { let identifier = vec![]; diff --git a/src/trie/merkle_tree.rs b/src/trie/merkle_tree.rs index ac9d1f2..579c4b1 100644 --- a/src/trie/merkle_tree.rs +++ b/src/trie/merkle_tree.rs @@ -160,10 +160,6 @@ impl MerkleTrees KeyValueDB { - self.db - } - pub(crate) fn root_hash( &self, identifier: &[u8], @@ -296,7 +292,7 @@ pub struct MerkleTree { /// The list of nodes that should be removed from the underlying database during the next commit. death_row: Vec, /// The list of leaves that have been modified during the current commit. - cache_leaf_modified: HashMap, InsertOrRemove>, + pub cache_leaf_modified: HashMap, InsertOrRemove>, /// The hasher used to hash the nodes. _hasher: PhantomData, } From cec867b3aef1b5144ac53c660fb3b6cc48452087 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Fran=C3=A7ois?= Date: Thu, 18 Apr 2024 17:27:02 +0200 Subject: [PATCH 2/3] PR comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jean-François --- src/lib.rs | 18 +----------------- src/trie/merkle_tree.rs | 8 ++++++-- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index a761d0a..cebc594 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -489,22 +489,6 @@ where transactional_bonsai_storage: BonsaiStorage, ) -> Result<(), BonsaiStorageError<>::DatabaseError>> { - // Applying changes (all cache_leaf_modified) before calling - //self.tries.db_mut().merge() work for all test but one - //(merge_with_uncommitted_remove, it fails with RocksDB.Busy error, - //which seems to be related to use of OptimisticTransactions, but - //removing them does not solve this particular problem but causes - //others). - - //Applying changes after call to merge() would imply a big refactor as - //merge take ownership of its arguments, hence changes are not - //available anymore. - - //Hence the solution which is a tradeoff between the two solutions: - //1. memorize changes - //2. merge tries - //3. apply changes - // memoryze changes let MerkleTrees { db, trees, .. } = transactional_bonsai_storage.tries; @@ -512,7 +496,7 @@ where // apply changes for (identifier, tree) in trees { - for (k, op) in tree.cache_leaf_modified { + for (k, op) in tree.cache_leaf_modified() { match op { crate::trie::merkle_tree::InsertOrRemove::Insert(v) => { self.insert(&identifier, &bytes_to_bitvec(&k), &v) diff --git a/src/trie/merkle_tree.rs b/src/trie/merkle_tree.rs index 579c4b1..122d506 100644 --- a/src/trie/merkle_tree.rs +++ b/src/trie/merkle_tree.rs @@ -292,7 +292,7 @@ pub struct MerkleTree { /// The list of nodes that should be removed from the underlying database during the next commit. death_row: Vec, /// The list of leaves that have been modified during the current commit. - pub cache_leaf_modified: HashMap, InsertOrRemove>, + cache_leaf_modified: HashMap, InsertOrRemove>, /// The hasher used to hash the nodes. _hasher: PhantomData, } @@ -363,6 +363,10 @@ impl MerkleTree { self.root_hash } + pub fn cache_leaf_modified(&self) -> &HashMap, InsertOrRemove> { + &self.cache_leaf_modified + } + /// Remove all the modifications that have been done since the last commit. pub fn reset_to_last_commit( &mut self, @@ -1488,7 +1492,7 @@ mod tests { use starknet_types_core::{felt::Felt, hash::Pedersen}; use crate::{ - databases::{create_rocks_db, HashMapDb, RocksDB, RocksDBConfig}, + databases::{create_rocks_db, RocksDB, RocksDBConfig}, id::BasicId, BonsaiStorage, BonsaiStorageConfig, }; From 055de365393e35551f085b406b8d544a5861a65e Mon Sep 17 00:00:00 2001 From: AurelienFT Date: Mon, 22 Apr 2024 10:19:00 +0200 Subject: [PATCH 3/3] Update version of starknet type core and fix error with no_std --- Cargo.toml | 2 +- ensure_no_std/Cargo.lock | 32 ++++++++------------------------ src/lib.rs | 26 ++++++++++++-------------- 3 files changed, 21 insertions(+), 39 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 11afd5b..0dfc007 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,7 @@ serde = { version = "1.0.195", default-features = false, features = [ "derive", "alloc", ] } -starknet-types-core = { git = "https://github.com/starknet-io/types-rs", branch = "main", default-features = false, features = [ +starknet-types-core = { version = "0.1", default-features = false, features = [ "hash", "parity-scale-codec", ] } diff --git a/ensure_no_std/Cargo.lock b/ensure_no_std/Cargo.lock index 30a9d11..689c82e 100644 --- a/ensure_no_std/Cargo.lock +++ b/ensure_no_std/Cargo.lock @@ -229,9 +229,9 @@ dependencies = [ [[package]] name = "lambdaworks-crypto" -version = "0.5.0" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2d4c222d5b2fdc0faf702d3ab361d14589b097f40eac9dc550e27083483edc65" +checksum = "458fee521f12d0aa97a2e06eaf134398a5d2ae7b2074af77eb402b0d93138c47" dependencies = [ "lambdaworks-math", "sha2", @@ -240,18 +240,9 @@ dependencies = [ [[package]] name = "lambdaworks-math" -version = "0.5.0" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9ee7dcab3968c71896b8ee4dc829147acc918cffe897af6265b1894527fe3add" - -[[package]] -name = "lazy_static" -version = "1.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" -dependencies = [ - "spin", -] +checksum = "6c74ce6f0d9cb672330b6ca59e85a6c3607a3329e0372ab0d3fe38c2d38e50f9" [[package]] name = "libc" @@ -300,9 +291,9 @@ dependencies = [ [[package]] name = "num-traits" -version = "0.2.17" +version = "0.2.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "39e3200413f237f41ab11ad6d161bc7239c84dcb631773ccd7de3dfe4b5c267c" +checksum = "da0df0e5185db44f69b44f26786fe401b6c293d1907744beaa7fa62b2e5a517a" dependencies = [ "autocfg", ] @@ -438,22 +429,15 @@ version = "1.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6ecd384b10a64542d77071bd64bd7b231f4ed5940fba55e98c3de13824cf3d7" -[[package]] -name = "spin" -version = "0.5.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d" - [[package]] name = "starknet-types-core" -version = "0.0.7" +version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6d791c671fecde494f435170a01c6fcb2949d0dd61be0b31b7c410b041609f96" +checksum = "1051b4f4af0bb9b546388a404873ee1e6b9787b9d5b0b3319ecbfadf315ef276" dependencies = [ "bitvec", "lambdaworks-crypto", "lambdaworks-math", - "lazy_static", "num-bigint", "num-integer", "num-traits", diff --git a/src/lib.rs b/src/lib.rs index cebc594..0272a72 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -488,8 +488,10 @@ where &mut self, transactional_bonsai_storage: BonsaiStorage, ) -> Result<(), BonsaiStorageError<>::DatabaseError>> + where + ::DatabaseError: core::fmt::Debug, { - // memoryze changes + // memorize changes let MerkleTrees { db, trees, .. } = transactional_bonsai_storage.tries; self.tries.db_mut().merge(db)?; @@ -499,25 +501,21 @@ where for (k, op) in tree.cache_leaf_modified() { match op { crate::trie::merkle_tree::InsertOrRemove::Insert(v) => { - self.insert(&identifier, &bytes_to_bitvec(&k), &v) + self.insert(&identifier, &bytes_to_bitvec(k), v) .map_err(|e| { BonsaiStorageError::Merge(format!( - "While merging insert({:?} {}) faced error: {}", - k, - v, - e.to_string() + "While merging insert({:?} {}) faced error: {:?}", + k, v, e )) })?; } crate::trie::merkle_tree::InsertOrRemove::Remove => { - self.remove(&identifier, &bytes_to_bitvec(&k)) - .map_err(|e| { - BonsaiStorageError::Merge(format!( - "While merging remove({:?}) faced error: {}", - k, - e.to_string() - )) - })?; + self.remove(&identifier, &bytes_to_bitvec(k)).map_err(|e| { + BonsaiStorageError::Merge(format!( + "While merging remove({:?}) faced error: {:?}", + k, e + )) + })?; } } }