From b587b0fcadcc61b9f46336ffee5621363e409092 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Tue, 26 Nov 2024 22:05:08 -0300 Subject: [PATCH 1/9] refactor(chain)!: impl sqlite for ConfirmationBlockTime anchored changesets We want to not depend on serde_json. If we keep it around for serializing anchors we won't be able to remove it in the future because it will always be needed to do migrations. Currently there is only one widely used anchor, ConfirmationBlockTime. The desicion was to constrain support to just be for a single anchor type ConfirmationBlockTime. The anchor table will represent all fields of ConfirmationBlockTime, each one in its own column. The reasons: - No one is using rusqlite with any other anchor type, and if they are, they can do something custom anyway. - The anchor representation may change in the future, supporting for multiple Anchor types here will cause more problems for migration later on. --- crates/chain/src/rusqlite_impl.rs | 41 ++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/crates/chain/src/rusqlite_impl.rs b/crates/chain/src/rusqlite_impl.rs index cf3d6bc9a..6b7228273 100644 --- a/crates/chain/src/rusqlite_impl.rs +++ b/crates/chain/src/rusqlite_impl.rs @@ -211,10 +211,7 @@ fn to_sql_error(err: E) -> rusqlit rusqlite::Error::ToSqlConversionFailure(Box::new(err)) } -impl tx_graph::ChangeSet -where - A: Anchor + Clone + Ord + serde::Serialize + serde::de::DeserializeOwned, -{ +impl tx_graph::ChangeSet { /// Schema name for [`tx_graph::ChangeSet`]. pub const SCHEMA_NAME: &'static str = "bdk_txgraph"; /// Name of table that stores full transactions and `last_seen` timestamps. @@ -260,7 +257,21 @@ where Self::TXS_TABLE_NAME, ), ]; - migrate_schema(db_tx, Self::SCHEMA_NAME, &[schema_v0]) + let schema_v1: &[&str] = &[ + &format!( + "ALTER TABLE {} ADD COLUMN confirmation_time INTEGER DEFAULT -1 NOT NULL", + Self::ANCHORS_TABLE_NAME, + ), + &format!( + "UPDATE {} SET confirmation_time = json_extract(anchor, '$.confirmation_time')", + Self::ANCHORS_TABLE_NAME, + ), + &format!( + "ALTER TABLE {} DROP COLUMN anchor", + Self::ANCHORS_TABLE_NAME, + ), + ]; + migrate_schema(db_tx, Self::SCHEMA_NAME, &[schema_v0, schema_v1]) } /// Construct a [`TxGraph`] from an sqlite database. @@ -314,18 +325,26 @@ where } let mut statement = db_tx.prepare(&format!( - "SELECT json(anchor), txid FROM {}", + "SELECT block_hash, block_height, confirmation_time, txid FROM {}", Self::ANCHORS_TABLE_NAME, ))?; let row_iter = statement.query_map([], |row| { Ok(( - row.get::<_, AnchorImpl>("json(anchor)")?, + row.get::<_, Impl>("block_hash")?, + row.get::<_, u32>("block_height")?, + row.get::<_, u64>("confirmation_time")?, row.get::<_, Impl>("txid")?, )) })?; for row in row_iter { - let (AnchorImpl(anchor), Impl(txid)) = row?; - changeset.anchors.insert((anchor, txid)); + let (hash, height, confirmation_time, Impl(txid)) = row?; + changeset.anchors.insert(( + ConfirmationBlockTime { + block_id: BlockId::from((&height, &hash.0)), + confirmation_time, + }, + txid, + )); } Ok(changeset) @@ -373,7 +392,7 @@ where } let mut statement = db_tx.prepare_cached(&format!( - "REPLACE INTO {}(txid, block_height, block_hash, anchor) VALUES(:txid, :block_height, :block_hash, jsonb(:anchor))", + "REPLACE INTO {}(txid, block_height, block_hash, confirmation_time) VALUES(:txid, :block_height, :block_hash, :confirmation_time)", Self::ANCHORS_TABLE_NAME, ))?; let mut statement_txid = db_tx.prepare_cached(&format!( @@ -389,7 +408,7 @@ where ":txid": Impl(*txid), ":block_height": anchor_block.height, ":block_hash": Impl(anchor_block.hash), - ":anchor": AnchorImpl(anchor.clone()), + ":confirmation_time": anchor.confirmation_time, })?; } From 7319f510016014a2ce4f9ab6e194e36345829357 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Thu, 28 Nov 2024 20:58:36 -0300 Subject: [PATCH 2/9] refactor(chain)!: remove AnchorImpl wrapper for Anchor implementors AnchorImpl was a wrapper created to allow the implementation of foreign traits, like From/ToJson from serde_json for external unknown structs implementing the Anchor trait. As the Anchor generic in the rusqlite implementation for anchored ChangeSets was the only place where this AnchorImpl was used and it has been fixed to the anchor ConfirmationBlockTime, there is no more reason to keep this wrapper around. --- crates/chain/src/lib.rs | 24 ------------------------ crates/chain/src/rusqlite_impl.rs | 16 ---------------- 2 files changed, 40 deletions(-) diff --git a/crates/chain/src/lib.rs b/crates/chain/src/lib.rs index 8d87da583..557d53494 100644 --- a/crates/chain/src/lib.rs +++ b/crates/chain/src/lib.rs @@ -100,27 +100,3 @@ impl core::ops::Deref for Impl { &self.0 } } - -/// A wrapper that we use to impl remote traits for types in our crate or dependency crates that impl [`Anchor`]. -pub struct AnchorImpl(pub T); - -impl AnchorImpl { - /// Returns the inner `T`. - pub fn into_inner(self) -> T { - self.0 - } -} - -impl From for AnchorImpl { - fn from(value: T) -> Self { - Self(value) - } -} - -impl core::ops::Deref for AnchorImpl { - type Target = T; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} diff --git a/crates/chain/src/rusqlite_impl.rs b/crates/chain/src/rusqlite_impl.rs index 6b7228273..f3abca0f2 100644 --- a/crates/chain/src/rusqlite_impl.rs +++ b/crates/chain/src/rusqlite_impl.rs @@ -157,22 +157,6 @@ impl ToSql for Impl { } } -impl FromSql for AnchorImpl { - fn column_result(value: ValueRef<'_>) -> FromSqlResult { - serde_json::from_str(value.as_str()?) - .map(AnchorImpl) - .map_err(from_sql_error) - } -} - -impl ToSql for AnchorImpl { - fn to_sql(&self) -> rusqlite::Result> { - serde_json::to_string(&self.0) - .map(Into::into) - .map_err(to_sql_error) - } -} - #[cfg(feature = "miniscript")] impl FromSql for Impl> { fn column_result(value: ValueRef<'_>) -> FromSqlResult { From d41cb62193973481cb18bb40d61a4b17609a8248 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Thu, 28 Nov 2024 21:10:53 -0300 Subject: [PATCH 3/9] build(chain): remove serde_json dependency --- crates/chain/Cargo.toml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/chain/Cargo.toml b/crates/chain/Cargo.toml index cfdaa1cab..2dc7d9cc8 100644 --- a/crates/chain/Cargo.toml +++ b/crates/chain/Cargo.toml @@ -23,7 +23,6 @@ miniscript = { version = "12.0.0", optional = true, default-features = false } # Feature dependencies rusqlite = { version = "0.31.0", features = ["bundled"], optional = true } -serde_json = { version = "1", optional = true } [dev-dependencies] rand = "0.8" @@ -36,4 +35,4 @@ default = ["std", "miniscript"] std = ["bitcoin/std", "miniscript?/std", "bdk_core/std"] serde = ["dep:serde", "bitcoin/serde", "miniscript?/serde", "bdk_core/serde"] hashbrown = ["bdk_core/hashbrown"] -rusqlite = ["std", "dep:rusqlite", "serde", "serde_json"] +rusqlite = ["std", "dep:rusqlite", "serde"] From c3ea54d3bc8383148f86800bc60d2469486885a4 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Thu, 28 Nov 2024 21:42:13 -0300 Subject: [PATCH 4/9] test(wallet): check persisted anchors does not lose data --- crates/wallet/tests/wallet.rs | 52 +++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs index 05baa5c33..f7bf4363f 100644 --- a/crates/wallet/tests/wallet.rs +++ b/crates/wallet/tests/wallet.rs @@ -223,6 +223,58 @@ fn wallet_load_checks() -> anyhow::Result<()> { Ok(()) } +#[test] +fn wallet_should_persist_anchors_and_recover() { + use bdk_chain::rusqlite; + let temp_dir = tempfile::tempdir().unwrap(); + let db_path = temp_dir.path().join("wallet.db"); + let mut db = rusqlite::Connection::open(db_path).unwrap(); + + let desc = get_test_tr_single_sig_xprv(); + let mut wallet = Wallet::create_single(desc) + .network(Network::Testnet) + .create_wallet(&mut db) + .unwrap(); + let small_output_tx = Transaction { + input: vec![], + output: vec![TxOut { + script_pubkey: wallet + .next_unused_address(KeychainKind::External) + .script_pubkey(), + value: Amount::from_sat(25_000), + }], + version: transaction::Version::non_standard(0), + lock_time: absolute::LockTime::ZERO, + }; + let txid = small_output_tx.compute_txid(); + insert_tx(&mut wallet, small_output_tx); + let anchor = ConfirmationBlockTime { + block_id: wallet.latest_checkpoint().block_id(), + confirmation_time: 200, + }; + insert_anchor(&mut wallet, txid, anchor); + assert!(wallet.persist(&mut db).unwrap()); + + // should recover persisted wallet + let secp = wallet.secp_ctx(); + let (_, keymap) = >::parse_descriptor(secp, desc).unwrap(); + assert!(!keymap.is_empty()); + let wallet = Wallet::load() + .descriptor(KeychainKind::External, Some(desc)) + .extract_keys() + .load_wallet(&mut db) + .unwrap() + .expect("must have loaded changeset"); + // stored anchor should be retrieved in the same condition it was persisted + assert_eq!( + wallet + .get_tx(txid) + .expect("should retrieve stored tx") + .chain_position, + ChainPosition::Confirmed(&anchor), + ); +} + #[test] fn single_descriptor_wallet_persist_and_recover() { use bdk_chain::miniscript::Descriptor; From 5603e9f6d1d3ff130317d6fd16332bf8b049b409 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Fri, 29 Nov 2024 12:58:40 -0300 Subject: [PATCH 5/9] test(chain): use ChangeSet instead of ChangeSet The only struct implementing rustqlite is ChangeSet from c49ea85423a20ebc29e3bf4ae3a6d2acdc78b8a5 on. --- crates/chain/src/rusqlite_impl.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/chain/src/rusqlite_impl.rs b/crates/chain/src/rusqlite_impl.rs index f3abca0f2..346cee792 100644 --- a/crates/chain/src/rusqlite_impl.rs +++ b/crates/chain/src/rusqlite_impl.rs @@ -549,7 +549,7 @@ mod test { #[test] fn can_persist_anchors_and_txs_independently() -> anyhow::Result<()> { - type ChangeSet = tx_graph::ChangeSet; + type ChangeSet = tx_graph::ChangeSet; let mut conn = rusqlite::Connection::open_in_memory()?; // init tables @@ -567,9 +567,12 @@ mod test { }; let tx = Arc::new(tx); let txid = tx.compute_txid(); - let anchor = BlockId { - height: 21, - hash: hash!("anchor"), + let anchor = ConfirmationBlockTime { + block_id: BlockId { + height: 21, + hash: hash!("anchor"), + }, + confirmation_time: 1342, }; // First persist the anchor From 2eb19f5073abd7d5e9c9b4183aa5efac6e76aca9 Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Mon, 2 Dec 2024 12:26:55 -0300 Subject: [PATCH 6/9] refactor(chain)!: move schemas to their own methods We would like to test backward compatibility of new schemas. To do so, we should be able to apply schemas independently. Why to change `rusqlite::execute` by `rusqlite::execute_batch`? - we are going to need to return the statements of the schemas as Strings, because we are now returning them from methods, it seemed redundant to keep getting references to something is already referencing data, i.e., keep moving around &String instead of String (consider `rusqlite::execute_batch` takes multiple statements as a single String) - we were calling `rusqlite::execute` with empty params, so we weren't trapped by the method's signature - `rustqlite::execute_batch` does the same than we were doing applying statements secuentially in a loop - the code doesn't lose error expresivity: `rusqlite::execute_batch` is going to fail with the same errors `rusqlite::execute` does BREAKING CHANGE: changes public method `migrate_schema` signature. --- crates/chain/src/rusqlite_impl.rs | 172 ++++++++++++++------------ crates/wallet/src/wallet/changeset.rs | 19 ++- 2 files changed, 109 insertions(+), 82 deletions(-) diff --git a/crates/chain/src/rusqlite_impl.rs b/crates/chain/src/rusqlite_impl.rs index 346cee792..bcb379a70 100644 --- a/crates/chain/src/rusqlite_impl.rs +++ b/crates/chain/src/rusqlite_impl.rs @@ -3,7 +3,13 @@ use crate::*; use core::str::FromStr; -use alloc::{borrow::ToOwned, boxed::Box, string::ToString, sync::Arc, vec::Vec}; +use alloc::{ + borrow::ToOwned, + boxed::Box, + string::{String, ToString}, + sync::Arc, + vec::Vec, +}; use bitcoin::consensus::{Decodable, Encodable}; use rusqlite; use rusqlite::named_params; @@ -55,17 +61,15 @@ fn set_schema_version( pub fn migrate_schema( db_tx: &Transaction, schema_name: &str, - versioned_scripts: &[&[&str]], + versioned_scripts: &[String], ) -> rusqlite::Result<()> { init_schemas_table(db_tx)?; let current_version = schema_version(db_tx, schema_name)?; let exec_from = current_version.map_or(0_usize, |v| v as usize + 1); let scripts_to_exec = versioned_scripts.iter().enumerate().skip(exec_from); - for (version, &script) in scripts_to_exec { + for (version, script) in scripts_to_exec { set_schema_version(db_tx, schema_name, version as u32)?; - for statement in script { - db_tx.execute(statement, ())?; - } + db_tx.execute_batch(script)?; } Ok(()) } @@ -205,57 +209,68 @@ impl tx_graph::ChangeSet { /// Name of table that stores [`Anchor`]s. pub const ANCHORS_TABLE_NAME: &'static str = "bdk_anchors"; + /// Get v0 of sqlite [tx_graph::ChangeSet] schema + pub fn schema_v0() -> String { + // full transactions + let create_txs_table = format!( + "CREATE TABLE {} ( \ + txid TEXT PRIMARY KEY NOT NULL, \ + raw_tx BLOB, \ + last_seen INTEGER \ + ) STRICT", + Self::TXS_TABLE_NAME, + ); + // floating txouts + let create_txouts_table = format!( + "CREATE TABLE {} ( \ + txid TEXT NOT NULL, \ + vout INTEGER NOT NULL, \ + value INTEGER NOT NULL, \ + script BLOB NOT NULL, \ + PRIMARY KEY (txid, vout) \ + ) STRICT", + Self::TXOUTS_TABLE_NAME, + ); + // anchors + let create_anchors_table = format!( + "CREATE TABLE {} ( \ + txid TEXT NOT NULL REFERENCES {} (txid), \ + block_height INTEGER NOT NULL, \ + block_hash TEXT NOT NULL, \ + anchor BLOB NOT NULL, \ + PRIMARY KEY (txid, block_height, block_hash) \ + ) STRICT", + Self::ANCHORS_TABLE_NAME, + Self::TXS_TABLE_NAME, + ); + + format!("{create_txs_table}; {create_txouts_table}; {create_anchors_table}") + } + + /// Get v1 of sqlite [tx_graph::ChangeSet] schema + pub fn schema_v1() -> String { + let add_confirmation_time_column = format!( + "ALTER TABLE {} ADD COLUMN confirmation_time INTEGER DEFAULT -1 NOT NULL", + Self::ANCHORS_TABLE_NAME, + ); + let extract_confirmation_time_from_anchor_column = format!( + "UPDATE {} SET confirmation_time = json_extract(anchor, '$.confirmation_time')", + Self::ANCHORS_TABLE_NAME, + ); + let drop_anchor_column = format!( + "ALTER TABLE {} DROP COLUMN anchor", + Self::ANCHORS_TABLE_NAME, + ); + format!("{add_confirmation_time_column}; {extract_confirmation_time_from_anchor_column}; {drop_anchor_column}") + } + /// Initialize sqlite tables. pub fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> { - let schema_v0: &[&str] = &[ - // full transactions - &format!( - "CREATE TABLE {} ( \ - txid TEXT PRIMARY KEY NOT NULL, \ - raw_tx BLOB, \ - last_seen INTEGER \ - ) STRICT", - Self::TXS_TABLE_NAME, - ), - // floating txouts - &format!( - "CREATE TABLE {} ( \ - txid TEXT NOT NULL, \ - vout INTEGER NOT NULL, \ - value INTEGER NOT NULL, \ - script BLOB NOT NULL, \ - PRIMARY KEY (txid, vout) \ - ) STRICT", - Self::TXOUTS_TABLE_NAME, - ), - // anchors - &format!( - "CREATE TABLE {} ( \ - txid TEXT NOT NULL REFERENCES {} (txid), \ - block_height INTEGER NOT NULL, \ - block_hash TEXT NOT NULL, \ - anchor BLOB NOT NULL, \ - PRIMARY KEY (txid, block_height, block_hash) \ - ) STRICT", - Self::ANCHORS_TABLE_NAME, - Self::TXS_TABLE_NAME, - ), - ]; - let schema_v1: &[&str] = &[ - &format!( - "ALTER TABLE {} ADD COLUMN confirmation_time INTEGER DEFAULT -1 NOT NULL", - Self::ANCHORS_TABLE_NAME, - ), - &format!( - "UPDATE {} SET confirmation_time = json_extract(anchor, '$.confirmation_time')", - Self::ANCHORS_TABLE_NAME, - ), - &format!( - "ALTER TABLE {} DROP COLUMN anchor", - Self::ANCHORS_TABLE_NAME, - ), - ]; - migrate_schema(db_tx, Self::SCHEMA_NAME, &[schema_v0, schema_v1]) + migrate_schema( + db_tx, + Self::SCHEMA_NAME, + &[Self::schema_v0(), Self::schema_v1()], + ) } /// Construct a [`TxGraph`] from an sqlite database. @@ -406,19 +421,21 @@ impl local_chain::ChangeSet { /// Name of sqlite table that stores blocks of [`LocalChain`](local_chain::LocalChain). pub const BLOCKS_TABLE_NAME: &'static str = "bdk_blocks"; + /// Get v0 of sqlite [local_chain::ChangeSet] schema + pub fn schema_v0() -> String { + // blocks + format!( + "CREATE TABLE {} ( \ + block_height INTEGER PRIMARY KEY NOT NULL, \ + block_hash TEXT NOT NULL \ + ) STRICT", + Self::BLOCKS_TABLE_NAME, + ) + } + /// Initialize sqlite tables for persisting [`local_chain::LocalChain`]. pub fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> { - let schema_v0: &[&str] = &[ - // blocks - &format!( - "CREATE TABLE {} ( \ - block_height INTEGER PRIMARY KEY NOT NULL, \ - block_hash TEXT NOT NULL \ - ) STRICT", - Self::BLOCKS_TABLE_NAME, - ), - ]; - migrate_schema(db_tx, Self::SCHEMA_NAME, &[schema_v0]) + migrate_schema(db_tx, Self::SCHEMA_NAME, &[Self::schema_v0()]) } /// Construct a [`LocalChain`](local_chain::LocalChain) from sqlite database. @@ -480,20 +497,21 @@ impl keychain_txout::ChangeSet { /// Name for table that stores last revealed indices per descriptor id. pub const LAST_REVEALED_TABLE_NAME: &'static str = "bdk_descriptor_last_revealed"; + /// Get v0 of sqlite [keychain_txout::ChangeSet] schema + pub fn schema_v0() -> String { + format!( + "CREATE TABLE {} ( \ + descriptor_id TEXT PRIMARY KEY NOT NULL, \ + last_revealed INTEGER NOT NULL \ + ) STRICT", + Self::LAST_REVEALED_TABLE_NAME, + ) + } + /// Initialize sqlite tables for persisting /// [`KeychainTxOutIndex`](keychain_txout::KeychainTxOutIndex). pub fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> { - let schema_v0: &[&str] = &[ - // last revealed - &format!( - "CREATE TABLE {} ( \ - descriptor_id TEXT PRIMARY KEY NOT NULL, \ - last_revealed INTEGER NOT NULL \ - ) STRICT", - Self::LAST_REVEALED_TABLE_NAME, - ), - ]; - migrate_schema(db_tx, Self::SCHEMA_NAME, &[schema_v0]) + migrate_schema(db_tx, Self::SCHEMA_NAME, &[Self::schema_v0()]) } /// Construct [`KeychainTxOutIndex`](keychain_txout::KeychainTxOutIndex) from sqlite database diff --git a/crates/wallet/src/wallet/changeset.rs b/crates/wallet/src/wallet/changeset.rs index 9c75a289d..5a67fcd07 100644 --- a/crates/wallet/src/wallet/changeset.rs +++ b/crates/wallet/src/wallet/changeset.rs @@ -1,3 +1,4 @@ +use alloc::string::String; use bdk_chain::{ indexed_tx_graph, keychain_txout, local_chain, tx_graph, ConfirmationBlockTime, Merge, }; @@ -71,9 +72,9 @@ impl ChangeSet { /// Name of table to store wallet descriptors and network. pub const WALLET_TABLE_NAME: &'static str = "bdk_wallet"; - /// Initialize sqlite tables for wallet tables. - pub fn init_sqlite_tables(db_tx: &chain::rusqlite::Transaction) -> chain::rusqlite::Result<()> { - let schema_v0: &[&str] = &[&format!( + /// Get v0 sqlite [ChangeSet] schema + pub fn schema_v0() -> String { + format!( "CREATE TABLE {} ( \ id INTEGER PRIMARY KEY NOT NULL CHECK (id = 0), \ descriptor TEXT, \ @@ -81,8 +82,16 @@ impl ChangeSet { network TEXT \ ) STRICT;", Self::WALLET_TABLE_NAME, - )]; - crate::rusqlite_impl::migrate_schema(db_tx, Self::WALLET_SCHEMA_NAME, &[schema_v0])?; + ) + } + + /// Initialize sqlite tables for wallet tables. + pub fn init_sqlite_tables(db_tx: &chain::rusqlite::Transaction) -> chain::rusqlite::Result<()> { + crate::rusqlite_impl::migrate_schema( + db_tx, + Self::WALLET_SCHEMA_NAME, + &[Self::schema_v0()], + )?; bdk_chain::local_chain::ChangeSet::init_sqlite_tables(db_tx)?; bdk_chain::tx_graph::ChangeSet::::init_sqlite_tables(db_tx)?; From 4ec7940158e80c0e8d657ffa07f540de81e3cbfa Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Mon, 2 Dec 2024 12:29:14 -0300 Subject: [PATCH 7/9] test(wallet): pattern match ChainPosition::Confirmed in anchors persistence check --- crates/wallet/tests/wallet.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs index f7bf4363f..4a53c350f 100644 --- a/crates/wallet/tests/wallet.rs +++ b/crates/wallet/tests/wallet.rs @@ -248,11 +248,11 @@ fn wallet_should_persist_anchors_and_recover() { }; let txid = small_output_tx.compute_txid(); insert_tx(&mut wallet, small_output_tx); - let anchor = ConfirmationBlockTime { + let expected_anchor = ConfirmationBlockTime { block_id: wallet.latest_checkpoint().block_id(), confirmation_time: 200, }; - insert_anchor(&mut wallet, txid, anchor); + insert_anchor(&mut wallet, txid, expected_anchor); assert!(wallet.persist(&mut db).unwrap()); // should recover persisted wallet @@ -266,13 +266,18 @@ fn wallet_should_persist_anchors_and_recover() { .unwrap() .expect("must have loaded changeset"); // stored anchor should be retrieved in the same condition it was persisted - assert_eq!( - wallet - .get_tx(txid) - .expect("should retrieve stored tx") - .chain_position, - ChainPosition::Confirmed(&anchor), - ); + if let ChainPosition::Confirmed { + anchor: &obtained_anchor, + .. + } = wallet + .get_tx(txid) + .expect("should retrieve stored tx") + .chain_position + { + assert_eq!(obtained_anchor, expected_anchor) + } else { + panic!("Should have got ChainPosition::Confirmed)"); + } } #[test] From 265b59b6ef1c148875f4d6601e5a7e5f0b01785a Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Wed, 4 Dec 2024 12:36:57 -0300 Subject: [PATCH 8/9] test(chain): add compatibility test for v0 to v1 sqlite schema migration Why just v0 to v1 test and not a general backward compatibility test? Is harder to craft a general compatibility test without prior knowledge of how future schemas would look like. Also, the creation of a backward compatibility test for each new schema change will allow the execution of incremental backward compatibility tests with better granularity. --- crates/chain/src/rusqlite_impl.rs | 92 +++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/crates/chain/src/rusqlite_impl.rs b/crates/chain/src/rusqlite_impl.rs index bcb379a70..2a52a2ac1 100644 --- a/crates/chain/src/rusqlite_impl.rs +++ b/crates/chain/src/rusqlite_impl.rs @@ -626,4 +626,96 @@ mod test { Ok(()) } + + #[test] + fn v0_to_v1_schema_migration_is_backward_compatible() -> anyhow::Result<()> { + type ChangeSet = tx_graph::ChangeSet; + let mut conn = rusqlite::Connection::open_in_memory()?; + + // Create initial database with v0 sqlite schema + { + let db_tx = conn.transaction()?; + migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[ChangeSet::schema_v0()])?; + db_tx.commit()?; + } + + let tx = bitcoin::Transaction { + version: transaction::Version::TWO, + lock_time: absolute::LockTime::ZERO, + input: vec![TxIn::default()], + output: vec![TxOut::NULL], + }; + let tx = Arc::new(tx); + let txid = tx.compute_txid(); + let anchor = ConfirmationBlockTime { + block_id: BlockId { + height: 21, + hash: hash!("anchor"), + }, + confirmation_time: 1342, + }; + + // Persist anchor with v0 sqlite schema + { + let changeset = ChangeSet { + anchors: [(anchor, txid)].into(), + ..Default::default() + }; + let mut statement = conn.prepare_cached(&format!( + "REPLACE INTO {} (txid, block_height, block_hash, anchor) + VALUES( + :txid, + :block_height, + :block_hash, + jsonb('{{ + \"block_id\": {{\"height\": {},\"hash\":\"{}\"}}, + \"confirmation_time\": {} + }}') + )", + ChangeSet::ANCHORS_TABLE_NAME, + anchor.block_id.height, + anchor.block_id.hash, + anchor.confirmation_time, + ))?; + let mut statement_txid = conn.prepare_cached(&format!( + "INSERT OR IGNORE INTO {}(txid) VALUES(:txid)", + ChangeSet::TXS_TABLE_NAME, + ))?; + for (anchor, txid) in &changeset.anchors { + let anchor_block = anchor.anchor_block(); + statement_txid.execute(named_params! { + ":txid": Impl(*txid) + })?; + match statement.execute(named_params! { + ":txid": Impl(*txid), + ":block_height": anchor_block.height, + ":block_hash": Impl(anchor_block.hash), + }) { + Ok(updated) => assert_eq!(updated, 1), + Err(err) => panic!("update failed: {}", err), + } + } + } + + // Apply v1 sqlite schema to tables with data + { + let db_tx = conn.transaction()?; + migrate_schema( + &db_tx, + ChangeSet::SCHEMA_NAME, + &[ChangeSet::schema_v0(), ChangeSet::schema_v1()], + )?; + db_tx.commit()?; + } + + // Loading changeset from sqlite should succeed + { + let db_tx = conn.transaction()?; + let changeset = ChangeSet::from_sqlite(&db_tx)?; + db_tx.commit()?; + assert!(changeset.anchors.contains(&(anchor, txid))); + } + + Ok(()) + } } From 1c81cd53fb859f4fa9e416c1a776f5295f7da44b Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Wed, 4 Dec 2024 14:08:43 -0300 Subject: [PATCH 9/9] refactor(chain)!: change String by &str for versioned scripts in migrate_schema `&str` is documenting clearly that `migrate_schema` should only read `versioned_strings`. Also, as `schema_vN` methods will return `String`, rust will always automatically deref `&String` to `&str`. BREAKING CHANGE: changes parameter versioned_strings from public function migrate_schema from type &[String] to &[&str]. --- crates/chain/src/rusqlite_impl.rs | 12 ++++++------ crates/wallet/src/wallet/changeset.rs | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/chain/src/rusqlite_impl.rs b/crates/chain/src/rusqlite_impl.rs index 2a52a2ac1..7b39f53c0 100644 --- a/crates/chain/src/rusqlite_impl.rs +++ b/crates/chain/src/rusqlite_impl.rs @@ -61,7 +61,7 @@ fn set_schema_version( pub fn migrate_schema( db_tx: &Transaction, schema_name: &str, - versioned_scripts: &[String], + versioned_scripts: &[&str], ) -> rusqlite::Result<()> { init_schemas_table(db_tx)?; let current_version = schema_version(db_tx, schema_name)?; @@ -269,7 +269,7 @@ impl tx_graph::ChangeSet { migrate_schema( db_tx, Self::SCHEMA_NAME, - &[Self::schema_v0(), Self::schema_v1()], + &[&Self::schema_v0(), &Self::schema_v1()], ) } @@ -435,7 +435,7 @@ impl local_chain::ChangeSet { /// Initialize sqlite tables for persisting [`local_chain::LocalChain`]. pub fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> { - migrate_schema(db_tx, Self::SCHEMA_NAME, &[Self::schema_v0()]) + migrate_schema(db_tx, Self::SCHEMA_NAME, &[&Self::schema_v0()]) } /// Construct a [`LocalChain`](local_chain::LocalChain) from sqlite database. @@ -511,7 +511,7 @@ impl keychain_txout::ChangeSet { /// Initialize sqlite tables for persisting /// [`KeychainTxOutIndex`](keychain_txout::KeychainTxOutIndex). pub fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> { - migrate_schema(db_tx, Self::SCHEMA_NAME, &[Self::schema_v0()]) + migrate_schema(db_tx, Self::SCHEMA_NAME, &[&Self::schema_v0()]) } /// Construct [`KeychainTxOutIndex`](keychain_txout::KeychainTxOutIndex) from sqlite database @@ -635,7 +635,7 @@ mod test { // Create initial database with v0 sqlite schema { let db_tx = conn.transaction()?; - migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[ChangeSet::schema_v0()])?; + migrate_schema(&db_tx, ChangeSet::SCHEMA_NAME, &[&ChangeSet::schema_v0()])?; db_tx.commit()?; } @@ -703,7 +703,7 @@ mod test { migrate_schema( &db_tx, ChangeSet::SCHEMA_NAME, - &[ChangeSet::schema_v0(), ChangeSet::schema_v1()], + &[&ChangeSet::schema_v0(), &ChangeSet::schema_v1()], )?; db_tx.commit()?; } diff --git a/crates/wallet/src/wallet/changeset.rs b/crates/wallet/src/wallet/changeset.rs index 5a67fcd07..36a5b4936 100644 --- a/crates/wallet/src/wallet/changeset.rs +++ b/crates/wallet/src/wallet/changeset.rs @@ -90,7 +90,7 @@ impl ChangeSet { crate::rusqlite_impl::migrate_schema( db_tx, Self::WALLET_SCHEMA_NAME, - &[Self::schema_v0()], + &[&Self::schema_v0()], )?; bdk_chain::local_chain::ChangeSet::init_sqlite_tables(db_tx)?;