Skip to content

Simplify wallet persistence #1547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
notmandatory merged 6 commits into from
Aug 19, 2024
Merged

Simplify wallet persistence #1547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
039622f
feat(wallet)!: introduce `WalletPersister`
evanlinjin Aug 9, 2024
06a9d6c
feat(chain,wallet)!: publicize `.init_sqlite_tables` changeset methods
evanlinjin Aug 11, 2024
a9c5f76
feat(wallet)!: remove dependency on `bdk_chain::Staged`
evanlinjin Aug 11, 2024
0616057
revert(chain)!: rm `persit` module
evanlinjin Aug 11, 2024
9600293
feat(wallet)!: add persister (`P`) type param to `PersistedWallet<P>`
evanlinjin Aug 15, 2024
340808e
docs(wallet): fixes/improvements for `persisted` and `params` types
evanlinjin Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions crates/chain/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ pub use tx_data_traits::*;
pub use tx_graph::TxGraph;
mod chain_oracle;
pub use chain_oracle::*;
mod persist;
pub use persist::*;

#[doc(hidden)]
pub mod example_utils;
Expand Down
169 changes: 0 additions & 169 deletions crates/chain/src/persist.rs
View file
Open in desktop

This file was deleted.

30 changes: 15 additions & 15 deletions crates/chain/src/rusqlite_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ where
pub const ANCHORS_TABLE_NAME: &'static str = "bdk_anchors";

