From 052f9781450361f2d9d3f7b68aa87e1af0b56701 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 07:29:03 -0700 Subject: [PATCH 01/18] fix(test): isolate each test in its own database, remove destructive DropAll Security review finding H5 (high): the DropAll test helper hardcoded DROP TABLE statements for 15 unqualified table names, including tables belonging to a different application (users, psbts, vault_*, _sqlx_migrations). Running the test suite with a mispointed DATABASE_TEST_URL silently and irreversibly destroyed those tables, while never actually cleaning the crate's own schema-qualified bdk_wallet.* tables - which is why tests contaminated each other and required --test-threads=1. - Delete the DropAll trait/impl and the dead _drop_tables helper; the test suite no longer issues a single DROP TABLE. - create_test_stores now creates a uniquely named bdk_sqlx_test_* database per store, so tests are isolated, parallel-safe, and repeatable. Leftover test databases are reaped opportunistically, never while any session is connected, with creation/cleanup serialized against races. - Use Store::new_with_url(None, ..) for the sqlite in-memory store so the pool is correctly limited to a single connection. - Also fixes L8 (redundant url.clone()). - README: document the new test database behavior; drop --test-threads=1. Fixes a broken baseline: 3 of 5 tests failed on a shared database due to leftover wallet data. Full suite now passes in parallel and on repeat runs. --- Cargo.toml | 2 +- README.md | 17 ++++--- src/test.rs | 132 ++++++++++++++++++++++------------------------------ 3 files changed, 65 insertions(+), 86 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e181ee1..fb1e805 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,7 +9,7 @@ serde = { version = "1.0.208", features = ["derive"] } serde_json = "1.0.125" sqlx = { version = "0.8.1", default-features = false, features = ["runtime-tokio", "tls-rustls-ring","derive", "postgres", "sqlite", "json", "chrono", "uuid", "sqlx-macros", "migrate"] } thiserror = "1" -tokio = { version = "1.40.0", features = ["macros", "rt-multi-thread"] } +tokio = { version = "1.40.0", features = ["macros", "rt-multi-thread", "sync"] } tracing = "0.1.40" tracing-subscriber = { version = "0.3.18", features = ["env-filter", "serde_json", "json"] } sqlx-postgres-tester = "0.1.1" diff --git a/README.md b/README.md index 6ce4bdb..ffdedb1 100644 --- a/README.md +++ b/README.md @@ -11,18 +11,17 @@ This crate is still **EXPERIMENTAL** do not use with mainnet wallets. brew update brew install postgresql ``` -2. Create empty test database: +2. Set DATABASE_TEST_URL to a postgres server the tests may use: ``` - psql postgres - postgres=# create database test_bdk_wallet; - ``` -3. Set DATABASE_URL to test database: - ``` - export DATABASE_TEST_URL=postgresql://localhost/test_bdk_wallet + export DATABASE_TEST_URL=postgresql://localhost/postgres ``` -4. Run tests, must use a single test thread since we reuse the postgres db: + The connected role must be allowed to `CREATE DATABASE`: every test creates + (and later cleans up) its own uniquely named `bdk_sqlx_test_*` database, so + tests never touch existing data and are safe to run in parallel. Do not + point this at a production server. +3. Run tests: ``` - cargo test -- --test-threads=1 + cargo test ``` ## Example diff --git a/src/test.rs b/src/test.rs index 9a4b7fd..65cf2c6 100644 --- a/src/test.rs +++ b/src/test.rs @@ -1,6 +1,7 @@ use std::env; use std::ops::Add; use std::str::FromStr; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Once; use assert_matches::assert_matches; @@ -21,7 +22,8 @@ use bitcoin::{ Network::{self, Regtest}, OutPoint, Transaction, TxIn, TxOut, Txid, }; -use sqlx::{Pool, Postgres, Sqlite, SqlitePool}; +use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; +use sqlx::{Pool, Postgres, Sqlite}; use test_utils::{ get_test_tr_single_sig_xprv_and_change_desc, get_test_wpkh, insert_anchor, insert_checkpoint, insert_tx, new_tx, @@ -59,45 +61,56 @@ fn initialize() { }); } -trait DropAll { - async fn drop_all(&self) -> anyhow::Result<()>; -} +static TEST_DB_COUNTER: AtomicU64 = AtomicU64::new(0); +static TEST_DB_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); -impl DropAll for Pool { - /// Drops all tables. - /// - /// Clean up (optional, depending on your test database strategy) - /// You might want to delete the test wallet from the database here. - #[tracing::instrument] - async fn drop_all(&self) -> anyhow::Result<()> { - let drop_statements = vec![ - "DROP TABLE IF EXISTS _sqlx_migrations", - "DROP TABLE IF EXISTS vault_addresses", - "DROP TABLE IF EXISTS used_anchorwatch_keys", - "DROP TABLE IF EXISTS anchorwatch_keys", - "DROP TABLE IF EXISTS psbts", - "DROP TABLE IF EXISTS whitelist_update", - "DROP TABLE IF EXISTS vault_parameters", - "DROP TABLE IF EXISTS users", - "DROP TABLE IF EXISTS version", - "DROP TABLE IF EXISTS anchor_tx", - "DROP TABLE IF EXISTS txout", - "DROP TABLE IF EXISTS tx", - "DROP TABLE IF EXISTS block", - "DROP TABLE IF EXISTS keychain", - "DROP TABLE IF EXISTS network", - ]; - - let mut tx = self.begin().await?; - - for statement in drop_statements { - sqlx::query(statement).execute(&mut *tx).await?; - } - - tx.commit().await?; +/// Creates a uniquely named database on the postgres server at `DATABASE_TEST_URL` and +/// returns a pool connected to it, so every test gets an isolated database and no +/// pre-existing tables are ever dropped. +/// +/// Databases left behind by previous test runs are removed opportunistically; a database +/// is never dropped while any session is connected to it, and creation/cleanup are +/// serialized so a parallel test cannot drop a database between its creation and first +/// connection. +async fn create_test_pg_pool() -> anyhow::Result> { + let admin_url = env::var("DATABASE_TEST_URL").expect("DATABASE_TEST_URL must be set for tests"); + let admin_pool = Pool::::connect(&admin_url).await?; + + let db_name = format!( + "bdk_sqlx_test_{}_{}", + std::process::id(), + TEST_DB_COUNTER.fetch_add(1, Ordering::Relaxed) + ); - Ok(()) + let guard = TEST_DB_LOCK.lock().await; + + let stale: Vec = sqlx::query_scalar( + "SELECT datname::text FROM pg_database d + WHERE datname LIKE 'bdk_sqlx_test_%' + AND NOT EXISTS (SELECT 1 FROM pg_stat_activity a WHERE a.datname = d.datname)", + ) + .fetch_all(&admin_pool) + .await?; + for stale_db in stale { + let _ = sqlx::query(&format!(r#"DROP DATABASE IF EXISTS "{stale_db}""#)) + .execute(&admin_pool) + .await; } + + sqlx::query(&format!(r#"CREATE DATABASE "{db_name}""#)) + .execute(&admin_pool) + .await?; + + // min_connections(1) keeps a session open for the pool's lifetime, which protects + // this database from the stale-database cleanup of tests in other processes. + let opts = PgConnectOptions::from_str(&admin_url)?.database(&db_name); + let pool = PgPoolOptions::new() + .min_connections(1) + .connect_with(opts) + .await?; + drop(guard); + + Ok(pool) } #[derive(Debug)] @@ -137,54 +150,21 @@ impl AsyncWalletPersister for TestStore { } } -pub async fn _drop_tables() -> anyhow::Result<()> { - let url = env::var("DATABASE_TEST_URL").expect("DATABASE_TEST_URL must be set for tests"); - let pool = Pool::::connect(&url.clone()).await?; - - let mut tx = pool.begin().await?; - - // Drop tables in reverse order of creation to handle foreign key constraints - let queries = [ - r#"DROP INDEX IF EXISTS "bdk_wallet"."idx_anchor_tx_txid""#, - r#"DROP TABLE IF EXISTS "bdk_wallet"."anchor_tx""#, - r#"DROP TABLE IF EXISTS "bdk_wallet"."txout""#, - r#"DROP TABLE IF EXISTS "bdk_wallet"."tx""#, - r#"DROP INDEX IF EXISTS "bdk_wallet"."idx_block_height""#, - r#"DROP TABLE IF EXISTS "bdk_wallet"."block""#, - r#"DROP TABLE IF EXISTS "bdk_wallet"."keychain""#, - r#"DROP TABLE IF EXISTS "bdk_wallet"."network""#, - r#"DROP SCHEMA IF EXISTS "bdk_wallet" CASCADE"#, - ]; - - // Execute each query separately - for query in &queries { - sqlx::query(query).execute(&mut *tx).await?; - } - - tx.commit().await?; - - Ok(()) -} - async fn create_test_stores(wallet_name: String) -> anyhow::Result> { let mut stores: Vec = Vec::new(); - // Set up postgres database URL (you might want to use a test-specific database) - let url = env::var("DATABASE_TEST_URL").expect("DATABASE_TEST_URL must be set for tests"); - let pool = Pool::::connect(&url.clone()).await?; - - // Drop all before creating new store for testing - pool.drop_all().await?; + let pool = create_test_pg_pool().await?; let postgres_store = PgStoreBuilder::new(wallet_name.clone()) .network(NETWORK) .migrate(true) - .build_with_url(&url) + .pool(pool) + .build() .await?; stores.push(TestStore::Postgres(postgres_store)); - // Setup sqlite in-memory database - let pool = SqlitePool::connect(":memory:").await?; - let sqlite_store = Store::::new(pool.clone(), wallet_name.clone(), true).await?; + // Setup sqlite in-memory database. `new_with_url(None, ..)` configures the + // single-connection pool a shared in-memory database requires. + let sqlite_store = Store::::new_with_url(None, wallet_name.clone(), true).await?; stores.push(TestStore::Sqlite(sqlite_store)); Ok(stores) From f4fc6979dd94cf8e1f9b4ff9908f0e280e208fc2 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 07:56:06 -0700 Subject: [PATCH 02/18] fix: cascade anchor_tx foreign keys so reorgs cannot wedge persistence Security review finding H1 (high): anchor_tx declared FKs to block and tx with no ON DELETE clause. On a reorg, BDK's local_chain changeset carries (height, None) and the store deletes the block row; while any anchor_tx row still references it (the normal case for an anchored tx), the database rejected the DELETE and the entire persist transaction aborted - after which the wallet could never persist again. anchor_tx rows were never deleted anywhere, so any reorg over an anchored block triggered this. Reproduced before the fix: postgres error 23503 on anchor_tx_wallet_name_block_hash_fkey, sqlite FOREIGN KEY constraint failed - matching the report's PoC evidence. - sqlite: migration 02 rebuilds anchor_tx with ON DELETE CASCADE on both FKs (sqlite cannot alter FK clauses in place); migration 01 is untouched to preserve sqlx checksums for existing databases. Verified against a database with existing anchor rows: data survives the rebuild and the reorg delete now cascades. - postgres: hardcoded schema gains ON DELETE CASCADE, plus an idempotent DO block that upgrades the constraints of databases created with the old schema. Verified idempotent and cascading on an old-schema database. - migrations/postgres/01 (currently unused by code) kept in sync. - New regression test reorged_out_anchored_block_can_be_deleted covers both backends: persist anchored txs, disconnect the anchored block, assert persistence continues and anchors are dropped with the block. --- migrations/postgres/01_bdk_wallet.sql | 4 +- .../sqlite/02_anchor_tx_on_delete_cascade.sql | 18 ++++++ src/postgres.rs | 28 +++++++++- src/test.rs | 56 +++++++++++++++++++ 4 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 migrations/sqlite/02_anchor_tx_on_delete_cascade.sql diff --git a/migrations/postgres/01_bdk_wallet.sql b/migrations/postgres/01_bdk_wallet.sql index 91242c3..4c0dd2a 100644 --- a/migrations/postgres/01_bdk_wallet.sql +++ b/migrations/postgres/01_bdk_wallet.sql @@ -69,7 +69,7 @@ CREATE TABLE IF NOT EXISTS bdk_wallet.anchor_tx ( anchor JSONB NOT NULL, txid TEXT NOT NULL, PRIMARY KEY (wallet_name, block_hash, txid), - FOREIGN KEY (wallet_name, block_hash) REFERENCES bdk_wallet.block(wallet_name, hash), - FOREIGN KEY (wallet_name, txid) REFERENCES bdk_wallet.tx(wallet_name, txid) + FOREIGN KEY (wallet_name, block_hash) REFERENCES bdk_wallet.block(wallet_name, hash) ON DELETE CASCADE, + FOREIGN KEY (wallet_name, txid) REFERENCES bdk_wallet.tx(wallet_name, txid) ON DELETE CASCADE ); CREATE INDEX IF NOT EXISTS idx_anchor_tx_txid ON bdk_wallet.anchor_tx (txid); \ No newline at end of file diff --git a/migrations/sqlite/02_anchor_tx_on_delete_cascade.sql b/migrations/sqlite/02_anchor_tx_on_delete_cascade.sql new file mode 100644 index 0000000..cff7386 --- /dev/null +++ b/migrations/sqlite/02_anchor_tx_on_delete_cascade.sql @@ -0,0 +1,18 @@ +-- Rebuild anchor_tx so its foreign keys cascade on block/tx deletion. +-- Without this, a reorg (which deletes the disconnected block row) is rejected +-- with a FK violation while anchor_tx rows still reference the block, wedging +-- all further persistence. SQLite cannot alter FK clauses in place, so the +-- table is rebuilt. +CREATE TABLE anchor_tx_new ( + wallet_name TEXT NOT NULL, + block_hash TEXT NOT NULL, + anchor BLOB NOT NULL, + txid TEXT NOT NULL, + PRIMARY KEY (wallet_name, block_hash, txid), + FOREIGN KEY (wallet_name, block_hash) REFERENCES block(wallet_name, hash) ON DELETE CASCADE, + FOREIGN KEY (wallet_name, txid) REFERENCES tx(wallet_name, txid) ON DELETE CASCADE +); +INSERT INTO anchor_tx_new SELECT wallet_name, block_hash, anchor, txid FROM anchor_tx; +DROP TABLE anchor_tx; +ALTER TABLE anchor_tx_new RENAME TO anchor_tx; +CREATE INDEX idx_anchor_tx_txid ON anchor_tx (txid); diff --git a/src/postgres.rs b/src/postgres.rs index d4acec8..7a8f35c 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -266,10 +266,34 @@ impl Store { anchor JSONB NOT NULL, txid TEXT NOT NULL, PRIMARY KEY (wallet_name, block_hash, txid), - FOREIGN KEY (wallet_name, block_hash) REFERENCES "bdk_wallet"."block"(wallet_name, hash), - FOREIGN KEY (wallet_name, txid) REFERENCES "bdk_wallet"."tx"(wallet_name, txid) + FOREIGN KEY (wallet_name, block_hash) REFERENCES "bdk_wallet"."block"(wallet_name, hash) ON DELETE CASCADE, + FOREIGN KEY (wallet_name, txid) REFERENCES "bdk_wallet"."tx"(wallet_name, txid) ON DELETE CASCADE )"#, r#"CREATE INDEX IF NOT EXISTS idx_anchor_tx_txid ON "bdk_wallet"."anchor_tx" (txid)"#, + // Databases created before the FK clauses above included ON DELETE CASCADE + // reject reorg-driven block deletion while anchor_tx rows still reference the + // block, wedging all further persistence. Recreate such constraints in place. + r#"DO $$ + BEGIN + IF EXISTS ( + SELECT 1 FROM pg_constraint c + JOIN pg_class t ON t.oid = c.conrelid + JOIN pg_namespace n ON n.oid = t.relnamespace + WHERE n.nspname = 'bdk_wallet' AND t.relname = 'anchor_tx' + AND c.contype = 'f' AND c.confdeltype <> 'c' + ) THEN + ALTER TABLE "bdk_wallet"."anchor_tx" + DROP CONSTRAINT IF EXISTS anchor_tx_wallet_name_block_hash_fkey, + DROP CONSTRAINT IF EXISTS anchor_tx_wallet_name_txid_fkey; + ALTER TABLE "bdk_wallet"."anchor_tx" + ADD CONSTRAINT anchor_tx_wallet_name_block_hash_fkey + FOREIGN KEY (wallet_name, block_hash) + REFERENCES "bdk_wallet"."block"(wallet_name, hash) ON DELETE CASCADE, + ADD CONSTRAINT anchor_tx_wallet_name_txid_fkey + FOREIGN KEY (wallet_name, txid) + REFERENCES "bdk_wallet"."tx"(wallet_name, txid) ON DELETE CASCADE; + END IF; + END $$"#, ]; // Execute each query separately diff --git a/src/test.rs b/src/test.rs index 65cf2c6..90e32b6 100644 --- a/src/test.rs +++ b/src/test.rs @@ -528,6 +528,62 @@ async fn single_descriptor_wallet_persist_and_recover() -> anyhow::Result<()> { Ok(()) } +/// Regression test for a reorg wedging persistence: deleting a block that still has +/// `anchor_tx` rows referencing it must succeed (the anchors are dropped with the block) +/// instead of aborting the whole persist transaction with a FK violation. +#[tokio::test] +async fn reorged_out_anchored_block_can_be_deleted() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + let stores = create_test_stores(wallet_name).await?; + for mut store in stores { + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + + // Anchor transactions to blocks and persist, so anchor_tx rows reference block rows. + let _txid = insert_fake_tx( + &mut wallet, + Amount::from_sat(20_000), + Amount::from_sat(10_000), + Amount::from_sat(1_000), + ); + assert!(wallet.persist_async(&mut store).await?); + + // Simulate a reorg disconnecting the anchored block: BDK's local_chain + // changeset carries (height, None), which deletes the block row. + let mut reorg = ChangeSet::default(); + reorg.local_chain.blocks.insert(2_000, None); + TestStore::persist(&mut store, &reorg) + .await + .expect("persisting a reorg over an anchored block must not fail"); + + // The disconnected block and its anchors are gone; the rest survives. + let changeset = TestStore::initialize(&mut store).await?; + assert!(!changeset.local_chain.blocks.contains_key(&2_000)); + assert!(changeset.tx_graph.anchors.is_empty()); + assert_eq!(changeset.tx_graph.txs.len(), 2); + + // Persistence still works afterwards. + let mut new_tip = ChangeSet::default(); + new_tip + .local_chain + .blocks + .insert(2_001, Some(BlockHash::from_byte_array([1u8; 32]))); + TestStore::persist(&mut store, &new_tip).await?; + } + Ok(()) +} + #[tracing::instrument] #[tokio::test] async fn two_wallets_load() -> anyhow::Result<()> { From 6cde51b76976ce21a18680956f518adc81b93774 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 08:06:28 -0700 Subject: [PATCH 03/18] fix: error on corrupt stored data instead of silently dropping it on load Security review finding H2 (high) + L3: the read path decoded stored rows with error-swallowing patterns (if let Ok(..) = consensus_decode / serde_json::from_value), so corrupted whole_tx or anchor rows were silently skipped: the wallet loaded successfully with less history and a wrong balance, and nothing was ever reported. The integrity policy was also inconsistent - other columns of the same tables already hard-failed the load. Both backends now apply one uniform fail-loud policy: - Undecodable whole_tx bytes return BdkSqlxError::Consensus. - consensus::deserialize replaces consensus_decode, so trailing bytes after a valid transaction are rejected too (L3). - The decoded transaction's computed txid is cross-checked against the stored txid column (new TxidMismatch error). - Unparseable anchor JSON returns BdkSqlxError::SerdeJson. - The anchor payload's block hash is cross-checked against the stored block_hash column (new AnchorBlockHashMismatch error). New regression test corrupt_rows_error_on_load covers all five scenarios on both backends and verifies the store loads cleanly again once the corruption is repaired. --- src/lib.rs | 21 ++++- src/postgres.rs | 48 ++++++---- src/sqlite.rs | 40 +++++--- src/test.rs | 238 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 314 insertions(+), 33 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index ae613ac..dac08c6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,7 +12,7 @@ use std::future::Future; use std::pin::Pin; use bdk_wallet::bitcoin; -use bdk_wallet::bitcoin::Network; +use bdk_wallet::bitcoin::{BlockHash, Network, Txid}; use bdk_wallet::chain::miniscript; pub use sqlx; use sqlx::Pool; @@ -24,6 +24,25 @@ pub enum BdkSqlxError { /// bitcoin parse hex error #[error("bitoin parse hex error: {0}")] HexToArray(#[from] bitcoin::hex::HexToArrayError), + /// bitcoin consensus decode error + #[error("bitcoin consensus decode error: {0}")] + Consensus(#[from] bitcoin::consensus::encode::Error), + /// stored transaction bytes decode to a different txid than the stored txid + #[error("decoded transaction txid {computed} does not match stored txid {stored}")] + TxidMismatch { + /// txid stored alongside the transaction bytes + stored: Txid, + /// txid computed from the decoded transaction + computed: Txid, + }, + /// stored anchor references a different block hash than the stored block_hash + #[error("anchor block hash {computed} does not match stored block_hash {stored}")] + AnchorBlockHashMismatch { + /// block hash stored in the block_hash column + stored: BlockHash, + /// block hash contained in the anchor payload + computed: BlockHash, + }, /// miniscript error #[error("miniscript error: {0}")] Miniscript(#[from] miniscript::Error), diff --git a/src/postgres.rs b/src/postgres.rs index 7a8f35c..bff4bd4 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -13,10 +13,7 @@ use bdk_chain::{ }; use bdk_wallet::{ bitcoin::{ - self, - consensus::{self, Decodable}, - hashes::Hash, - Amount, BlockHash, Network, OutPoint, ScriptBuf, TxOut, Txid, + self, consensus, hashes::Hash, Amount, BlockHash, Network, OutPoint, ScriptBuf, TxOut, Txid, }, chain as bdk_chain, descriptor::{Descriptor, DescriptorPublicKey, ExtendedDescriptor}, @@ -553,9 +550,15 @@ pub async fn tx_graph_changeset_from_postgres( let last_seen: Option = row.get("last_seen"); if let Some(tx_bytes) = whole_tx { - if let Ok(tx) = bitcoin::Transaction::consensus_decode(&mut tx_bytes.as_slice()) { - changeset.txs.insert(Arc::new(tx)); + let tx: bitcoin::Transaction = consensus::deserialize(&tx_bytes)?; + let computed = tx.compute_txid(); + if computed != txid { + return Err(BdkSqlxError::TxidMismatch { + stored: txid, + computed, + }); } + changeset.txs.insert(Arc::new(tx)); } if let Some(last_seen) = last_seen { changeset.last_seen.insert(txid, last_seen as u64); @@ -594,24 +597,33 @@ pub async fn tx_graph_changeset_from_postgres( } // Fetch anchors - let rows = - sqlx::query(r#"SELECT anchor, txid FROM "bdk_wallet"."anchor_tx" WHERE wallet_name = $1"#) - .bind(wallet_name) - .fetch_all(&mut **db_tx) - .await - .map_err(|e| BdkSqlxError::QueryError { - table: "select anchor tx".to_string(), - source: e, - })?; + let rows = sqlx::query( + r#"SELECT anchor, txid, block_hash FROM "bdk_wallet"."anchor_tx" WHERE wallet_name = $1"#, + ) + .bind(wallet_name) + .fetch_all(&mut **db_tx) + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "select anchor tx".to_string(), + source: e, + })?; for row in rows { let anchor: serde_json::Value = row.get("anchor"); let txid: String = row.get("txid"); let txid = Txid::from_str(&txid)?; - - if let Ok(anchor) = serde_json::from_value::(anchor) { - changeset.anchors.insert((anchor, txid)); + let block_hash: String = row.get("block_hash"); + let block_hash = BlockHash::from_str(&block_hash)?; + + let anchor: ConfirmationBlockTime = serde_json::from_value(anchor)?; + let computed = anchor.anchor_block().hash; + if computed != block_hash { + return Err(BdkSqlxError::AnchorBlockHashMismatch { + stored: block_hash, + computed, + }); } + changeset.anchors.insert((anchor, txid)); } Ok(changeset) diff --git a/src/sqlite.rs b/src/sqlite.rs index 590054b..a07c87a 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -10,10 +10,7 @@ use bdk_chain::{ local_chain, tx_graph, Anchor, ConfirmationBlockTime, DescriptorExt, DescriptorId, Merge, }; use bdk_wallet::bitcoin::{ - self, - consensus::{self, Decodable}, - hashes::Hash, - Amount, BlockHash, Network, OutPoint, ScriptBuf, TxOut, Txid, + self, consensus, hashes::Hash, Amount, BlockHash, Network, OutPoint, ScriptBuf, TxOut, Txid, }; use bdk_wallet::chain as bdk_chain; use bdk_wallet::descriptor::{Descriptor, DescriptorPublicKey, ExtendedDescriptor}; @@ -296,9 +293,15 @@ pub async fn tx_graph_changeset_from_sqlite( let last_seen: Option = row.get("last_seen"); if let Some(tx_bytes) = whole_tx { - if let Ok(tx) = bitcoin::Transaction::consensus_decode(&mut tx_bytes.as_slice()) { - changeset.txs.insert(Arc::new(tx)); + let tx: bitcoin::Transaction = consensus::deserialize(&tx_bytes)?; + let computed = tx.compute_txid(); + if computed != txid { + return Err(BdkSqlxError::TxidMismatch { + stored: txid, + computed, + }); } + changeset.txs.insert(Arc::new(tx)); } if let Some(last_seen) = last_seen { changeset.last_seen.insert(txid, last_seen as u64); @@ -331,20 +334,29 @@ pub async fn tx_graph_changeset_from_sqlite( } // Fetch anchors - let rows = - sqlx::query("SELECT json(anchor) as anchor, txid FROM anchor_tx WHERE wallet_name = $1") - .bind(wallet_name) - .fetch_all(&mut **db_tx) - .await?; + let rows = sqlx::query( + "SELECT json(anchor) as anchor, txid, block_hash FROM anchor_tx WHERE wallet_name = $1", + ) + .bind(wallet_name) + .fetch_all(&mut **db_tx) + .await?; for row in rows { let anchor: serde_json::Value = row.get("anchor"); let txid: String = row.get("txid"); let txid = Txid::from_str(&txid)?; - - if let Ok(anchor) = serde_json::from_value::(anchor) { - changeset.anchors.insert((anchor, txid)); + let block_hash: String = row.get("block_hash"); + let block_hash = BlockHash::from_str(&block_hash)?; + + let anchor: ConfirmationBlockTime = serde_json::from_value(anchor)?; + let computed = anchor.anchor_block().hash; + if computed != block_hash { + return Err(BdkSqlxError::AnchorBlockHashMismatch { + stored: block_hash, + computed, + }); } + changeset.anchors.insert((anchor, txid)); } Ok(changeset) diff --git a/src/test.rs b/src/test.rs index 90e32b6..ea07bba 100644 --- a/src/test.rs +++ b/src/test.rs @@ -528,6 +528,244 @@ async fn single_descriptor_wallet_persist_and_recover() -> anyhow::Result<()> { Ok(()) } +const BOGUS_TXID: &str = "1111111111111111111111111111111111111111111111111111111111111111"; + +async fn corrupt_and_check_postgres( + store: &Store, + wallet_name: &str, + txid: Txid, +) -> anyhow::Result<()> { + let pool = store.pool.clone(); + let txid = txid.to_string(); + + let valid_tx: Vec = sqlx::query_scalar( + r#"SELECT whole_tx FROM "bdk_wallet"."tx" WHERE wallet_name=$1 AND txid=$2"#, + ) + .bind(wallet_name) + .bind(&txid) + .fetch_one(&pool) + .await?; + let set_whole_tx = + r#"UPDATE "bdk_wallet"."tx" SET whole_tx=$3 WHERE wallet_name=$1 AND txid=$2"#; + + // undecodable tx bytes must fail the load, not silently drop the tx + sqlx::query(set_whole_tx) + .bind(wallet_name) + .bind(&txid) + .bind(vec![0xde_u8, 0xad, 0xbe, 0xef]) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::Consensus(_))); + + // trailing bytes after a valid tx must be rejected + let mut trailing = valid_tx.clone(); + trailing.push(0); + sqlx::query(set_whole_tx) + .bind(wallet_name) + .bind(&txid) + .bind(trailing) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::Consensus(_))); + + sqlx::query(set_whole_tx) + .bind(wallet_name) + .bind(&txid) + .bind(&valid_tx) + .execute(&pool) + .await?; + + // tx bytes that decode to a different txid than the stored txid must be rejected + sqlx::query(r#"INSERT INTO "bdk_wallet"."tx" (wallet_name, txid, whole_tx) VALUES ($1,$2,$3)"#) + .bind(wallet_name) + .bind(BOGUS_TXID) + .bind(&valid_tx) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::TxidMismatch { .. })); + sqlx::query(r#"DELETE FROM "bdk_wallet"."tx" WHERE wallet_name=$1 AND txid=$2"#) + .bind(wallet_name) + .bind(BOGUS_TXID) + .execute(&pool) + .await?; + + // an unparseable anchor must fail the load, not silently drop the anchor + let valid_anchor: serde_json::Value = sqlx::query_scalar( + r#"SELECT anchor FROM "bdk_wallet"."anchor_tx" WHERE wallet_name=$1 AND txid=$2"#, + ) + .bind(wallet_name) + .bind(&txid) + .fetch_one(&pool) + .await?; + let set_anchor = + r#"UPDATE "bdk_wallet"."anchor_tx" SET anchor=$3 WHERE wallet_name=$1 AND txid=$2"#; + sqlx::query(set_anchor) + .bind(wallet_name) + .bind(&txid) + .bind(serde_json::json!({"bogus": 1})) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::SerdeJson(_))); + + // an anchor whose payload points at a different block than the stored block_hash + let mut mutated = valid_anchor.clone(); + mutated["block_id"]["hash"] = serde_json::json!(BOGUS_TXID); + sqlx::query(set_anchor) + .bind(wallet_name) + .bind(&txid) + .bind(mutated) + .execute(&pool) + .await?; + assert_matches!( + store.read().await, + Err(BdkSqlxError::AnchorBlockHashMismatch { .. }) + ); + + sqlx::query(set_anchor) + .bind(wallet_name) + .bind(&txid) + .bind(valid_anchor) + .execute(&pool) + .await?; + store.read().await?; + Ok(()) +} + +async fn corrupt_and_check_sqlite( + store: &Store, + wallet_name: &str, + txid: Txid, +) -> anyhow::Result<()> { + let pool = store.pool.clone(); + let txid = txid.to_string(); + + let valid_tx: Vec = + sqlx::query_scalar("SELECT whole_tx FROM tx WHERE wallet_name=$1 AND txid=$2") + .bind(wallet_name) + .bind(&txid) + .fetch_one(&pool) + .await?; + let set_whole_tx = "UPDATE tx SET whole_tx=$3 WHERE wallet_name=$1 AND txid=$2"; + + // undecodable tx bytes must fail the load, not silently drop the tx + sqlx::query(set_whole_tx) + .bind(wallet_name) + .bind(&txid) + .bind(vec![0xde_u8, 0xad, 0xbe, 0xef]) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::Consensus(_))); + + // trailing bytes after a valid tx must be rejected + let mut trailing = valid_tx.clone(); + trailing.push(0); + sqlx::query(set_whole_tx) + .bind(wallet_name) + .bind(&txid) + .bind(trailing) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::Consensus(_))); + + sqlx::query(set_whole_tx) + .bind(wallet_name) + .bind(&txid) + .bind(&valid_tx) + .execute(&pool) + .await?; + + // tx bytes that decode to a different txid than the stored txid must be rejected + sqlx::query("INSERT INTO tx (wallet_name, txid, whole_tx) VALUES ($1,$2,$3)") + .bind(wallet_name) + .bind(BOGUS_TXID) + .bind(&valid_tx) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::TxidMismatch { .. })); + sqlx::query("DELETE FROM tx WHERE wallet_name=$1 AND txid=$2") + .bind(wallet_name) + .bind(BOGUS_TXID) + .execute(&pool) + .await?; + + // an unparseable anchor must fail the load, not silently drop the anchor + let valid_anchor: serde_json::Value = + sqlx::query_scalar("SELECT json(anchor) FROM anchor_tx WHERE wallet_name=$1 AND txid=$2") + .bind(wallet_name) + .bind(&txid) + .fetch_one(&pool) + .await?; + let set_anchor = "UPDATE anchor_tx SET anchor=jsonb($3) WHERE wallet_name=$1 AND txid=$2"; + sqlx::query(set_anchor) + .bind(wallet_name) + .bind(&txid) + .bind(serde_json::json!({"bogus": 1}).to_string()) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::SerdeJson(_))); + + // an anchor whose payload points at a different block than the stored block_hash + let mut mutated = valid_anchor.clone(); + mutated["block_id"]["hash"] = serde_json::json!(BOGUS_TXID); + sqlx::query(set_anchor) + .bind(wallet_name) + .bind(&txid) + .bind(mutated.to_string()) + .execute(&pool) + .await?; + assert_matches!( + store.read().await, + Err(BdkSqlxError::AnchorBlockHashMismatch { .. }) + ); + + sqlx::query(set_anchor) + .bind(wallet_name) + .bind(&txid) + .bind(valid_anchor.to_string()) + .execute(&pool) + .await?; + store.read().await?; + Ok(()) +} + +/// Regression test for silent data loss: corrupted rows must produce an explicit error +/// on load instead of an `Ok` changeset that is quietly missing data. +#[tokio::test] +async fn corrupt_rows_error_on_load() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + let stores = create_test_stores(wallet_name.clone()).await?; + for mut store in stores { + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + let txid = insert_fake_tx( + &mut wallet, + Amount::from_sat(20_000), + Amount::from_sat(10_000), + Amount::from_sat(1_000), + ); + assert!(wallet.persist_async(&mut store).await?); + + match &store { + TestStore::Postgres(store) => { + corrupt_and_check_postgres(store, &wallet_name, txid).await? + } + TestStore::Sqlite(store) => corrupt_and_check_sqlite(store, &wallet_name, txid).await?, + } + } + Ok(()) +} + /// Regression test for a reorg wedging persistence: deleting a block that still has /// `anchor_tx` rows referencing it must succeed (the anchors are dropped with the block) /// instead of aborting the whole persist transaction with a FK violation. From 8686af92b7bf44cd1e277df3491f90d4042030a4 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 08:48:50 -0700 Subject: [PATCH 04/18] fix: stop tracing spans from recording descriptors and changesets Security review finding H3 (high) + L10: every instrumented function in the sqlite backend used bare #[tracing::instrument], which records all arguments into the span - full descriptors (xpub form today: public keys, derivation structure), whole changesets, wallet names, and pool internals - and emitted them at INFO, exactly the verbosity the shipped example enables (RUST_LOG=bdk_sqlx=debug). The postgres module already used skip_all/skip; the sqlite module now matches it. - #[tracing::instrument(skip_all)] on every instrumented fn in sqlite.rs and on the TestStore persister shims in test.rs. - sqlite info! events downgraded to trace! to match the postgres backend's noise level (L10). - New regression test tracing_output_contains_no_descriptor_material captures tracing output at TRACE while creating, persisting, and loading a wallet, and asserts no descriptor/xkey/pubkey material appears. Before the fix it failed with XPub material in the captured spans. Note bdk_wallet 1.2 strips xprv->xpub at Wallet::create, so what leaked today was watch-only surveillance data, not private keys - but spans record whatever a future changeset carries, so the missing skip was latent-critical. --- src/sqlite.rs | 66 ++++++++++++++++++++++----------------------- src/test.rs | 74 +++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 105 insertions(+), 35 deletions(-) diff --git a/src/sqlite.rs b/src/sqlite.rs index a07c87a..13e9737 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -21,21 +21,21 @@ use sqlx::sqlite::SqliteRow; use sqlx::sqlite::{SqlitePool, SqlitePoolOptions}; use sqlx::sqlx_macros::migrate; use sqlx::{sqlite::Sqlite, FromRow, Pool, Row, Transaction}; -use tracing::info; +use tracing::trace; impl AsyncWalletPersister for Store { type Error = BdkSqlxError; - #[tracing::instrument] + #[tracing::instrument(skip_all)] fn initialize<'a>(store: &'a mut Self) -> FutureResult<'a, ChangeSet, Self::Error> where Self: 'a, { - info!("initialize store"); + trace!("initialize store"); Box::pin(store.read()) } - #[tracing::instrument] + #[tracing::instrument(skip_all)] fn persist<'a>( store: &'a mut Self, changeset: &'a ChangeSet, @@ -43,22 +43,22 @@ impl AsyncWalletPersister for Store { where Self: 'a, { - info!("persist store"); + trace!("persist store"); Box::pin(store.write(changeset)) } } impl Store { /// Construct a new [`Store`] with an existing sqlite connection pool. - #[tracing::instrument] + #[tracing::instrument(skip_all)] pub async fn new( pool: Pool, wallet_name: String, migrate: bool, ) -> Result { - info!("new sqlite store"); + trace!("new sqlite store"); if migrate { - info!("migrate"); + trace!("migrate"); migrate!("./migrations/sqlite").run(&pool).await?; } Ok(Self { pool, wallet_name }) @@ -70,13 +70,13 @@ impl Store { /// /// If no URL is given a memory DB (non-persisted) will be used. A memory DB /// is useful for testing. - #[tracing::instrument] + #[tracing::instrument(skip_all)] pub async fn new_with_url( url: Option, wallet_name: String, migrate: bool, ) -> Result, BdkSqlxError> { - info!("new store with url"); + trace!("new store with url"); let pool = if let Some(url) = url { SqlitePool::connect(url.as_str()).await? } else { @@ -94,9 +94,9 @@ impl Store { } impl Store { - #[tracing::instrument] + #[tracing::instrument(skip_all)] pub(crate) async fn read(&self) -> Result { - info!("migrate and read"); + trace!("migrate and read"); let mut tx = self.pool.begin().await?; let mut changeset = ChangeSet::default(); let sql = @@ -123,14 +123,14 @@ impl Store { Ok(changeset) } - //#[tracing::instrument] + //#[tracing::instrument(skip_all)] pub(crate) async fn changeset_from_row( tx: &mut Transaction<'_, Sqlite>, changeset: &mut ChangeSet, row: SqliteRow, wallet_name: &str, ) -> Result<(), BdkSqlxError> { - info!("changeset from row"); + trace!("changeset from row"); let network: String = row.get("network"); let internal_last_revealed: Option = row.get("internal_last_revealed"); @@ -163,9 +163,9 @@ impl Store { Ok(()) } - #[tracing::instrument] + #[tracing::instrument(skip_all)] pub(crate) async fn write(&self, changeset: &ChangeSet) -> Result<(), BdkSqlxError> { - info!("changeset write"); + trace!("changeset write"); if changeset.is_empty() { return Ok(()); } @@ -203,14 +203,14 @@ impl Store { } /// Insert keychain descriptors. -#[tracing::instrument] +#[tracing::instrument(skip_all)] async fn insert_descriptor( tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, descriptor: &ExtendedDescriptor, keychain: KeychainKind, ) -> Result<(), BdkSqlxError> { - info!("insert descriptor"); + trace!("insert descriptor"); let descriptor_str = descriptor.to_string(); let descriptor_id = descriptor.descriptor_id().to_byte_array(); @@ -233,13 +233,13 @@ async fn insert_descriptor( } /// Insert network. -#[tracing::instrument] +#[tracing::instrument(skip_all)] async fn insert_network( tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, network: Network, ) -> Result<(), BdkSqlxError> { - info!("insert network"); + trace!("insert network"); sqlx::query("INSERT INTO network (wallet_name, name) VALUES ($1, $2)") .bind(wallet_name) .bind(network.to_string()) @@ -250,14 +250,14 @@ async fn insert_network( } /// Update keychain last revealed -#[tracing::instrument] +#[tracing::instrument(skip_all)] async fn update_last_revealed( tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, descriptor_id: DescriptorId, last_revealed: u32, ) -> Result<(), BdkSqlxError> { - info!("update last revealed"); + trace!("update last revealed"); sqlx::query::( "UPDATE keychain SET last_revealed = $1 WHERE wallet_name = $2 AND descriptor_id = $3", @@ -272,12 +272,12 @@ async fn update_last_revealed( } /// Select transactions, txouts, and anchors. -#[tracing::instrument] +#[tracing::instrument(skip_all)] pub async fn tx_graph_changeset_from_sqlite( db_tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, ) -> Result, BdkSqlxError> { - info!("tx graph changeset from sqlite"); + trace!("tx graph changeset from sqlite"); let mut changeset = tx_graph::ChangeSet::default(); // Fetch transactions @@ -363,13 +363,13 @@ pub async fn tx_graph_changeset_from_sqlite( } /// Insert transactions, txouts, and anchors. -#[tracing::instrument] +#[tracing::instrument(skip_all)] pub async fn tx_graph_changeset_persist_to_sqlite( db_tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, changeset: &tx_graph::ChangeSet, ) -> Result<(), BdkSqlxError> { - info!("tx graph changeset from sqlite"); + trace!("tx graph changeset from sqlite"); for tx in &changeset.txs { sqlx::query( "INSERT INTO tx (wallet_name, txid, whole_tx) VALUES ($1, $2, $3) @@ -424,12 +424,12 @@ pub async fn tx_graph_changeset_persist_to_sqlite( } /// Select blocks. -#[tracing::instrument] +#[tracing::instrument(skip_all)] pub async fn local_chain_changeset_from_sqlite( db_tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, ) -> Result { - info!("local chain changeset from sqlite"); + trace!("local chain changeset from sqlite"); let mut changeset = local_chain::ChangeSet::default(); let rows = sqlx::query("SELECT hash, height FROM block WHERE wallet_name = $1") @@ -448,13 +448,13 @@ pub async fn local_chain_changeset_from_sqlite( } /// Insert blocks. -#[tracing::instrument] +#[tracing::instrument(skip_all)] pub async fn local_chain_changeset_persist_to_sqlite( db_tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, changeset: &local_chain::ChangeSet, ) -> Result<(), BdkSqlxError> { - info!("local chain changeset to sqlite"); + trace!("local chain changeset to sqlite"); for (&height, &hash) in &changeset.blocks { match hash { Some(hash) => { @@ -482,9 +482,9 @@ pub async fn local_chain_changeset_persist_to_sqlite( } /// Collects information on all the wallets in the database and dumps it to stdout. -#[tracing::instrument] +#[tracing::instrument(skip_all)] pub async fn easy_backup(db: Pool) -> Result<(), BdkSqlxError> { - info!("Starting easy backup"); + trace!("Starting easy backup"); let statement = "SELECT * FROM keychain"; @@ -495,7 +495,7 @@ pub async fn easy_backup(db: Pool) -> Result<(), BdkSqlxError> { let json_array = json!(results); println!("{}", serde_json::to_string_pretty(&json_array)?); - info!("Easy backup completed successfully"); + trace!("Easy backup completed successfully"); Ok(()) } diff --git a/src/test.rs b/src/test.rs index ea07bba..5d710a4 100644 --- a/src/test.rs +++ b/src/test.rs @@ -122,7 +122,7 @@ enum TestStore { impl AsyncWalletPersister for TestStore { type Error = BdkSqlxError; - #[tracing::instrument] + #[tracing::instrument(skip_all)] fn initialize<'a>(store: &'a mut Self) -> FutureResult<'a, ChangeSet, Self::Error> where Self: 'a, @@ -134,7 +134,7 @@ impl AsyncWalletPersister for TestStore { } } - #[tracing::instrument] + #[tracing::instrument(skip_all)] fn persist<'a>( store: &'a mut Self, changeset: &'a ChangeSet, @@ -150,6 +150,76 @@ impl AsyncWalletPersister for TestStore { } } +#[derive(Clone)] +struct SharedWriter(std::sync::Arc>>); + +impl std::io::Write for SharedWriter { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +/// Regression test for descriptor material leaking into tracing output: even at TRACE +/// verbosity, spans and events emitted while creating, persisting, and loading a wallet +/// must not record descriptors, public keys, or changesets. +#[tokio::test] +async fn tracing_output_contains_no_descriptor_material() -> anyhow::Result<()> { + use tracing::instrument::WithSubscriber; + + initialize(); + + let buf = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let writer_buf = buf.clone(); + let subscriber = tracing_subscriber::registry() + .with(EnvFilter::new("trace")) + .with( + tracing_subscriber::fmt::layer().with_writer(move || SharedWriter(writer_buf.clone())), + ); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + async { + let mut store = Store::::new_with_url(None, wallet_name.clone(), true).await?; + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + let _ = wallet.reveal_next_address(External); + wallet.persist_async(&mut store).await?; + Wallet::load().load_wallet_async(&mut store).await?; + anyhow::Ok(()) + } + .with_subscriber(subscriber) + .await?; + + let logs = String::from_utf8_lossy(&buf.lock().unwrap()).into_owned(); + assert!(!logs.is_empty(), "expected tracing output to be captured"); + for needle in [ + "tprv", + "tpub", + "XPub", + "XPrv", + "DescriptorXKey", + "PublicKey(", + ] { + assert!( + !logs.contains(needle), + "tracing output leaked descriptor material ({needle}):\n{logs}" + ); + } + Ok(()) +} + async fn create_test_stores(wallet_name: String) -> anyhow::Result> { let mut stores: Vec = Vec::new(); From 195d02fcf335e0612c4684f790427c5976b0f8bf Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 08:55:58 -0700 Subject: [PATCH 05/18] fix: remove easy_backup, which dumped every tenant's keychain to stdout Security review finding H4 (high): easy_backup ran an unscoped SELECT * FROM keychain - no wallet_name filter - and pretty-printed every wallet's rows, including full descriptor strings, to stdout, where CI logs, journald, and container log drivers capture them. One call exposed all tenants' descriptors (xpub form today; latent-critical for anything secret-bearing stored later). Both copies were also dead code: pub fns inside private modules, never re-exported and never constructed (the compiler flagged KeychainEntry as never constructed), so removal breaks no user. This also makes cargo clippy --all-targets -- -Dwarnings pass again, which was failing on master because of exactly this dead code. A real backup facility, if wanted later, should require a wallet_name scope, write to a caller-provided sink instead of stdout, and document the sensitivity of descriptor data. --- src/postgres.rs | 32 ++------------------------------ src/sqlite.rs | 31 +------------------------------ 2 files changed, 3 insertions(+), 60 deletions(-) diff --git a/src/postgres.rs b/src/postgres.rs index bff4bd4..cfb004d 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -20,12 +20,11 @@ use bdk_wallet::{ AsyncWalletPersister, ChangeSet, KeychainKind, KeychainKind::{External, Internal}, }; -use serde_json::json; use sqlx::{ postgres::{PgPool, PgRow, Postgres}, - FromRow, Pool, Row, Transaction, + Pool, Row, Transaction, }; -use tracing::{info, trace, warn}; +use tracing::{trace, warn}; // First party imports use super::{BdkSqlxError, FutureResult, PgStoreBuilder, Store}; @@ -780,30 +779,3 @@ pub async fn local_chain_changeset_persist_to_postgres( Ok(()) } - -/// Collects information on all the wallets in the database and dumps it to stdout. -#[tracing::instrument] -pub async fn easy_backup(db: Pool) -> Result<()> { - trace!("Starting easy backup"); - - let statement = r#"SELECT * FROM "bdk_wallet"."keychain""#; - - #[derive(serde::Serialize, FromRow)] - struct KeychainEntry { - wallet_name: String, - keychainkind: String, - descriptor: String, - descriptor_id: Vec, - last_revealed: i32, - } - - let results = sqlx::query_as::<_, KeychainEntry>(statement) - .fetch_all(&db) - .await?; - - let json_array = json!(results); - println!("{}", serde_json::to_string_pretty(&json_array)?); - - info!("Easy backup completed successfully"); - Ok(()) -} diff --git a/src/sqlite.rs b/src/sqlite.rs index 13e9737..98e17c6 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -16,11 +16,10 @@ use bdk_wallet::chain as bdk_chain; use bdk_wallet::descriptor::{Descriptor, DescriptorPublicKey, ExtendedDescriptor}; use bdk_wallet::KeychainKind::{External, Internal}; use bdk_wallet::{AsyncWalletPersister, ChangeSet, KeychainKind}; -use serde_json::json; use sqlx::sqlite::SqliteRow; use sqlx::sqlite::{SqlitePool, SqlitePoolOptions}; use sqlx::sqlx_macros::migrate; -use sqlx::{sqlite::Sqlite, FromRow, Pool, Row, Transaction}; +use sqlx::{sqlite::Sqlite, Pool, Row, Transaction}; use tracing::trace; impl AsyncWalletPersister for Store { @@ -480,31 +479,3 @@ pub async fn local_chain_changeset_persist_to_sqlite( Ok(()) } - -/// Collects information on all the wallets in the database and dumps it to stdout. -#[tracing::instrument(skip_all)] -pub async fn easy_backup(db: Pool) -> Result<(), BdkSqlxError> { - trace!("Starting easy backup"); - - let statement = "SELECT * FROM keychain"; - - let results = sqlx::query_as::<_, KeychainEntry>(statement) - .fetch_all(&db) - .await?; - - let json_array = json!(results); - println!("{}", serde_json::to_string_pretty(&json_array)?); - - trace!("Easy backup completed successfully"); - Ok(()) -} - -/// Represents a row in the keychain table. -#[derive(serde::Serialize, FromRow)] -struct KeychainEntry { - wallet_name: String, - keychainkind: String, - descriptor: String, - descriptor_id: Vec, - last_revealed: i32, -} From fd05b5e2824cecc5b86294dbe7812775831a3979 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 09:01:21 -0700 Subject: [PATCH 06/18] fix: enforce stored-network validation on load, remove unwrap panic path Security review finding M1 (medium): the process-global NETWORK OnceLock existed for 'network validation', yet the read path never compared the DB-stored network against it - InvalidNetwork only fired on unparseable strings, and that error path called get_network().unwrap(), which would panic if the global was unset. - postgres: after parsing the stored network, it is now checked against the configured global network and the load fails with InvalidNetwork on mismatch. The unwrap is gone; when no network is configured the error falls back to a descriptive placeholder instead of panicking. - sqlite: an unparseable stored network string returned via .expect("parse Network") - a panic on corrupt data. It now returns InvalidNetwork like the postgres backend. - New regression test mismatched_network_errors_on_load covers both the wrong-network and unparseable-network cases. --- src/postgres.rs | 25 ++++++++++++++++------- src/sqlite.rs | 8 +++++++- src/test.rs | 53 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 8 deletions(-) diff --git a/src/postgres.rs b/src/postgres.rs index cfb004d..27843d5 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -370,13 +370,24 @@ impl Store { let internal_desc_str: Option = row.get("internal_descriptor"); let external_desc_str: Option = row.get("external_descriptor"); - changeset.network = - Some( - Network::from_str(&network).map_err(|got| BdkSqlxError::InvalidNetwork { - expected: get_network().unwrap().to_string(), - got: got.to_string(), - })?, - ); + let stored_network = + Network::from_str(&network).map_err(|_| BdkSqlxError::InvalidNetwork { + expected: get_network() + .map(|n| n.to_string()) + .unwrap_or_else(|_| "a known network".to_string()), + got: network.clone(), + })?; + // Reject data persisted for a different network than this process was + // configured for, instead of silently loading it. + if let Ok(configured) = get_network() { + if configured != stored_network { + return Err(BdkSqlxError::InvalidNetwork { + expected: configured.to_string(), + got: stored_network.to_string(), + }); + } + } + changeset.network = Some(stored_network); if let Some(desc_str) = external_desc_str { let descriptor: Descriptor = desc_str.parse()?; diff --git a/src/sqlite.rs b/src/sqlite.rs index 98e17c6..33c9309 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -137,7 +137,13 @@ impl Store { let internal_desc_str: Option = row.get("internal_descriptor"); let external_desc_str: Option = row.get("external_descriptor"); - changeset.network = Some(Network::from_str(&network).expect("parse Network")); + changeset.network = + Some( + Network::from_str(&network).map_err(|_| BdkSqlxError::InvalidNetwork { + expected: "a known network".to_string(), + got: network.clone(), + })?, + ); if let Some(desc_str) = external_desc_str { let descriptor: Descriptor = desc_str.parse()?; diff --git a/src/test.rs b/src/test.rs index 5d710a4..93315f3 100644 --- a/src/test.rs +++ b/src/test.rs @@ -150,6 +150,59 @@ impl AsyncWalletPersister for TestStore { } } +/// Data stored for a different network than the store was configured with (or an +/// unparseable network string) must fail the load instead of being silently accepted. +#[tokio::test] +async fn mismatched_network_errors_on_load() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + let pool = create_test_pg_pool().await?; + let mut store = PgStoreBuilder::new(wallet_name.clone()) + .network(NETWORK) + .migrate(true) + .pool(pool.clone()) + .build() + .await?; + Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + + let set_network = r#"UPDATE "bdk_wallet"."network" SET name=$2 WHERE wallet_name=$1"#; + + // a parseable but different network than the configured one + sqlx::query(set_network) + .bind(&wallet_name) + .bind("bitcoin") + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::InvalidNetwork { .. })); + + // an unparseable network string + sqlx::query(set_network) + .bind(&wallet_name) + .bind("junknet") + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::InvalidNetwork { .. })); + + sqlx::query(set_network) + .bind(&wallet_name) + .bind(NETWORK.to_string()) + .execute(&pool) + .await?; + store.read().await?; + Ok(()) +} + #[derive(Clone)] struct SharedWriter(std::sync::Arc>>); From 5218db7d47a9317d88b89c16da64596fd6529ed0 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 09:09:33 -0700 Subject: [PATCH 07/18] fix: checked integer conversions at the database boundary Security review finding M2 (medium): values crossing the DB boundary were converted with unchecked 'as' casts in both backends - value as u64, height as u32, vout as u32, last_seen as u64, last_revealed as u32 on the read path (and the mirror-image casts on the write path). A negative or oversized stored value wrapped silently: value=-1 became ~18.4 quintillion sats, height=-1 became height 4294967295. All casts are replaced with a checked_conv helper that returns the new BdkSqlxError::IntOutOfRange error naming the offending column and value. New regression test out_of_range_values_error_on_load verifies that negative txout values and block heights error on load on both backends instead of wrapping. --- src/lib.rs | 21 ++++++++++ src/postgres.rs | 35 ++++++++++------ src/sqlite.rs | 41 ++++++++++++------ src/test.rs | 108 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 181 insertions(+), 24 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index dac08c6..b876bca 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -86,6 +86,14 @@ pub enum BdkSqlxError { /// Config error #[error("Cant get network because its not set")] GetNetworkFailure, + /// integer value outside the range representable for its destination + #[error("integer value out of range for {context}: {value}")] + IntOutOfRange { + /// column or field the value belongs to + context: &'static str, + /// offending value + value: i128, + }, /// Query execution error #[error("Failed to execute query on {table}: {source}")] QueryError { @@ -112,3 +120,16 @@ pub struct PgStoreBuilder { } type FutureResult<'a, T, E> = Pin> + Send + 'a>>; + +/// Converts an integer crossing the database boundary, erroring instead of wrapping +/// when the value does not fit the destination type (e.g. a negative amount or height). +pub(crate) fn checked_conv(value: T, context: &'static str) -> Result +where + T: Copy + Into, + U: TryFrom, +{ + U::try_from(value).map_err(|_| BdkSqlxError::IntOutOfRange { + context, + value: value.into(), + }) +} diff --git a/src/postgres.rs b/src/postgres.rs index 27843d5..9cf1881 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -394,7 +394,10 @@ impl Store { let did = descriptor.descriptor_id(); changeset.descriptor = Some(descriptor); if let Some(last_rev) = external_last_revealed { - changeset.indexer.last_revealed.insert(did, last_rev as u32); + changeset.indexer.last_revealed.insert( + did, + crate::checked_conv(last_rev, "keychain.last_revealed")?, + ); } } @@ -403,7 +406,10 @@ impl Store { let did = descriptor.descriptor_id(); changeset.change_descriptor = Some(descriptor); if let Some(last_rev) = internal_last_revealed { - changeset.indexer.last_revealed.insert(did, last_rev as u32); + changeset.indexer.last_revealed.insert( + did, + crate::checked_conv(last_rev, "keychain.last_revealed")?, + ); } } @@ -519,7 +525,7 @@ async fn update_last_revealed( sqlx::query( r#"UPDATE "bdk_wallet"."keychain" SET last_revealed = $1 WHERE wallet_name = $2 AND descriptor_id = $3"#, ) - .bind(last_revealed as i32) + .bind(crate::checked_conv::<_, i32>(last_revealed, "keychain.last_revealed")?) .bind(wallet_name) .bind(descriptor_id.to_byte_array()) .execute(&mut **db_tx) @@ -571,7 +577,9 @@ pub async fn tx_graph_changeset_from_postgres( changeset.txs.insert(Arc::new(tx)); } if let Some(last_seen) = last_seen { - changeset.last_seen.insert(txid, last_seen as u64); + changeset + .last_seen + .insert(txid, crate::checked_conv(last_seen, "tx.last_seen")?); } } @@ -597,10 +605,10 @@ pub async fn tx_graph_changeset_from_postgres( changeset.txouts.insert( OutPoint { txid, - vout: vout as u32, + vout: crate::checked_conv(vout, "txout.vout")?, }, TxOut { - value: Amount::from_sat(value as u64), + value: Amount::from_sat(crate::checked_conv(value, "txout.value")?), script_pubkey: ScriptBuf::from(script), }, ); @@ -667,7 +675,7 @@ pub async fn tx_graph_changeset_persist_to_postgres( sqlx::query( r#"UPDATE "bdk_wallet"."tx" SET last_seen = $1 WHERE wallet_name = $2 AND txid = $3"#, ) - .bind(last_seen as i64) + .bind(crate::checked_conv::<_, i64>(last_seen, "tx.last_seen")?) .bind(wallet_name) .bind(txid.to_string()) .execute(&mut **db_tx) @@ -685,8 +693,8 @@ pub async fn tx_graph_changeset_persist_to_postgres( ) .bind(wallet_name) .bind(op.txid.to_string()) - .bind(op.vout as i32) - .bind(txo.value.to_sat() as i64) + .bind(crate::checked_conv::<_, i32>(op.vout, "txout.vout")?) + .bind(crate::checked_conv::<_, i64>(txo.value.to_sat(), "txout.value")?) .bind(txo.script_pubkey.as_bytes()) .execute(&mut **db_tx) .await @@ -741,7 +749,10 @@ pub async fn local_chain_changeset_from_postgres( let hash: String = row.get("hash"); let height: i32 = row.get("height"); let block_hash = BlockHash::from_str(&hash)?; - changeset.blocks.insert(height as u32, Some(block_hash)); + changeset.blocks.insert( + crate::checked_conv(height, "block.height")?, + Some(block_hash), + ); } Ok(changeset) @@ -764,7 +775,7 @@ pub async fn local_chain_changeset_persist_to_postgres( ) .bind(wallet_name) .bind(hash.to_string()) - .bind(height as i32) + .bind(crate::checked_conv::<_, i32>(height, "block.height")?) .execute(&mut **db_tx) .await .map_err(|e| BdkSqlxError::QueryError { @@ -777,7 +788,7 @@ pub async fn local_chain_changeset_persist_to_postgres( r#"DELETE FROM "bdk_wallet"."block" WHERE wallet_name = $1 AND height = $2"#, ) .bind(wallet_name) - .bind(height as i32) + .bind(crate::checked_conv::<_, i32>(height, "block.height")?) .execute(&mut **db_tx) .await .map_err(|e| BdkSqlxError::QueryError { diff --git a/src/sqlite.rs b/src/sqlite.rs index 33c9309..f182ffa 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -150,7 +150,10 @@ impl Store { let did = descriptor.descriptor_id(); changeset.descriptor = Some(descriptor); if let Some(last_rev) = external_last_revealed { - changeset.indexer.last_revealed.insert(did, last_rev as u32); + changeset.indexer.last_revealed.insert( + did, + crate::checked_conv(last_rev, "keychain.last_revealed")?, + ); } } @@ -159,7 +162,10 @@ impl Store { let did = descriptor.descriptor_id(); changeset.change_descriptor = Some(descriptor); if let Some(last_rev) = internal_last_revealed { - changeset.indexer.last_revealed.insert(did, last_rev as u32); + changeset.indexer.last_revealed.insert( + did, + crate::checked_conv(last_rev, "keychain.last_revealed")?, + ); } } @@ -267,7 +273,10 @@ async fn update_last_revealed( sqlx::query::( "UPDATE keychain SET last_revealed = $1 WHERE wallet_name = $2 AND descriptor_id = $3", ) - .bind(last_revealed as i32) + .bind(crate::checked_conv::<_, i32>( + last_revealed, + "keychain.last_revealed", + )?) .bind(wallet_name) .bind(descriptor_id.to_byte_array().as_slice()) .execute(&mut **tx) @@ -309,7 +318,9 @@ pub async fn tx_graph_changeset_from_sqlite( changeset.txs.insert(Arc::new(tx)); } if let Some(last_seen) = last_seen { - changeset.last_seen.insert(txid, last_seen as u64); + changeset + .last_seen + .insert(txid, crate::checked_conv(last_seen, "tx.last_seen")?); } } @@ -329,10 +340,10 @@ pub async fn tx_graph_changeset_from_sqlite( changeset.txouts.insert( OutPoint { txid, - vout: vout as u32, + vout: crate::checked_conv(vout, "txout.vout")?, }, TxOut { - value: Amount::from_sat(value as u64), + value: Amount::from_sat(crate::checked_conv(value, "txout.value")?), script_pubkey: ScriptBuf::from(script), }, ); @@ -389,7 +400,7 @@ pub async fn tx_graph_changeset_persist_to_sqlite( for (&txid, &last_seen) in &changeset.last_seen { sqlx::query("UPDATE tx SET last_seen = $1 WHERE wallet_name = $2 AND txid = $3") - .bind(last_seen as i64) + .bind(crate::checked_conv::<_, i64>(last_seen, "tx.last_seen")?) .bind(wallet_name) .bind(txid.to_string()) .execute(&mut **db_tx) @@ -403,8 +414,11 @@ pub async fn tx_graph_changeset_persist_to_sqlite( ) .bind(wallet_name) .bind(op.txid.to_string()) - .bind(op.vout as i32) - .bind(txo.value.to_sat() as i64) + .bind(crate::checked_conv::<_, i32>(op.vout, "txout.vout")?) + .bind(crate::checked_conv::<_, i64>( + txo.value.to_sat(), + "txout.value", + )?) .bind(txo.script_pubkey.as_bytes()) .execute(&mut **db_tx) .await?; @@ -446,7 +460,10 @@ pub async fn local_chain_changeset_from_sqlite( let hash: String = row.get("hash"); let height: i32 = row.get("height"); let block_hash = BlockHash::from_str(&hash)?; - changeset.blocks.insert(height as u32, Some(block_hash)); + changeset.blocks.insert( + crate::checked_conv(height, "block.height")?, + Some(block_hash), + ); } Ok(changeset) @@ -469,14 +486,14 @@ pub async fn local_chain_changeset_persist_to_sqlite( ) .bind(wallet_name) .bind(hash.to_string()) - .bind(height as i32) + .bind(crate::checked_conv::<_, i32>(height, "block.height")?) .execute(&mut **db_tx) .await?; } None => { sqlx::query("DELETE FROM block WHERE wallet_name = $1 AND height = $2") .bind(wallet_name) - .bind(height as i32) + .bind(crate::checked_conv::<_, i32>(height, "block.height")?) .execute(&mut **db_tx) .await?; } diff --git a/src/test.rs b/src/test.rs index 93315f3..3406416 100644 --- a/src/test.rs +++ b/src/test.rs @@ -150,6 +150,114 @@ impl AsyncWalletPersister for TestStore { } } +async fn corrupt_ranges_postgres(store: &Store, wallet_name: &str) -> anyhow::Result<()> { + let pool = store.pool.clone(); + + // a negative sat value must not wrap into astronomical amounts + sqlx::query(r#"UPDATE "bdk_wallet"."txout" SET value=-1 WHERE wallet_name=$1"#) + .bind(wallet_name) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::IntOutOfRange { .. })); + sqlx::query(r#"UPDATE "bdk_wallet"."txout" SET value=1 WHERE wallet_name=$1"#) + .bind(wallet_name) + .execute(&pool) + .await?; + + // a negative block height must not wrap into a huge height + sqlx::query( + r#"UPDATE "bdk_wallet"."block" SET height=-1 WHERE wallet_name=$1 AND height=2000"#, + ) + .bind(wallet_name) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::IntOutOfRange { .. })); + sqlx::query( + r#"UPDATE "bdk_wallet"."block" SET height=2000 WHERE wallet_name=$1 AND height=-1"#, + ) + .bind(wallet_name) + .execute(&pool) + .await?; + + store.read().await?; + Ok(()) +} + +async fn corrupt_ranges_sqlite(store: &Store, wallet_name: &str) -> anyhow::Result<()> { + let pool = store.pool.clone(); + + // a negative sat value must not wrap into astronomical amounts + sqlx::query("UPDATE txout SET value=-1 WHERE wallet_name=$1") + .bind(wallet_name) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::IntOutOfRange { .. })); + sqlx::query("UPDATE txout SET value=1 WHERE wallet_name=$1") + .bind(wallet_name) + .execute(&pool) + .await?; + + // a negative block height must not wrap into a huge height + sqlx::query("UPDATE block SET height=-1 WHERE wallet_name=$1 AND height=2000") + .bind(wallet_name) + .execute(&pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::IntOutOfRange { .. })); + sqlx::query("UPDATE block SET height=2000 WHERE wallet_name=$1 AND height=-1") + .bind(wallet_name) + .execute(&pool) + .await?; + + store.read().await?; + Ok(()) +} + +/// Out-of-range integers stored in the database (e.g. negative amounts or heights) +/// must error on load instead of wrapping around to huge unsigned values. +#[tokio::test] +async fn out_of_range_values_error_on_load() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + let stores = create_test_stores(wallet_name.clone()).await?; + for mut store in stores { + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + let txid = insert_fake_tx( + &mut wallet, + Amount::from_sat(20_000), + Amount::from_sat(10_000), + Amount::from_sat(1_000), + ); + // add a floating txout row so the txout table is populated + let mut extra = ChangeSet::default(); + extra.tx_graph.txouts.insert( + OutPoint { txid, vout: 0 }, + TxOut { + value: Amount::from_sat(1), + script_pubkey: Default::default(), + }, + ); + assert!(wallet.persist_async(&mut store).await?); + TestStore::persist(&mut store, &extra).await?; + + match &store { + TestStore::Postgres(store) => corrupt_ranges_postgres(store, &wallet_name).await?, + TestStore::Sqlite(store) => corrupt_ranges_sqlite(store, &wallet_name).await?, + } + } + Ok(()) +} + /// Data stored for a different network than the store was configured with (or an /// unparseable network string) must fail the load instead of being silently accepted. #[tokio::test] From 55305b235cb0a3cc4f894789a061b3a3566cac65 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 09:13:49 -0700 Subject: [PATCH 08/18] fix: make descriptor/network writes idempotent, verify last_revealed updates Security review finding M3 (medium): insert_descriptor and insert_network were bare INSERTs - unlike every other writer in the crate and unlike upstream BDK stores - so re-persisting a merged changeset that carried the descriptor or network again aborted with a unique-constraint violation. update_last_revealed silently updated 0 rows when the keychain row was missing, which would lose derivation state and lead to address reuse. - insert_descriptor/insert_network now upsert (ON CONFLICT DO UPDATE) in both backends. - update_last_revealed returns QueryError{keychain, RowNotFound} when no row matched, in both backends. - New regression test repersisting_full_changeset_is_idempotent covers both behaviors on both backends. --- src/postgres.rs | 34 +++++++++++++++++++++++----------- src/sqlite.rs | 26 +++++++++++++++++++------- src/test.rs | 46 +++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 87 insertions(+), 19 deletions(-) diff --git a/src/postgres.rs b/src/postgres.rs index 9cf1881..f302ffb 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -475,7 +475,8 @@ async fn insert_descriptor( }; sqlx::query( - r#"INSERT INTO "bdk_wallet"."keychain" (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ($1, $2, $3, $4)"#, + r#"INSERT INTO "bdk_wallet"."keychain" (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ($1, $2, $3, $4) + ON CONFLICT (wallet_name, keychainkind) DO UPDATE SET descriptor = excluded.descriptor, descriptor_id = excluded.descriptor_id"#, ) .bind(wallet_name) .bind(keychain) @@ -499,15 +500,18 @@ async fn insert_network( network: Network, ) -> Result<()> { trace!("insert network"); - sqlx::query(r#"INSERT INTO "bdk_wallet"."network" (wallet_name, name) VALUES ($1, $2)"#) - .bind(wallet_name) - .bind(network.to_string()) - .execute(&mut **db_tx) - .await - .map_err(|e| BdkSqlxError::QueryError { - table: "insert network".to_string(), - source: e, - })?; + sqlx::query( + r#"INSERT INTO "bdk_wallet"."network" (wallet_name, name) VALUES ($1, $2) + ON CONFLICT (wallet_name) DO UPDATE SET name = excluded.name"#, + ) + .bind(wallet_name) + .bind(network.to_string()) + .execute(&mut **db_tx) + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "insert network".to_string(), + source: e, + })?; Ok(()) } @@ -522,7 +526,7 @@ async fn update_last_revealed( ) -> Result<()> { trace!("update last revealed"); - sqlx::query( + let result = sqlx::query( r#"UPDATE "bdk_wallet"."keychain" SET last_revealed = $1 WHERE wallet_name = $2 AND descriptor_id = $3"#, ) .bind(crate::checked_conv::<_, i32>(last_revealed, "keychain.last_revealed")?) @@ -535,6 +539,14 @@ async fn update_last_revealed( source: e, })?; + // Silently updating 0 rows would lose derivation state and cause address reuse. + if result.rows_affected() == 0 { + return Err(BdkSqlxError::QueryError { + table: "keychain".to_string(), + source: sqlx::Error::RowNotFound, + }); + } + Ok(()) } diff --git a/src/sqlite.rs b/src/sqlite.rs index f182ffa..cce35a3 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -231,7 +231,8 @@ async fn insert_descriptor( }; sqlx::query( - "INSERT INTO keychain (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ($1, $2, $3, $4)", + "INSERT INTO keychain (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ($1, $2, $3, $4) + ON CONFLICT (wallet_name, keychainkind) DO UPDATE SET descriptor = excluded.descriptor, descriptor_id = excluded.descriptor_id", ) .bind(wallet_name) .bind(keychain) @@ -251,11 +252,14 @@ async fn insert_network( network: Network, ) -> Result<(), BdkSqlxError> { trace!("insert network"); - sqlx::query("INSERT INTO network (wallet_name, name) VALUES ($1, $2)") - .bind(wallet_name) - .bind(network.to_string()) - .execute(&mut **tx) - .await?; + sqlx::query( + "INSERT INTO network (wallet_name, name) VALUES ($1, $2) + ON CONFLICT (wallet_name) DO UPDATE SET name = excluded.name", + ) + .bind(wallet_name) + .bind(network.to_string()) + .execute(&mut **tx) + .await?; Ok(()) } @@ -270,7 +274,7 @@ async fn update_last_revealed( ) -> Result<(), BdkSqlxError> { trace!("update last revealed"); - sqlx::query::( + let result = sqlx::query::( "UPDATE keychain SET last_revealed = $1 WHERE wallet_name = $2 AND descriptor_id = $3", ) .bind(crate::checked_conv::<_, i32>( @@ -282,6 +286,14 @@ async fn update_last_revealed( .execute(&mut **tx) .await?; + // Silently updating 0 rows would lose derivation state and cause address reuse. + if result.rows_affected() == 0 { + return Err(BdkSqlxError::QueryError { + table: "keychain".to_string(), + source: sqlx::Error::RowNotFound, + }); + } + Ok(()) } diff --git a/src/test.rs b/src/test.rs index 3406416..348b6d9 100644 --- a/src/test.rs +++ b/src/test.rs @@ -5,7 +5,7 @@ use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Once; use assert_matches::assert_matches; -use bdk_chain::{BlockId, ConfirmationBlockTime}; +use bdk_chain::{BlockId, ConfirmationBlockTime, DescriptorId}; use bdk_wallet::{ bitcoin, chain as bdk_chain, descriptor::ExtendedDescriptor, @@ -150,6 +150,50 @@ impl AsyncWalletPersister for TestStore { } } +/// Re-persisting a merged/full changeset must be idempotent (upsert, not bare INSERT), +/// and updating last_revealed for a keychain that was never stored must error rather +/// than silently updating 0 rows and losing derivation state. +#[tokio::test] +async fn repersisting_full_changeset_is_idempotent() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + let stores = create_test_stores(wallet_name).await?; + for mut store in stores { + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + let _ = wallet.reveal_next_address(External); + assert!(wallet.persist_async(&mut store).await?); + + // A merged changeset carries the descriptors and network again; persisting it + // previously failed with a unique-constraint violation on the bare INSERTs. + let full = TestStore::initialize(&mut store).await?; + assert!(full.descriptor.is_some() && full.network.is_some()); + TestStore::persist(&mut store, &full).await?; + + // last_revealed for a keychain that is not stored must error loudly + let mut cs = ChangeSet::default(); + cs.indexer.last_revealed.insert( + DescriptorId(bitcoin::hashes::sha256::Hash::hash(b"missing keychain")), + 5, + ); + assert_matches!( + TestStore::persist(&mut store, &cs).await, + Err(BdkSqlxError::QueryError { .. }) + ); + } + Ok(()) +} + async fn corrupt_ranges_postgres(store: &Store, wallet_name: &str) -> anyhow::Result<()> { let pool = store.pool.clone(); From 720ee4bfc8e30c82dfd2a91761ed74700c416caa Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 09:17:00 -0700 Subject: [PATCH 09/18] fix: adopt sqlx versioned migrations for the postgres backend Security review finding M4 (medium): the postgres backend hand-rolled its schema with CREATE TABLE IF NOT EXISTS strings in migrate(), wrote a version row it never read back, and had no way to alter existing databases - blocking schema fixes like H1's FK change. The sqlite backend already used sqlx::migrate!(); both backends now share one scheme. - migrate() now runs sqlx::migrate!("./migrations/postgres"). - The H1 constraint upgrade moves from the hardcoded query list into versioned migration 02. - Databases created by earlier releases (no _sqlx_migrations bookkeeping) are adopted transparently: migration 01 is pure IF NOT EXISTS, and migration 02 upgrades pre-cascade anchor_tx constraints in place. Verified against a database built with the old hand-rolled schema and live rows: both migrations apply, and a reorg block delete cascades. --- .../02_anchor_tx_on_delete_cascade.sql | 25 ++++ src/postgres.rs | 125 ++---------------- 2 files changed, 33 insertions(+), 117 deletions(-) create mode 100644 migrations/postgres/02_anchor_tx_on_delete_cascade.sql diff --git a/migrations/postgres/02_anchor_tx_on_delete_cascade.sql b/migrations/postgres/02_anchor_tx_on_delete_cascade.sql new file mode 100644 index 0000000..f753f7f --- /dev/null +++ b/migrations/postgres/02_anchor_tx_on_delete_cascade.sql @@ -0,0 +1,25 @@ +-- Databases created before the anchor_tx foreign keys included ON DELETE CASCADE +-- reject reorg-driven block deletion while anchor_tx rows still reference the +-- block, wedging all further persistence. Recreate such constraints in place. +-- No-op for databases created from the current 01 migration. +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM pg_constraint c + JOIN pg_class t ON t.oid = c.conrelid + JOIN pg_namespace n ON n.oid = t.relnamespace + WHERE n.nspname = 'bdk_wallet' AND t.relname = 'anchor_tx' + AND c.contype = 'f' AND c.confdeltype <> 'c' + ) THEN + ALTER TABLE "bdk_wallet"."anchor_tx" + DROP CONSTRAINT IF EXISTS anchor_tx_wallet_name_block_hash_fkey, + DROP CONSTRAINT IF EXISTS anchor_tx_wallet_name_txid_fkey; + ALTER TABLE "bdk_wallet"."anchor_tx" + ADD CONSTRAINT anchor_tx_wallet_name_block_hash_fkey + FOREIGN KEY (wallet_name, block_hash) + REFERENCES "bdk_wallet"."block"(wallet_name, hash) ON DELETE CASCADE, + ADD CONSTRAINT anchor_tx_wallet_name_txid_fkey + FOREIGN KEY (wallet_name, txid) + REFERENCES "bdk_wallet"."tx"(wallet_name, txid) ON DELETE CASCADE; + END IF; +END $$; diff --git a/src/postgres.rs b/src/postgres.rs index f302ffb..4553de2 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -22,6 +22,7 @@ use bdk_wallet::{ }; use sqlx::{ postgres::{PgPool, PgRow, Postgres}, + sqlx_macros::migrate, Pool, Row, Transaction, }; use tracing::{trace, warn}; @@ -200,126 +201,16 @@ impl PgStoreBuilder { } impl Store { - /// Runs Migrations for a [`Store`] without an existing pg connection. + /// Runs the versioned migrations in `migrations/postgres` for this [`Store`]. + /// + /// Databases created by earlier releases (which created the schema without + /// migration bookkeeping) are adopted transparently: migration 01 only uses + /// `CREATE ... IF NOT EXISTS`, and migration 02 upgrades pre-existing + /// `anchor_tx` constraints in place. #[tracing::instrument(skip_all)] pub async fn migrate(&self) -> Result<()> { trace!("migrating bdk sqlx"); - - let mut tx = self.pool.begin().await?; - - // Create the schema first - let create_schema_query = r#"CREATE SCHEMA IF NOT EXISTS "bdk_wallet""#; - sqlx::query(create_schema_query) - .execute(&mut *tx) - .await - .map_err(|e| BdkSqlxError::QueryError { - table: "create schema bdk_wallet".to_string(), - source: e, - })?; - - // Create the tables one by one - let queries = [ - r#"CREATE TABLE IF NOT EXISTS "bdk_wallet"."version" ( - version INTEGER PRIMARY KEY - )"#, - r#"CREATE TABLE IF NOT EXISTS "bdk_wallet"."network" ( - wallet_name TEXT PRIMARY KEY, - name TEXT NOT NULL - )"#, - r#"CREATE TABLE IF NOT EXISTS "bdk_wallet"."keychain" ( - wallet_name TEXT NOT NULL, - keychainkind TEXT NOT NULL, - descriptor TEXT NOT NULL, - descriptor_id BYTEA NOT NULL, - last_revealed INTEGER DEFAULT 0, - PRIMARY KEY (wallet_name, keychainkind) - )"#, - r#"CREATE TABLE IF NOT EXISTS "bdk_wallet"."block" ( - wallet_name TEXT NOT NULL, - hash TEXT NOT NULL, - height INTEGER NOT NULL, - PRIMARY KEY (wallet_name, hash) - )"#, - r#"CREATE INDEX IF NOT EXISTS idx_block_height ON "bdk_wallet"."block" (height)"#, - r#"CREATE TABLE IF NOT EXISTS "bdk_wallet"."tx" ( - wallet_name TEXT NOT NULL, - txid TEXT NOT NULL, - whole_tx BYTEA, - last_seen BIGINT, - PRIMARY KEY (wallet_name, txid) - )"#, - r#"CREATE TABLE IF NOT EXISTS "bdk_wallet"."txout" ( - wallet_name TEXT NOT NULL, - txid TEXT NOT NULL, - vout INTEGER NOT NULL, - value BIGINT NOT NULL, - script BYTEA NOT NULL, - PRIMARY KEY (wallet_name, txid, vout) - )"#, - r#"CREATE TABLE IF NOT EXISTS "bdk_wallet"."anchor_tx" ( - wallet_name TEXT NOT NULL, - block_hash TEXT NOT NULL, - anchor JSONB NOT NULL, - txid TEXT NOT NULL, - PRIMARY KEY (wallet_name, block_hash, txid), - FOREIGN KEY (wallet_name, block_hash) REFERENCES "bdk_wallet"."block"(wallet_name, hash) ON DELETE CASCADE, - FOREIGN KEY (wallet_name, txid) REFERENCES "bdk_wallet"."tx"(wallet_name, txid) ON DELETE CASCADE - )"#, - r#"CREATE INDEX IF NOT EXISTS idx_anchor_tx_txid ON "bdk_wallet"."anchor_tx" (txid)"#, - // Databases created before the FK clauses above included ON DELETE CASCADE - // reject reorg-driven block deletion while anchor_tx rows still reference the - // block, wedging all further persistence. Recreate such constraints in place. - r#"DO $$ - BEGIN - IF EXISTS ( - SELECT 1 FROM pg_constraint c - JOIN pg_class t ON t.oid = c.conrelid - JOIN pg_namespace n ON n.oid = t.relnamespace - WHERE n.nspname = 'bdk_wallet' AND t.relname = 'anchor_tx' - AND c.contype = 'f' AND c.confdeltype <> 'c' - ) THEN - ALTER TABLE "bdk_wallet"."anchor_tx" - DROP CONSTRAINT IF EXISTS anchor_tx_wallet_name_block_hash_fkey, - DROP CONSTRAINT IF EXISTS anchor_tx_wallet_name_txid_fkey; - ALTER TABLE "bdk_wallet"."anchor_tx" - ADD CONSTRAINT anchor_tx_wallet_name_block_hash_fkey - FOREIGN KEY (wallet_name, block_hash) - REFERENCES "bdk_wallet"."block"(wallet_name, hash) ON DELETE CASCADE, - ADD CONSTRAINT anchor_tx_wallet_name_txid_fkey - FOREIGN KEY (wallet_name, txid) - REFERENCES "bdk_wallet"."tx"(wallet_name, txid) ON DELETE CASCADE; - END IF; - END $$"#, - ]; - - // Execute each query separately - for query in &queries { - sqlx::query(query) - .execute(&mut *tx) - .await - .map_err(|e| BdkSqlxError::QueryError { - table: query.to_string(), - source: e, - })?; - } - - // At the end of migration, insert the current version - // After all tables are created but before tx.commit() - sqlx::query( - r#"INSERT INTO "bdk_wallet"."version" (version) - VALUES ($1) - ON CONFLICT (version) DO NOTHING"#, - ) - .bind(1) // Current schema version - .execute(&mut *tx) - .await - .map_err(|e| BdkSqlxError::QueryError { - table: "insert version".to_string(), - source: e, - })?; - - tx.commit().await?; - + migrate!("./migrations/postgres").run(&self.pool).await?; Ok(()) } } From ba5d90fd564a29a218b587a49a441f6b1ab8e505 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 09:41:11 -0700 Subject: [PATCH 10/18] chore: dependency, TLS, and CI hygiene Security review finding M5 (medium): - Remove sqlx-postgres-tester: it was an unused [dependencies] entry that pulled in the obsolete sqlx-core 0.6.3 (flagged by cargo's future-incompatibility report) into every consumer's tree. - Move bdk_wallet's test-utils feature to [dev-dependencies] so test helpers are no longer compiled into release builds of consumers. - Document the postgres TLS default (sslmode=prefer silently falls back to plaintext) on build_with_url and in a new README security-notes section, along with least-privilege role guidance. - Add #![forbid(unsafe_code)]. - Add a rustsec cargo-audit job to CI. --- .github/workflows/rust.yml | 11 ++++++++++- Cargo.toml | 4 ++-- README.md | 11 +++++++++++ src/lib.rs | 1 + src/postgres.rs | 8 ++++++++ 5 files changed, 32 insertions(+), 3 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index c5058d6..539c731 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -172,4 +172,13 @@ jobs: - name: Check fmt run: cargo fmt --all -- --check - name: Clippy - run: cargo clippy --all-targets -- -Dwarnings \ No newline at end of file + run: cargo clippy --all-targets -- -Dwarnings + + audit: + name: Audit dependencies + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: rustsec/audit-check@v2 + with: + token: ${{ secrets.GITHUB_TOKEN }} \ No newline at end of file diff --git a/Cargo.toml b/Cargo.toml index fb1e805..bff003b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ version = "0.0.1" edition = "2021" [dependencies] -bdk_wallet = { version = "1.2.0", features = ["test-utils"] } +bdk_wallet = { version = "1.2.0" } serde = { version = "1.0.208", features = ["derive"] } serde_json = "1.0.125" sqlx = { version = "0.8.1", default-features = false, features = ["runtime-tokio", "tls-rustls-ring","derive", "postgres", "sqlite", "json", "chrono", "uuid", "sqlx-macros", "migrate"] } @@ -12,12 +12,12 @@ thiserror = "1" tokio = { version = "1.40.0", features = ["macros", "rt-multi-thread", "sync"] } tracing = "0.1.40" tracing-subscriber = { version = "0.3.18", features = ["env-filter", "serde_json", "json"] } -sqlx-postgres-tester = "0.1.1" [dev-dependencies] assert_matches = "1.5.0" anyhow = "1.0.89" bdk_electrum = { version = "0.20.1"} +bdk_wallet = { version = "1.2.0", features = ["test-utils"] } rustls = "0.23.14" [[example]] diff --git a/README.md b/README.md index ffdedb1..ebe30ce 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,17 @@ This crate is still **EXPERIMENTAL** do not use with mainnet wallets. +## Security notes + +- Without an explicit `sslmode`, postgres connections default to `prefer`, which + silently falls back to plaintext if TLS negotiation fails. For any deployment where + the database is not on the same host, require TLS in the connection URL + (`?sslmode=require`, or `verify-full` to also authenticate the server). +- Connect with a least-privilege database role: the store only needs DML on the + `bdk_wallet` schema (plus DDL when running migrations). +- Stored descriptors are sensitive (xpubs reveal the entire wallet history and + structure); protect database backups and access accordingly. + ## Testing 1. Install postgresql with `psql` tool. For example (macos): diff --git a/src/lib.rs b/src/lib.rs index b876bca..0a93a7e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,7 @@ //! bdk-sqlx #![warn(missing_docs)] +#![forbid(unsafe_code)] mod postgres; mod sqlite; diff --git a/src/postgres.rs b/src/postgres.rs index 4553de2..9f97644 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -188,6 +188,14 @@ impl PgStoreBuilder { /// This is a convenience method that creates a connection pool from the URL /// and then builds the [`Store`] using that pool. /// + /// # Security + /// + /// Without an explicit `sslmode`, postgres connections default to `prefer`, which + /// silently falls back to plaintext if TLS negotiation fails. For any deployment + /// where the database is not on the same host, require TLS in the URL (e.g. + /// `?sslmode=require`, or `verify-full` to also authenticate the server), and + /// connect with a least-privilege database role. + /// /// # Errors /// /// Returns an error if: From f7571db75f2a1abfad0f9a84a4fd0455c60d54e8 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 09:52:09 -0700 Subject: [PATCH 11/18] fix: enforce one block row per height, clean up replaced blocks on reorg Security review finding L5 (low): the block table's only uniqueness was (wallet_name, hash), so a reorg that replaced the block at a height with a different hash (local_chain changeset entry (height, Some(new_hash))) inserted a second row at the same height and left the old one behind - subsequent loads picked one of the two rows nondeterministically. - Persisting a block now first deletes any row at the same height with a different hash (its stale anchors cascade away via the H1 FKs), then upserts, in both backends. - Migration 03 (both backends) deduplicates existing rows and adds a UNIQUE index on (wallet_name, height) so the invariant is enforced by the database from now on. - New regression test reorg_replaced_block_leaves_single_row verifies a replaced block leaves exactly one row, the new hash wins the load, and the stale anchors are gone - on both backends. --- .../postgres/03_block_unique_height.sql | 11 ++++ migrations/sqlite/03_block_unique_height.sql | 9 +++ src/postgres.rs | 15 +++++ src/sqlite.rs | 11 ++++ src/test.rs | 62 +++++++++++++++++++ 5 files changed, 108 insertions(+) create mode 100644 migrations/postgres/03_block_unique_height.sql create mode 100644 migrations/sqlite/03_block_unique_height.sql diff --git a/migrations/postgres/03_block_unique_height.sql b/migrations/postgres/03_block_unique_height.sql new file mode 100644 index 0000000..d8ddc5b --- /dev/null +++ b/migrations/postgres/03_block_unique_height.sql @@ -0,0 +1,11 @@ +-- A wallet's chain has exactly one block hash per height, but the block table only +-- enforced uniqueness on (wallet_name, hash): a reorg that replaced the block at a +-- height left both rows behind and made loads nondeterministic. Remove duplicates +-- (survivor arbitrary among the duplicates; their anchors cascade away) and enforce +-- one row per height from here on. +DELETE FROM "bdk_wallet"."block" b +WHERE EXISTS ( + SELECT 1 FROM "bdk_wallet"."block" b2 + WHERE b2.wallet_name = b.wallet_name AND b2.height = b.height AND b2.ctid > b.ctid +); +CREATE UNIQUE INDEX IF NOT EXISTS idx_block_wallet_height ON "bdk_wallet"."block" (wallet_name, height); diff --git a/migrations/sqlite/03_block_unique_height.sql b/migrations/sqlite/03_block_unique_height.sql new file mode 100644 index 0000000..c0fd703 --- /dev/null +++ b/migrations/sqlite/03_block_unique_height.sql @@ -0,0 +1,9 @@ +-- A wallet's chain has exactly one block hash per height, but the block table only +-- enforced uniqueness on (wallet_name, hash): a reorg that replaced the block at a +-- height left both rows behind and made loads nondeterministic. Remove duplicates, +-- keeping the most recently inserted row (their anchors cascade away), and enforce +-- one row per height from here on. +DELETE FROM block WHERE rowid NOT IN ( + SELECT MAX(rowid) FROM block GROUP BY wallet_name, height +); +CREATE UNIQUE INDEX idx_block_wallet_height ON block (wallet_name, height); diff --git a/src/postgres.rs b/src/postgres.rs index 9f97644..172314d 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -680,6 +680,21 @@ pub async fn local_chain_changeset_persist_to_postgres( for (&height, &hash) in &changeset.blocks { match hash { Some(hash) => { + // A reorg can replace the block at this height with a different hash; + // remove the stale row first (its anchors cascade away) so exactly one + // row per (wallet_name, height) remains. + sqlx::query( + r#"DELETE FROM "bdk_wallet"."block" WHERE wallet_name = $1 AND height = $2 AND hash != $3"#, + ) + .bind(wallet_name) + .bind(crate::checked_conv::<_, i32>(height, "block.height")?) + .bind(hash.to_string()) + .execute(&mut **db_tx) + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "delete stale block".to_string(), + source: e, + })?; sqlx::query( r#"INSERT INTO "bdk_wallet"."block" (wallet_name, hash, height) VALUES ($1, $2, $3) ON CONFLICT (wallet_name, hash) DO UPDATE SET height = $3"#, diff --git a/src/sqlite.rs b/src/sqlite.rs index cce35a3..5b01f1d 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -492,6 +492,17 @@ pub async fn local_chain_changeset_persist_to_sqlite( for (&height, &hash) in &changeset.blocks { match hash { Some(hash) => { + // A reorg can replace the block at this height with a different hash; + // remove the stale row first (its anchors cascade away) so exactly one + // row per (wallet_name, height) remains. + sqlx::query( + "DELETE FROM block WHERE wallet_name = $1 AND height = $2 AND hash != $3", + ) + .bind(wallet_name) + .bind(crate::checked_conv::<_, i32>(height, "block.height")?) + .bind(hash.to_string()) + .execute(&mut **db_tx) + .await?; sqlx::query( "INSERT INTO block (wallet_name, hash, height) VALUES ($1, $2, $3) ON CONFLICT (wallet_name, hash) DO UPDATE SET height = $3", diff --git a/src/test.rs b/src/test.rs index 348b6d9..b90ab6b 100644 --- a/src/test.rs +++ b/src/test.rs @@ -150,6 +150,68 @@ impl AsyncWalletPersister for TestStore { } } +/// A reorg that replaces the block at a height with a different hash must leave exactly +/// one row for that height (the old row's anchors cascade away), not accumulate +/// duplicate rows that make loads nondeterministic. +#[tokio::test] +async fn reorg_replaced_block_leaves_single_row() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + let stores = create_test_stores(wallet_name.clone()).await?; + for mut store in stores { + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + let _txid = insert_fake_tx( + &mut wallet, + Amount::from_sat(20_000), + Amount::from_sat(10_000), + Amount::from_sat(1_000), + ); + assert!(wallet.persist_async(&mut store).await?); + + // Replace the block at height 2000 with a different hash, as a reorg does. + let new_hash = BlockHash::from_byte_array([2u8; 32]); + let mut reorg = ChangeSet::default(); + reorg.local_chain.blocks.insert(2_000, Some(new_hash)); + TestStore::persist(&mut store, &reorg).await?; + + let cs = TestStore::initialize(&mut store).await?; + assert_eq!(cs.local_chain.blocks.get(&2_000), Some(&Some(new_hash))); + // anchors referenced the replaced block and must be gone with it + assert!(cs.tx_graph.anchors.is_empty()); + + // exactly one row must remain at that height + let rows_at_height: i64 = match &store { + TestStore::Postgres(store) => sqlx::query_scalar( + r#"SELECT count(*) FROM "bdk_wallet"."block" WHERE wallet_name=$1 AND height=2000"#, + ) + .bind(&wallet_name) + .fetch_one(&store.pool) + .await?, + TestStore::Sqlite(store) => { + sqlx::query_scalar( + "SELECT count(*) FROM block WHERE wallet_name=$1 AND height=2000", + ) + .bind(&wallet_name) + .fetch_one(&store.pool) + .await? + } + }; + assert_eq!(rows_at_height, 1); + } + Ok(()) +} + /// Re-persisting a merged/full changeset must be idempotent (upsert, not bare INSERT), /// and updating last_revealed for a keychain that was never stored must error rather /// than silently updating 0 rows and losing derivation state. From 31ff6712f8f5d149cdc431e1a29b42c8952318d0 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 09:54:32 -0700 Subject: [PATCH 12/18] fix: commit the read snapshot transaction instead of dropping it Security review finding L2 (low): read() opened a transaction for its multi-query snapshot but never committed it, relying on implicit rollback at drop. Commit it explicitly on the success path in both backends. --- src/postgres.rs | 4 ++++ src/sqlite.rs | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/postgres.rs b/src/postgres.rs index 172314d..fee7577 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -251,6 +251,10 @@ impl Store { Self::changeset_from_row(&mut db_tx, &mut changeset, row, &self.wallet_name).await?; } + // The reads happened inside one transaction for a consistent snapshot; + // close it out explicitly instead of relying on drop-rollback. + db_tx.commit().await?; + Ok(changeset) } diff --git a/src/sqlite.rs b/src/sqlite.rs index 5b01f1d..dcbee98 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -119,6 +119,10 @@ impl Store { Self::changeset_from_row(&mut tx, &mut changeset, row, &self.wallet_name).await?; } + // The reads happened inside one transaction for a consistent snapshot; + // close it out explicitly instead of relying on drop-rollback. + tx.commit().await?; + Ok(changeset) } From 57eab296239f4da2583020b372307b50dff0e851 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 10:01:37 -0700 Subject: [PATCH 13/18] docs: warn about multi-connection pools on sqlite :memory: databases Security review finding L1 (low): Store::::new accepts any pool, but a multi-connection pool on :memory: gives every connection its own private database - reads and writes silently diverge and per-connection PRAGMAs don't apply pool-wide. Document the constraint and point callers at new_with_url(None, ..), which configures a single-connection pool. The test suite already switched to that constructor in the H5 commit. --- src/sqlite.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/sqlite.rs b/src/sqlite.rs index dcbee98..cbbbee7 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -49,6 +49,15 @@ impl AsyncWalletPersister for Store { impl Store { /// Construct a new [`Store`] with an existing sqlite connection pool. + /// + /// # Warning + /// + /// Do not pass a pool connected to `:memory:` with more than one connection: + /// each sqlite connection gets its *own* private in-memory database, so a + /// multi-connection pool silently reads and writes different databases (and + /// per-connection `PRAGMA`s only apply to the connection that ran them). + /// Use [`Store::new_with_url`] with `None` instead, which configures a + /// single-connection pool correctly. #[tracing::instrument(skip_all)] pub async fn new( pool: Pool, From e09a0d79d68f44199b1b5f944afa1a1735fecce7 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 10:03:31 -0700 Subject: [PATCH 14/18] fix: correct error message typo and misleading MissingPool description Security review finding L7 (low): fix 'bitoin' typo in the HexToArray error message, and reword MissingPool - it fired when the builder had no pool configured, not when a postgres connection failed to initialize. The sqlite/postgres builder API asymmetry noted in the same finding (no SqliteStoreBuilder) is a feature addition and is left as a follow-up. --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 0a93a7e..b19d551 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -23,7 +23,7 @@ use sqlx::{Database, PgPool}; #[derive(Debug, thiserror::Error)] pub enum BdkSqlxError { /// bitcoin parse hex error - #[error("bitoin parse hex error: {0}")] + #[error("bitcoin parse hex error: {0}")] HexToArray(#[from] bitcoin::hex::HexToArrayError), /// bitcoin consensus decode error #[error("bitcoin consensus decode error: {0}")] @@ -79,7 +79,7 @@ pub enum BdkSqlxError { #[error("Network Missing")] MissingNetwork, /// Config error - #[error("Cant initialize Postgres connection")] + #[error("No database connection pool provided to the builder")] MissingPool, /// Config error #[error("Network Failed to set")] From f92edefc5df60992e5ff93898df6fb9bea47f5e1 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 10:22:04 -0700 Subject: [PATCH 15/18] docs: document that pub use sqlx couples the API to sqlx's major version Security review finding L9 (low): the sqlx re-export is part of the public API surface, so a sqlx major version bump is a breaking change for this crate. Document that, and steer consumers to import sqlx types through the re-export to stay version-aligned. --- src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index b19d551..1eefa43 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -15,6 +15,12 @@ use std::pin::Pin; use bdk_wallet::bitcoin; use bdk_wallet::bitcoin::{BlockHash, Network, Txid}; use bdk_wallet::chain::miniscript; +/// Re-export of the [`sqlx`] crate this library is built on. +/// +/// Consumers that construct pools themselves should import sqlx types through this +/// re-export so their sqlx version always matches the one this crate links against. +/// Note this couples the crate's public API to sqlx's major version: a sqlx major +/// bump is a breaking change for this crate as well. pub use sqlx; use sqlx::Pool; use sqlx::{Database, PgPool}; From 0843f8f3ecb022a119914bdbbdc80751fc32e817 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 10:22:33 -0700 Subject: [PATCH 16/18] ci: collapse five copy-pasted per-test jobs into one test-suite job Security review finding L6 (low): CI ran five identical postgres jobs, one per test, because tests could not share a database. Now that every test isolates itself in its own database (H5 fix), a single job runs the whole suite in parallel. The manual psql database-creation steps are gone too - tests create their own databases against the service's default postgres database. --- .github/workflows/rust.yml | 136 ++----------------------------------- 1 file changed, 7 insertions(+), 129 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 539c731..075761e 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -3,7 +3,7 @@ name: CI on: push: branches: [ "master" ] - + pull_request: types: [ opened, synchronize, reopened ] branches: @@ -11,128 +11,11 @@ on: env: CARGO_TERM_COLOR: auto - PGPASSWORD: password - DATABASE_TEST_URL: postgres://postgres:password@localhost:5432/testdb + DATABASE_TEST_URL: postgres://postgres:password@localhost:5432/postgres jobs: - wallet-is-persisted: - name: Test wallet persistence - runs-on: ubuntu-latest - services: - postgres: - image: postgres:14 - env: - POSTGRES_USER: postgres - POSTGRES_PASSWORD: password - POSTGRES_DB: postgres - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 5432:5432 - steps: - - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 - - name: Create database - run: | - sudo apt-get install libpq-dev -y - psql -h localhost -p 5432 -U postgres -d postgres -c 'create user testuser' - psql -h localhost -p 5432 -U postgres -d postgres -c 'create database testdb with owner = testuser' - - name: Test wallet_is_persisted - run: cargo test wallet_is_persisted -- --show-output - - test-three-wallets: - name: Test three wallets list transactions - runs-on: ubuntu-latest - services: - postgres: - image: postgres:14 - env: - POSTGRES_USER: postgres - POSTGRES_PASSWORD: password - POSTGRES_DB: postgres - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 5432:5432 - steps: - - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 - - name: Create database - run: | - sudo apt-get install libpq-dev -y - psql -h localhost -p 5432 -U postgres -d postgres -c 'create user testuser' - psql -h localhost -p 5432 -U postgres -d postgres -c 'create database testdb with owner = testuser' - - name: Test test_three_wallets_list_transactions - run: cargo test test_three_wallets_list_transactions -- --show-output - - wallet-load-checks: - name: Test wallet load checks - runs-on: ubuntu-latest - services: - postgres: - image: postgres:14 - env: - POSTGRES_USER: postgres - POSTGRES_PASSWORD: password - POSTGRES_DB: postgres - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 5432:5432 - steps: - - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 - - name: Create database - run: | - sudo apt-get install libpq-dev -y - psql -h localhost -p 5432 -U postgres -d postgres -c 'create user testuser' - psql -h localhost -p 5432 -U postgres -d postgres -c 'create database testdb with owner = testuser' - - name: Test wallet_load_checks - run: cargo test wallet_load_checks -- --show-output - - single-descriptor-wallet: - name: Test single descriptor wallet - runs-on: ubuntu-latest - services: - postgres: - image: postgres:14 - env: - POSTGRES_USER: postgres - POSTGRES_PASSWORD: password - POSTGRES_DB: postgres - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 5432:5432 - steps: - - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 - - name: Create database - run: | - sudo apt-get install libpq-dev -y - psql -h localhost -p 5432 -U postgres -d postgres -c 'create user testuser' - psql -h localhost -p 5432 -U postgres -d postgres -c 'create database testdb with owner = testuser' - - name: Test single_descriptor_wallet_persist_and_recover - run: cargo test single_descriptor_wallet_persist_and_recover -- --show-output - - two-wallets-load: - name: Test two wallets load + test: + name: Test suite runs-on: ubuntu-latest services: postgres: @@ -152,13 +35,8 @@ jobs: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - - name: Create database - run: | - sudo apt-get install libpq-dev -y - psql -h localhost -p 5432 -U postgres -d postgres -c 'create user testuser' - psql -h localhost -p 5432 -U postgres -d postgres -c 'create database testdb with owner = testuser' - - name: Test two_wallets_load - run: cargo test two_wallets_load -- --show-output + - name: Test + run: cargo test -- --show-output fmt-clippy: name: Check @@ -181,4 +59,4 @@ jobs: - uses: actions/checkout@v4 - uses: rustsec/audit-check@v2 with: - token: ${{ secrets.GITHUB_TOKEN }} \ No newline at end of file + token: ${{ secrets.GITHUB_TOKEN }} From cae370719a210e47b85c6b4b1b372ae0da9261ec Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 14:39:20 -0700 Subject: [PATCH 17/18] fix: close silent data-loss and corruption paths across both backends A hardening pass over the whole store, each defect pinned by an always-on regression test (src/test.rs, tests/builder_network.rs): - tx.last_seen for a tx not yet stored was silently dropped (the UPDATE affected 0 rows); the write now upserts a stub row (whole_tx is nullable). - Reads anchored on the network row, so rows persisted by a changeset that carried no network were written but never read back; tx, block and keychain tables are now read unconditionally. - A changeset mapping the same block hash to several heights silently collapsed to one block row, losing checkpoints; such changesets are now rejected with DuplicateBlockHash. - The postgres write path did not validate changeset.network against the configured network, letting a foreign network overwrite the row and wedge all subsequent reads; the write is now rejected with InvalidNetwork. - keychain.last_revealed INTEGER DEFAULT 0 made a wallet persisted before its first address reveal reload with index 0 marked used, skipping it forever. New rows now store NULL explicitly and migration 04 drops the default. Existing rows are deliberately untouched: a stored 0 is ambiguous ('revealed index 0' vs 'never revealed') and rewriting it could cause address reuse. - update_last_revealed was a plain UPDATE, letting a stale/replayed changeset move the derivation index backwards and silently reuse addresses; the update now never decreases the stored value. - Store::::read ran at READ COMMITTED (per-statement snapshots), so a concurrent writer could produce a mixed-generation changeset; the read transaction now uses REPEATABLE READ. - initialize_network had a check-then-set race that failed concurrent same-network builds spuriously with SetNetworkFailure; a lost race now re-validates instead. The race regression test lives in its own integration-test binary (tests/builder_network.rs) because the configured network is process-global. - Store derived Clone with a DB: Clone bound that sqlx's Postgres/ Sqlite marker types do not satisfy, making the impl unusable; a manual bound-free impl is provided. - The sqlite backend had no network validation at all: its constructor took no network and any stored or incoming network was accepted. It now takes the network at construction (shared process-global with the postgres backend) and applies the same read/write guards. Store::::new and new_with_url therefore take a network argument. - insert_descriptor's conflict update kept the stored last_revealed unconditionally, so replacing a descriptor under the same (wallet_name, keychainkind) made the new descriptor inherit the old derivation index and silently skip those addresses on load. The keep is now conditional on the descriptor being unchanged. - A keychainkind value outside 'External'/'Internal' was silently ignored on load, dropping a keychain; corrupt rows now fail with InvalidKeychainKind. - Reorgs left duplicate block rows per height behind and made loads nondeterministic; migration 03 dedupes and enforces one row per (wallet_name, height), and the write path removes replaced blocks so their anchors cascade away. - Corrupt stored data (undecodable tx bytes, txid/anchor payload mismatches, negative or overflowing integers at the database boundary) now fails the load loudly via TxidMismatch, AnchorBlockHashMismatch and checked_conv instead of being silently skipped or wrapping. - Migration 05 drops the dead version table and the redundant idx_block_height index on both backends. Also: tokio and tracing-subscriber move to dev-dependencies (library consumers should not pay for test-only deps), and the README gains a Resolved defects section and security notes (TLS sslmode, least- privilege roles, descriptor sensitivity). --- Cargo.toml | 4 +- README.md | 55 + ...04_keychain_last_revealed_drop_default.sql | 7 + ...p_version_table_and_block_height_index.sql | 7 + ...04_keychain_last_revealed_drop_default.sql | 17 + ...p_version_table_and_block_height_index.sql | 6 + src/lib.rs | 139 +- src/postgres.rs | 228 +- src/sqlite.rs | 183 +- src/test.rs | 2013 ++++++++++++++++- tests/builder_network.rs | 81 + 11 files changed, 2465 insertions(+), 275 deletions(-) create mode 100644 migrations/postgres/04_keychain_last_revealed_drop_default.sql create mode 100644 migrations/postgres/05_drop_version_table_and_block_height_index.sql create mode 100644 migrations/sqlite/04_keychain_last_revealed_drop_default.sql create mode 100644 migrations/sqlite/05_drop_version_table_and_block_height_index.sql create mode 100644 tests/builder_network.rs diff --git a/Cargo.toml b/Cargo.toml index bff003b..2243270 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,9 +9,7 @@ serde = { version = "1.0.208", features = ["derive"] } serde_json = "1.0.125" sqlx = { version = "0.8.1", default-features = false, features = ["runtime-tokio", "tls-rustls-ring","derive", "postgres", "sqlite", "json", "chrono", "uuid", "sqlx-macros", "migrate"] } thiserror = "1" -tokio = { version = "1.40.0", features = ["macros", "rt-multi-thread", "sync"] } tracing = "0.1.40" -tracing-subscriber = { version = "0.3.18", features = ["env-filter", "serde_json", "json"] } [dev-dependencies] assert_matches = "1.5.0" @@ -19,6 +17,8 @@ anyhow = "1.0.89" bdk_electrum = { version = "0.20.1"} bdk_wallet = { version = "1.2.0", features = ["test-utils"] } rustls = "0.23.14" +tokio = { version = "1.40.0", features = ["macros", "rt-multi-thread", "sync"] } +tracing-subscriber = { version = "0.3.18", features = ["env-filter", "serde_json", "json"] } [[example]] name = "bdk_sqlx_postgres" diff --git a/README.md b/README.md index ebe30ce..945a01f 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,61 @@ This crate is still **EXPERIMENTAL** do not use with mainnet wallets. +## Resolved defects + +The following defects were found in review and are fixed; each is guarded by +an always-on regression test in the suite (`src/test.rs` and +`tests/builder_network.rs`): + +- `tx.last_seen` for a tx not yet stored was silently dropped (the `UPDATE` + affected 0 rows); the write now upserts a stub row (`whole_tx` is nullable). +- Reads anchored on the `network` row, so rows persisted by a changeset that + carried no network were written but never read back; tx/block tables are now + read unconditionally. +- A changeset mapping the same block hash to several heights silently + collapsed to one block row, losing checkpoints; such changesets are now + rejected with `DuplicateBlockHash`. +- The postgres `write` path did not validate `changeset.network` against the + configured network, letting a foreign network overwrite the row and wedge + all subsequent reads; the write is now rejected with `InvalidNetwork`. +- `keychain.last_revealed INTEGER DEFAULT 0` made a wallet persisted before + its first address reveal reload with index 0 marked as used, skipping it + forever. New rows now store NULL explicitly and migration 04 drops the + default. Existing rows are deliberately untouched: a stored `0` is + ambiguous ("revealed index 0" vs "never revealed") and rewriting it could + cause address reuse. +- `update_last_revealed` was a plain `UPDATE`, letting a stale/replayed + changeset move the derivation index backwards and silently reuse addresses; + the update now never decreases the stored value. +- `Store::::read` ran at READ COMMITTED (per-statement snapshots), + so a concurrent writer could produce a mixed-generation changeset; the read + transaction now uses REPEATABLE READ. +- `initialize_network` had a check-then-set race that failed concurrent + same-network builds spuriously with `SetNetworkFailure`; a lost race now + re-validates instead. +- `Store` derived `Clone` with a `DB: Clone` bound that sqlx's `Postgres`/ + `Sqlite` marker types do not satisfy, making the impl unusable; a manual + bound-free impl is provided. +- Reads anchored keychain rows on the `network` row, so descriptors and + derivation state persisted by a changeset that carried no network were + written but never read back (the tx/block invisibility defect, one table + over); keychain rows are now read unconditionally. +- The sqlite backend had no network validation at all: its constructor took + no network and any stored or incoming network was accepted. It now takes + the network at construction (shared process-global with the postgres + backend, so one process can never mix networks) and applies the same + read/write guards. `Store::::new` and `new_with_url` therefore + take a `network` argument. +- `insert_descriptor`'s conflict update kept the stored `last_revealed` + unconditionally, so replacing a descriptor under the same + `(wallet_name, keychainkind)` made the new descriptor inherit the old + derivation index and silently skip those addresses on load. The keep is + now conditional on the descriptor being unchanged; a replaced descriptor + restarts derivation at NULL. +- A `keychainkind` value outside `'External'`/`'Internal'` was silently + ignored on load, dropping a keychain; corrupt rows now fail with + `InvalidKeychainKind`. + ## Security notes - Without an explicit `sslmode`, postgres connections default to `prefer`, which diff --git a/migrations/postgres/04_keychain_last_revealed_drop_default.sql b/migrations/postgres/04_keychain_last_revealed_drop_default.sql new file mode 100644 index 0000000..e4d97fc --- /dev/null +++ b/migrations/postgres/04_keychain_last_revealed_drop_default.sql @@ -0,0 +1,7 @@ +-- The keychain table declared `last_revealed INTEGER DEFAULT 0`, which made a +-- wallet persisted before its first address reveal reload as if index 0 had +-- been revealed, skipping it forever. The store now inserts NULL explicitly, +-- so the default is removed. Existing values are left untouched on purpose: +-- a stored 0 is ambiguous ("revealed index 0" vs "never revealed") and cannot +-- be rewritten without risking address reuse. +ALTER TABLE "bdk_wallet"."keychain" ALTER COLUMN last_revealed DROP DEFAULT; diff --git a/migrations/postgres/05_drop_version_table_and_block_height_index.sql b/migrations/postgres/05_drop_version_table_and_block_height_index.sql new file mode 100644 index 0000000..35165ad --- /dev/null +++ b/migrations/postgres/05_drop_version_table_and_block_height_index.sql @@ -0,0 +1,7 @@ +-- Schema hygiene. The version table is dead schema from the hand-rolled +-- versioning scheme that sqlx's migration bookkeeping replaced; migration 01 +-- created it only so databases from before the migrator could be adopted +-- unchanged. idx_block_height(height) was made redundant by migration 03's +-- unique (wallet_name, height) index, which serves the same lookups. +DROP TABLE IF EXISTS "bdk_wallet"."version"; +DROP INDEX IF EXISTS "bdk_wallet"."idx_block_height"; diff --git a/migrations/sqlite/04_keychain_last_revealed_drop_default.sql b/migrations/sqlite/04_keychain_last_revealed_drop_default.sql new file mode 100644 index 0000000..aa8d23b --- /dev/null +++ b/migrations/sqlite/04_keychain_last_revealed_drop_default.sql @@ -0,0 +1,17 @@ +-- The keychain table declared `last_revealed INTEGER DEFAULT 0`, which made a +-- wallet persisted before its first address reveal reload as if index 0 had +-- been revealed, skipping it forever. The store now inserts NULL explicitly. +-- SQLite cannot alter a column default in place, so the table is rebuilt (data +-- preserved; a stored 0 is ambiguous and deliberately left as-is). +CREATE TABLE keychain_new ( + wallet_name TEXT NOT NULL, + keychainkind TEXT NOT NULL, + descriptor TEXT NOT NULL, + descriptor_id BLOB NOT NULL, + last_revealed INTEGER, + PRIMARY KEY (wallet_name, keychainkind) +); +INSERT INTO keychain_new + SELECT wallet_name, keychainkind, descriptor, descriptor_id, last_revealed FROM keychain; +DROP TABLE keychain; +ALTER TABLE keychain_new RENAME TO keychain; diff --git a/migrations/sqlite/05_drop_version_table_and_block_height_index.sql b/migrations/sqlite/05_drop_version_table_and_block_height_index.sql new file mode 100644 index 0000000..983c00c --- /dev/null +++ b/migrations/sqlite/05_drop_version_table_and_block_height_index.sql @@ -0,0 +1,6 @@ +-- Schema hygiene. The version table is dead schema from before sqlx's +-- migration bookkeeping, and idx_block_height(height) was made redundant by +-- migration 03's unique (wallet_name, height) index, which serves the same +-- lookups. +DROP TABLE IF EXISTS version; +DROP INDEX IF EXISTS idx_block_height; diff --git a/src/lib.rs b/src/lib.rs index 1eefa43..f0f2208 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,10 +11,15 @@ mod test; use std::future::Future; use std::pin::Pin; +use std::str::FromStr; +use std::sync::OnceLock; use bdk_wallet::bitcoin; use bdk_wallet::bitcoin::{BlockHash, Network, Txid}; use bdk_wallet::chain::miniscript; +use bdk_wallet::chain::DescriptorExt; +use bdk_wallet::descriptor::{Descriptor, DescriptorPublicKey}; +use bdk_wallet::ChangeSet; /// Re-export of the [`sqlx`] crate this library is built on. /// /// Consumers that construct pools themselves should import sqlx types through this @@ -50,6 +55,19 @@ pub enum BdkSqlxError { /// block hash contained in the anchor payload computed: BlockHash, }, + /// a changeset maps the same block hash to more than one height, which the + /// block table cannot represent (its key is the hash itself) + #[error( + "changeset maps block hash {hash} to both height {first_height} and height {second_height}" + )] + DuplicateBlockHash { + /// offending block hash + hash: BlockHash, + /// first height the hash was mapped to + first_height: u32, + /// second, conflicting height + second_height: u32, + }, /// miniscript error #[error("miniscript error: {0}")] Miniscript(#[from] miniscript::Error), @@ -78,9 +96,14 @@ pub enum BdkSqlxError { /// New network network: Network, }, - /// Init failure - #[error("Cant initialize network correctly with: {0}")] - NetworkInitFailure(Network), + /// a stored keychainkind is neither of the two kinds this store writes + /// ('External'/'Internal'); the row is corrupt and must not be silently + /// skipped, or a keychain would vanish from the loaded wallet + #[error("stored keychain kind '{got}' is not 'External' or 'Internal'")] + InvalidKeychainKind { + /// offending keychainkind value + got: String, + }, /// Config error #[error("Network Missing")] MissingNetwork, @@ -112,12 +135,24 @@ pub enum BdkSqlxError { } /// Manages a pool of database connections. -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct Store { pub(crate) pool: Pool, wallet_name: String, } +// Manual impl: deriving Clone would bound `DB: Clone`, which sqlx's `Postgres` +// and `Sqlite` marker types do not satisfy, making the derived impl unusable +// for the actual backends. Cloning a store shares the connection pool. +impl Clone for Store { + fn clone(&self) -> Self { + Self { + pool: self.pool.clone(), + wallet_name: self.wallet_name.clone(), + } + } +} + /// Build a new instance of the PgStoreBuilder pub struct PgStoreBuilder { wallet_name: String, @@ -140,3 +175,99 @@ where value: value.into(), }) } + +/// Process-global network configuration, shared by every store in the process +/// regardless of backend. Persisting data for the wrong network wedges a +/// wallet store, so the first store built fixes the network for the whole +/// process and all stores validate against it. +static NETWORK: OnceLock = OnceLock::new(); + +/// Returns the process-global network, or an error if no store has set it yet. +pub(crate) fn get_network() -> Result { + NETWORK + .get() + .copied() + .ok_or(BdkSqlxError::GetNetworkFailure) +} + +/// Fixes the process-global network on first call. Later calls with the same +/// network are no-ops; a different network is rejected. +pub(crate) fn initialize_network(network: Network) -> Result<(), BdkSqlxError> { + if NETWORK.get().is_none() { + // A racing `set` is fine: the winner's value is validated below, so + // concurrent same-network builds can no longer fail spuriously. + let _ = NETWORK.set(network); + } + match NETWORK.get() { + Some(current) if *current == network => Ok(()), + Some(current) => Err(BdkSqlxError::DuplicateInitNetwork { + current: *current, + network, + }), + // Unreachable in practice: either our `set` won or a racer's did. + None => Err(BdkSqlxError::SetNetworkFailure(network)), + } +} + +/// Rejects `network` when this process was configured for a different one. +/// +/// Applied on the write path (a changeset's network) and the read path (the +/// stored network) of every backend. When the process-global network is not +/// set there is nothing to validate against and the check passes; stores +/// built through the public constructors always set it. +pub(crate) fn validate_network_matches_configured(network: Network) -> Result<(), BdkSqlxError> { + if let Ok(configured) = get_network() { + if configured != network { + return Err(BdkSqlxError::InvalidNetwork { + expected: configured.to_string(), + got: network.to_string(), + }); + } + } + Ok(()) +} + +/// Parses a network name read back from the database, rejecting both +/// unparseable names and networks this process was not configured for. +pub(crate) fn parse_and_validate_network(stored: &str) -> Result { + let network = Network::from_str(stored).map_err(|_| BdkSqlxError::InvalidNetwork { + expected: get_network() + .map(|n| n.to_string()) + .unwrap_or_else(|_| "a known network".to_string()), + got: stored.to_string(), + })?; + validate_network_matches_configured(network)?; + Ok(network) +} + +/// Applies one stored keychain row to `changeset`. +/// +/// Shared by both backends so the parse and validation rules cannot drift +/// apart. `keychainkind` comes from a free-text column; anything other than +/// the two kinds this store writes is corrupt data and must fail the load +/// loudly rather than silently drop a keychain. +pub(crate) fn keychain_changeset_from_parts( + changeset: &mut ChangeSet, + keychainkind: &str, + descriptor_str: &str, + last_revealed: Option, +) -> Result<(), BdkSqlxError> { + let descriptor: Descriptor = descriptor_str.parse()?; + let did = descriptor.descriptor_id(); + match keychainkind { + "External" => changeset.descriptor = Some(descriptor), + "Internal" => changeset.change_descriptor = Some(descriptor), + other => { + return Err(BdkSqlxError::InvalidKeychainKind { + got: other.to_string(), + }) + } + } + if let Some(last_rev) = last_revealed { + changeset + .indexer + .last_revealed + .insert(did, checked_conv(last_rev, "keychain.last_revealed")?); + } + Ok(()) +} diff --git a/src/postgres.rs b/src/postgres.rs index fee7577..a46ffef 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -3,10 +3,7 @@ #![warn(missing_docs)] // Standard library imports -use std::{ - str::FromStr, - sync::{Arc, OnceLock}, -}; +use std::{str::FromStr, sync::Arc}; // Third party crates use bdk_chain::{ local_chain, tx_graph, Anchor, ConfirmationBlockTime, DescriptorExt, DescriptorId, Merge, @@ -16,55 +13,22 @@ use bdk_wallet::{ self, consensus, hashes::Hash, Amount, BlockHash, Network, OutPoint, ScriptBuf, TxOut, Txid, }, chain as bdk_chain, - descriptor::{Descriptor, DescriptorPublicKey, ExtendedDescriptor}, + descriptor::ExtendedDescriptor, AsyncWalletPersister, ChangeSet, KeychainKind, KeychainKind::{External, Internal}, }; use sqlx::{ - postgres::{PgPool, PgRow, Postgres}, + postgres::{PgPool, Postgres}, sqlx_macros::migrate, Pool, Row, Transaction, }; -use tracing::{trace, warn}; +use tracing::trace; // First party imports use super::{BdkSqlxError, FutureResult, PgStoreBuilder, Store}; type Result = core::result::Result; -/// Thread-safe storage for the network configuration that's shared across all Store instances. -/// This ensures consistent network validation across multiple threads. -static NETWORK: OnceLock = OnceLock::new(); - -/// Retrieves the current global network configuration for validation operations. -/// -/// Returns the current network configuration or an error if not initialized. -fn get_network() -> Result { - NETWORK - .get() - .copied() - .ok_or_else(|| BdkSqlxError::GetNetworkFailure) -} - -/// Sets the global network configuration to ensure consistent validation across threads. -/// -/// Returns an error if the network is already initialized with a different network. -fn initialize_network(network: Network) -> Result<()> { - match NETWORK.get() { - Some(current) if *current == network => { - warn!("initialize_network called more than once"); - Ok(()) - } - Some(current) => Err(BdkSqlxError::DuplicateInitNetwork { - current: *current, - network, - }), - None => NETWORK - .set(network) - .map_err(BdkSqlxError::SetNetworkFailure), - } -} - impl AsyncWalletPersister for Store { type Error = BdkSqlxError; @@ -144,6 +108,12 @@ impl PgStoreBuilder { /// /// The network is required to build a valid [`Store`]. If not provided, /// the build operation will fail with a MissingNetwork error. + /// + /// The network is process-global and shared across backends: the first + /// store built (postgres or sqlite) fixes it for the whole process, and + /// later builds with a different network fail with + /// [`BdkSqlxError::DuplicateInitNetwork`]. Every store validates stored + /// and incoming data against it. pub fn network(mut self, network: Network) -> Self { self.network = Some(network); self @@ -175,7 +145,7 @@ impl PgStoreBuilder { store.migrate().await?; } - initialize_network(network)?; + crate::initialize_network(network)?; Ok(store) } @@ -228,29 +198,65 @@ impl Store { pub(crate) async fn read(&self) -> Result { trace!("reading"); let mut db_tx = self.pool.begin().await?; + // READ COMMITTED (the default) snapshots per statement, so a concurrent + // writer committing between the SELECTs below could produce a + // mixed-generation changeset. REPEATABLE READ gives one snapshot for + // the whole read. + sqlx::query("SET TRANSACTION ISOLATION LEVEL REPEATABLE READ") + .execute(&mut *db_tx) + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "read".to_string(), + source: e, + })?; let mut changeset = ChangeSet::default(); - let sql = r#"SELECT n.name as network, - k_int.descriptor as internal_descriptor, k_int.last_revealed as internal_last_revealed, - k_ext.descriptor as external_descriptor, k_ext.last_revealed as external_last_revealed - FROM "bdk_wallet"."network" n - LEFT JOIN "bdk_wallet"."keychain" k_int ON n.wallet_name = k_int.wallet_name AND k_int.keychainkind = 'Internal' - LEFT JOIN "bdk_wallet"."keychain" k_ext ON n.wallet_name = k_ext.wallet_name AND k_ext.keychainkind = 'External' - WHERE n.wallet_name = $1"#; - - // Fetch wallet data - let row = sqlx::query(sql) + + // Fetch the network row. It is optional: rows persisted by a changeset + // that carried no network must still be visible. + let row = sqlx::query(r#"SELECT name FROM "bdk_wallet"."network" WHERE wallet_name = $1"#) .bind(&self.wallet_name) .fetch_optional(&mut *db_tx) .await .map_err(|e| BdkSqlxError::QueryError { - table: "read".to_string(), + table: "read network".to_string(), source: e, })?; - if let Some(row) = row { - Self::changeset_from_row(&mut db_tx, &mut changeset, row, &self.wallet_name).await?; + let network: String = row.get("name"); + changeset.network = Some(crate::parse_and_validate_network(&network)?); + } + + // Fetch keychain rows unconditionally: anchoring them on the network + // row (as the old join did) made descriptors persisted without a + // network vanish from every subsequent read while sitting in the + // database. + let rows = sqlx::query( + r#"SELECT keychainkind, descriptor, last_revealed FROM "bdk_wallet"."keychain" WHERE wallet_name = $1"#, + ) + .bind(&self.wallet_name) + .fetch_all(&mut *db_tx) + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "read keychain".to_string(), + source: e, + })?; + for row in rows { + let keychainkind: String = row.get("keychainkind"); + let descriptor: String = row.get("descriptor"); + let last_revealed: Option = row.get("last_revealed"); + crate::keychain_changeset_from_parts( + &mut changeset, + &keychainkind, + &descriptor, + last_revealed, + )?; } + changeset.tx_graph = + tx_graph_changeset_from_postgres(&mut db_tx, &self.wallet_name).await?; + changeset.local_chain = + local_chain_changeset_from_postgres(&mut db_tx, &self.wallet_name).await?; + // The reads happened inside one transaction for a consistent snapshot; // close it out explicitly instead of relying on drop-rollback. db_tx.commit().await?; @@ -258,69 +264,6 @@ impl Store { Ok(changeset) } - #[tracing::instrument(skip(db_tx, changeset, row))] - pub(crate) async fn changeset_from_row( - db_tx: &mut Transaction<'_, Postgres>, - changeset: &mut ChangeSet, - row: PgRow, - wallet_name: &str, - ) -> Result<()> { - trace!("changeset from row"); - - let network: String = row.get("network"); - let internal_last_revealed: Option = row.get("internal_last_revealed"); - let external_last_revealed: Option = row.get("external_last_revealed"); - let internal_desc_str: Option = row.get("internal_descriptor"); - let external_desc_str: Option = row.get("external_descriptor"); - - let stored_network = - Network::from_str(&network).map_err(|_| BdkSqlxError::InvalidNetwork { - expected: get_network() - .map(|n| n.to_string()) - .unwrap_or_else(|_| "a known network".to_string()), - got: network.clone(), - })?; - // Reject data persisted for a different network than this process was - // configured for, instead of silently loading it. - if let Ok(configured) = get_network() { - if configured != stored_network { - return Err(BdkSqlxError::InvalidNetwork { - expected: configured.to_string(), - got: stored_network.to_string(), - }); - } - } - changeset.network = Some(stored_network); - - if let Some(desc_str) = external_desc_str { - let descriptor: Descriptor = desc_str.parse()?; - let did = descriptor.descriptor_id(); - changeset.descriptor = Some(descriptor); - if let Some(last_rev) = external_last_revealed { - changeset.indexer.last_revealed.insert( - did, - crate::checked_conv(last_rev, "keychain.last_revealed")?, - ); - } - } - - if let Some(desc_str) = internal_desc_str { - let descriptor: Descriptor = desc_str.parse()?; - let did = descriptor.descriptor_id(); - changeset.change_descriptor = Some(descriptor); - if let Some(last_rev) = internal_last_revealed { - changeset.indexer.last_revealed.insert( - did, - crate::checked_conv(last_rev, "keychain.last_revealed")?, - ); - } - } - - changeset.tx_graph = tx_graph_changeset_from_postgres(db_tx, wallet_name).await?; - changeset.local_chain = local_chain_changeset_from_postgres(db_tx, wallet_name).await?; - Ok(()) - } - #[tracing::instrument(skip_all)] pub(crate) async fn write(&self, changeset: &ChangeSet) -> Result<()> { trace!("changeset write"); @@ -340,6 +283,10 @@ impl Store { } if let Some(network) = changeset.network { + // Refuse to persist data for a different network than this process + // was configured for; overwriting the network row would wedge all + // subsequent reads with InvalidNetwork. + crate::validate_network_matches_configured(network)?; insert_network(&mut tx, wallet_name, network).await?; } @@ -377,9 +324,21 @@ async fn insert_descriptor( Internal => "Internal", }; + // last_revealed is inserted explicitly as NULL: "no address revealed yet" + // must be distinguishable from "address index 0 was revealed". The + // historical DEFAULT 0 conflated the two and made never-revealed wallets + // skip index 0 on reload. The conflict update keeps the stored + // last_revealed only when the descriptor itself is unchanged; a different + // descriptor under the same (wallet_name, keychainkind) must NOT inherit + // the old derivation index, or the replacement wallet would silently skip + // those addresses on load. sqlx::query( - r#"INSERT INTO "bdk_wallet"."keychain" (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ($1, $2, $3, $4) - ON CONFLICT (wallet_name, keychainkind) DO UPDATE SET descriptor = excluded.descriptor, descriptor_id = excluded.descriptor_id"#, + r#"INSERT INTO "bdk_wallet"."keychain" (wallet_name, keychainkind, descriptor, descriptor_id, last_revealed) VALUES ($1, $2, $3, $4, NULL) + ON CONFLICT (wallet_name, keychainkind) DO UPDATE SET + descriptor = excluded.descriptor, + descriptor_id = excluded.descriptor_id, + last_revealed = CASE WHEN keychain.descriptor_id = excluded.descriptor_id + THEN keychain.last_revealed ELSE NULL END"#, ) .bind(wallet_name) .bind(keychain) @@ -429,8 +388,12 @@ async fn update_last_revealed( ) -> Result<()> { trace!("update last revealed"); + // Derivation state must never move backwards: a stale or replayed + // changeset carrying a smaller index would silently re-reveal already + // handed-out addresses on the next load. let result = sqlx::query( - r#"UPDATE "bdk_wallet"."keychain" SET last_revealed = $1 WHERE wallet_name = $2 AND descriptor_id = $3"#, + r#"UPDATE "bdk_wallet"."keychain" SET last_revealed = CASE WHEN last_revealed IS NULL OR $1 > last_revealed THEN $1 ELSE last_revealed END + WHERE wallet_name = $2 AND descriptor_id = $3"#, ) .bind(crate::checked_conv::<_, i32>(last_revealed, "keychain.last_revealed")?) .bind(wallet_name) @@ -587,12 +550,16 @@ pub async fn tx_graph_changeset_persist_to_postgres( } for (&txid, &last_seen) in &changeset.last_seen { + // Upsert a stub row when the full tx is not stored yet; a bare UPDATE + // would affect 0 rows and silently drop the timestamp. whole_tx stays + // NULL until a changeset carrying the full tx fills it in. sqlx::query( - r#"UPDATE "bdk_wallet"."tx" SET last_seen = $1 WHERE wallet_name = $2 AND txid = $3"#, + r#"INSERT INTO "bdk_wallet"."tx" (wallet_name, txid, last_seen) VALUES ($1, $2, $3) + ON CONFLICT (wallet_name, txid) DO UPDATE SET last_seen = $3"#, ) - .bind(crate::checked_conv::<_, i64>(last_seen, "tx.last_seen")?) .bind(wallet_name) .bind(txid.to_string()) + .bind(crate::checked_conv::<_, i64>(last_seen, "tx.last_seen")?) .execute(&mut **db_tx) .await .map_err(|e| BdkSqlxError::QueryError { @@ -681,6 +648,23 @@ pub async fn local_chain_changeset_persist_to_postgres( changeset: &local_chain::ChangeSet, ) -> Result<()> { trace!("local chain changeset to postgres"); + // The block table keys rows by (wallet_name, hash), so a changeset mapping + // one hash to several heights cannot be represented: persisting it would + // silently collapse to a single row and lose checkpoints. Reject it loudly + // instead. Real chains never produce such changesets. + let mut seen = std::collections::HashMap::new(); + for (&height, &hash) in &changeset.blocks { + if let Some(hash) = hash { + if let Some(&first_height) = seen.get(&hash) { + return Err(BdkSqlxError::DuplicateBlockHash { + hash, + first_height, + second_height: height, + }); + } + seen.insert(hash, height); + } + } for (&height, &hash) in &changeset.blocks { match hash { Some(hash) => { diff --git a/src/sqlite.rs b/src/sqlite.rs index cbbbee7..c9ac131 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -13,10 +13,9 @@ use bdk_wallet::bitcoin::{ self, consensus, hashes::Hash, Amount, BlockHash, Network, OutPoint, ScriptBuf, TxOut, Txid, }; use bdk_wallet::chain as bdk_chain; -use bdk_wallet::descriptor::{Descriptor, DescriptorPublicKey, ExtendedDescriptor}; +use bdk_wallet::descriptor::ExtendedDescriptor; use bdk_wallet::KeychainKind::{External, Internal}; use bdk_wallet::{AsyncWalletPersister, ChangeSet, KeychainKind}; -use sqlx::sqlite::SqliteRow; use sqlx::sqlite::{SqlitePool, SqlitePoolOptions}; use sqlx::sqlx_macros::migrate; use sqlx::{sqlite::Sqlite, Pool, Row, Transaction}; @@ -50,6 +49,11 @@ impl AsyncWalletPersister for Store { impl Store { /// Construct a new [`Store`] with an existing sqlite connection pool. /// + /// `network` fixes the process-global network (shared with the postgres + /// backend): the first store built in the process sets it, later stores + /// must use the same one, and stored or incoming data for a different + /// network is rejected with [`BdkSqlxError::InvalidNetwork`]. + /// /// # Warning /// /// Do not pass a pool connected to `:memory:` with more than one connection: @@ -58,10 +62,16 @@ impl Store { /// per-connection `PRAGMA`s only apply to the connection that ran them). /// Use [`Store::new_with_url`] with `None` instead, which configures a /// single-connection pool correctly. + /// + /// The pool must not disable `PRAGMA foreign_keys` (sqlx enables it by + /// default): reorg handling relies on `ON DELETE CASCADE`, and with + /// foreign keys off a disconnected block's anchor rows are silently left + /// behind and reload forever. #[tracing::instrument(skip_all)] pub async fn new( pool: Pool, wallet_name: String, + network: Network, migrate: bool, ) -> Result { trace!("new sqlite store"); @@ -69,6 +79,7 @@ impl Store { trace!("migrate"); migrate!("./migrations/sqlite").run(&pool).await?; } + crate::initialize_network(network)?; Ok(Self { pool, wallet_name }) } @@ -78,10 +89,13 @@ impl Store { /// /// If no URL is given a memory DB (non-persisted) will be used. A memory DB /// is useful for testing. + /// + /// `network` has the same process-global semantics as [`Store::new`]. #[tracing::instrument(skip_all)] pub async fn new_with_url( url: Option, wallet_name: String, + network: Network, migrate: bool, ) -> Result, BdkSqlxError> { trace!("new store with url"); @@ -97,37 +111,54 @@ impl Store { .connect(":memory:") .await? }; - Self::new(pool, wallet_name, migrate).await + Self::new(pool, wallet_name, network, migrate).await } } impl Store { #[tracing::instrument(skip_all)] pub(crate) async fn read(&self) -> Result { - trace!("migrate and read"); + trace!("read"); let mut tx = self.pool.begin().await?; let mut changeset = ChangeSet::default(); - let sql = - "SELECT n.name as network, - k_int.descriptor as internal_descriptor, k_int.last_revealed as internal_last_revealed, - k_ext.descriptor as external_descriptor, k_ext.last_revealed as external_last_revealed - FROM network n - LEFT JOIN keychain k_int ON n.wallet_name = k_int.wallet_name AND k_int.keychainkind = 'Internal' - LEFT JOIN keychain k_ext ON n.wallet_name = k_ext.wallet_name AND k_ext.keychainkind = 'External' - WHERE n.wallet_name = $1"; - - // Fetch wallet data - let row = sqlx::query(sql) + + // Fetch the network row. It is optional: rows persisted by a changeset + // that carried no network must still be visible. + let row = sqlx::query("SELECT name FROM network WHERE wallet_name = $1") .bind(&self.wallet_name) .fetch_optional(&mut *tx) .await?; - - //dbg!(&row); - if let Some(row) = row { - Self::changeset_from_row(&mut tx, &mut changeset, row, &self.wallet_name).await?; + let network: String = row.get("name"); + changeset.network = Some(crate::parse_and_validate_network(&network)?); + } + + // Fetch keychain rows unconditionally: anchoring them on the network + // row (as the old join did) made descriptors persisted without a + // network vanish from every subsequent read while sitting in the + // database. + let rows = sqlx::query( + "SELECT keychainkind, descriptor, last_revealed FROM keychain WHERE wallet_name = $1", + ) + .bind(&self.wallet_name) + .fetch_all(&mut *tx) + .await?; + for row in rows { + let keychainkind: String = row.get("keychainkind"); + let descriptor: String = row.get("descriptor"); + let last_revealed: Option = row.get("last_revealed"); + crate::keychain_changeset_from_parts( + &mut changeset, + &keychainkind, + &descriptor, + last_revealed, + )?; } + changeset.tx_graph = tx_graph_changeset_from_sqlite(&mut tx, &self.wallet_name).await?; + changeset.local_chain = + local_chain_changeset_from_sqlite(&mut tx, &self.wallet_name).await?; + // The reads happened inside one transaction for a consistent snapshot; // close it out explicitly instead of relying on drop-rollback. tx.commit().await?; @@ -135,58 +166,6 @@ impl Store { Ok(changeset) } - //#[tracing::instrument(skip_all)] - pub(crate) async fn changeset_from_row( - tx: &mut Transaction<'_, Sqlite>, - changeset: &mut ChangeSet, - row: SqliteRow, - wallet_name: &str, - ) -> Result<(), BdkSqlxError> { - trace!("changeset from row"); - - let network: String = row.get("network"); - let internal_last_revealed: Option = row.get("internal_last_revealed"); - let external_last_revealed: Option = row.get("external_last_revealed"); - let internal_desc_str: Option = row.get("internal_descriptor"); - let external_desc_str: Option = row.get("external_descriptor"); - - changeset.network = - Some( - Network::from_str(&network).map_err(|_| BdkSqlxError::InvalidNetwork { - expected: "a known network".to_string(), - got: network.clone(), - })?, - ); - - if let Some(desc_str) = external_desc_str { - let descriptor: Descriptor = desc_str.parse()?; - let did = descriptor.descriptor_id(); - changeset.descriptor = Some(descriptor); - if let Some(last_rev) = external_last_revealed { - changeset.indexer.last_revealed.insert( - did, - crate::checked_conv(last_rev, "keychain.last_revealed")?, - ); - } - } - - if let Some(desc_str) = internal_desc_str { - let descriptor: Descriptor = desc_str.parse()?; - let did = descriptor.descriptor_id(); - changeset.change_descriptor = Some(descriptor); - if let Some(last_rev) = internal_last_revealed { - changeset.indexer.last_revealed.insert( - did, - crate::checked_conv(last_rev, "keychain.last_revealed")?, - ); - } - } - - changeset.tx_graph = tx_graph_changeset_from_sqlite(tx, wallet_name).await?; - changeset.local_chain = local_chain_changeset_from_sqlite(tx, wallet_name).await?; - Ok(()) - } - #[tracing::instrument(skip_all)] pub(crate) async fn write(&self, changeset: &ChangeSet) -> Result<(), BdkSqlxError> { trace!("changeset write"); @@ -206,6 +185,11 @@ impl Store { } if let Some(network) = changeset.network { + // Refuse to persist data for a different network than this process + // was configured for (the same guard the postgres backend + // applies); overwriting the network row would wedge all subsequent + // reads with InvalidNetwork. + crate::validate_network_matches_configured(network)?; insert_network(&mut tx, wallet_name, network).await?; } @@ -243,9 +227,21 @@ async fn insert_descriptor( Internal => "Internal", }; + // last_revealed is inserted explicitly as NULL: "no address revealed yet" + // must be distinguishable from "address index 0 was revealed". The + // historical DEFAULT 0 conflated the two and made never-revealed wallets + // skip index 0 on reload. The conflict update keeps the stored + // last_revealed only when the descriptor itself is unchanged; a different + // descriptor under the same (wallet_name, keychainkind) must NOT inherit + // the old derivation index, or the replacement wallet would silently skip + // those addresses on load. sqlx::query( - "INSERT INTO keychain (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ($1, $2, $3, $4) - ON CONFLICT (wallet_name, keychainkind) DO UPDATE SET descriptor = excluded.descriptor, descriptor_id = excluded.descriptor_id", + "INSERT INTO keychain (wallet_name, keychainkind, descriptor, descriptor_id, last_revealed) VALUES ($1, $2, $3, $4, NULL) + ON CONFLICT (wallet_name, keychainkind) DO UPDATE SET + descriptor = excluded.descriptor, + descriptor_id = excluded.descriptor_id, + last_revealed = CASE WHEN keychain.descriptor_id = excluded.descriptor_id + THEN keychain.last_revealed ELSE NULL END", ) .bind(wallet_name) .bind(keychain) @@ -287,8 +283,12 @@ async fn update_last_revealed( ) -> Result<(), BdkSqlxError> { trace!("update last revealed"); + // Derivation state must never move backwards: a stale or replayed + // changeset carrying a smaller index would silently re-reveal already + // handed-out addresses on the next load. let result = sqlx::query::( - "UPDATE keychain SET last_revealed = $1 WHERE wallet_name = $2 AND descriptor_id = $3", + "UPDATE keychain SET last_revealed = CASE WHEN last_revealed IS NULL OR $1 > last_revealed THEN $1 ELSE last_revealed END + WHERE wallet_name = $2 AND descriptor_id = $3", ) .bind(crate::checked_conv::<_, i32>( last_revealed, @@ -424,12 +424,18 @@ pub async fn tx_graph_changeset_persist_to_sqlite( } for (&txid, &last_seen) in &changeset.last_seen { - sqlx::query("UPDATE tx SET last_seen = $1 WHERE wallet_name = $2 AND txid = $3") - .bind(crate::checked_conv::<_, i64>(last_seen, "tx.last_seen")?) - .bind(wallet_name) - .bind(txid.to_string()) - .execute(&mut **db_tx) - .await?; + // Upsert a stub row when the full tx is not stored yet; a bare UPDATE + // would affect 0 rows and silently drop the timestamp. whole_tx stays + // NULL until a changeset carrying the full tx fills it in. + sqlx::query( + "INSERT INTO tx (wallet_name, txid, last_seen) VALUES ($1, $2, $3) + ON CONFLICT (wallet_name, txid) DO UPDATE SET last_seen = $3", + ) + .bind(wallet_name) + .bind(txid.to_string()) + .bind(crate::checked_conv::<_, i64>(last_seen, "tx.last_seen")?) + .execute(&mut **db_tx) + .await?; } for (op, txo) in &changeset.txouts { @@ -502,6 +508,23 @@ pub async fn local_chain_changeset_persist_to_sqlite( changeset: &local_chain::ChangeSet, ) -> Result<(), BdkSqlxError> { trace!("local chain changeset to sqlite"); + // The block table keys rows by (wallet_name, hash), so a changeset mapping + // one hash to several heights cannot be represented: persisting it would + // silently collapse to a single row and lose checkpoints. Reject it loudly + // instead. Real chains never produce such changesets. + let mut seen = std::collections::HashMap::new(); + for (&height, &hash) in &changeset.blocks { + if let Some(hash) = hash { + if let Some(&first_height) = seen.get(&hash) { + return Err(BdkSqlxError::DuplicateBlockHash { + hash, + first_height, + second_height: height, + }); + } + seen.insert(hash, height); + } + } for (&height, &hash) in &changeset.blocks { match hash { Some(hash) => { diff --git a/src/test.rs b/src/test.rs index b90ab6b..4129ee5 100644 --- a/src/test.rs +++ b/src/test.rs @@ -2,10 +2,10 @@ use std::env; use std::ops::Add; use std::str::FromStr; use std::sync::atomic::{AtomicU64, Ordering}; -use std::sync::Once; +use std::sync::{Arc, Mutex, Once, OnceLock}; use assert_matches::assert_matches; -use bdk_chain::{BlockId, ConfirmationBlockTime, DescriptorId}; +use bdk_chain::{BlockId, ConfirmationBlockTime, DescriptorExt, DescriptorId, Merge}; use bdk_wallet::{ bitcoin, chain as bdk_chain, descriptor::ExtendedDescriptor, @@ -20,7 +20,7 @@ use bitcoin::{ secp256k1::Secp256k1, Address, Amount, BlockHash, Network::{self, Regtest}, - OutPoint, Transaction, TxIn, TxOut, Txid, + OutPoint, ScriptBuf, Transaction, TxIn, TxOut, Txid, }; use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; use sqlx::{Pool, Postgres, Sqlite}; @@ -29,7 +29,7 @@ use test_utils::{ insert_tx, new_tx, }; use tracing::info; -use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt, EnvFilter}; +use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt, EnvFilter, Layer}; use crate::{BdkSqlxError, FutureResult, PgStoreBuilder, Store}; @@ -48,14 +48,35 @@ fn parse_descriptor(s: &str) -> ExtendedDescriptor { static INIT: Once = Once::new(); +type SharedLogBuffer = Arc>>; +static LOG_BUFFER: OnceLock = OnceLock::new(); + +/// Buffer that captures every trace event emitted in this test process. +fn log_buffer() -> SharedLogBuffer { + LOG_BUFFER.get().expect("initialize() first").clone() +} + // This must only be called once. fn initialize() { INIT.call_once(|| { + let buf: SharedLogBuffer = Arc::new(Mutex::new(Vec::new())); + let writer_buf = buf.clone(); + LOG_BUFFER.set(buf).expect("log buffer set once"); tracing_subscriber::registry() - .with(EnvFilter::new( + .with(tracing_subscriber::fmt::layer().with_filter(EnvFilter::new( env::var("RUST_LOG").unwrap_or_else(|_| "sqlx=warn,bdk_sqlx=warn".into()), - )) - .with(tracing_subscriber::fmt::layer()) + ))) + // A permanent global capture layer for the descriptor-leak test. A + // scoped subscriber (`with_subscriber`) must NOT be used here: sqlx's + // sqlite worker threads hold span references, and tearing the scoped + // registry down while those threads are alive panics the worker and + // wedges the pool. Each layer carries its own filter: a bare EnvFilter + // layer would disable trace events globally for every layer. + .with( + tracing_subscriber::fmt::layer() + .with_writer(move || SharedWriter(writer_buf.clone())) + .with_filter(EnvFilter::new("trace")), + ) .try_init() .expect("setup tracing"); }); @@ -64,14 +85,46 @@ fn initialize() { static TEST_DB_COUNTER: AtomicU64 = AtomicU64::new(0); static TEST_DB_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); +/// Name hashed into the postgres advisory lock that serializes test-database +/// management. `TEST_DB_LOCK` cannot exclude OTHER test processes sharing the +/// server, so creation/cleanup additionally runs under a session-scoped +/// advisory lock: only one process manages databases at a time, and a stale +/// database can never be dropped between its creation and first connection. +/// If a lock holder crashes, its session ends and postgres releases the lock. +const PG_MGMT_ADVISORY_NAME: &str = "bdk_sqlx_test_db_mgmt"; + +/// Takes the cross-process database-management lock on a dedicated session. +/// Must be paired with [`pg_mgmt_unlock`]; callers hold `TEST_DB_LOCK` first, +/// so contention can only come from other processes and no deadlock cycle +/// exists. +async fn pg_mgmt_lock( + admin_pool: &Pool, +) -> anyhow::Result> { + let mut conn = admin_pool.acquire().await?; + sqlx::query("SELECT pg_advisory_lock(hashtext($1))") + .bind(PG_MGMT_ADVISORY_NAME) + .execute(&mut *conn) + .await?; + Ok(conn) +} + +/// Releases the cross-process database-management lock. Best-effort: if the +/// session is already gone, postgres has released the lock anyway. +async fn pg_mgmt_unlock(mut conn: sqlx::pool::PoolConnection) { + let _ = sqlx::query("SELECT pg_advisory_unlock(hashtext($1))") + .bind(PG_MGMT_ADVISORY_NAME) + .execute(&mut *conn) + .await; +} + /// Creates a uniquely named database on the postgres server at `DATABASE_TEST_URL` and /// returns a pool connected to it, so every test gets an isolated database and no /// pre-existing tables are ever dropped. /// /// Databases left behind by previous test runs are removed opportunistically; a database /// is never dropped while any session is connected to it, and creation/cleanup are -/// serialized so a parallel test cannot drop a database between its creation and first -/// connection. +/// serialized (in-process by `TEST_DB_LOCK`, cross-process by the advisory lock) so a +/// parallel test cannot drop a database between its creation and first connection. async fn create_test_pg_pool() -> anyhow::Result> { let admin_url = env::var("DATABASE_TEST_URL").expect("DATABASE_TEST_URL must be set for tests"); let admin_pool = Pool::::connect(&admin_url).await?; @@ -82,35 +135,40 @@ async fn create_test_pg_pool() -> anyhow::Result> { TEST_DB_COUNTER.fetch_add(1, Ordering::Relaxed) ); - let guard = TEST_DB_LOCK.lock().await; - - let stale: Vec = sqlx::query_scalar( - "SELECT datname::text FROM pg_database d - WHERE datname LIKE 'bdk_sqlx_test_%' - AND NOT EXISTS (SELECT 1 FROM pg_stat_activity a WHERE a.datname = d.datname)", - ) - .fetch_all(&admin_pool) - .await?; - for stale_db in stale { - let _ = sqlx::query(&format!(r#"DROP DATABASE IF EXISTS "{stale_db}""#)) - .execute(&admin_pool) - .await; - } + let _guard = TEST_DB_LOCK.lock().await; + let mut mgmt = pg_mgmt_lock(&admin_pool).await?; - sqlx::query(&format!(r#"CREATE DATABASE "{db_name}""#)) - .execute(&admin_pool) + let result = async { + let stale: Vec = sqlx::query_scalar( + "SELECT datname::text FROM pg_database d + WHERE datname LIKE 'bdk_sqlx_test_%' + AND NOT EXISTS (SELECT 1 FROM pg_stat_activity a WHERE a.datname = d.datname)", + ) + .fetch_all(&mut *mgmt) .await?; + for stale_db in stale { + let _ = sqlx::query(&format!(r#"DROP DATABASE IF EXISTS "{stale_db}""#)) + .execute(&mut *mgmt) + .await; + } - // min_connections(1) keeps a session open for the pool's lifetime, which protects - // this database from the stale-database cleanup of tests in other processes. - let opts = PgConnectOptions::from_str(&admin_url)?.database(&db_name); - let pool = PgPoolOptions::new() - .min_connections(1) - .connect_with(opts) - .await?; - drop(guard); + sqlx::query(&format!(r#"CREATE DATABASE "{db_name}""#)) + .execute(&mut *mgmt) + .await?; - Ok(pool) + // min_connections(1) keeps a session open for the pool's lifetime, which protects + // this database from the stale-database cleanup of tests in other processes. + let opts = PgConnectOptions::from_str(&admin_url)?.database(&db_name); + let pool = PgPoolOptions::new() + .min_connections(1) + .connect_with(opts) + .await?; + anyhow::Ok(pool) + } + .await; + + pg_mgmt_unlock(mgmt).await; + result } #[derive(Debug)] @@ -119,6 +177,16 @@ enum TestStore { Sqlite(Store), } +impl TestStore { + /// Read the full changeset, matching `Store::read` on either backend. + async fn read(&self) -> Result { + match self { + TestStore::Postgres(store) => store.read().await, + TestStore::Sqlite(store) => store.read().await, + } + } +} + impl AsyncWalletPersister for TestStore { type Error = BdkSqlxError; @@ -180,15 +248,16 @@ async fn reorg_replaced_block_leaves_single_row() -> anyhow::Result<()> { assert!(wallet.persist_async(&mut store).await?); // Replace the block at height 2000 with a different hash, as a reorg does. - let new_hash = BlockHash::from_byte_array([2u8; 32]); + let new_hash = BlockHash::from_byte_array([77u8; 32]); let mut reorg = ChangeSet::default(); reorg.local_chain.blocks.insert(2_000, Some(new_hash)); TestStore::persist(&mut store, &reorg).await?; let cs = TestStore::initialize(&mut store).await?; assert_eq!(cs.local_chain.blocks.get(&2_000), Some(&Some(new_hash))); - // anchors referenced the replaced block and must be gone with it - assert!(cs.tx_graph.anchors.is_empty()); + // the anchor that referenced the replaced block is gone with it; the + // anchor on the untouched block at height 1000 survives + assert_eq!(cs.tx_graph.anchors.len(), 1); // exactly one row must remain at that height let rows_at_height: i64 = match &store { @@ -433,19 +502,16 @@ impl std::io::Write for SharedWriter { /// Regression test for descriptor material leaking into tracing output: even at TRACE /// verbosity, spans and events emitted while creating, persisting, and loading a wallet /// must not record descriptors, public keys, or changesets. +/// +/// The events are captured through the permanent global layer installed by +/// `initialize()`. A scoped subscriber previously used here raced sqlx's sqlite +/// worker threads at registry teardown and made the whole suite flaky. #[tokio::test] async fn tracing_output_contains_no_descriptor_material() -> anyhow::Result<()> { - use tracing::instrument::WithSubscriber; - initialize(); - let buf = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); - let writer_buf = buf.clone(); - let subscriber = tracing_subscriber::registry() - .with(EnvFilter::new("trace")) - .with( - tracing_subscriber::fmt::layer().with_writer(move || SharedWriter(writer_buf.clone())), - ); + let buf = log_buffer(); + buf.lock().unwrap().clear(); let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); let wallet_name = wallet_name_from_descriptor( @@ -455,19 +521,14 @@ async fn tracing_output_contains_no_descriptor_material() -> anyhow::Result<()> &Secp256k1::new(), )?; - async { - let mut store = Store::::new_with_url(None, wallet_name.clone(), true).await?; - let mut wallet = Wallet::create(external_desc, internal_desc) - .network(NETWORK) - .create_wallet_async(&mut store) - .await?; - let _ = wallet.reveal_next_address(External); - wallet.persist_async(&mut store).await?; - Wallet::load().load_wallet_async(&mut store).await?; - anyhow::Ok(()) - } - .with_subscriber(subscriber) - .await?; + let mut store = Store::::new_with_url(None, wallet_name.clone(), NETWORK, true).await?; + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + let _ = wallet.reveal_next_address(External); + wallet.persist_async(&mut store).await?; + Wallet::load().load_wallet_async(&mut store).await?; let logs = String::from_utf8_lossy(&buf.lock().unwrap()).into_owned(); assert!(!logs.is_empty(), "expected tracing output to be captured"); @@ -501,7 +562,8 @@ async fn create_test_stores(wallet_name: String) -> anyhow::Result::new_with_url(None, wallet_name.clone(), true).await?; + let sqlite_store = + Store::::new_with_url(None, wallet_name.clone(), NETWORK, true).await?; stores.push(TestStore::Sqlite(sqlite_store)); Ok(stores) @@ -551,11 +613,14 @@ pub fn insert_fake_tx(wallet: &mut Wallet, spent: Amount, change: Amount, fee: A ..new_tx(1) }; + // Checkpoints must use a distinct hash per height: the store rejects + // changesets that map one hash to several heights (it cannot represent + // them without silently losing checkpoints). insert_checkpoint( wallet, BlockId { height: 42, - hash: BlockHash::all_zeros(), + hash: block_hash(42), }, ); @@ -563,21 +628,21 @@ pub fn insert_fake_tx(wallet: &mut Wallet, spent: Amount, change: Amount, fee: A wallet, BlockId { height: 1_000, - hash: BlockHash::all_zeros(), + hash: block_hash(1), }, ); insert_checkpoint( wallet, BlockId { height: 2_000, - hash: BlockHash::all_zeros(), + hash: block_hash(2), }, ); let anchor = ConfirmationBlockTime { block_id: BlockId { height: 1_000, - hash: BlockHash::all_zeros(), + hash: block_hash(1), }, confirmation_time: 100, }; @@ -587,7 +652,7 @@ pub fn insert_fake_tx(wallet: &mut Wallet, spent: Amount, change: Amount, fee: A let anchor = ConfirmationBlockTime { block_id: BlockId { height: 2_000, - hash: BlockHash::all_zeros(), + hash: block_hash(2), }, confirmation_time: 200, }; @@ -1142,10 +1207,14 @@ async fn reorged_out_anchored_block_can_be_deleted() -> anyhow::Result<()> { .await .expect("persisting a reorg over an anchored block must not fail"); - // The disconnected block and its anchors are gone; the rest survives. + // The disconnected block and its anchor are gone; the rest survives. let changeset = TestStore::initialize(&mut store).await?; assert!(!changeset.local_chain.blocks.contains_key(&2_000)); - assert!(changeset.tx_graph.anchors.is_empty()); + assert_eq!( + changeset.tx_graph.anchors.len(), + 1, + "only the anchor on the disconnected block is dropped" + ); assert_eq!(changeset.tx_graph.txs.len(), 2); // Persistence still works afterwards. @@ -1243,3 +1312,1813 @@ async fn two_wallets_load() -> anyhow::Result<()> { } Ok(()) } + +// --------------------------------------------------------------------------- +// Pure unit tests (no database) +// --------------------------------------------------------------------------- + +#[test] +fn checked_conv_accepts_in_range_values() { + assert_eq!(crate::checked_conv::(42, "t").unwrap(), 42u32); + assert_eq!( + crate::checked_conv::(i32::MAX, "t").unwrap(), + i32::MAX as u32 + ); + assert_eq!(crate::checked_conv::(0, "t").unwrap(), 0i32); + assert_eq!( + crate::checked_conv::(i32::MAX as u32, "t").unwrap(), + i32::MAX + ); + assert_eq!( + crate::checked_conv::(i64::MAX, "t").unwrap(), + i64::MAX as u64 + ); + assert_eq!( + crate::checked_conv::(i64::MAX as u64, "t").unwrap(), + i64::MAX + ); + assert_eq!(crate::checked_conv::(0, "t").unwrap(), 0u32); +} + +#[test] +fn checked_conv_rejects_out_of_range_values() { + // negative into unsigned + assert_matches!( + crate::checked_conv::(-1, "height"), + Err(BdkSqlxError::IntOutOfRange { + context: "height", + value: -1 + }) + ); + assert_matches!( + crate::checked_conv::(-5, "last_seen"), + Err(BdkSqlxError::IntOutOfRange { + context: "last_seen", + value: -5 + }) + ); + assert_matches!( + crate::checked_conv::(i32::MIN, "t"), + Err(BdkSqlxError::IntOutOfRange { .. }) + ); + // too large for destination + assert_matches!( + crate::checked_conv::(u32::MAX, "last_revealed"), + Err(BdkSqlxError::IntOutOfRange { + context: "last_revealed", + value + }) if value == u32::MAX as i128 + ); + assert_matches!( + crate::checked_conv::(u64::MAX, "value"), + Err(BdkSqlxError::IntOutOfRange { context: "value", value }) if value == u64::MAX as i128 + ); + assert_matches!( + crate::checked_conv::(i32::MAX as u32 + 1, "t"), + Err(BdkSqlxError::IntOutOfRange { .. }) + ); +} + +#[test] +fn error_display_messages_are_stable() { + assert_eq!(BdkSqlxError::MissingNetwork.to_string(), "Network Missing"); + assert_eq!( + BdkSqlxError::MissingPool.to_string(), + "No database connection pool provided to the builder" + ); + assert_eq!( + BdkSqlxError::IntOutOfRange { + context: "txout.value", + value: -1 + } + .to_string(), + "integer value out of range for txout.value: -1" + ); + assert_eq!( + BdkSqlxError::InvalidNetwork { + expected: "regtest".into(), + got: "bitcoin".into() + } + .to_string(), + "Invalid Network expected regtest, got bitcoin" + ); + assert!(BdkSqlxError::GetNetworkFailure + .to_string() + .contains("not set")); +} + +#[tokio::test] +async fn pg_builder_requires_network_and_pool() { + initialize(); + + // neither network nor pool set: network is validated first + assert_matches!( + PgStoreBuilder::new("w".into()).build().await, + Err(BdkSqlxError::MissingNetwork) + ); + + // network set but no pool. This must fail with MissingPool and must NOT + // touch the process-global network (build only initializes the global + // network once a pool exists). + assert_matches!( + PgStoreBuilder::new("w".into()) + .network(Regtest) + .build() + .await, + Err(BdkSqlxError::MissingPool) + ); +} + +// --------------------------------------------------------------------------- +// Shared helpers for the store-level tests +// --------------------------------------------------------------------------- + +/// A transaction with a single default input and a single output; `lock_time` and +/// `value` make each minted txid distinct. +fn sample_tx(lock_time: u32, value: u64) -> Transaction { + Transaction { + input: vec![TxIn::default()], + output: vec![TxOut { + value: Amount::from_sat(value), + script_pubkey: ScriptBuf::new(), + }], + ..new_tx(lock_time) + } +} + +fn block_hash(byte: u8) -> BlockHash { + BlockHash::from_byte_array([byte; 32]) +} + +fn anchor_at(height: u32, hash: BlockHash, confirmation_time: u64) -> ConfirmationBlockTime { + ConfirmationBlockTime { + block_id: BlockId { height, hash }, + confirmation_time, + } +} + +/// A changeset exercising every table: the network, a three-block chain with +/// distinct hashes, one confirmed tx (anchored to the block at height 10), one +/// unconfirmed tx (last_seen only), and txouts on both transactions. +fn populated_changeset() -> ChangeSet { + let mut cs = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + + cs.local_chain.blocks.insert(5, Some(block_hash(1))); + cs.local_chain.blocks.insert(10, Some(block_hash(2))); + cs.local_chain.blocks.insert(15, Some(block_hash(3))); + + let tx_a = sample_tx(0, 50_000); + let tx_b = sample_tx(1, 30_000); + let txid_a = tx_a.compute_txid(); + let txid_b = tx_b.compute_txid(); + cs.tx_graph.txs.insert(Arc::new(tx_a)); + cs.tx_graph.txs.insert(Arc::new(tx_b)); + cs.tx_graph.txouts.insert( + OutPoint { + txid: txid_a, + vout: 0, + }, + TxOut { + value: Amount::from_sat(50_000), + script_pubkey: ScriptBuf::from(vec![0x51]), + }, + ); + cs.tx_graph.txouts.insert( + OutPoint { + txid: txid_b, + vout: 1, + }, + TxOut { + value: Amount::from_sat(30_000), + script_pubkey: ScriptBuf::from(vec![0x52]), + }, + ); + cs.tx_graph.last_seen.insert(txid_b, 1_700_000_123); + cs.tx_graph + .anchors + .insert((anchor_at(10, block_hash(2), 12_345), txid_a)); + + cs +} + +fn assert_populated(loaded: &ChangeSet, expected: &ChangeSet) { + assert_eq!(loaded.network, expected.network); + assert_eq!(loaded.tx_graph, expected.tx_graph); + assert_eq!(loaded.local_chain, expected.local_chain); +} + +/// Row count for one of the wallet tables; table names are internal constants. +async fn table_count(store: &TestStore, table: &str, wallet_name: &str) -> anyhow::Result { + let count = match store { + TestStore::Postgres(store) => { + sqlx::query_scalar(&format!( + r#"SELECT count(*) FROM "bdk_wallet"."{table}" WHERE wallet_name=$1"# + )) + .bind(wallet_name) + .fetch_one(&store.pool) + .await? + } + TestStore::Sqlite(store) => { + sqlx::query_scalar(&format!( + "SELECT count(*) FROM {table} WHERE wallet_name=$1" + )) + .bind(wallet_name) + .fetch_one(&store.pool) + .await? + } + }; + Ok(count) +} + +const ALL_TABLES: [&str; 6] = ["network", "keychain", "block", "tx", "txout", "anchor_tx"]; + +// --------------------------------------------------------------------------- +// Store behaviour: empty stores and empty changesets +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn empty_store_reads_default_changeset() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "empty_store_reads_default_changeset".to_string(); + for mut store in create_test_stores(wallet_name).await? { + let loaded = TestStore::initialize(&mut store).await?; + assert!(loaded.is_empty(), "fresh store must read back empty"); + } + Ok(()) +} + +#[tokio::test] +async fn persist_empty_changeset_writes_nothing() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "persist_empty_changeset_writes_nothing".to_string(); + for mut store in create_test_stores(wallet_name.clone()).await? { + TestStore::persist(&mut store, &ChangeSet::default()).await?; + for table in ALL_TABLES { + assert_eq!(table_count(&store, table, &wallet_name).await?, 0); + } + } + Ok(()) +} + +// --------------------------------------------------------------------------- +// Roundtrips +// --------------------------------------------------------------------------- + +/// Every table must survive a persist/load roundtrip unchanged, and reading +/// twice must be stable. +#[tokio::test] +async fn populated_changeset_roundtrip() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "populated_changeset_roundtrip".to_string(); + for mut store in create_test_stores(wallet_name).await? { + let cs = populated_changeset(); + TestStore::persist(&mut store, &cs).await?; + + let loaded = TestStore::initialize(&mut store).await?; + assert_populated(&loaded, &cs); + + let loaded_again = TestStore::initialize(&mut store).await?; + assert_eq!(loaded_again, loaded, "repeated reads must be identical"); + } + Ok(()) +} + +/// Changesets persisted in sequence must accumulate (merge), not replace. +#[tokio::test] +async fn changesets_merge_across_persists() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "changesets_merge_across_persists".to_string(); + for mut store in create_test_stores(wallet_name).await? { + let tx_a = sample_tx(0, 50_000); + let tx_b = sample_tx(1, 30_000); + let txid_a = tx_a.compute_txid(); + let txid_b = tx_b.compute_txid(); + + let mut delta1 = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + delta1.tx_graph.txs.insert(Arc::new(tx_a)); + TestStore::persist(&mut store, &delta1).await?; + + let mut delta2 = ChangeSet::default(); + delta2.tx_graph.txs.insert(Arc::new(tx_b)); + delta2.tx_graph.last_seen.insert(txid_b, 42); + delta2.local_chain.blocks.insert(5, Some(block_hash(1))); + TestStore::persist(&mut store, &delta2).await?; + + let mut delta3 = ChangeSet::default(); + delta3.tx_graph.txouts.insert( + OutPoint { + txid: txid_a, + vout: 0, + }, + TxOut { + value: Amount::from_sat(50_000), + script_pubkey: ScriptBuf::new(), + }, + ); + TestStore::persist(&mut store, &delta3).await?; + + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!(loaded.network, Some(Regtest)); + assert_eq!(loaded.tx_graph.txs.len(), 2); + assert!(loaded + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_a)); + assert!(loaded + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_b)); + assert_eq!(loaded.tx_graph.last_seen.get(&txid_b), Some(&42)); + assert_eq!( + loaded.local_chain.blocks.get(&5), + Some(&Some(block_hash(1))) + ); + assert_eq!(loaded.tx_graph.txouts.len(), 1); + } + Ok(()) +} + +/// A reorg that evicts an anchored block drops the anchor with it; anchoring the +/// same tx to the replacement block must then roundtrip cleanly. +#[tokio::test] +async fn reanchor_after_reorg_roundtrip() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "reanchor_after_reorg_roundtrip".to_string(); + for mut store in create_test_stores(wallet_name).await? { + TestStore::persist(&mut store, &populated_changeset()).await?; + + // replace the anchored block at height 10 with a different hash + let mut reorg = ChangeSet::default(); + reorg.local_chain.blocks.insert(10, Some(block_hash(9))); + TestStore::persist(&mut store, &reorg).await?; + + let loaded = TestStore::initialize(&mut store).await?; + assert!( + loaded.tx_graph.anchors.is_empty(), + "anchors of the reorged-out block must be gone" + ); + + // re-anchor the same tx to the replacement block + let txid_a = sample_tx(0, 50_000).compute_txid(); + let mut reanchor = ChangeSet::default(); + reanchor + .tx_graph + .anchors + .insert((anchor_at(10, block_hash(9), 77_777), txid_a)); + TestStore::persist(&mut store, &reanchor).await?; + + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!(loaded.tx_graph.anchors.len(), 1); + assert!(loaded + .tx_graph + .anchors + .contains(&(anchor_at(10, block_hash(9), 77_777), txid_a))); + assert_eq!( + loaded.local_chain.blocks.get(&10), + Some(&Some(block_hash(9))) + ); + assert_eq!(loaded.tx_graph.txs.len(), 2, "txs must survive reorgs"); + } + Ok(()) +} + +/// Advancing the derivation index across multiple persist/load cycles. +#[tokio::test] +async fn derivation_index_advances_across_loads() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + for mut store in create_test_stores(wallet_name).await? { + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + let _ = wallet.reveal_addresses_to(External, 3); + assert!(wallet.persist_async(&mut store).await?); + + let mut wallet = Wallet::load() + .load_wallet_async(&mut store) + .await? + .expect("wallet must exist"); + assert_eq!(wallet.derivation_index(External), Some(3)); + let addr = wallet.reveal_addresses_to(External, 7).last().unwrap(); + assert_eq!(addr.index, 7); + assert!(wallet.persist_async(&mut store).await?); + + let wallet = Wallet::load() + .load_wallet_async(&mut store) + .await? + .expect("wallet must exist"); + assert_eq!(wallet.derivation_index(External), Some(7)); + assert_eq!(wallet.peek_address(External, 7).address, addr.address); + } + Ok(()) +} + +/// Persisting a descriptor without any derivation state must load back with +/// `last_revealed` UNSET (NULL), not 0: a reloaded fresh wallet must behave +/// exactly like the never-persisted one (its next address is index 0). +#[tokio::test] +async fn descriptor_persist_leaves_last_revealed_unset() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let ext = parse_descriptor(external_desc); + let int = parse_descriptor(internal_desc); + + let wallet_name = "descriptor_persist_leaves_last_revealed_unset".to_string(); + for mut store in create_test_stores(wallet_name).await? { + let cs = ChangeSet { + network: Some(Regtest), + descriptor: Some(ext.clone()), + change_descriptor: Some(int.clone()), + ..Default::default() + }; + TestStore::persist(&mut store, &cs).await?; + + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!(loaded.descriptor, Some(ext.clone())); + assert_eq!(loaded.change_descriptor, Some(int.clone())); + assert!( + loaded.indexer.last_revealed.is_empty(), + "no address was revealed, so no last_revealed entries must exist" + ); + } + Ok(()) +} + +// --------------------------------------------------------------------------- +// Transactionality +// --------------------------------------------------------------------------- + +/// A changeset that fails partway (here: an anchor referencing a block that was +/// never persisted, which violates the FK) must roll back everything it wrote. +#[tokio::test] +async fn failed_persist_rolls_back_everything() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "failed_persist_rolls_back_everything".to_string(); + for mut store in create_test_stores(wallet_name.clone()).await? { + let tx_a = sample_tx(0, 50_000); + let txid_a = tx_a.compute_txid(); + + let mut cs = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + cs.tx_graph.txs.insert(Arc::new(tx_a)); + // no block row for this anchor -> FK violation on anchor_tx + cs.tx_graph + .anchors + .insert((anchor_at(99, block_hash(9), 1), txid_a)); + + let result = TestStore::persist(&mut store, &cs).await; + match &store { + TestStore::Postgres(_) => { + assert_matches!(result, Err(BdkSqlxError::QueryError { .. })) + } + TestStore::Sqlite(_) => assert_matches!(result, Err(BdkSqlxError::Sqlx(_))), + } + + let loaded = TestStore::initialize(&mut store).await?; + assert!( + loaded.is_empty(), + "a failed persist must not leave partial data behind" + ); + for table in ALL_TABLES { + assert_eq!(table_count(&store, table, &wallet_name).await?, 0); + } + } + Ok(()) +} + +// --------------------------------------------------------------------------- +// Integer boundaries at the database boundary +// --------------------------------------------------------------------------- + +/// Values that do not fit the column types must error loudly on persist instead +/// of wrapping; maximum in-range values must roundtrip exactly. +#[tokio::test] +async fn integer_boundaries_checked_on_persist() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let ext = parse_descriptor(external_desc); + let ext_did = ext.descriptor_id(); + let txid = sample_tx(0, 1_000).compute_txid(); + + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + for mut store in create_test_stores(wallet_name).await? { + // arrange a network row and keychain row + let base = ChangeSet { + network: Some(Regtest), + descriptor: Some(ext.clone()), + ..Default::default() + }; + TestStore::persist(&mut store, &base).await?; + + // last_revealed: u32 that does not fit i32 must error + let mut cs = ChangeSet::default(); + cs.indexer.last_revealed.insert(ext_did, u32::MAX); + assert_matches!( + TestStore::persist(&mut store, &cs).await, + Err(BdkSqlxError::IntOutOfRange { .. }), + "u32::MAX last_revealed must not wrap into i32" + ); + // the maximum representable value roundtrips + cs.indexer.last_revealed.insert(ext_did, i32::MAX as u32); + TestStore::persist(&mut store, &cs).await?; + + // txout value: u64 sats that do not fit BIGINT must error + let mut cs = ChangeSet::default(); + cs.tx_graph.txouts.insert( + OutPoint { txid, vout: 0 }, + TxOut { + value: Amount::from_sat(u64::MAX), + script_pubkey: ScriptBuf::new(), + }, + ); + assert_matches!( + TestStore::persist(&mut store, &cs).await, + Err(BdkSqlxError::IntOutOfRange { .. }), + "u64::MAX sats must not wrap into i64" + ); + let mut cs = ChangeSet::default(); + cs.tx_graph.txouts.insert( + OutPoint { txid, vout: 0 }, + TxOut { + value: Amount::from_sat(i64::MAX as u64), + script_pubkey: ScriptBuf::new(), + }, + ); + TestStore::persist(&mut store, &cs).await?; + + // vout: u32 that does not fit INTEGER must error + let mut cs = ChangeSet::default(); + cs.tx_graph.txouts.insert( + OutPoint { + txid, + vout: u32::MAX, + }, + TxOut { + value: Amount::from_sat(1), + script_pubkey: ScriptBuf::new(), + }, + ); + assert_matches!( + TestStore::persist(&mut store, &cs).await, + Err(BdkSqlxError::IntOutOfRange { .. }), + "u32::MAX vout must not wrap into i32" + ); + let mut cs = ChangeSet::default(); + cs.tx_graph.txouts.insert( + OutPoint { + txid, + vout: i32::MAX as u32, + }, + TxOut { + value: Amount::from_sat(1), + script_pubkey: ScriptBuf::new(), + }, + ); + TestStore::persist(&mut store, &cs).await?; + + // block height: u32 that does not fit INTEGER must error + let mut cs = ChangeSet::default(); + cs.local_chain.blocks.insert(u32::MAX, Some(block_hash(8))); + assert_matches!( + TestStore::persist(&mut store, &cs).await, + Err(BdkSqlxError::IntOutOfRange { .. }), + "u32::MAX height must not wrap into i32" + ); + let mut cs = ChangeSet::default(); + cs.local_chain + .blocks + .insert(i32::MAX as u32, Some(block_hash(8))); + TestStore::persist(&mut store, &cs).await?; + + // last_seen: u64 epoch that does not fit BIGINT must error + let mut cs = ChangeSet::default(); + cs.tx_graph.txs.insert(Arc::new(sample_tx(0, 1_000))); + cs.tx_graph.last_seen.insert(txid, u64::MAX); + assert_matches!( + TestStore::persist(&mut store, &cs).await, + Err(BdkSqlxError::IntOutOfRange { .. }), + "u64::MAX last_seen must not wrap into i64" + ); + let mut cs = ChangeSet::default(); + cs.tx_graph.txs.insert(Arc::new(sample_tx(0, 1_000))); + cs.tx_graph.last_seen.insert(txid, i64::MAX as u64); + TestStore::persist(&mut store, &cs).await?; + + // the failed persists rolled back; only the in-range values survive + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!( + loaded.indexer.last_revealed.get(&ext_did), + Some(&(i32::MAX as u32)) + ); + assert_eq!(loaded.tx_graph.txouts.len(), 2); + assert_eq!( + loaded + .tx_graph + .txouts + .get(&OutPoint { txid, vout: 0 }) + .map(|o| o.value), + Some(Amount::from_sat(i64::MAX as u64)) + ); + assert_eq!( + loaded.local_chain.blocks.get(&(i32::MAX as u32)), + Some(&Some(block_hash(8))) + ); + assert_eq!( + loaded.tx_graph.last_seen.get(&txid), + Some(&(i64::MAX as u64)) + ); + } + Ok(()) +} + +// --------------------------------------------------------------------------- +// Multi-tenancy and hostile input +// --------------------------------------------------------------------------- + +/// Wallet names are user-controlled input; they must be treated as data, never +/// as SQL. Names containing injection payloads and unicode must roundtrip and +/// leave the schema intact. +#[tokio::test] +async fn hostile_wallet_names_are_inert() -> anyhow::Result<()> { + initialize(); + + for wallet_name in [ + "'; DROP TABLE bdk_wallet.network; --".to_string(), + "钱包💰\"'\\;".to_string(), + ] { + for mut store in create_test_stores(wallet_name.clone()).await? { + let cs = populated_changeset(); + TestStore::persist(&mut store, &cs).await?; + let loaded = TestStore::initialize(&mut store).await?; + assert_populated(&loaded, &cs); + assert_eq!( + table_count(&store, "network", &wallet_name).await?, + 1, + "schema and rows must survive a hostile wallet name" + ); + } + } + Ok(()) +} + +/// Two wallets sharing one connection pool must only ever see their own rows. +#[tokio::test] +async fn wallets_sharing_a_pool_are_isolated() -> anyhow::Result<()> { + initialize(); + + let tx_a = sample_tx(0, 50_000); + let tx_b = sample_tx(1, 30_000); + let txid_a = tx_a.compute_txid(); + let txid_b = tx_b.compute_txid(); + + let mut cs_a = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + cs_a.tx_graph.txs.insert(Arc::new(tx_a)); + cs_a.local_chain.blocks.insert(5, Some(block_hash(1))); + + let mut cs_b = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + cs_b.tx_graph.txs.insert(Arc::new(tx_b)); + + // postgres: two builders over one pool + let pool = create_test_pg_pool().await?; + let pg_a = PgStoreBuilder::new("pg_wallet_a".into()) + .network(Regtest) + .migrate(true) + .pool(pool.clone()) + .build() + .await?; + let pg_b = PgStoreBuilder::new("pg_wallet_b".into()) + .network(Regtest) + .migrate(true) + .pool(pool) + .build() + .await?; + pg_a.write(&cs_a).await?; + pg_b.write(&cs_b).await?; + let loaded_a = pg_a.read().await?; + let loaded_b = pg_b.read().await?; + assert!(loaded_a + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_a)); + assert!(!loaded_a + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_b)); + assert!(loaded_b + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_b)); + assert!(!loaded_b + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_a)); + + // sqlite: two stores over one pool + let lite_a = Store::::new_with_url(None, "lite_wallet_a".into(), NETWORK, true).await?; + let lite_b = + Store::::new(lite_a.pool.clone(), "lite_wallet_b".into(), NETWORK, true).await?; + lite_a.write(&cs_a).await?; + lite_b.write(&cs_b).await?; + let loaded_a = lite_a.read().await?; + let loaded_b = lite_b.read().await?; + assert!(loaded_a + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_a)); + assert!(!loaded_a + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_b)); + assert!(loaded_b + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_b)); + assert!(!loaded_b + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_a)); + + Ok(()) +} + +/// Concurrent writers on separate connections of the same pool must both +/// succeed and both land. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn concurrent_persists_both_land() -> anyhow::Result<()> { + initialize(); + + let tx_a = sample_tx(0, 50_000); + let tx_b = sample_tx(1, 30_000); + let txid_a = tx_a.compute_txid(); + let txid_b = tx_b.compute_txid(); + + let mut cs_a = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + cs_a.tx_graph.txs.insert(Arc::new(tx_a)); + + let mut cs_b = ChangeSet::default(); + cs_b.tx_graph.txs.insert(Arc::new(tx_b)); + cs_b.tx_graph.last_seen.insert(txid_b, 123); + + let wallet_name = "concurrent_persists_both_land".to_string(); + for store in create_test_stores(wallet_name).await? { + // Cloning a store shares its connection pool; this also guards the + // manual `Clone` impl on `Store` (the derived impl was unusable because + // it bounded `DB: Clone`, which sqlx's marker types don't satisfy). + let (s1, s2) = match &store { + TestStore::Postgres(store) => ( + TestStore::Postgres(store.clone()), + TestStore::Postgres(store.clone()), + ), + TestStore::Sqlite(store) => ( + TestStore::Sqlite(store.clone()), + TestStore::Sqlite(store.clone()), + ), + }; + + let mut s1 = s1; + let mut s2 = s2; + let cs_a2 = cs_a.clone(); + let cs_b2 = cs_b.clone(); + let h1 = tokio::spawn(async move { TestStore::persist(&mut s1, &cs_a2).await }); + let h2 = tokio::spawn(async move { TestStore::persist(&mut s2, &cs_b2).await }); + h1.await??; + h2.await??; + + let mut store = store; + let loaded = TestStore::initialize(&mut store).await?; + assert!(loaded + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_a)); + assert!(loaded + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid_b)); + assert_eq!(loaded.tx_graph.last_seen.get(&txid_b), Some(&123)); + } + Ok(()) +} + +// --------------------------------------------------------------------------- +// Corrupt stored data must fail the load loudly +// --------------------------------------------------------------------------- + +/// An unparseable descriptor string in the keychain table must error, not be +/// silently skipped. +#[tokio::test] +async fn corrupt_stored_descriptor_errors_on_load() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + for mut store in create_test_stores(wallet_name.clone()).await? { + let cs = ChangeSet { + network: Some(Regtest), + descriptor: Some(parse_descriptor(external_desc)), + ..Default::default() + }; + TestStore::persist(&mut store, &cs).await?; + + match &store { + TestStore::Postgres(store) => { + sqlx::query( + r#"UPDATE "bdk_wallet"."keychain" SET descriptor=$2 WHERE wallet_name=$1"#, + ) + .bind(&wallet_name) + .bind("wpkh(definitely-not-a-descriptor)") + .execute(&store.pool) + .await?; + } + TestStore::Sqlite(store) => { + sqlx::query("UPDATE keychain SET descriptor=$2 WHERE wallet_name=$1") + .bind(&wallet_name) + .bind("wpkh(definitely-not-a-descriptor)") + .execute(&store.pool) + .await?; + } + } + assert_matches!(store.read().await, Err(BdkSqlxError::Miniscript(_))); + } + Ok(()) +} + +/// A txid column that is not a valid txid must error on load. +#[tokio::test] +async fn corrupt_stored_txid_errors_on_load() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "corrupt_stored_txid_errors_on_load".to_string(); + for mut store in create_test_stores(wallet_name.clone()).await? { + let mut cs = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + cs.tx_graph.txs.insert(Arc::new(sample_tx(0, 1_000))); + TestStore::persist(&mut store, &cs).await?; + + match &store { + TestStore::Postgres(store) => { + sqlx::query(r#"UPDATE "bdk_wallet"."tx" SET txid=$2 WHERE wallet_name=$1"#) + .bind(&wallet_name) + .bind("not-a-txid") + .execute(&store.pool) + .await?; + } + TestStore::Sqlite(store) => { + sqlx::query("UPDATE tx SET txid=$2 WHERE wallet_name=$1") + .bind(&wallet_name) + .bind("not-a-txid") + .execute(&store.pool) + .await?; + } + } + assert_matches!(store.read().await, Err(BdkSqlxError::HexToArray(_))); + } + Ok(()) +} + +/// A block hash column that is not a valid hash must error on load. +#[tokio::test] +async fn corrupt_stored_block_hash_errors_on_load() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "corrupt_stored_block_hash_errors_on_load".to_string(); + for mut store in create_test_stores(wallet_name.clone()).await? { + let mut cs = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + cs.local_chain.blocks.insert(5, Some(block_hash(1))); + TestStore::persist(&mut store, &cs).await?; + + match &store { + TestStore::Postgres(store) => { + sqlx::query(r#"UPDATE "bdk_wallet"."block" SET hash=$2 WHERE wallet_name=$1"#) + .bind(&wallet_name) + .bind("not-a-block-hash") + .execute(&store.pool) + .await?; + } + TestStore::Sqlite(store) => { + sqlx::query("UPDATE block SET hash=$2 WHERE wallet_name=$1") + .bind(&wallet_name) + .bind("not-a-block-hash") + .execute(&store.pool) + .await?; + } + } + assert_matches!(store.read().await, Err(BdkSqlxError::HexToArray(_))); + } + Ok(()) +} + +/// Negative values in columns whose domain is unsigned must error on load: +/// last_revealed, last_seen, and vout. +#[tokio::test] +async fn negative_stored_values_error_on_load() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + let txid = sample_tx(0, 1_000).compute_txid(); + + for mut store in create_test_stores(wallet_name.clone()).await? { + let mut cs = ChangeSet { + network: Some(Regtest), + descriptor: Some(parse_descriptor(external_desc)), + ..Default::default() + }; + cs.tx_graph.txs.insert(Arc::new(sample_tx(0, 1_000))); + cs.tx_graph.last_seen.insert(txid, 100); + cs.tx_graph.txouts.insert( + OutPoint { txid, vout: 0 }, + TxOut { + value: Amount::from_sat(1_000), + script_pubkey: ScriptBuf::new(), + }, + ); + TestStore::persist(&mut store, &cs).await?; + + for (pg_sql, lite_sql) in [ + ( + r#"UPDATE "bdk_wallet"."keychain" SET last_revealed=-1 WHERE wallet_name=$1"#, + "UPDATE keychain SET last_revealed=-1 WHERE wallet_name=$1", + ), + ( + r#"UPDATE "bdk_wallet"."tx" SET last_seen=-1 WHERE wallet_name=$1"#, + "UPDATE tx SET last_seen=-1 WHERE wallet_name=$1", + ), + ( + r#"UPDATE "bdk_wallet"."txout" SET vout=-1 WHERE wallet_name=$1"#, + "UPDATE txout SET vout=-1 WHERE wallet_name=$1", + ), + ] { + match &store { + TestStore::Postgres(store) => { + sqlx::query(pg_sql) + .bind(&wallet_name) + .execute(&store.pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::IntOutOfRange { .. })); + sqlx::query(&pg_sql.replace("=-1", "=0")) + .bind(&wallet_name) + .execute(&store.pool) + .await?; + } + TestStore::Sqlite(store) => { + sqlx::query(lite_sql) + .bind(&wallet_name) + .execute(&store.pool) + .await?; + assert_matches!(store.read().await, Err(BdkSqlxError::IntOutOfRange { .. })); + sqlx::query(&lite_sql.replace("=-1", "=0")) + .bind(&wallet_name) + .execute(&store.pool) + .await?; + } + } + } + + store.read().await?; + } + Ok(()) +} + +// --------------------------------------------------------------------------- +// Backend construction +// --------------------------------------------------------------------------- + +/// Reading/writing a store whose migrations never ran must produce an error, +/// not a panic or silent success. +#[tokio::test] +async fn unmigrated_store_errors() -> anyhow::Result<()> { + initialize(); + + // postgres wraps statement failures in QueryError + let pool = create_test_pg_pool().await?; + let store = PgStoreBuilder::new("unmigrated".into()) + .network(Regtest) + .migrate(false) + .pool(pool) + .build() + .await?; + assert_matches!( + store.read().await, + Err(BdkSqlxError::QueryError { .. }), + "postgres read on unmigrated schema must error" + ); + let cs = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + assert_matches!( + store.write(&cs).await, + Err(BdkSqlxError::QueryError { .. }), + "postgres write on unmigrated schema must error" + ); + + // sqlite propagates the raw sqlx error + let store = Store::::new_with_url(None, "unmigrated".into(), NETWORK, false).await?; + assert_matches!( + store.read().await, + Err(BdkSqlxError::Sqlx(_)), + "sqlite read on unmigrated schema must error" + ); + assert_matches!( + store.write(&cs).await, + Err(BdkSqlxError::Sqlx(_)), + "sqlite write on unmigrated schema must error" + ); + Ok(()) +} + +/// A file-backed sqlite store must keep data across connections. +#[tokio::test] +async fn sqlite_file_backed_store_persists_across_connections() -> anyhow::Result<()> { + initialize(); + + let path = std::env::temp_dir().join(format!( + "bdk_sqlx_test_{}_{}.sqlite3", + std::process::id(), + TEST_DB_COUNTER.fetch_add(1, Ordering::Relaxed) + )); + let url = format!("sqlite://{}?mode=rwc", path.display()); + let wallet_name = "sqlite_file_backed".to_string(); + + { + let store = + Store::::new_with_url(Some(url.clone()), wallet_name.clone(), NETWORK, true) + .await?; + store.write(&populated_changeset()).await?; + } + + { + // a fresh pool against the same file; migrate=false proves the schema + // really lives in the file + let store = + Store::::new_with_url(Some(url.clone()), wallet_name.clone(), NETWORK, false) + .await?; + let loaded = store.read().await?; + assert_populated(&loaded, &populated_changeset()); + } + + std::fs::remove_file(&path)?; + Ok(()) +} + +/// Running migrations twice must be a no-op the second time. +#[tokio::test] +async fn postgres_migrate_is_idempotent() -> anyhow::Result<()> { + initialize(); + + let pool = create_test_pg_pool().await?; + let store = PgStoreBuilder::new("migrate_twice".into()) + .network(Regtest) + .migrate(true) + .pool(pool) + .build() + .await?; + store.migrate().await?; + store.migrate().await?; + Ok(()) +} + +/// The builder's URL path must produce a working store end to end. +#[tokio::test] +async fn pg_build_with_url_creates_working_store() -> anyhow::Result<()> { + initialize(); + + let admin_url = env::var("DATABASE_TEST_URL").expect("DATABASE_TEST_URL must be set for tests"); + let admin_pool = Pool::::connect(&admin_url).await?; + let db_name = format!( + "bdk_sqlx_test_{}_{}", + std::process::id(), + TEST_DB_COUNTER.fetch_add(1, Ordering::Relaxed) + ); + + let base_url = admin_url + .rsplit_once('/') + .map(|(base, _)| base) + .expect("DATABASE_TEST_URL has a database path"); + let store_url = format!("{base_url}/{db_name}"); + + // Hold the database-management locks for the whole test: the scratch + // database's pool has no minimum connections, so without the advisory + // lock a concurrent cleanup could drop it between operations. + let _guard = TEST_DB_LOCK.lock().await; + let mut mgmt = pg_mgmt_lock(&admin_pool).await?; + + let result = async { + sqlx::query(&format!(r#"CREATE DATABASE "{db_name}""#)) + .execute(&mut *mgmt) + .await?; + + let result = async { + let store = PgStoreBuilder::new("with_url".into()) + .network(Regtest) + .migrate(true) + .build_with_url(&store_url) + .await?; + assert!(store.read().await?.is_empty()); + store.write(&populated_changeset()).await?; + assert_populated(&store.read().await?, &populated_changeset()); + anyhow::Ok(()) + } + .await; + + // best-effort cleanup of the scratch database + let _ = sqlx::query(&format!(r#"DROP DATABASE IF EXISTS "{db_name}""#)) + .execute(&mut *mgmt) + .await; + + result + } + .await; + + pg_mgmt_unlock(mgmt).await; + result +} + +// --------------------------------------------------------------------------- +// Regression tests for previously confirmed defects +// +// Each test below pinned down a bug that has since been fixed. They are kept +// as always-on regression tests guarding the fixed behaviour. +// --------------------------------------------------------------------------- + +/// Regression test: `tx.last_seen` was persisted with a bare `UPDATE tx ...` +/// that affected 0 rows when the tx row did not exist, silently dropping the +/// timestamp. The write now upserts a stub row (the schema's nullable +/// `whole_tx` column exists precisely for metadata-only rows). +#[tokio::test] +async fn bug_last_seen_without_tx_row_is_dropped() -> anyhow::Result<()> { + initialize(); + + let txid = sample_tx(0, 1_000).compute_txid(); + let wallet_name = "bug_last_seen_without_tx_row_is_dropped".to_string(); + for mut store in create_test_stores(wallet_name).await? { + let mut cs = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + // note: only last_seen, the full tx is not part of this changeset + cs.tx_graph.last_seen.insert(txid, 1_700_000_000); + TestStore::persist(&mut store, &cs).await?; + + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!( + loaded.tx_graph.last_seen.get(&txid), + Some(&1_700_000_000), + "last_seen must survive even when the full tx was never persisted" + ); + } + Ok(()) +} + +/// Regression test: `read()` anchored its entire load on the `network` row, +/// so rows persisted by a changeset that carried no network landed in the +/// database but were invisible to every subsequent read. Reads now fetch the +/// tx/block tables unconditionally. +#[tokio::test] +async fn bug_rows_persisted_before_network_are_invisible() -> anyhow::Result<()> { + initialize(); + + let txid = sample_tx(0, 1_000).compute_txid(); + let wallet_name = "bug_rows_persisted_before_network_are_invisible".to_string(); + for mut store in create_test_stores(wallet_name).await? { + let mut cs = ChangeSet::default(); + cs.tx_graph.txs.insert(Arc::new(sample_tx(0, 1_000))); + cs.local_chain.blocks.insert(5, Some(block_hash(1))); + // deliberately no network in this changeset + TestStore::persist(&mut store, &cs).await?; + + let loaded = TestStore::initialize(&mut store).await?; + assert!( + loaded + .tx_graph + .txs + .iter() + .any(|tx| tx.compute_txid() == txid), + "a persisted tx must be visible on load" + ); + assert_eq!( + loaded.local_chain.blocks.get(&5), + Some(&Some(block_hash(1))), + "a persisted block must be visible on load" + ); + } + Ok(()) +} + +/// Regression test: the block table keys rows by `(wallet_name, hash)` and +/// upserts on the hash, so a changeset mapping the same hash to two heights +/// silently moved the row and lost the other checkpoints. Such changesets are +/// now rejected with `DuplicateBlockHash` instead of being lossily persisted. +#[tokio::test] +async fn bug_same_hash_at_multiple_heights_collapses() -> anyhow::Result<()> { + initialize(); + + let hash = block_hash(7); + let wallet_name = "bug_same_hash_at_multiple_heights_collapses".to_string(); + for mut store in create_test_stores(wallet_name.clone()).await? { + let mut cs = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + cs.local_chain.blocks.insert(1, Some(hash)); + cs.local_chain.blocks.insert(2, Some(hash)); + cs.local_chain.blocks.insert(3, Some(hash)); + assert_matches!( + TestStore::persist(&mut store, &cs).await, + Err(BdkSqlxError::DuplicateBlockHash { + hash: h, + first_height: 1, + second_height: 2, + }) if h == hash, + "a non-injective block changeset must be rejected" + ); + assert_eq!( + table_count(&store, "block", &wallet_name).await?, + 0, + "the rejected changeset must not leave partial rows behind" + ); + + // the same heights with distinct hashes roundtrip fine + let mut cs = ChangeSet { + network: Some(Regtest), + ..Default::default() + }; + cs.local_chain.blocks.insert(1, Some(block_hash(1))); + cs.local_chain.blocks.insert(2, Some(block_hash(2))); + cs.local_chain.blocks.insert(3, Some(block_hash(3))); + TestStore::persist(&mut store, &cs).await?; + + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!(loaded.local_chain.blocks.len(), 3); + assert_eq!(table_count(&store, "block", &wallet_name).await?, 3); + } + Ok(()) +} + +/// Regression test: the postgres `write()` path never validated +/// `changeset.network` against the configured network, letting a foreign +/// network overwrite the network row and wedge all subsequent reads with +/// `InvalidNetwork`. The write is now rejected up front. +#[tokio::test] +async fn bug_postgres_write_accepts_foreign_network() -> anyhow::Result<()> { + initialize(); + + let pool = create_test_pg_pool().await?; + let store = PgStoreBuilder::new("bug_foreign_network".into()) + .network(Regtest) + .migrate(true) + .pool(pool) + .build() + .await?; + + let cs = ChangeSet { + network: Some(Network::Bitcoin), + ..Default::default() + }; + + // a store configured for regtest must refuse to persist a bitcoin row + assert_matches!( + store.write(&cs).await, + Err(_), + "write() must reject a changeset for a different network" + ); + // and the store must still be readable afterwards + assert!(store.read().await?.is_empty()); + Ok(()) +} + +/// Regression test: both schemas declared `keychain.last_revealed INTEGER +/// DEFAULT 0`, so a wallet persisted before ever revealing an address reloaded +/// with `last_revealed = Some(0)` instead of `None` and skipped index 0 +/// forever. New rows now store NULL explicitly (and migration 04 drops the +/// default), matching upstream bdk's semantics. +/// +/// Note: no data migration rewrites existing rows -- a stored `0` is ambiguous +/// ("revealed index 0" vs "never revealed") and guessing could cause address +/// reuse. Only new rows are protected. +#[tokio::test] +async fn bug_unrevealed_wallet_reloads_skipping_index_zero() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + for mut store in create_test_stores(wallet_name).await? { + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + // never reveal anything; the creation changeset is already persisted + assert_eq!(wallet.derivation_index(External), None); + assert_eq!(wallet.reveal_next_address(External).index, 0); + + let mut loaded = Wallet::load() + .load_wallet_async(&mut store) + .await? + .expect("wallet must exist"); + assert_eq!( + loaded.derivation_index(External), + None, + "a never-revealed wallet must reload with no derivation index" + ); + assert_eq!( + loaded.reveal_next_address(External).index, + 0, + "a reloaded wallet must not skip address index 0" + ); + } + Ok(()) +} + +/// Regression test: `update_last_revealed` was a plain `UPDATE`, so a stale or +/// replayed changeset moved `last_revealed` BACKWARDS and the next load +/// silently re-revealed already handed-out addresses. The update now never +/// decreases the stored value. +#[tokio::test] +async fn bug_last_revealed_regresses_causing_address_reuse() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let wallet_name = wallet_name_from_descriptor( + external_desc, + Some(internal_desc), + NETWORK, + &Secp256k1::new(), + )?; + + for mut store in create_test_stores(wallet_name).await? { + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(NETWORK) + .create_wallet_async(&mut store) + .await?; + let _ = wallet.reveal_addresses_to(External, 5); + assert!(wallet.persist_async(&mut store).await?); + + // a stale changeset (replayed backup, older app instance) regresses + // the derivation state to 2 + let mut stale = ChangeSet::default(); + stale + .indexer + .last_revealed + .insert(parse_descriptor(external_desc).descriptor_id(), 2); + TestStore::persist(&mut store, &stale).await?; + + let loaded = Wallet::load() + .load_wallet_async(&mut store) + .await? + .expect("wallet must exist"); + assert_eq!( + loaded.derivation_index(External), + Some(5), + "last_revealed must never move backwards" + ); + } + Ok(()) +} + +/// Regression test: `Store::::read` claimed "a consistent snapshot" +/// from running inside one transaction, but postgres ran it at the default +/// READ COMMITTED +/// isolation, which takes a NEW snapshot for every statement, so a writer +/// committing between the keychain SELECT and the tx/block SELECTs produced a +/// mixed-generation changeset. `Store::read` now opens its transaction with +/// REPEATABLE READ; this test pins the mechanism deterministically. +#[tokio::test] +async fn bug_postgres_read_tx_is_not_snapshot_consistent() -> anyhow::Result<()> { + initialize(); + + let pool = create_test_pg_pool().await?; + let _store = PgStoreBuilder::new("isolation_demo".into()) + .network(Regtest) + .migrate(true) + .pool(pool.clone()) + .build() + .await?; + + // one REPEATABLE READ transaction, two identical statements, a concurrent + // commit in between -- the second statement must see the same snapshot + let mut read_tx = pool.begin().await?; + sqlx::query("SET TRANSACTION ISOLATION LEVEL REPEATABLE READ") + .execute(&mut *read_tx) + .await?; + let before: i64 = sqlx::query_scalar(r#"SELECT count(*) FROM "bdk_wallet"."block""#) + .fetch_one(&mut *read_tx) + .await?; + + sqlx::query( + r#"INSERT INTO "bdk_wallet"."block" (wallet_name, hash, height) VALUES ('isolation_demo', $1, 1)"#, + ) + .bind(block_hash(9).to_string()) + .execute(&pool) + .await?; + + let after: i64 = sqlx::query_scalar(r#"SELECT count(*) FROM "bdk_wallet"."block""#) + .fetch_one(&mut *read_tx) + .await?; + read_tx.rollback().await?; + + assert_eq!( + before, after, + "statements inside one read transaction must observe a single snapshot" + ); + Ok(()) +} +/// Migration 04 upgrade path: a database created with the old 01-03 schema +/// (with `last_revealed INTEGER DEFAULT 0`) must keep its data verbatim when 04 +/// is applied -- the ambiguous stored 0s are NOT rewritten -- and afterwards +/// new rows must default to NULL instead of 0. +#[tokio::test] +async fn migration_04_preserves_data_and_drops_default() -> anyhow::Result<()> { + initialize(); + + // sqlite: 01-03 by hand, old-schema rows, then 04 + let path = std::env::temp_dir().join(format!( + "bdk_sqlx_mig04_{}_{}.sqlite3", + std::process::id(), + TEST_DB_COUNTER.fetch_add(1, Ordering::Relaxed) + )); + let url = format!("sqlite://{}?mode=rwc", path.display()); + let pool = sqlx::SqlitePool::connect(&url).await?; + for file in [ + "01_bdk_wallet.sql", + "02_anchor_tx_on_delete_cascade.sql", + "03_block_unique_height.sql", + ] { + let sql = std::fs::read_to_string(format!("migrations/sqlite/{file}"))?; + sqlx::raw_sql(&sql).execute(&pool).await?; + } + // an old-schema row relying on DEFAULT 0 (never revealed) and one with a + // real revealed index + sqlx::query("INSERT INTO keychain (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ('w','External','d1',x'01')").execute(&pool).await?; + sqlx::query("INSERT INTO keychain (wallet_name, keychainkind, descriptor, descriptor_id, last_revealed) VALUES ('w','Internal','d2',x'02',9)").execute(&pool).await?; + let default_row: Option = + sqlx::query_scalar("SELECT last_revealed FROM keychain WHERE keychainkind='External'") + .fetch_one(&pool) + .await?; + assert_eq!(default_row, Some(0), "old schema must have DEFAULT 0"); + + let sql04 = + std::fs::read_to_string("migrations/sqlite/04_keychain_last_revealed_drop_default.sql")?; + sqlx::raw_sql(&sql04).execute(&pool).await?; + + let ext: Option = + sqlx::query_scalar("SELECT last_revealed FROM keychain WHERE keychainkind='External'") + .fetch_one(&pool) + .await?; + let int: Option = + sqlx::query_scalar("SELECT last_revealed FROM keychain WHERE keychainkind='Internal'") + .fetch_one(&pool) + .await?; + assert_eq!(ext, Some(0), "existing 0 must NOT be rewritten (ambiguous)"); + assert_eq!(int, Some(9), "revealed index must survive the rebuild"); + let rows: i64 = sqlx::query_scalar("SELECT count(*) FROM keychain") + .fetch_one(&pool) + .await?; + assert_eq!(rows, 2); + + sqlx::query("INSERT INTO keychain (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ('w2','External','d3',x'03')").execute(&pool).await?; + let new_row: Option = + sqlx::query_scalar("SELECT last_revealed FROM keychain WHERE wallet_name='w2'") + .fetch_one(&pool) + .await?; + assert_eq!(new_row, None, "new rows must default to NULL after 04"); + drop(pool); + std::fs::remove_file(&path)?; + + // postgres: same upgrade path + let pool = create_test_pg_pool().await?; + for file in [ + "01_bdk_wallet.sql", + "02_anchor_tx_on_delete_cascade.sql", + "03_block_unique_height.sql", + ] { + let sql = std::fs::read_to_string(format!("migrations/postgres/{file}"))?; + sqlx::raw_sql(&sql).execute(&pool).await?; + } + sqlx::query(r#"INSERT INTO "bdk_wallet"."keychain" (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ('w','External','d1','\x01'::bytea)"#).execute(&pool).await?; + sqlx::query(r#"INSERT INTO "bdk_wallet"."keychain" (wallet_name, keychainkind, descriptor, descriptor_id, last_revealed) VALUES ('w','Internal','d2','\x02'::bytea,9)"#).execute(&pool).await?; + let default_row: Option = sqlx::query_scalar( + r#"SELECT last_revealed FROM "bdk_wallet"."keychain" WHERE keychainkind='External'"#, + ) + .fetch_one(&pool) + .await?; + assert_eq!(default_row, Some(0), "old schema must have DEFAULT 0"); + + let sql04 = + std::fs::read_to_string("migrations/postgres/04_keychain_last_revealed_drop_default.sql")?; + sqlx::raw_sql(&sql04).execute(&pool).await?; + + let ext: Option = sqlx::query_scalar( + r#"SELECT last_revealed FROM "bdk_wallet"."keychain" WHERE keychainkind='External'"#, + ) + .fetch_one(&pool) + .await?; + let int: Option = sqlx::query_scalar( + r#"SELECT last_revealed FROM "bdk_wallet"."keychain" WHERE keychainkind='Internal'"#, + ) + .fetch_one(&pool) + .await?; + assert_eq!(ext, Some(0), "existing 0 must NOT be rewritten (ambiguous)"); + assert_eq!(int, Some(9), "revealed index must survive"); + + sqlx::query(r#"INSERT INTO "bdk_wallet"."keychain" (wallet_name, keychainkind, descriptor, descriptor_id) VALUES ('w2','External','d3','\x03'::bytea)"#).execute(&pool).await?; + let new_row: Option = sqlx::query_scalar( + r#"SELECT last_revealed FROM "bdk_wallet"."keychain" WHERE wallet_name='w2'"#, + ) + .fetch_one(&pool) + .await?; + assert_eq!(new_row, None, "new rows must default to NULL after 04"); + Ok(()) +} + +/// Regression test: reads anchored keychain rows on the `network` row, so a +/// changeset carrying descriptors (and derivation state) but no network wrote +/// rows that every subsequent read silently skipped -- the same defect class +/// as the tx/block invisibility bug, one table over. Keychain rows are now +/// read unconditionally, like the tx/block tables. +#[tokio::test] +async fn keychain_persisted_without_network_roundtrips() -> anyhow::Result<()> { + initialize(); + + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + let ext = parse_descriptor(external_desc); + let int = parse_descriptor(internal_desc); + let ext_did = ext.descriptor_id(); + + let wallet_name = "keychain_persisted_without_network_roundtrips".to_string(); + for mut store in create_test_stores(wallet_name).await? { + let mut cs = ChangeSet { + descriptor: Some(ext.clone()), + change_descriptor: Some(int.clone()), + ..Default::default() + }; + cs.indexer.last_revealed.insert(ext_did, 3); + // deliberately no network in this changeset + TestStore::persist(&mut store, &cs).await?; + + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!(loaded.network, None); + assert_eq!( + loaded.descriptor, + Some(ext.clone()), + "a descriptor persisted without a network must be visible on load" + ); + assert_eq!(loaded.change_descriptor, Some(int.clone())); + assert_eq!(loaded.indexer.last_revealed.get(&ext_did), Some(&3)); + } + Ok(()) +} + +/// Regression test: the sqlite backend had no network validation at all -- it +/// accepted and loaded data for any network, while postgres rejects foreign +/// networks on write and validates the stored network on read. The sqlite +/// store now takes the process-global network at construction and applies the +/// same guards. +#[tokio::test] +async fn sqlite_store_enforces_configured_network() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "sqlite_store_enforces_configured_network".to_string(); + let store = Store::::new_with_url(None, wallet_name.clone(), NETWORK, true).await?; + + // a write carrying a foreign network is rejected, and the store survives + let cs = ChangeSet { + network: Some(Network::Bitcoin), + ..Default::default() + }; + assert_matches!( + store.write(&cs).await, + Err(BdkSqlxError::InvalidNetwork { .. }), + "sqlite write must reject a changeset for a different network" + ); + assert!(store.read().await?.is_empty()); + + // data for the configured network roundtrips + store + .write(&ChangeSet { + network: Some(NETWORK), + ..Default::default() + }) + .await?; + assert_eq!(store.read().await?.network, Some(NETWORK)); + + // a foreign network written behind the store's back fails the load + sqlx::query("UPDATE network SET name=$2 WHERE wallet_name=$1") + .bind(&wallet_name) + .bind("bitcoin") + .execute(&store.pool) + .await?; + assert_matches!( + store.read().await, + Err(BdkSqlxError::InvalidNetwork { .. }), + "sqlite read must reject a stored foreign network" + ); + Ok(()) +} + +/// Regression test: `insert_descriptor`'s conflict update kept the stored +/// `last_revealed` unconditionally, so replacing a descriptor under the same +/// (wallet_name, keychainkind) made the NEW descriptor inherit the old +/// derivation index -- the wallet would silently skip those addresses on +/// load. The keep is now conditional on the descriptor being unchanged. +#[tokio::test] +async fn descriptor_rotation_resets_last_revealed() -> anyhow::Result<()> { + initialize(); + + let (external_desc, _) = get_test_tr_single_sig_xprv_and_change_desc(); + let other_desc = get_test_wpkh(); + let ext = parse_descriptor(external_desc); + let other = parse_descriptor(other_desc); + let ext_did = ext.descriptor_id(); + + let wallet_name = "descriptor_rotation_resets_last_revealed".to_string(); + for mut store in create_test_stores(wallet_name).await? { + let mut cs = ChangeSet { + network: Some(Regtest), + descriptor: Some(ext.clone()), + ..Default::default() + }; + cs.indexer.last_revealed.insert(ext_did, 5); + TestStore::persist(&mut store, &cs).await?; + + // re-persisting the SAME descriptor keeps the derivation state + TestStore::persist(&mut store, &cs).await?; + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!(loaded.indexer.last_revealed.get(&ext_did), Some(&5)); + + // replacing the descriptor resets the derivation state + let rotated = ChangeSet { + descriptor: Some(other.clone()), + ..Default::default() + }; + TestStore::persist(&mut store, &rotated).await?; + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!(loaded.descriptor, Some(other.clone())); + assert!( + loaded.indexer.last_revealed.is_empty(), + "a replaced descriptor must not inherit the old derivation index" + ); + } + Ok(()) +} + +/// A keychainkind value the store never writes is corrupt data and must fail +/// the load loudly rather than silently drop the keychain. +#[tokio::test] +async fn corrupt_keychainkind_errors_on_load() -> anyhow::Result<()> { + initialize(); + + let (external_desc, _) = get_test_tr_single_sig_xprv_and_change_desc(); + let ext = parse_descriptor(external_desc); + let wallet_name = "corrupt_keychainkind_errors_on_load".to_string(); + + for mut store in create_test_stores(wallet_name.clone()).await? { + let cs = ChangeSet { + network: Some(Regtest), + descriptor: Some(ext.clone()), + ..Default::default() + }; + TestStore::persist(&mut store, &cs).await?; + + match &store { + TestStore::Postgres(store) => { + sqlx::query( + r#"UPDATE "bdk_wallet"."keychain" SET keychainkind=$2 WHERE wallet_name=$1"#, + ) + .bind(&wallet_name) + .bind("Bogus") + .execute(&store.pool) + .await?; + } + TestStore::Sqlite(store) => { + sqlx::query("UPDATE keychain SET keychainkind=$2 WHERE wallet_name=$1") + .bind(&wallet_name) + .bind("Bogus") + .execute(&store.pool) + .await?; + } + } + assert_matches!( + store.read().await, + Err(BdkSqlxError::InvalidKeychainKind { .. }) + ); + } + Ok(()) +} + +/// Migration 05 drops the dead `version` table and the redundant +/// `idx_block_height` index; the store must keep working afterwards. +#[tokio::test] +async fn migration_05_drops_dead_schema() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "migration_05_drops_dead_schema".to_string(); + for mut store in create_test_stores(wallet_name.clone()).await? { + match &store { + TestStore::Postgres(store) => { + let version_exists: bool = sqlx::query_scalar( + "SELECT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_schema='bdk_wallet' AND table_name='version')", + ) + .fetch_one(&store.pool) + .await?; + assert!(!version_exists, "version table must be dropped"); + let idx_exists: bool = sqlx::query_scalar( + "SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE schemaname='bdk_wallet' AND indexname='idx_block_height')", + ) + .fetch_one(&store.pool) + .await?; + assert!(!idx_exists, "idx_block_height must be dropped"); + } + TestStore::Sqlite(store) => { + let objects: Vec = sqlx::query_scalar( + "SELECT name FROM sqlite_master WHERE name IN ('version','idx_block_height')", + ) + .fetch_all(&store.pool) + .await?; + assert!( + objects.is_empty(), + "dead schema objects must be dropped: {objects:?}" + ); + } + } + + // the store still roundtrips every table + let cs = populated_changeset(); + TestStore::persist(&mut store, &cs).await?; + assert_populated(&TestStore::initialize(&mut store).await?, &cs); + } + Ok(()) +} diff --git a/tests/builder_network.rs b/tests/builder_network.rs new file mode 100644 index 0000000..a5b56e6 --- /dev/null +++ b/tests/builder_network.rs @@ -0,0 +1,81 @@ +//! Tests for the process-global network configuration of `PgStoreBuilder`. +//! +//! The configured network is held in a process-wide `OnceLock`, so these tests +//! live in their own integration-test binary: in-process unit tests share the +//! global with every other test and could not exercise it deterministically. +//! +//! The pools are lazy, so no database server is needed: `build()` with +//! `migrate(false)` never touches the pool. + +use bdk_sqlx::sqlx::postgres::PgPoolOptions; +use bdk_sqlx::sqlx::PgPool; +use bdk_sqlx::{BdkSqlxError, PgStoreBuilder}; +use bdk_wallet::bitcoin::Network; + +fn lazy_pool() -> PgPool { + PgPoolOptions::new() + .connect_lazy("postgres://127.0.0.1:1/bdk_sqlx_offline") + .expect("lazy pool creation does not connect") +} + +/// All scenarios in one test so ordering inside this process is deterministic: +/// the first build fixes the global network for the rest of the process. +#[tokio::test] +async fn network_config_is_process_global() { + // first build initializes the global network + PgStoreBuilder::new("wallet_a".into()) + .network(Network::Regtest) + .pool(lazy_pool()) + .build() + .await + .expect("first build must succeed"); + + // re-initializing with the same network is tolerated + PgStoreBuilder::new("wallet_b".into()) + .network(Network::Regtest) + .pool(lazy_pool()) + .build() + .await + .expect("re-init with the same network must succeed"); + + // a different network in the same process is rejected + let result = PgStoreBuilder::new("wallet_c".into()) + .network(Network::Bitcoin) + .pool(lazy_pool()) + .build() + .await; + assert!( + matches!( + result, + Err(BdkSqlxError::DuplicateInitNetwork { + current: Network::Regtest, + network: Network::Bitcoin, + }) + ), + "expected DuplicateInitNetwork, got {result:?}" + ); +} + +/// Regression test: `initialize_network` used to read the `OnceLock` and then +/// set it, so two threads racing the first initialization with the SAME +/// network could produce a spurious `SetNetworkFailure` for the loser. +/// Same-network initialization must be idempotent under concurrency. +#[tokio::test(flavor = "multi_thread", worker_threads = 8)] +async fn concurrent_same_network_init_never_fails() { + let mut handles = Vec::new(); + for i in 0..16 { + handles.push(tokio::spawn(async move { + PgStoreBuilder::new(format!("race_{i}")) + .network(Network::Regtest) + .pool(lazy_pool()) + .build() + .await + })); + } + for handle in handles { + handle + .await + .expect("task panicked") + .expect("same-network initialization must never fail"); + } +} From ad02915a97d81de385265177cf1fdb981f7fe436 Mon Sep 17 00:00:00 2001 From: russeree Date: Wed, 5 Aug 2026 14:55:40 -0700 Subject: [PATCH 18/18] fix: serialize block writes per wallet, monotonic last_seen, backend parity Resolves the remaining findings of a full review of the crate. Each fix is pinned by an always-on regression test; verified with fmt, clippy -Dwarnings, and the full suite (58 tests) against postgres and sqlite. - fix(postgres): two writers persisting different block hashes for the same previously unoccupied height raced the block table's two unique indexes. The loser's DELETE could not see the winner's uncommitted row, and its INSERT then violated idx_block_wallet_height -- an index the upsert's (wallet_name, hash) conflict target does not cover -- aborting the loser's whole changeset with a raw 23505. Reproduced before the fix (duplicate key value violates unique constraint "idx_block_wallet_height"). Block writes are now serialized per wallet with a transaction-scoped advisory lock (pg_advisory_xact_lock(hashtext(wallet))): the loser waits for the winner to commit, then sees and replaces its row, last-writer-wins, exactly as if the writes had been issued sequentially. The lock is released by commit/rollback and keyed per wallet, so different wallets never block each other. sqlite needs no equivalent: its single-writer lock already serializes the same interleaving. Regression test concurrent_block_writes_at_same_height_both_land races two writers over ten heights on both backends and was verified to fail without the lock. - fix(both): the tx.last_seen upsert overwrote unconditionally, so a stale or replayed changeset moved the timestamp backwards, contradicting bdk_chain's own Merge (last_seen only ever increases). The conflict update now keeps the maximum on both backends, matching the monotonic guarantee update_last_revealed already enforces for derivation state. Regression test last_seen_never_regresses covers both backends. - fix(sqlite): statement failures propagated raw BdkSqlxError::Sqlx while postgres wrapped them in BdkSqlxError::QueryError with table context, so callers could not match one error kind for 'the write failed at the database'. sqlite now wraps with the same table labels as postgres; the two tests that encoded the asymmetry (unmigrated_store_errors, failed_persist_rolls_back_everything) now assert QueryError on both backends. - feat(sqlite): SqliteStoreBuilder mirroring PgStoreBuilder (new(wallet_name).network(..).migrate(..).pool(..).build() / build_with_url(..); build_with_url(None) builds the single-connection in-memory store), closing the constructor/builder API asymmetry between backends. Store::::new_with_url now delegates to it, so pool construction lives in exactly one place. - feat(sqlite): Store::::migrate(), mirroring Store::::migrate; sqlite_migrate_is_idempotent covers it. - test: bare 'cargo test' without DATABASE_TEST_URL no longer fails 40+ tests with .expect panics; postgres-backend tests skip gracefully with a one-time notice while the sqlite backend still runs. CI sets the variable, so full coverage always runs there. - style: #[tracing::instrument] on persist-path helpers is uniformly skip_all on both backends (one rule: no span records arguments); module-internal free functions demoted from unreachable pub to pub(crate); get_test_minisicript_with_change_desc typo renamed; README's Resolved defects section moved to CHANGELOG.md. Deliberately unchanged: the process-global network (OnceLock) is a documented design decision -- one process, one network, validated on every read and write -- and remains as-is. --- CHANGELOG.md | 102 +++++++++++++++++++ README.md | 59 +---------- src/lib.rs | 8 ++ src/postgres.rs | 47 +++++++-- src/sqlite.rs | 259 ++++++++++++++++++++++++++++++++++++++++++----- src/test.rs | 263 ++++++++++++++++++++++++++++++++++++++++++------ 6 files changed, 617 insertions(+), 121 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..6201729 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,102 @@ +# Changelog + +All notable defects found in review and fixed are listed here. Each fix is +guarded by an always-on regression test in the suite (`src/test.rs` and +`tests/builder_network.rs`). + +## Unreleased + +### Fixed + +- Two writers persisting different block hashes for the same previously + unoccupied height raced the block table's two unique indexes on postgres: + the loser's `DELETE` could not see the winner's uncommitted row, and its + `INSERT` then violated `idx_block_wallet_height` — an index the upsert's + `(wallet_name, hash)` conflict target does not cover — aborting the loser's + whole changeset with a raw `23505`. Block writes are now serialized per + wallet with a transaction-scoped advisory lock; the loser waits for the + winner to commit, then sees and replaces its row (last-writer-wins), exactly + as if the writes had been issued sequentially. sqlite needs no equivalent: + its single-writer lock already serializes the same interleaving. Reproduced + before the fix (`duplicate key value violates unique constraint + "idx_block_wallet_height"`); regression test + `concurrent_block_writes_at_same_height_both_land` covers both backends. +- The `tx.last_seen` upsert overwrote unconditionally, so a stale or replayed + changeset moved the timestamp backwards, contradicting bdk_chain's own + `Merge` (last_seen only ever increases). The conflict update now keeps the + maximum on both backends, matching the monotonic `last_revealed` update. + Regression test `last_seen_never_regresses` covers both backends. +- The sqlite backend propagated raw `BdkSqlxError::Sqlx` for statement + failures while postgres wrapped them in `BdkSqlxError::QueryError` with + table context, so callers could not match on one error kind for "the write + failed at the database". sqlite now wraps with the same table labels. + +### Added + +- `SqliteStoreBuilder`, mirroring `PgStoreBuilder` + (`new(wallet_name).network(..).migrate(..).pool(..).build()` / + `build_with_url(..)`; `build_with_url(None)` builds the single-connection + in-memory store). `Store::::new_with_url` now delegates to it. +- `Store::::migrate()`, mirroring `Store::::migrate`. + +### Changed + +- Tests no longer panic when `DATABASE_TEST_URL` is unset: postgres-backend + tests skip gracefully (with a one-time notice) and the sqlite backend still + runs. CI sets the variable, so full coverage always runs there. +- `#[tracing::instrument]` on persist-path helpers is uniformly `skip_all` on + both backends: one rule, no span records arguments. +- Module-internal free functions were `pub` in private modules (unreachable, + misleading); they are now `pub(crate)`. +- The README's "Resolved defects" section moved here. + +## Resolved defects (previous review round) + +- `tx.last_seen` for a tx not yet stored was silently dropped (the `UPDATE` + affected 0 rows); the write now upserts a stub row (`whole_tx` is nullable). +- Reads anchored on the `network` row, so rows persisted by a changeset that + carried no network were written but never read back; tx/block tables are now + read unconditionally. +- A changeset mapping the same block hash to several heights silently + collapsed to one block row, losing checkpoints; such changesets are now + rejected with `DuplicateBlockHash`. +- The postgres `write` path did not validate `changeset.network` against the + configured network, letting a foreign network overwrite the row and wedge + all subsequent reads; the write is now rejected with `InvalidNetwork`. +- `keychain.last_revealed INTEGER DEFAULT 0` made a wallet persisted before + its first address reveal reload with index 0 marked as used, skipping it + forever. New rows now store NULL explicitly and migration 04 drops the + default. Existing rows are deliberately untouched: a stored `0` is + ambiguous ("revealed index 0" vs "never revealed") and rewriting it could + cause address reuse. +- `update_last_revealed` was a plain `UPDATE`, letting a stale/replayed + changeset move the derivation index backwards and silently reuse addresses; + the update now never decreases the stored value. +- `Store::::read` ran at READ COMMITTED (per-statement snapshots), + so a concurrent writer could produce a mixed-generation changeset; the read + transaction now uses REPEATABLE READ. +- `initialize_network` had a check-then-set race that failed concurrent + same-network builds spuriously with `SetNetworkFailure`; a lost race now + re-validates instead. +- `Store` derived `Clone` with a `DB: Clone` bound that sqlx's `Postgres`/ + `Sqlite` marker types do not satisfy, making the impl unusable; a manual + bound-free impl is provided. +- Reads anchored keychain rows on the `network` row, so descriptors and + derivation state persisted by a changeset that carried no network were + written but never read back (the tx/block invisibility defect, one table + over); keychain rows are now read unconditionally. +- The sqlite backend had no network validation at all: its constructor took + no network and any stored or incoming network was accepted. It now takes + the network at construction (shared process-global with the postgres + backend, so one process can never mix networks) and applies the same + read/write guards. `Store::::new` and `new_with_url` therefore + take a `network` argument. +- `insert_descriptor`'s conflict update kept the stored `last_revealed` + unconditionally, so replacing a descriptor under the same + `(wallet_name, keychainkind)` made the new descriptor inherit the old + derivation index and silently skip those addresses on load. The keep is + now conditional on the descriptor being unchanged; a replaced descriptor + restarts derivation at NULL. +- A `keychainkind` value outside `'External'`/`'Internal'` was silently + ignored on load, dropping a keychain; corrupt rows now fail with + `InvalidKeychainKind`. diff --git a/README.md b/README.md index 945a01f..5c35ff9 100644 --- a/README.md +++ b/README.md @@ -4,60 +4,8 @@ This crate is still **EXPERIMENTAL** do not use with mainnet wallets. -## Resolved defects - -The following defects were found in review and are fixed; each is guarded by -an always-on regression test in the suite (`src/test.rs` and -`tests/builder_network.rs`): - -- `tx.last_seen` for a tx not yet stored was silently dropped (the `UPDATE` - affected 0 rows); the write now upserts a stub row (`whole_tx` is nullable). -- Reads anchored on the `network` row, so rows persisted by a changeset that - carried no network were written but never read back; tx/block tables are now - read unconditionally. -- A changeset mapping the same block hash to several heights silently - collapsed to one block row, losing checkpoints; such changesets are now - rejected with `DuplicateBlockHash`. -- The postgres `write` path did not validate `changeset.network` against the - configured network, letting a foreign network overwrite the row and wedge - all subsequent reads; the write is now rejected with `InvalidNetwork`. -- `keychain.last_revealed INTEGER DEFAULT 0` made a wallet persisted before - its first address reveal reload with index 0 marked as used, skipping it - forever. New rows now store NULL explicitly and migration 04 drops the - default. Existing rows are deliberately untouched: a stored `0` is - ambiguous ("revealed index 0" vs "never revealed") and rewriting it could - cause address reuse. -- `update_last_revealed` was a plain `UPDATE`, letting a stale/replayed - changeset move the derivation index backwards and silently reuse addresses; - the update now never decreases the stored value. -- `Store::::read` ran at READ COMMITTED (per-statement snapshots), - so a concurrent writer could produce a mixed-generation changeset; the read - transaction now uses REPEATABLE READ. -- `initialize_network` had a check-then-set race that failed concurrent - same-network builds spuriously with `SetNetworkFailure`; a lost race now - re-validates instead. -- `Store` derived `Clone` with a `DB: Clone` bound that sqlx's `Postgres`/ - `Sqlite` marker types do not satisfy, making the impl unusable; a manual - bound-free impl is provided. -- Reads anchored keychain rows on the `network` row, so descriptors and - derivation state persisted by a changeset that carried no network were - written but never read back (the tx/block invisibility defect, one table - over); keychain rows are now read unconditionally. -- The sqlite backend had no network validation at all: its constructor took - no network and any stored or incoming network was accepted. It now takes - the network at construction (shared process-global with the postgres - backend, so one process can never mix networks) and applies the same - read/write guards. `Store::::new` and `new_with_url` therefore - take a `network` argument. -- `insert_descriptor`'s conflict update kept the stored `last_revealed` - unconditionally, so replacing a descriptor under the same - `(wallet_name, keychainkind)` made the new descriptor inherit the old - derivation index and silently skip those addresses on load. The keep is - now conditional on the descriptor being unchanged; a replaced descriptor - restarts derivation at NULL. -- A `keychainkind` value outside `'External'`/`'Internal'` was silently - ignored on load, dropping a keychain; corrupt rows now fail with - `InvalidKeychainKind`. +Defects found in review and fixed (each guarded by an always-on regression +test) are listed in [CHANGELOG.md](CHANGELOG.md). ## Security notes @@ -85,6 +33,9 @@ an always-on regression test in the suite (`src/test.rs` and (and later cleans up) its own uniquely named `bdk_sqlx_test_*` database, so tests never touch existing data and are safe to run in parallel. Do not point this at a production server. + + Without `DATABASE_TEST_URL` the postgres-backend tests skip gracefully and + only the sqlite backend runs; set it for full coverage (CI always does). 3. Run tests: ``` cargo test diff --git a/src/lib.rs b/src/lib.rs index f0f2208..0b04bda 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -161,6 +161,14 @@ pub struct PgStoreBuilder { network: Option, } +/// Build a new instance of the SqliteStoreBuilder +pub struct SqliteStoreBuilder { + wallet_name: String, + pool: Option>, + migrate: bool, + network: Option, +} + type FutureResult<'a, T, E> = Pin> + Send + 'a>>; /// Converts an integer crossing the database boundary, erroring instead of wrapping diff --git a/src/postgres.rs b/src/postgres.rs index a46ffef..2382a65 100644 --- a/src/postgres.rs +++ b/src/postgres.rs @@ -308,7 +308,7 @@ impl Store { } /// Insert keychain descriptors. -#[tracing::instrument(skip(db_tx, descriptor))] +#[tracing::instrument(skip_all)] async fn insert_descriptor( db_tx: &mut Transaction<'_, Postgres>, wallet_name: &str, @@ -355,7 +355,7 @@ async fn insert_descriptor( } /// Insert network. -#[tracing::instrument(skip(db_tx, network))] +#[tracing::instrument(skip_all)] async fn insert_network( db_tx: &mut Transaction<'_, Postgres>, wallet_name: &str, @@ -379,7 +379,7 @@ async fn insert_network( } /// Update keychain last revealed -#[tracing::instrument(skip(db_tx, descriptor_id, last_revealed))] +#[tracing::instrument(skip_all)] async fn update_last_revealed( db_tx: &mut Transaction<'_, Postgres>, wallet_name: &str, @@ -418,7 +418,7 @@ async fn update_last_revealed( /// Select transactions, txouts, and anchors. #[tracing::instrument(skip(db_tx))] -pub async fn tx_graph_changeset_from_postgres( +pub(crate) async fn tx_graph_changeset_from_postgres( db_tx: &mut Transaction<'_, Postgres>, wallet_name: &str, ) -> Result> { @@ -527,7 +527,7 @@ pub async fn tx_graph_changeset_from_postgres( /// Insert transactions, txouts, and anchors. #[tracing::instrument(skip(db_tx, changeset))] -pub async fn tx_graph_changeset_persist_to_postgres( +pub(crate) async fn tx_graph_changeset_persist_to_postgres( db_tx: &mut Transaction<'_, Postgres>, wallet_name: &str, changeset: &tx_graph::ChangeSet, @@ -552,10 +552,16 @@ pub async fn tx_graph_changeset_persist_to_postgres( for (&txid, &last_seen) in &changeset.last_seen { // Upsert a stub row when the full tx is not stored yet; a bare UPDATE // would affect 0 rows and silently drop the timestamp. whole_tx stays - // NULL until a changeset carrying the full tx fills it in. + // NULL until a changeset carrying the full tx fills it in. The + // conflict update never moves the timestamp backwards: bdk_chain's own + // Merge only ever increases last_seen, and a stale or replayed + // changeset must not regress the stored value (the same guarantee + // update_last_revealed enforces for derivation state). sqlx::query( r#"INSERT INTO "bdk_wallet"."tx" (wallet_name, txid, last_seen) VALUES ($1, $2, $3) - ON CONFLICT (wallet_name, txid) DO UPDATE SET last_seen = $3"#, + ON CONFLICT (wallet_name, txid) DO UPDATE SET + last_seen = CASE WHEN tx.last_seen IS NULL OR $3 > tx.last_seen + THEN $3 ELSE tx.last_seen END"#, ) .bind(wallet_name) .bind(txid.to_string()) @@ -610,7 +616,7 @@ pub async fn tx_graph_changeset_persist_to_postgres( /// Select blocks. #[tracing::instrument(skip(db_tx))] -pub async fn local_chain_changeset_from_postgres( +pub(crate) async fn local_chain_changeset_from_postgres( db_tx: &mut Transaction<'_, Postgres>, wallet_name: &str, ) -> Result { @@ -642,12 +648,35 @@ pub async fn local_chain_changeset_from_postgres( /// Insert blocks. #[tracing::instrument(skip(db_tx, changeset))] -pub async fn local_chain_changeset_persist_to_postgres( +pub(crate) async fn local_chain_changeset_persist_to_postgres( db_tx: &mut Transaction<'_, Postgres>, wallet_name: &str, changeset: &local_chain::ChangeSet, ) -> Result<()> { trace!("local chain changeset to postgres"); + if changeset.blocks.is_empty() { + return Ok(()); + } + // The delete+insert below keeps exactly one row per (wallet_name, height) + // and must observe every committed row at that height. A concurrent writer + // doing the same for a different hash at the same height is invisible to + // the DELETE until it commits, and its INSERT then loses to the + // idx_block_wallet_height unique index (which the upsert's + // (wallet_name, hash) conflict target does not cover), aborting its whole + // changeset with a raw 23505. Serialize the block-table section per wallet + // instead: the loser waits for the winner to commit, then sees and + // replaces its row (last-writer-wins), exactly as if the writes had been + // issued sequentially. The lock is transaction-scoped, so commit or + // rollback releases it. sqlite needs no equivalent: its single-writer + // lock already serializes the same interleaving. + sqlx::query("SELECT pg_advisory_xact_lock(hashtext($1))") + .bind(format!("bdk_sqlx_local_chain:{wallet_name}")) + .execute(&mut **db_tx) + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "lock local_chain".to_string(), + source: e, + })?; // The block table keys rows by (wallet_name, hash), so a changeset mapping // one hash to several heights cannot be represented: persisting it would // silently collapse to a single row and lose checkpoints. Reject it loudly diff --git a/src/sqlite.rs b/src/sqlite.rs index c9ac131..376f3e8 100644 --- a/src/sqlite.rs +++ b/src/sqlite.rs @@ -75,12 +75,24 @@ impl Store { migrate: bool, ) -> Result { trace!("new sqlite store"); + let store = Self { pool, wallet_name }; if migrate { trace!("migrate"); - migrate!("./migrations/sqlite").run(&pool).await?; + store.migrate().await?; } crate::initialize_network(network)?; - Ok(Self { pool, wallet_name }) + Ok(store) + } + + /// Runs the versioned migrations in `migrations/sqlite` for this [`Store`]. + /// + /// Mirrors [`Store::::migrate`]: migrations are recorded in + /// sqlx's bookkeeping table, so re-running them is a no-op. + #[tracing::instrument(skip_all)] + pub async fn migrate(&self) -> Result<(), BdkSqlxError> { + trace!("migrating bdk sqlx"); + migrate!("./migrations/sqlite").run(&self.pool).await?; + Ok(()) } /// Construct a new [`Store`] without an existing sqlite connection pool. @@ -99,8 +111,124 @@ impl Store { migrate: bool, ) -> Result, BdkSqlxError> { trace!("new store with url"); + crate::SqliteStoreBuilder::new(wallet_name) + .network(network) + .migrate(migrate) + .build_with_url(url.as_deref()) + .await + } +} + +impl crate::SqliteStoreBuilder { + /// Creates a new builder for a [`Store`] with the given wallet name. + /// + /// # Required fields + /// Before building, you must set: + /// - `network` - The Bitcoin network to use + /// - Either provide a connection pool with `pool()` or a database URL with `build_with_url()` + /// + /// # Example + /// ``` + /// # async fn example() -> Result<(), bdk_sqlx::BdkSqlxError> { + /// use bdk_wallet::bitcoin::Network; + /// use bdk_sqlx::SqliteStoreBuilder; + /// + /// let store = SqliteStoreBuilder::new("bdk_wallet_name".to_string()) + /// .network(Network::Testnet) + /// .migrate(true) + /// .build_with_url(Some("sqlite://bdk_wallet.sqlite?mode=rwc")) + /// .await?; + /// # Ok(()) + /// # } + /// ``` + #[tracing::instrument] + pub fn new(wallet_name: String) -> Self { + Self { + wallet_name, + pool: None, + migrate: false, + network: None, + } + } + + /// Sets the database connection pool for the [`Store`]. + /// + /// The pool is required to build a valid [`Store`]. If not provided, + /// the build operation will fail with a MissingPool error. + /// + /// # Warning + /// + /// Do not pass a pool connected to `:memory:` with more than one connection: + /// each sqlite connection gets its *own* private in-memory database, so a + /// multi-connection pool silently reads and writes different databases (and + /// per-connection `PRAGMA`s only apply to the connection that ran them). + /// Use [`SqliteStoreBuilder::build_with_url`] with `None` instead, which + /// configures a single-connection pool correctly. + pub fn pool(mut self, pool: Pool) -> Self { + self.pool = Some(pool); + self + } + + /// Sets whether database migrations should be run during [`Store`] initialization. + /// + /// When set to true, the necessary database schema and tables will be created + /// if they don't already exist. + pub fn migrate(mut self, migrate: bool) -> Self { + self.migrate = migrate; + self + } + + /// Sets the Bitcoin network for the [`Store`]. + /// + /// The network is required to build a valid [`Store`]. If not provided, + /// the build operation will fail with a MissingNetwork error. + /// + /// The network is process-global and shared across backends: the first + /// store built (postgres or sqlite) fixes it for the whole process, and + /// later builds with a different network fail with + /// [`BdkSqlxError::DuplicateInitNetwork`]. Every store validates stored + /// and incoming data against it. + pub fn network(mut self, network: Network) -> Self { + self.network = Some(network); + self + } + + /// Builds the [`Store`] with the configured options. + /// + /// This method creates a new [`Store`] instance using the options that have been + /// set on this builder. It requires both a network and a pool to be specified + /// before building. + /// + /// # Errors + /// + /// Returns an error if: + /// - No network has been specified (MissingNetwork) + /// - No pool has been specified (MissingPool) + /// - Migration fails + /// - Network initialization fails + pub async fn build(self) -> Result, BdkSqlxError> { + let network = self.network.ok_or(BdkSqlxError::MissingNetwork)?; + match self.pool { + Some(pool) => Store::new(pool, self.wallet_name, network, self.migrate).await, + None => Err(BdkSqlxError::MissingPool), + } + } + + /// Builds the [`Store`] with a new connection pool created from the provided URL. + /// + /// This is a convenience method that creates a connection pool from the URL + /// and then builds the [`Store`] using that pool. The SQLite DB URL should + /// look like "sqlite://bdk_wallet.sqlite?mode=rwc". If no URL is given, a + /// single-connection in-memory database (useful for testing) is created. + /// + /// # Errors + /// + /// Returns an error if: + /// - Database connection fails + /// - Any error that could occur in the build() method + pub async fn build_with_url(self, url: Option<&str>) -> Result, BdkSqlxError> { let pool = if let Some(url) = url { - SqlitePool::connect(url.as_str()).await? + SqlitePool::connect(url).await? } else { // must limit to one connection and no timeout if using memory DB SqlitePoolOptions::new() @@ -111,7 +239,7 @@ impl Store { .connect(":memory:") .await? }; - Self::new(pool, wallet_name, network, migrate).await + self.pool(pool).build().await } } @@ -127,7 +255,11 @@ impl Store { let row = sqlx::query("SELECT name FROM network WHERE wallet_name = $1") .bind(&self.wallet_name) .fetch_optional(&mut *tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "read network".to_string(), + source: e, + })?; if let Some(row) = row { let network: String = row.get("name"); changeset.network = Some(crate::parse_and_validate_network(&network)?); @@ -142,7 +274,11 @@ impl Store { ) .bind(&self.wallet_name) .fetch_all(&mut *tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "read keychain".to_string(), + source: e, + })?; for row in rows { let keychainkind: String = row.get("keychainkind"); let descriptor: String = row.get("descriptor"); @@ -248,7 +384,11 @@ async fn insert_descriptor( .bind(descriptor_str) .bind(descriptor_id.as_slice()) .execute(&mut **tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "insert keychain".to_string(), + source: e, + })?; Ok(()) } @@ -268,7 +408,11 @@ async fn insert_network( .bind(wallet_name) .bind(network.to_string()) .execute(&mut **tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "insert network".to_string(), + source: e, + })?; Ok(()) } @@ -297,7 +441,11 @@ async fn update_last_revealed( .bind(wallet_name) .bind(descriptor_id.to_byte_array().as_slice()) .execute(&mut **tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "update keychain".to_string(), + source: e, + })?; // Silently updating 0 rows would lose derivation state and cause address reuse. if result.rows_affected() == 0 { @@ -312,7 +460,7 @@ async fn update_last_revealed( /// Select transactions, txouts, and anchors. #[tracing::instrument(skip_all)] -pub async fn tx_graph_changeset_from_sqlite( +pub(crate) async fn tx_graph_changeset_from_sqlite( db_tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, ) -> Result, BdkSqlxError> { @@ -323,7 +471,11 @@ pub async fn tx_graph_changeset_from_sqlite( let rows = sqlx::query("SELECT txid, whole_tx, last_seen FROM tx WHERE wallet_name = $1") .bind(wallet_name) .fetch_all(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "select tx".to_string(), + source: e, + })?; for row in rows { let txid: String = row.get("txid"); @@ -353,7 +505,11 @@ pub async fn tx_graph_changeset_from_sqlite( let rows = sqlx::query("SELECT txid, vout, value, script FROM txout WHERE wallet_name = $1") .bind(wallet_name) .fetch_all(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "select txout".to_string(), + source: e, + })?; for row in rows { let txid: String = row.get("txid"); @@ -380,7 +536,11 @@ pub async fn tx_graph_changeset_from_sqlite( ) .bind(wallet_name) .fetch_all(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "select anchor tx".to_string(), + source: e, + })?; for row in rows { let anchor: serde_json::Value = row.get("anchor"); @@ -405,7 +565,7 @@ pub async fn tx_graph_changeset_from_sqlite( /// Insert transactions, txouts, and anchors. #[tracing::instrument(skip_all)] -pub async fn tx_graph_changeset_persist_to_sqlite( +pub(crate) async fn tx_graph_changeset_persist_to_sqlite( db_tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, changeset: &tx_graph::ChangeSet, @@ -420,22 +580,36 @@ pub async fn tx_graph_changeset_persist_to_sqlite( .bind(tx.compute_txid().to_string()) .bind(consensus::serialize(tx.as_ref())) .execute(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "insert tx".to_string(), + source: e, + })?; } for (&txid, &last_seen) in &changeset.last_seen { // Upsert a stub row when the full tx is not stored yet; a bare UPDATE // would affect 0 rows and silently drop the timestamp. whole_tx stays - // NULL until a changeset carrying the full tx fills it in. + // NULL until a changeset carrying the full tx fills it in. The + // conflict update never moves the timestamp backwards: bdk_chain's own + // Merge only ever increases last_seen, and a stale or replayed + // changeset must not regress the stored value (the same guarantee + // update_last_revealed enforces for derivation state). sqlx::query( "INSERT INTO tx (wallet_name, txid, last_seen) VALUES ($1, $2, $3) - ON CONFLICT (wallet_name, txid) DO UPDATE SET last_seen = $3", + ON CONFLICT (wallet_name, txid) DO UPDATE SET + last_seen = CASE WHEN tx.last_seen IS NULL OR $3 > tx.last_seen + THEN $3 ELSE tx.last_seen END", ) .bind(wallet_name) .bind(txid.to_string()) .bind(crate::checked_conv::<_, i64>(last_seen, "tx.last_seen")?) .execute(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "update tx".to_string(), + source: e, + })?; } for (op, txo) in &changeset.txouts { @@ -452,7 +626,11 @@ pub async fn tx_graph_changeset_persist_to_sqlite( )?) .bind(txo.script_pubkey.as_bytes()) .execute(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "insert txout".to_string(), + source: e, + })?; } for (anchor, txid) in &changeset.anchors { @@ -467,7 +645,11 @@ pub async fn tx_graph_changeset_persist_to_sqlite( .bind(anchor) .bind(txid.to_string()) .execute(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "insert anchor tx".to_string(), + source: e, + })?; } Ok(()) @@ -475,7 +657,7 @@ pub async fn tx_graph_changeset_persist_to_sqlite( /// Select blocks. #[tracing::instrument(skip_all)] -pub async fn local_chain_changeset_from_sqlite( +pub(crate) async fn local_chain_changeset_from_sqlite( db_tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, ) -> Result { @@ -485,7 +667,11 @@ pub async fn local_chain_changeset_from_sqlite( let rows = sqlx::query("SELECT hash, height FROM block WHERE wallet_name = $1") .bind(wallet_name) .fetch_all(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "select block".to_string(), + source: e, + })?; for row in rows { let hash: String = row.get("hash"); @@ -502,12 +688,15 @@ pub async fn local_chain_changeset_from_sqlite( /// Insert blocks. #[tracing::instrument(skip_all)] -pub async fn local_chain_changeset_persist_to_sqlite( +pub(crate) async fn local_chain_changeset_persist_to_sqlite( db_tx: &mut Transaction<'_, Sqlite>, wallet_name: &str, changeset: &local_chain::ChangeSet, ) -> Result<(), BdkSqlxError> { trace!("local chain changeset to sqlite"); + if changeset.blocks.is_empty() { + return Ok(()); + } // The block table keys rows by (wallet_name, hash), so a changeset mapping // one hash to several heights cannot be represented: persisting it would // silently collapse to a single row and lose checkpoints. Reject it loudly @@ -525,6 +714,12 @@ pub async fn local_chain_changeset_persist_to_sqlite( seen.insert(hash, height); } } + // Concurrent writers persisting different hashes at the same height need + // no explicit serialization here (unlike postgres, which takes an advisory + // lock): sqlite admits only one writer at a time, so the second writer's + // DELETE blocks on the database write lock until the first commits, then + // sees and replaces its row -- last-writer-wins, as if the writes had been + // issued sequentially. for (&height, &hash) in &changeset.blocks { match hash { Some(hash) => { @@ -538,7 +733,11 @@ pub async fn local_chain_changeset_persist_to_sqlite( .bind(crate::checked_conv::<_, i32>(height, "block.height")?) .bind(hash.to_string()) .execute(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "delete stale block".to_string(), + source: e, + })?; sqlx::query( "INSERT INTO block (wallet_name, hash, height) VALUES ($1, $2, $3) ON CONFLICT (wallet_name, hash) DO UPDATE SET height = $3", @@ -547,14 +746,22 @@ pub async fn local_chain_changeset_persist_to_sqlite( .bind(hash.to_string()) .bind(crate::checked_conv::<_, i32>(height, "block.height")?) .execute(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "insert block".to_string(), + source: e, + })?; } None => { sqlx::query("DELETE FROM block WHERE wallet_name = $1 AND height = $2") .bind(wallet_name) .bind(crate::checked_conv::<_, i32>(height, "block.height")?) .execute(&mut **db_tx) - .await?; + .await + .map_err(|e| BdkSqlxError::QueryError { + table: "delete block".to_string(), + source: e, + })?; } } } diff --git a/src/test.rs b/src/test.rs index 4129ee5..3c2bd88 100644 --- a/src/test.rs +++ b/src/test.rs @@ -31,9 +31,9 @@ use test_utils::{ use tracing::info; use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt, EnvFilter, Layer}; -use crate::{BdkSqlxError, FutureResult, PgStoreBuilder, Store}; +use crate::{BdkSqlxError, FutureResult, PgStoreBuilder, SqliteStoreBuilder, Store}; -pub fn get_test_minisicript_with_change_desc() -> (&'static str, &'static str) { +fn get_test_miniscript_with_change_desc() -> (&'static str, &'static str) { ("wsh(andor(multi(2,[a0d3c79c/48'/1'/79'/2']tpubDEsGdqFaKUVnVNZZw8AixJ8C3yD8o6nN7hsdLfbtVRDTk3PNrQ2pcWNWNbxhdcNSgQP25pUpgRQ7qiVtN3YvSzACKizrvzSwH9SQ2Bjbbwt/0/*,[ea2484f9/48'/1'/79'/2']tpubDFjkswBXoRHKkvmHsxv4xdDqbjg1peX9zJytLeSLbXuwVgYhXgbABzC2r5MAWxqWoaUr7hWGW5TPjA9sNvxa3mX6DrNBdynDsEvwDoXGFpm/0/*,[93f245d7/48'/1'/79'/2']tpubDEVnR72gRgTsqaPFMacV6fCfaSEe56gcDomuGhk9MFeUdEi18riJCokgsZr2x1KKGRM59TJ4AQ6FuNun3khh95ceoH2ytN13nVD7yDLP5LJ/0/*),or_i(and_v(v:pkh([61cdf766/48'/1'/79'/2']tpubDEXETCw2WurhazfW5gW1z4njP6yLXDQmCGfjWGP5k3BuTQ5iZqovMr1zz1zWPhDMRn11hXGpZHodus1LysXnwREsD1ig96M24JhQCpPPpf6/0/*),after(1753228800)),thresh(2,pk([39bf48a9/48'/1'/0'/2']tpubDEr9rVFQbT1keErwxb6GuGy3RM6TEACSkFxBgziUvrDprYuM1Wm7wi6jb1gcaLrSgk6MSkGx84dS2kQQwJKxGRJ59rAvmuKTU7E3saHJLf5/0/*),s:pk([9467fdb3/48'/1'/0'/2']tpubDFEjX5BY88AbWpshPwGscwgKLtcCjeVodMbmhS6D6cbz1eGNUs3546ephbVmbHpxEhbCDrezGmFBArLxBKzPEfBcBdzQuncPm8ww2xa6UUQ/0/*),s:pk([01adf45e/48'/1'/0'/2']tpubDFPYZPeShApyWndvDUtpLSjDHGYK4tTT4BkMyTukGqbP9AXQeQhiWsbwEzyZhxgud9ZPew1FPsoLbWjfnE3veSXLeU4ViofrhVAHNXtjQWE/0/*),snl:after(1739836800))),and_v(v:thresh(2,pkh([39bf48a9/48'/1'/0'/2']tpubDEr9rVFQbT1keErwxb6GuGy3RM6TEACSkFxBgziUvrDprYuM1Wm7wi6jb1gcaLrSgk6MSkGx84dS2kQQwJKxGRJ59rAvmuKTU7E3saHJLf5/2/*),a:pkh([9467fdb3/48'/1'/0'/2']tpubDFEjX5BY88AbWpshPwGscwgKLtcCjeVodMbmhS6D6cbz1eGNUs3546ephbVmbHpxEhbCDrezGmFBArLxBKzPEfBcBdzQuncPm8ww2xa6UUQ/2/*),a:pkh([01adf45e/48'/1'/0'/2']tpubDFPYZPeShApyWndvDUtpLSjDHGYK4tTT4BkMyTukGqbP9AXQeQhiWsbwEzyZhxgud9ZPew1FPsoLbWjfnE3veSXLeU4ViofrhVAHNXtjQWE/2/*)),after(1757116800))))", "wsh(andor(multi(2,[a0d3c79c/48'/1'/79'/2']tpubDEsGdqFaKUVnVNZZw8AixJ8C3yD8o6nN7hsdLfbtVRDTk3PNrQ2pcWNWNbxhdcNSgQP25pUpgRQ7qiVtN3YvSzACKizrvzSwH9SQ2Bjbbwt/1/*,[ea2484f9/48'/1'/79'/2']tpubDFjkswBXoRHKkvmHsxv4xdDqbjg1peX9zJytLeSLbXuwVgYhXgbABzC2r5MAWxqWoaUr7hWGW5TPjA9sNvxa3mX6DrNBdynDsEvwDoXGFpm/1/*,[93f245d7/48'/1'/79'/2']tpubDEVnR72gRgTsqaPFMacV6fCfaSEe56gcDomuGhk9MFeUdEi18riJCokgsZr2x1KKGRM59TJ4AQ6FuNun3khh95ceoH2ytN13nVD7yDLP5LJ/1/*),or_i(and_v(v:pkh([61cdf766/48'/1'/79'/2']tpubDEXETCw2WurhazfW5gW1z4njP6yLXDQmCGfjWGP5k3BuTQ5iZqovMr1zz1zWPhDMRn11hXGpZHodus1LysXnwREsD1ig96M24JhQCpPPpf6/1/*),after(1753228800)),thresh(2,pk([39bf48a9/48'/1'/0'/2']tpubDEr9rVFQbT1keErwxb6GuGy3RM6TEACSkFxBgziUvrDprYuM1Wm7wi6jb1gcaLrSgk6MSkGx84dS2kQQwJKxGRJ59rAvmuKTU7E3saHJLf5/1/*),s:pk([9467fdb3/48'/1'/0'/2']tpubDFEjX5BY88AbWpshPwGscwgKLtcCjeVodMbmhS6D6cbz1eGNUs3546ephbVmbHpxEhbCDrezGmFBArLxBKzPEfBcBdzQuncPm8ww2xa6UUQ/1/*),s:pk([01adf45e/48'/1'/0'/2']tpubDFPYZPeShApyWndvDUtpLSjDHGYK4tTT4BkMyTukGqbP9AXQeQhiWsbwEzyZhxgud9ZPew1FPsoLbWjfnE3veSXLeU4ViofrhVAHNXtjQWE/1/*),snl:after(1739836800))),and_v(v:thresh(2,pkh([39bf48a9/48'/1'/0'/2']tpubDEr9rVFQbT1keErwxb6GuGy3RM6TEACSkFxBgziUvrDprYuM1Wm7wi6jb1gcaLrSgk6MSkGx84dS2kQQwJKxGRJ59rAvmuKTU7E3saHJLf5/3/*),a:pkh([9467fdb3/48'/1'/0'/2']tpubDFEjX5BY88AbWpshPwGscwgKLtcCjeVodMbmhS6D6cbz1eGNUs3546ephbVmbHpxEhbCDrezGmFBArLxBKzPEfBcBdzQuncPm8ww2xa6UUQ/3/*),a:pkh([01adf45e/48'/1'/0'/2']tpubDFPYZPeShApyWndvDUtpLSjDHGYK4tTT4BkMyTukGqbP9AXQeQhiWsbwEzyZhxgud9ZPew1FPsoLbWjfnE3veSXLeU4ViofrhVAHNXtjQWE/3/*)),after(1757116800))))") } @@ -117,16 +117,37 @@ async fn pg_mgmt_unlock(mut conn: sqlx::pool::PoolConnection) { .await; } +/// Returns the `DATABASE_TEST_URL` postgres server the tests may use, or `None` +/// when it is not set. Postgres-backend tests skip gracefully in that case (a +/// contributor running bare `cargo test` without a server still exercises the +/// sqlite backend); CI always sets it, so full coverage runs there. +fn pg_test_url() -> Option { + static NOTICE: Once = Once::new(); + let url = env::var("DATABASE_TEST_URL").ok(); + if url.is_none() { + NOTICE.call_once(|| { + eprintln!( + "DATABASE_TEST_URL not set: skipping postgres-backend tests \ + (sqlite backend still runs)" + ); + }); + } + url +} + /// Creates a uniquely named database on the postgres server at `DATABASE_TEST_URL` and /// returns a pool connected to it, so every test gets an isolated database and no -/// pre-existing tables are ever dropped. +/// pre-existing tables are ever dropped. Returns `None` when `DATABASE_TEST_URL` +/// is not set; callers skip the postgres half of their scenario in that case. /// /// Databases left behind by previous test runs are removed opportunistically; a database /// is never dropped while any session is connected to it, and creation/cleanup are /// serialized (in-process by `TEST_DB_LOCK`, cross-process by the advisory lock) so a /// parallel test cannot drop a database between its creation and first connection. -async fn create_test_pg_pool() -> anyhow::Result> { - let admin_url = env::var("DATABASE_TEST_URL").expect("DATABASE_TEST_URL must be set for tests"); +async fn create_test_pg_pool() -> anyhow::Result>> { + let Some(admin_url) = pg_test_url() else { + return Ok(None); + }; let admin_pool = Pool::::connect(&admin_url).await?; let db_name = format!( @@ -163,7 +184,7 @@ async fn create_test_pg_pool() -> anyhow::Result> { .min_connections(1) .connect_with(opts) .await?; - anyhow::Ok(pool) + anyhow::Ok(Some(pool)) } .await; @@ -447,7 +468,9 @@ async fn mismatched_network_errors_on_load() -> anyhow::Result<()> { &Secp256k1::new(), )?; - let pool = create_test_pg_pool().await?; + let Some(pool) = create_test_pg_pool().await? else { + return Ok(()); + }; let mut store = PgStoreBuilder::new(wallet_name.clone()) .network(NETWORK) .migrate(true) @@ -551,14 +574,15 @@ async fn tracing_output_contains_no_descriptor_material() -> anyhow::Result<()> async fn create_test_stores(wallet_name: String) -> anyhow::Result> { let mut stores: Vec = Vec::new(); - let pool = create_test_pg_pool().await?; - let postgres_store = PgStoreBuilder::new(wallet_name.clone()) - .network(NETWORK) - .migrate(true) - .pool(pool) - .build() - .await?; - stores.push(TestStore::Postgres(postgres_store)); + if let Some(pool) = create_test_pg_pool().await? { + let postgres_store = PgStoreBuilder::new(wallet_name.clone()) + .network(NETWORK) + .migrate(true) + .pool(pool) + .build() + .await?; + stores.push(TestStore::Postgres(postgres_store)); + } // Setup sqlite in-memory database. `new_with_url(None, ..)` configures the // single-connection pool a shared in-memory database requires. @@ -769,7 +793,7 @@ async fn test_three_wallets_list_transactions() -> anyhow::Result<()> { TestCase::new(get_test_tr_single_sig_xprv_and_change_desc(), 20_000, 11_000, 2000).await, TestCase::new(("wpkh([bdb9a801/84'/1'/0']tpubDCopxf4CiXF9dicdGrXgZV9f8j3pYbWBVfF8WxjaFHtic4DZsgp1tQ58hZdsSu6M7FFzUyAh9rMn7RZASUkPgZCMdByYKXvVtigzGi8VJs6/0/*)#j8mkwdgr", "wpkh([bdb9a801/84'/1'/0']tpubDCopxf4CiXF9dicdGrXgZV9f8j3pYbWBVfF8WxjaFHtic4DZsgp1tQ58hZdsSu6M7FFzUyAh9rMn7RZASUkPgZCMdByYKXvVtigzGi8VJs6/1/*)#rn7hnccm"), 12_000, 30_000, 1500).await, - TestCase::new(get_test_minisicript_with_change_desc(), 44_444, 20_000, 5000).await + TestCase::new(get_test_miniscript_with_change_desc(), 44_444, 20_000, 5000).await ].into_iter().flatten().collect::>(); let mut saved_tx_ids = Vec::::new(); @@ -1793,12 +1817,11 @@ async fn failed_persist_rolls_back_everything() -> anyhow::Result<()> { .insert((anchor_at(99, block_hash(9), 1), txid_a)); let result = TestStore::persist(&mut store, &cs).await; - match &store { - TestStore::Postgres(_) => { - assert_matches!(result, Err(BdkSqlxError::QueryError { .. })) - } - TestStore::Sqlite(_) => assert_matches!(result, Err(BdkSqlxError::Sqlx(_))), - } + assert_matches!( + result, + Err(BdkSqlxError::QueryError { .. }), + "both backends wrap statement failures in QueryError" + ); let loaded = TestStore::initialize(&mut store).await?; assert!( @@ -2018,7 +2041,9 @@ async fn wallets_sharing_a_pool_are_isolated() -> anyhow::Result<()> { cs_b.tx_graph.txs.insert(Arc::new(tx_b)); // postgres: two builders over one pool - let pool = create_test_pg_pool().await?; + let Some(pool) = create_test_pg_pool().await? else { + return Ok(()); + }; let pg_a = PgStoreBuilder::new("pg_wallet_a".into()) .network(Regtest) .migrate(true) @@ -2151,6 +2176,73 @@ async fn concurrent_persists_both_land() -> anyhow::Result<()> { Ok(()) } +/// Regression test: two writers persisting different hashes for the same +/// (previously unoccupied) height raced the block table's two unique indexes. +/// The loser's DELETE could not see the winner's uncommitted row, and its +/// INSERT then violated idx_block_wallet_height -- an index the upsert's +/// (wallet_name, hash) conflict target does not cover -- aborting the loser's +/// whole changeset with a raw 23505. Block writes are now serialized per +/// wallet (a transaction-scoped advisory lock on postgres; sqlite's +/// single-writer lock), so both writes land and the loser cleanly replaces +/// the winner's row, exactly as if the writes had been issued sequentially. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn concurrent_block_writes_at_same_height_both_land() -> anyhow::Result<()> { + initialize(); + + let wallet_name = "concurrent_block_writes_at_same_height_both_land".to_string(); + for store in create_test_stores(wallet_name.clone()).await? { + for round in 0..10u8 { + let height = 100 + u32::from(round); + // distinct hashes everywhere: reusing a hash at two heights would + // trip the DuplicateBlockHash guard instead of exercising the race + let hash_a = block_hash(round * 2 + 1); + let hash_b = block_hash(round * 2 + 2); + + let (s1, s2) = match &store { + TestStore::Postgres(store) => ( + TestStore::Postgres(store.clone()), + TestStore::Postgres(store.clone()), + ), + TestStore::Sqlite(store) => ( + TestStore::Sqlite(store.clone()), + TestStore::Sqlite(store.clone()), + ), + }; + let barrier = Arc::new(tokio::sync::Barrier::new(2)); + + let mut cs_a = ChangeSet::default(); + cs_a.local_chain.blocks.insert(height, Some(hash_a)); + let mut cs_b = ChangeSet::default(); + cs_b.local_chain.blocks.insert(height, Some(hash_b)); + + let (mut s1, mut s2) = (s1, s2); + let b1 = barrier.clone(); + let h1 = tokio::spawn(async move { + b1.wait().await; + TestStore::persist(&mut s1, &cs_a).await + }); + let h2 = tokio::spawn(async move { + barrier.wait().await; + TestStore::persist(&mut s2, &cs_b).await + }); + h1.await??; + h2.await??; + + // both writes landed; exactly one row per height survives + assert_eq!( + table_count(&store, "block", &wallet_name).await?, + i64::from(round) + 1, + "each raced height must hold exactly one block row" + ); + } + + let mut store = store; + let loaded = TestStore::initialize(&mut store).await?; + assert_eq!(loaded.local_chain.blocks.len(), 10); + } + Ok(()) +} + // --------------------------------------------------------------------------- // Corrupt stored data must fail the load loudly // --------------------------------------------------------------------------- @@ -2358,7 +2450,9 @@ async fn unmigrated_store_errors() -> anyhow::Result<()> { initialize(); // postgres wraps statement failures in QueryError - let pool = create_test_pg_pool().await?; + let Some(pool) = create_test_pg_pool().await? else { + return Ok(()); + }; let store = PgStoreBuilder::new("unmigrated".into()) .network(Regtest) .migrate(false) @@ -2380,16 +2474,16 @@ async fn unmigrated_store_errors() -> anyhow::Result<()> { "postgres write on unmigrated schema must error" ); - // sqlite propagates the raw sqlx error + // sqlite wraps statement failures in QueryError, same as postgres let store = Store::::new_with_url(None, "unmigrated".into(), NETWORK, false).await?; assert_matches!( store.read().await, - Err(BdkSqlxError::Sqlx(_)), + Err(BdkSqlxError::QueryError { .. }), "sqlite read on unmigrated schema must error" ); assert_matches!( store.write(&cs).await, - Err(BdkSqlxError::Sqlx(_)), + Err(BdkSqlxError::QueryError { .. }), "sqlite write on unmigrated schema must error" ); Ok(()) @@ -2434,7 +2528,9 @@ async fn sqlite_file_backed_store_persists_across_connections() -> anyhow::Resul async fn postgres_migrate_is_idempotent() -> anyhow::Result<()> { initialize(); - let pool = create_test_pg_pool().await?; + let Some(pool) = create_test_pg_pool().await? else { + return Ok(()); + }; let store = PgStoreBuilder::new("migrate_twice".into()) .network(Regtest) .migrate(true) @@ -2446,12 +2542,63 @@ async fn postgres_migrate_is_idempotent() -> anyhow::Result<()> { Ok(()) } +/// Running migrations twice must be a no-op the second time. +#[tokio::test] +async fn sqlite_migrate_is_idempotent() -> anyhow::Result<()> { + initialize(); + + let store = + Store::::new_with_url(None, "sqlite_migrate_twice".into(), NETWORK, true).await?; + store.migrate().await?; + store.migrate().await?; + Ok(()) +} + +#[tokio::test] +async fn sqlite_builder_requires_network_and_pool() { + initialize(); + + // neither network nor pool set: network is validated first + assert_matches!( + SqliteStoreBuilder::new("w".into()).build().await, + Err(BdkSqlxError::MissingNetwork) + ); + + // network set but no pool + assert_matches!( + SqliteStoreBuilder::new("w".into()) + .network(Regtest) + .build() + .await, + Err(BdkSqlxError::MissingPool) + ); +} + +/// The sqlite builder's URL path must produce a working store end to end; +/// `None` builds the single-connection in-memory store. +#[tokio::test] +async fn sqlite_build_with_url_creates_working_store() -> anyhow::Result<()> { + initialize(); + + let store = SqliteStoreBuilder::new("sqlite_with_url".into()) + .network(Regtest) + .migrate(true) + .build_with_url(None) + .await?; + assert!(store.read().await?.is_empty()); + store.write(&populated_changeset()).await?; + assert_populated(&store.read().await?, &populated_changeset()); + Ok(()) +} + /// The builder's URL path must produce a working store end to end. #[tokio::test] async fn pg_build_with_url_creates_working_store() -> anyhow::Result<()> { initialize(); - let admin_url = env::var("DATABASE_TEST_URL").expect("DATABASE_TEST_URL must be set for tests"); + let Some(admin_url) = pg_test_url() else { + return Ok(()); + }; let admin_pool = Pool::::connect(&admin_url).await?; let db_name = format!( "bdk_sqlx_test_{}_{}", @@ -2538,6 +2685,52 @@ async fn bug_last_seen_without_tx_row_is_dropped() -> anyhow::Result<()> { Ok(()) } +/// Regression test: the `tx.last_seen` upsert overwrote unconditionally, so a +/// stale or replayed changeset moved the timestamp BACKWARDS, contradicting +/// bdk_chain's own `Merge` (last_seen only ever increases). The upsert now +/// keeps the maximum, matching the monotonic `last_revealed` update. +#[tokio::test] +async fn last_seen_never_regresses() -> anyhow::Result<()> { + initialize(); + + let txid = sample_tx(0, 1_000).compute_txid(); + let wallet_name = "last_seen_never_regresses".to_string(); + for mut store in create_test_stores(wallet_name).await? { + let mut cs = ChangeSet::default(); + cs.tx_graph.txs.insert(Arc::new(sample_tx(0, 1_000))); + cs.tx_graph.last_seen.insert(txid, 200); + TestStore::persist(&mut store, &cs).await?; + + // a stale/replayed changeset must not move the stored value backwards + let mut stale = ChangeSet::default(); + stale.tx_graph.last_seen.insert(txid, 100); + TestStore::persist(&mut store, &stale).await?; + assert_eq!( + TestStore::initialize(&mut store) + .await? + .tx_graph + .last_seen + .get(&txid), + Some(&200), + "last_seen must never move backwards" + ); + + // a newer changeset still advances it + let mut fresh = ChangeSet::default(); + fresh.tx_graph.last_seen.insert(txid, 300); + TestStore::persist(&mut store, &fresh).await?; + assert_eq!( + TestStore::initialize(&mut store) + .await? + .tx_graph + .last_seen + .get(&txid), + Some(&300) + ); + } + Ok(()) +} + /// Regression test: `read()` anchored its entire load on the `network` row, /// so rows persisted by a changeset that carried no network landed in the /// database but were invisible to every subsequent read. Reads now fetch the @@ -2631,7 +2824,9 @@ async fn bug_same_hash_at_multiple_heights_collapses() -> anyhow::Result<()> { async fn bug_postgres_write_accepts_foreign_network() -> anyhow::Result<()> { initialize(); - let pool = create_test_pg_pool().await?; + let Some(pool) = create_test_pg_pool().await? else { + return Ok(()); + }; let store = PgStoreBuilder::new("bug_foreign_network".into()) .network(Regtest) .migrate(true) @@ -2760,7 +2955,9 @@ async fn bug_last_revealed_regresses_causing_address_reuse() -> anyhow::Result<( async fn bug_postgres_read_tx_is_not_snapshot_consistent() -> anyhow::Result<()> { initialize(); - let pool = create_test_pg_pool().await?; + let Some(pool) = create_test_pg_pool().await? else { + return Ok(()); + }; let _store = PgStoreBuilder::new("isolation_demo".into()) .network(Regtest) .migrate(true) @@ -2859,7 +3056,9 @@ async fn migration_04_preserves_data_and_drops_default() -> anyhow::Result<()> { std::fs::remove_file(&path)?; // postgres: same upgrade path - let pool = create_test_pg_pool().await?; + let Some(pool) = create_test_pg_pool().await? else { + return Ok(()); + }; for file in [ "01_bdk_wallet.sql", "02_anchor_tx_on_delete_cascade.sql",