From 6b6f02dfe407bdc0c6823482382ffc996d4399ae Mon Sep 17 00:00:00 2001 From: codingp110 Date: Mon, 28 Jul 2025 16:38:53 +0530 Subject: [PATCH 1/3] fix!: remove panics in tx_graph persistence fns Since rusqlite also returns an error. Also refactored the tests to check if correct error is being returned. --- src/error.rs | 5 +++ src/lib.rs | 90 ++++++++++++++++++++++++++++++++++++---------------- 2 files changed, 68 insertions(+), 27 deletions(-) diff --git a/src/error.rs b/src/error.rs index 6ed7867..f5cee15 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,5 +1,6 @@ #![warn(missing_docs)] //! This module contains the crate's error type. +use bdk_chain::bitcoin; use std::io::Error as IoError; #[derive(Debug, thiserror::Error)] @@ -34,4 +35,8 @@ pub enum StoreError { /// [`BlockHash`]: #[error("BlockHash deserialization error: {0}")] BlockHashFromSlice(#[from] bdk_chain::bitcoin::hashes::FromSliceError), + /// Error thrown when tx corresponding to txid is not found while persisting + /// anchors, last_seen, last_evicted or first_seen. + #[error("Tx corresponding to txid is missing")] + TxMissing(bitcoin::Txid), } diff --git a/src/lib.rs b/src/lib.rs index 2fb50dc..5c1f7f6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -481,7 +481,7 @@ impl Store { bytes[4..].copy_from_slice(&anchor_block.hash.to_byte_array()); table.insert((txid.to_byte_array(), bytes), &anchor.metadata())?; } else { - panic!("txn corresponding to anchor must exist"); + return Err(StoreError::TxMissing(*txid)); } } Ok(()) @@ -504,7 +504,7 @@ impl Store { if txs_table.get(txid.to_byte_array())?.is_some() || found { table.insert(txid.to_byte_array(), *last_seen_time)?; } else { - panic!("txn must exist before persisting last_seen"); + return Err(StoreError::TxMissing(*txid)); } } Ok(()) @@ -527,7 +527,7 @@ impl Store { if txs_table.get(txid.to_byte_array())?.is_some() || found { table.insert(txid.to_byte_array(), last_evicted_time)?; } else { - panic!("txn must exist before persisting last_evicted"); + return Err(StoreError::TxMissing(*txid)); } } Ok(()) @@ -550,7 +550,7 @@ impl Store { if txs_table.get(txid.to_byte_array())?.is_some() || found { table.insert(txid.to_byte_array(), first_seen_time)?; } else { - panic!("txn must exist before persisting first_seen"); + return Err(StoreError::TxMissing(*txid)); } } Ok(()) @@ -1165,28 +1165,39 @@ mod test { } #[test] - #[should_panic] fn test_last_seen_missing_txn() { // to hit the branch for the panic case in persist_last_seen let tmpfile = NamedTempFile::new().unwrap(); let db = create_db(tmpfile.path()); let store = create_test_store(Arc::new(db), "wallet1"); - let last_seen: BTreeMap = - [(hash!("B"), 100), (hash!("T"), 120), (hash!("C"), 121)].into(); + let tx1 = Arc::new(create_one_inp_one_out_tx( + Txid::from_byte_array([0; 32]), + 30_000, + )); + let tx2 = Arc::new(create_one_inp_one_out_tx(tx1.compute_txid(), 20_000)); + + let last_seen: BTreeMap = [ + (hash!("B"), 100), + (tx1.compute_txid(), 120), + (tx2.compute_txid(), 121), + ] + .into(); let write_tx = store.db.begin_write().unwrap(); let _ = write_tx.open_table(store.txs_table_defn()).unwrap(); let _ = write_tx.open_table(store.last_seen_defn()).unwrap(); write_tx.commit().unwrap(); - let txs: BTreeSet> = BTreeSet::new(); + let txs: BTreeSet> = [tx1, tx2].into(); let write_tx = store.db.begin_write().unwrap(); let read_tx = store.db.begin_read().unwrap(); - store - .persist_last_seen(&write_tx, &read_tx, &last_seen, &txs) - .unwrap(); + match store.persist_last_seen(&write_tx, &read_tx, &last_seen, &txs) { + Ok(_) => panic!("should give error since tx missing"), + Err(StoreError::TxMissing(txid)) => assert_eq!(txid, hash!("B")), + Err(_) => panic!("error should only be due to missing tx"), + } write_tx.commit().unwrap(); } @@ -1262,15 +1273,24 @@ mod test { } #[test] - #[should_panic] fn test_last_evicted_missing_txs() { // to hit the branch for the panic case in persist_last_evicted let tmpfile = NamedTempFile::new().unwrap(); let db = create_db(tmpfile.path()); let store = create_test_store(Arc::new(db), "wallet1"); - let last_evicted: BTreeMap = - [(hash!("B"), 100), (hash!("D"), 120), (hash!("K"), 132)].into(); + let tx1 = Arc::new(create_one_inp_one_out_tx( + Txid::from_byte_array([0; 32]), + 30_000, + )); + let tx2 = Arc::new(create_one_inp_one_out_tx(tx1.compute_txid(), 20_000)); + + let last_evicted: BTreeMap = [ + (hash!("B"), 100), + (tx1.compute_txid(), 120), + (tx2.compute_txid(), 132), + ] + .into(); let write_tx = store.db.begin_write().unwrap(); let _ = write_tx.open_table(store.txs_table_defn()).unwrap(); @@ -1279,13 +1299,15 @@ mod test { .unwrap(); write_tx.commit().unwrap(); - let txs: BTreeSet> = BTreeSet::new(); + let txs: BTreeSet> = [tx1, tx2].into(); let write_tx = store.db.begin_write().unwrap(); let read_tx = store.db.begin_read().unwrap(); - store - .persist_last_evicted(&write_tx, &read_tx, &last_evicted, &txs) - .unwrap(); + match store.persist_last_evicted(&write_tx, &read_tx, &last_evicted, &txs) { + Ok(_) => panic!("should give error since tx missing"), + Err(StoreError::TxMissing(txid)) => assert_eq!(txid, hash!("B")), + Err(_) => panic!("error should only be due to missing tx"), + } write_tx.commit().unwrap(); } @@ -1357,28 +1379,39 @@ mod test { } #[test] - #[should_panic] fn test_first_seen_missing_tx() { // to hit the branch for the panic case persist_first_seen let tmpfile = NamedTempFile::new().unwrap(); let db = create_db(tmpfile.path()); let store = create_test_store(Arc::new(db), "wallet1"); - let first_seen: BTreeMap = - [(hash!("B"), 100), (hash!("T"), 120), (hash!("C"), 121)].into(); + let tx1 = Arc::new(create_one_inp_one_out_tx( + Txid::from_byte_array([0; 32]), + 30_000, + )); + let tx2 = Arc::new(create_one_inp_one_out_tx(tx1.compute_txid(), 20_000)); + + let first_seen: BTreeMap = [ + (hash!("B"), 100), + (tx1.compute_txid(), 120), + (tx2.compute_txid(), 121), + ] + .into(); let write_tx = store.db.begin_write().unwrap(); let _ = write_tx.open_table(store.txs_table_defn()).unwrap(); let _ = write_tx.open_table(store.first_seen_table_defn()).unwrap(); write_tx.commit().unwrap(); - let txs: BTreeSet> = BTreeSet::new(); + let txs: BTreeSet> = [tx1, tx2].into(); let write_tx = store.db.begin_write().unwrap(); let read_tx = store.db.begin_read().unwrap(); - store - .persist_first_seen(&write_tx, &read_tx, &first_seen, &txs) - .unwrap(); + match store.persist_first_seen(&write_tx, &read_tx, &first_seen, &txs) { + Ok(_) => panic!("should give error since tx missing"), + Err(StoreError::TxMissing(txid)) => assert_eq!(txid, hash!("B")), + Err(_) => panic!("error should only be due to missing tx"), + } write_tx.commit().unwrap(); } @@ -1577,7 +1610,6 @@ mod test { } #[test] - #[should_panic] fn test_anchors_missing_tx() { // to hit the branch for the panic case in persist_anchors let tmpfile = NamedTempFile::new().unwrap(); @@ -1602,7 +1634,11 @@ mod test { let write_tx = store.db.begin_write().unwrap(); let read_tx = store.db.begin_read().unwrap(); - let _ = store.persist_anchors(&write_tx, &read_tx, &anchors_missing_txs, &txs); + match store.persist_anchors(&write_tx, &read_tx, &anchors_missing_txs, &txs) { + Ok(_) => panic!("should give error since tx missing"), + Err(StoreError::TxMissing(txid)) => assert_eq!(txid, hash!("B")), + Err(_) => panic!("error should only be due to missing tx"), + } read_tx.close().unwrap(); write_tx.commit().unwrap(); } From f6b7d15ba4b624d6414c606c0b0d3712d689a066 Mon Sep 17 00:00:00 2001 From: codingp110 Date: Tue, 29 Jul 2025 12:41:42 +0530 Subject: [PATCH 2/3] feat: derive Debug for Store This helps in debugging. Especially in assert statements. --- src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lib.rs b/src/lib.rs index 5c1f7f6..d00ded0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -108,6 +108,7 @@ const NETWORK: TableDefinition<&str, String> = TableDefinition::new("network"); /// This is the primary struct of this crate. It holds the database corresponding to a wallet. /// It also holds the table names of redb tables which are specific to each wallet in a database /// file. +#[derive(Debug)] pub struct Store { // We use a reference so as to avoid taking ownership of the Database, allowing other // applications to write to it. Arc is for thread safety. From 7f40b3845e0bd70bb0f8ed5b9d69f291ca22b5b6 Mon Sep 17 00:00:00 2001 From: codingp110 Date: Tue, 29 Jul 2025 16:19:48 +0530 Subject: [PATCH 3/3] test: add tests to check WalletPersister impl --- Cargo.toml | 2 + src/lib.rs | 323 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 325 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index f3ccbc0..1e51eb4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,8 @@ wallet = ["bdk_wallet"] [dev-dependencies] anyhow = "1.0.98" bdk_testenv = { version = "0.13.0" } +bdk_wallet = {version = "2.0.0", features = ["test-utils"]} +assert_matches = "1.5.0" [lints.rust] unexpected_cfgs = { level = "warn", check-cfg = ['cfg(coverage,coverage_nightly)'] } \ No newline at end of file diff --git a/src/lib.rs b/src/lib.rs index d00ded0..631f73c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -897,6 +897,20 @@ mod test { use bdk_testenv::{block_id, hash, utils}; + #[cfg(feature = "wallet")] + use bdk_wallet::{ + KeychainKind, LoadError, LoadMismatch, LoadWithPersistError, Wallet, + bitcoin::{constants::ChainHash, key::Secp256k1}, + descriptor::IntoWalletDescriptor, + test_utils::{ + get_test_tr_single_sig_xprv, get_test_tr_single_sig_xprv_and_change_desc, + get_test_wpkh, insert_anchor, insert_tx, + }, + }; + + #[cfg(feature = "wallet")] + use assert_matches::assert_matches; + #[cfg(feature = "wallet")] const DESCRIPTORS: [&str; 4] = [ "tr([5940b9b9/86'/0'/0']tpubDDVNqmq75GNPWQ9UNKfP43UwjaHU4GYfoPavojQbfpyfZp2KetWgjGBRRAy4tYCrAA6SB11mhQAkqxjh1VtQHyKwT4oYxpwLaGHvoKmtxZf/1/*)#ypcpw2dr", @@ -2159,4 +2173,313 @@ mod test { store2.read_wallet(&mut changeset_read).unwrap(); assert_eq!(changeset_read, changeset2); } + + #[cfg(feature = "wallet")] + #[test] + fn wallet_is_persisted() { + use bdk_chain::keychain_txout::DEFAULT_LOOKAHEAD; + + let tmpfile = NamedTempFile::new().unwrap(); + let db = Arc::new(create_db(tmpfile.path())); + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + + // create new wallet + let wallet_spk_index = { + let mut store = create_test_store(db.clone(), "wallet"); + let mut wallet = Wallet::create(external_desc, internal_desc) + .network(Network::Testnet) + .use_spk_cache(true) + .create_wallet(&mut store) + .expect("should create wallet"); + wallet.reveal_next_address(KeychainKind::External); + + // persist new wallet changes + assert!( + wallet.persist(&mut store).expect("should persist"), + "must write" + ); + wallet.spk_index().clone() + }; + + // recover wallet + { + let mut store = create_test_store(db.clone(), "wallet"); + let wallet = Wallet::load() + .descriptor(KeychainKind::External, Some(external_desc)) + .descriptor(KeychainKind::Internal, Some(internal_desc)) + .check_network(Network::Testnet) + .load_wallet(&mut store) + .expect("should load wallet") + .expect("wallet must exist"); + + assert_eq!(wallet.network(), Network::Testnet); + assert_eq!( + wallet.spk_index().keychains().collect::>(), + wallet_spk_index.keychains().collect::>() + ); + assert_eq!( + wallet.spk_index().last_revealed_indices(), + wallet_spk_index.last_revealed_indices() + ); + let secp = Secp256k1::new(); + assert_eq!( + *wallet.public_descriptor(KeychainKind::External), + external_desc + .into_wallet_descriptor(&secp, wallet.network()) + .unwrap() + .0 + ); + } + // Test SPK cache + { + let mut store = create_test_store(db.clone(), "wallet"); + let mut wallet = Wallet::load() + .check_network(Network::Testnet) + .use_spk_cache(true) + .load_wallet(&mut store) + .expect("should load wallet") + .expect("wallet must exist"); + + let external_did = wallet + .public_descriptor(KeychainKind::External) + .descriptor_id(); + let internal_did = wallet + .public_descriptor(KeychainKind::Internal) + .descriptor_id(); + + assert!(wallet.staged().is_none()); + + let _addr = wallet.reveal_next_address(KeychainKind::External); + let cs = wallet.staged().expect("we should have staged a changeset"); + assert!(!cs.indexer.spk_cache.is_empty(), "failed to cache spks"); + assert_eq!(cs.indexer.spk_cache.len(), 2, "we persisted two keychains"); + let spk_cache: &BTreeMap = + cs.indexer.spk_cache.get(&external_did).unwrap(); + assert_eq!(spk_cache.len() as u32, 1 + 1 + DEFAULT_LOOKAHEAD); + assert_eq!(spk_cache.keys().last(), Some(&26)); + let spk_cache = cs.indexer.spk_cache.get(&internal_did).unwrap(); + assert_eq!(spk_cache.len() as u32, DEFAULT_LOOKAHEAD); + assert_eq!(spk_cache.keys().last(), Some(&24)); + // Clear the stage + let _ = wallet.take_staged(); + let _addr = wallet.reveal_next_address(KeychainKind::Internal); + let cs = wallet.staged().unwrap(); + assert_eq!(cs.indexer.spk_cache.len(), 1); + let spk_cache = cs.indexer.spk_cache.get(&internal_did).unwrap(); + assert_eq!(spk_cache.len(), 1); + assert_eq!(spk_cache.keys().next(), Some(&25)); + } + // SPK cache requires load params + { + let mut store = create_test_store(db.clone(), "wallet"); + let mut wallet = Wallet::load() + .check_network(Network::Testnet) + // .use_spk_cache(false) + .load_wallet(&mut store) + .expect("should load wallet") + .expect("wallet must exist"); + + let internal_did = wallet + .public_descriptor(KeychainKind::Internal) + .descriptor_id(); + + assert!(wallet.staged().is_none()); + + let _addr = wallet.reveal_next_address(KeychainKind::Internal); + let cs = wallet.staged().expect("we should have staged a changeset"); + assert_eq!(cs.indexer.last_revealed.get(&internal_did), Some(&0)); + assert!( + cs.indexer.spk_cache.is_empty(), + "we didn't set `use_spk_cache`" + ); + } + } + + #[cfg(feature = "wallet")] + #[test] + fn wallet_load_checks() { + let tmpfile = NamedTempFile::new().unwrap(); + let db = Arc::new(create_db(tmpfile.path())); + let network = Network::Testnet; + let (external_desc, internal_desc) = get_test_tr_single_sig_xprv_and_change_desc(); + + // create new wallet + + let _ = Wallet::create(external_desc, internal_desc) + .network(network) + .create_wallet(&mut create_test_store(db.clone(), "wallet")) + .expect("wallet should get created"); + + assert_matches!( + Wallet::load() + .check_network(Network::Regtest) + .load_wallet(&mut create_test_store(db.clone(), "wallet")), + Err(LoadWithPersistError::InvalidChangeSet(LoadError::Mismatch( + LoadMismatch::Network { + loaded: Network::Testnet, + expected: Network::Regtest, + } + ))), + "unexpected network check result: Regtest (check) is not Testnet (loaded)", + ); + + let mainnet_hash = BlockHash::from_byte_array(ChainHash::BITCOIN.to_bytes()); + assert_matches!( + Wallet::load() + .check_genesis_hash(mainnet_hash) + .load_wallet(&mut create_test_store(db.clone(), "wallet")), + Err(LoadWithPersistError::InvalidChangeSet(LoadError::Mismatch( + LoadMismatch::Genesis { .. } + ))), + "unexpected genesis hash check result: mainnet hash (check) is not testnet hash (loaded)", + ); + assert_matches!( + Wallet::load() + .descriptor(KeychainKind::External, Some(internal_desc)) + .load_wallet(&mut create_test_store(db.clone(), "wallet")), + Err(LoadWithPersistError::InvalidChangeSet(LoadError::Mismatch( + LoadMismatch::Descriptor { .. } + ))), + "unexpected descriptors check result", + ); + assert_matches!( + Wallet::load() + .descriptor(KeychainKind::External, Option::<&str>::None) + .load_wallet(&mut create_test_store(db.clone(), "wallet")), + Err(LoadWithPersistError::InvalidChangeSet(LoadError::Mismatch( + LoadMismatch::Descriptor { .. } + ))), + "unexpected descriptors check result", + ); + // check setting keymaps + let (_, external_keymap) = + >::parse_descriptor(&Secp256k1::new(), external_desc) + .expect("failed to parse descriptor"); + let (_, internal_keymap) = + >::parse_descriptor(&Secp256k1::new(), internal_desc) + .expect("failed to parse descriptor"); + let wallet = Wallet::load() + .keymap(KeychainKind::External, external_keymap) + .keymap(KeychainKind::Internal, internal_keymap) + .load_wallet(&mut create_test_store(db.clone(), "wallet")) + .expect("db should not fail") + .expect("wallet was persisted"); + for keychain in [KeychainKind::External, KeychainKind::Internal] { + let keymap = wallet.get_signers(keychain).as_key_map(wallet.secp_ctx()); + assert!( + !keymap.is_empty(), + "load should populate keymap for keychain {keychain:?}" + ); + } + } + + #[cfg(feature = "wallet")] + #[test] + fn wallet_should_persist_anchors_and_recover() { + use bdk_chain::ChainPosition; + let tmpfile = NamedTempFile::new().unwrap(); + let db = Arc::new(create_db(tmpfile.path())); + let mut store = create_test_store(db.clone(), "wallet"); + + let desc = get_test_tr_single_sig_xprv(); + let mut wallet = Wallet::create_single(desc) + .network(Network::Testnet) + .create_wallet(&mut store) + .unwrap(); + + let small_output_tx = Transaction { + input: vec![], + output: vec![TxOut { + script_pubkey: wallet + .next_unused_address(KeychainKind::External) + .script_pubkey(), + value: Amount::from_sat(25_000), + }], + version: transaction::Version::non_standard(0), + lock_time: absolute::LockTime::ZERO, + }; + let txid = small_output_tx.compute_txid(); + insert_tx(&mut wallet, small_output_tx); + let expected_anchor = ConfirmationBlockTime { + block_id: wallet.latest_checkpoint().block_id(), + confirmation_time: 200, + }; + insert_anchor(&mut wallet, txid, expected_anchor); + assert!(wallet.persist(&mut store).unwrap()); + + // should recover persisted wallet + let secp = wallet.secp_ctx(); + let (_, keymap) = >::parse_descriptor(secp, desc).unwrap(); + assert!(!keymap.is_empty()); + let wallet = Wallet::load() + .descriptor(KeychainKind::External, Some(desc)) + .extract_keys() + .load_wallet(&mut store) + .unwrap() + .expect("must have loaded changeset"); + // stored anchor should be retrieved in the same condition it was persisted + if let ChainPosition::Confirmed { + anchor: obtained_anchor, + .. + } = wallet + .get_tx(txid) + .expect("should retrieve stored tx") + .chain_position + { + assert_eq!(obtained_anchor, expected_anchor) + } else { + panic!("Should have got ChainPosition::Confirmed)"); + } + } + + #[cfg(feature = "wallet")] + #[test] + fn single_descriptor_wallet_persist_and_recover() { + use bdk_chain::miniscript::Descriptor; + use bdk_chain::miniscript::DescriptorPublicKey; + + let tmpfile = NamedTempFile::new().unwrap(); + let db = Arc::new(create_db(tmpfile.path())); + let mut store = create_test_store(db.clone(), "wallet"); + + let desc = get_test_tr_single_sig_xprv(); + let mut wallet = Wallet::create_single(desc) + .network(Network::Testnet) + .create_wallet(&mut store) + .unwrap(); + let _ = wallet.reveal_addresses_to(KeychainKind::External, 2); + assert!(wallet.persist(&mut store).unwrap()); + + // should recover persisted wallet + let secp = wallet.secp_ctx(); + let (_, keymap) = >::parse_descriptor(secp, desc).unwrap(); + assert!(!keymap.is_empty()); + let wallet = Wallet::load() + .descriptor(KeychainKind::External, Some(desc)) + .extract_keys() + .load_wallet(&mut store) + .unwrap() + .expect("must have loaded changeset"); + assert_eq!(wallet.derivation_index(KeychainKind::External), Some(2)); + // should have private key + assert_eq!( + wallet.get_signers(KeychainKind::External).as_key_map(secp), + keymap, + ); + + // should error on wrong internal params + let desc = get_test_wpkh(); + let (exp_desc, _) = + >::parse_descriptor(secp, desc).unwrap(); + let err = Wallet::load() + .descriptor(KeychainKind::Internal, Some(desc)) + .extract_keys() + .load_wallet(&mut store); + assert_matches!( + err, + Err(LoadWithPersistError::InvalidChangeSet(LoadError::Mismatch(LoadMismatch::Descriptor { keychain, loaded, expected }))) + if keychain == KeychainKind::Internal && loaded.is_none() && expected == Some(exp_desc), + "single descriptor wallet should refuse change descriptor param" + ); + } }