/// Initialize sqlite tables.
fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> {
pub fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> {
let schema_v0: &[&str] = &[
// full transactions
&format!(
Expand Down Expand Up @@ -264,9 +264,9 @@ where
}

/// Construct a [`TxGraph`] from an sqlite database.
///
/// Remember to call [`Self::init_sqlite_tables`] beforehand.
pub fn from_sqlite(db_tx: &rusqlite::Transaction) -> rusqlite::Result<Self> {
Self::init_sqlite_tables(db_tx)?;

let mut changeset = Self::default();

let mut statement = db_tx.prepare(&format!(
Expand Down Expand Up @@ -332,9 +332,9 @@ where
}

/// Persist `changeset` to the sqlite database.
///
/// Remember to call [`Self::init_sqlite_tables`] beforehand.
pub fn persist_to_sqlite(&self, db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> {
Self::init_sqlite_tables(db_tx)?;

let mut statement = db_tx.prepare_cached(&format!(
"INSERT INTO {}(txid, raw_tx) VALUES(:txid, :raw_tx) ON CONFLICT(txid) DO UPDATE SET raw_tx=:raw_tx",
Self::TXS_TABLE_NAME,
Expand Down Expand Up @@ -396,7 +396,7 @@ impl local_chain::ChangeSet {
pub const BLOCKS_TABLE_NAME: &'static str = "bdk_blocks";

/// Initialize sqlite tables for persisting [`local_chain::LocalChain`].
fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> {
pub fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> {
let schema_v0: &[&str] = &[
// blocks
&format!(
Expand All @@ -411,9 +411,9 @@ impl local_chain::ChangeSet {
}

/// Construct a [`LocalChain`](local_chain::LocalChain) from sqlite database.
///
/// Remember to call [`Self::init_sqlite_tables`] beforehand.
pub fn from_sqlite(db_tx: &rusqlite::Transaction) -> rusqlite::Result<Self> {
Self::init_sqlite_tables(db_tx)?;

let mut changeset = Self::default();

let mut statement = db_tx.prepare(&format!(
Expand All @@ -435,9 +435,9 @@ impl local_chain::ChangeSet {
}

/// Persist `changeset` to the sqlite database.
///
/// Remember to call [`Self::init_sqlite_tables`] beforehand.
pub fn persist_to_sqlite(&self, db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> {
Self::init_sqlite_tables(db_tx)?;

let mut replace_statement = db_tx.prepare_cached(&format!(
"REPLACE INTO {}(block_height, block_hash) VALUES(:block_height, :block_hash)",
Self::BLOCKS_TABLE_NAME,
Expand Down Expand Up @@ -471,7 +471,7 @@ impl keychain_txout::ChangeSet {

/// Initialize sqlite tables for persisting
/// [`KeychainTxOutIndex`](keychain_txout::KeychainTxOutIndex).
fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> {
pub fn init_sqlite_tables(db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> {
let schema_v0: &[&str] = &[
// last revealed
&format!(
Expand All @@ -487,9 +487,9 @@ impl keychain_txout::ChangeSet {

/// Construct [`KeychainTxOutIndex`](keychain_txout::KeychainTxOutIndex) from sqlite database
/// and given parameters.
///
/// Remember to call [`Self::init_sqlite_tables`] beforehand.
pub fn from_sqlite(db_tx: &rusqlite::Transaction) -> rusqlite::Result<Self> {
Self::init_sqlite_tables(db_tx)?;

let mut changeset = Self::default();

let mut statement = db_tx.prepare(&format!(
Expand All @@ -511,9 +511,9 @@ impl keychain_txout::ChangeSet {
}

/// Persist `changeset` to the sqlite database.
///
/// Remember to call [`Self::init_sqlite_tables`] beforehand.
pub fn persist_to_sqlite(&self, db_tx: &rusqlite::Transaction) -> rusqlite::Result<()> {
Self::init_sqlite_tables(db_tx)?;

let mut statement = db_tx.prepare_cached(&format!(
"REPLACE INTO {}(descriptor_id, last_revealed) VALUES(:descriptor_id, :last_revealed)",
Self::LAST_REVEALED_TABLE_NAME,
Expand Down
16 changes: 9 additions & 7 deletions crates/wallet/src/wallet/changeset.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,8 @@ impl ChangeSet {
/// Name of table to store wallet descriptors and network.
pub const WALLET_TABLE_NAME: &'static str = "bdk_wallet";

/// Initialize sqlite tables for wallet schema & table.
fn init_wallet_sqlite_tables(
db_tx: &chain::rusqlite::Transaction,
) -> chain::rusqlite::Result<()> {
/// Initialize sqlite tables for wallet tables.
pub fn init_sqlite_tables(db_tx: &chain::rusqlite::Transaction) -> chain::rusqlite::Result<()> {
let schema_v0: &[&str] = &[&format!(
"CREATE TABLE {} ( \
id INTEGER PRIMARY KEY NOT NULL CHECK (id = 0), \
Expand All @@ -85,12 +83,17 @@ impl ChangeSet {
) STRICT;",
Self::WALLET_TABLE_NAME,
)];
crate::rusqlite_impl::migrate_schema(db_tx, Self::WALLET_SCHEMA_NAME, &[schema_v0])
crate::rusqlite_impl::migrate_schema(db_tx, Self::WALLET_SCHEMA_NAME, &[schema_v0])?;

bdk_chain::local_chain::ChangeSet::init_sqlite_tables(db_tx)?;
bdk_chain::tx_graph::ChangeSet::<ConfirmationBlockTime>::init_sqlite_tables(db_tx)?;
bdk_chain::keychain_txout::ChangeSet::init_sqlite_tables(db_tx)?;

Ok(())
}

/// Recover a [`ChangeSet`] from sqlite database.
pub fn from_sqlite(db_tx: &chain::rusqlite::Transaction) -> chain::rusqlite::Result<Self> {
Self::init_wallet_sqlite_tables(db_tx)?;
use chain::rusqlite::OptionalExtension;
use chain::Impl;

Expand Down Expand Up @@ -129,7 +132,6 @@ impl ChangeSet {
&self,
db_tx: &chain::rusqlite::Transaction,
) -> chain::rusqlite::Result<()> {
Self::init_wallet_sqlite_tables(db_tx)?;
use chain::rusqlite::named_params;
use chain::Impl;

Expand Down
20 changes: 10 additions & 10 deletions crates/wallet/src/wallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ use bitcoin::{
use bitcoin::{consensus::encode::serialize, transaction, BlockHash, Psbt};
use bitcoin::{constants::genesis_block, Amount};
use bitcoin::{secp256k1::Secp256k1, Weight};
use chain::Staged;
use core::fmt;
use core::mem;
use core::ops::Deref;
Expand Down Expand Up @@ -123,14 +122,6 @@ pub struct Wallet {
secp: SecpCtx,
}

impl Staged for Wallet {
type ChangeSet = ChangeSet;

fn staged(&mut self) -> &mut Self::ChangeSet {
&mut self.stage
}
}

/// An update to [`Wallet`].
///
/// It updates [`KeychainTxOutIndex`], [`bdk_chain::TxGraph`] and [`local_chain::LocalChain`] atomically.
Expand Down Expand Up @@ -2303,7 +2294,7 @@ impl Wallet {
Ok(())
}

/// Get a reference of the staged [`ChangeSet`] that are yet to be committed (if any).
/// Get a reference of the staged [`ChangeSet`] that is yet to be committed (if any).
pub fn staged(&self) -> Option<&ChangeSet> {
if self.stage.is_empty() {
None
Expand All @@ -2312,6 +2303,15 @@ impl Wallet {
}
}

/// Get a mutable reference of the staged [`ChangeSet`] that is yet to be commited (if any).
pub fn staged_mut(&mut self) -> Option<&mut ChangeSet> {
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn staged_mut(&mut self) -> Option<&mut ChangeSet> {
pub(crate) fn staged_mut(&mut self) -> Option<&mut ChangeSet> {

Copy link
Copy Markdown
Member Author

@evanlinjin evanlinjin Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that this method may be useful for some users. I.e. they don't need to query the staged area twice if they wanted to do something like we do in PersistedWallet::persist.

Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of discussion I think you can do it without ever getting the stage mutably such as having a method on Wallet that resets the stage but doesn't return it, and this would prevent users from freely modifying their changesets.

  /// Clears the stage.
  pub fn clear_stage(&mut self) { self.stage = ChangeSet::default(); }

Then persist would look like

  /// Persist staged changes.
  pub fn persist(&mut self, persister: &mut P) -> Result<bool, P::Error> {
      if let Some(change) = self.inner.staged() {
          P::persist(persister, change)?;
          self.inner.clear_stage();
          return Ok(true);
      }
      Ok(false)
  }

Copy link
Copy Markdown
Collaborator

@oleonardolima oleonardolima Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I think it's a better approach to have/use a clear_stage method, having explicit and clear meaning.

if self.stage.is_empty() {
None
} else {
Some(&mut self.stage)
}
}

/// Take the staged [`ChangeSet`] to be persisted now (if any).
pub fn take_staged(&mut self) -> Option<ChangeSet> {
self.stage.take()
Expand Down
Loading