Merge #1292: fix(store): Remove lifetime

e6433fb feat(persist): Add stage_and_commit to Persist (LLFourn)
0bee46e fix(store): Remove lifetime (LLFourn)

Pull request description:

  Remove gratuitous use of lifetimes in the main persistence struct `Store`. Having lifetimes on this means that you have to keep the magic bytes alive longer than the database which is particularly offensive if you have to send the database to another thread. On top of that the bytes aren't even read.

ACKs for top commit:
  evanlinjin:
    ACK e6433fb

Tree-SHA512: 7f6d9d60951a8ceaee30719d0771e15632c6fad0702294af15409c5df492669a07299874ef5ee34e3d75bdecbbd41df29bced3ff16b2360d5d5c7687ef677ffc

Author: evanlinjin

crates/chain/src/persist.rs

@@ -55,6 +55,18 @@ where
             // if written successfully, take and return `self.stage`
             .map(|_| Some(core::mem::take(&mut self.stage)))
     }
+
+    /// Stages a new changeset and commits it (along with any other previously staged changes) to
+    /// the persistence backend
+    ///
+    /// Convience method for calling [`stage`] and then [`commit`].
+    ///
+    /// [`stage`]: Self::stage
+    /// [`commit`]: Self::commit
+    pub fn stage_and_commit(&mut self, changeset: C) -> Result<Option<C>, B::WriteError> {
+        self.stage(changeset);
+        self.commit()
+    }
 }
 
 /// A persistence backend for [`Persist`].

crates/file_store/src/store.rs

@@ -15,13 +15,13 @@ use crate::{bincode_options, EntryIter, FileError, IterError};
 ///
 /// The changesets are the results of altering a tracker implementation (`T`).
 #[derive(Debug)]
-pub struct Store<'a, C> {
-    magic: &'a [u8],
+pub struct Store<C> {
+    magic_len: usize,
     db_file: File,
     marker: PhantomData<C>,
 }
 
-impl<'a, C> PersistBackend<C> for Store<'a, C>
+impl<C> PersistBackend<C> for Store<C>
 where
     C: Append + serde::Serialize + serde::de::DeserializeOwned,
 {
@@ -38,7 +38,7 @@ where
     }
 }
 
-impl<'a, C> Store<'a, C>
+impl<C> Store<C>
 where
     C: Append + serde::Serialize + serde::de::DeserializeOwned,
 {
@@ -48,7 +48,7 @@ where
     /// the `Store` in the future with [`open`].
     ///
     /// [`open`]: Store::open
-    pub fn create_new<P>(magic: &'a [u8], file_path: P) -> Result<Self, FileError>
+    pub fn create_new<P>(magic: &[u8], file_path: P) -> Result<Self, FileError>
     where
         P: AsRef<Path>,
     {
@@ -67,7 +67,7 @@ where
             .open(file_path)?;
         f.write_all(magic)?;
         Ok(Self {
-            magic,
+            magic_len: magic.len(),
             db_file: f,
             marker: Default::default(),
         })
@@ -83,7 +83,7 @@ where
     /// [`FileError::InvalidMagicBytes`] error variant will be returned.
     ///
     /// [`create_new`]: Store::create_new
-    pub fn open<P>(magic: &'a [u8], file_path: P) -> Result<Self, FileError>
+    pub fn open<P>(magic: &[u8], file_path: P) -> Result<Self, FileError>
     where
         P: AsRef<Path>,
     {
@@ -99,7 +99,7 @@ where
         }
 
         Ok(Self {
-            magic,
+            magic_len: magic.len(),
             db_file: f,
             marker: Default::default(),
         })
@@ -111,7 +111,7 @@ where
     ///
     /// [`open`]: Store::open
     /// [`create_new`]: Store::create_new
-    pub fn open_or_create_new<P>(magic: &'a [u8], file_path: P) -> Result<Self, FileError>
+    pub fn open_or_create_new<P>(magic: &[u8], file_path: P) -> Result<Self, FileError>
     where
         P: AsRef<Path>,
     {
@@ -132,7 +132,7 @@ where
     /// always iterate over all entries until `None` is returned if you want your next write to go
     /// at the end; otherwise, you will write over existing entries.
     pub fn iter_changesets(&mut self) -> EntryIter<C> {
-        EntryIter::new(self.magic.len() as u64, &mut self.db_file)
+        EntryIter::new(self.magic_len as u64, &mut self.db_file)
     }
 
     /// Loads all the changesets that have been stored as one giant changeset.

example-crates/example_bitcoind_rpc_polling/src/main.rs

@@ -147,7 +147,7 @@ fn main() -> anyhow::Result<()> {
     let rpc_cmd = match args.command {
         example_cli::Commands::ChainSpecific(rpc_cmd) => rpc_cmd,
         general_cmd => {
-            let res = example_cli::handle_commands(
+            return example_cli::handle_commands(
                 &graph,
                 &db,
                 &chain,
@@ -160,8 +160,6 @@ fn main() -> anyhow::Result<()> {
                 },
                 general_cmd,
             );
-            db.lock().unwrap().commit()?;
-            return res;
         }
     };
 

example-crates/example_cli/src/lib.rs

@@ -29,7 +29,7 @@ pub type KeychainChangeSet<A> = (
     local_chain::ChangeSet,
     indexed_tx_graph::ChangeSet<A, keychain::ChangeSet<Keychain>>,
 );
-pub type Database<'m, C> = Persist<Store<'m, C>, C>;
+pub type Database<C> = Persist<Store<C>, C>;
 
 #[derive(Parser)]
 #[clap(author, version, about, long_about = None)]
@@ -457,11 +457,10 @@ where
 
                     let ((spk_i, spk), index_changeset) = spk_chooser(index, &Keychain::External);
                     let db = &mut *db.lock().unwrap();
-                    db.stage(C::from((
+                    db.stage_and_commit(C::from((
                         local_chain::ChangeSet::default(),
                         indexed_tx_graph::ChangeSet::from(index_changeset),
-                    )));
-                    db.commit()?;
+                    )))?;
                     let addr =
                         Address::from_script(spk, network).context("failed to derive address")?;
                     println!("[address @ {}] {}", spk_i, addr);
@@ -601,11 +600,10 @@ where
                     // If we're unable to persist this, then we don't want to broadcast.
                     {
                         let db = &mut *db.lock().unwrap();
-                        db.stage(C::from((
+                        db.stage_and_commit(C::from((
                             local_chain::ChangeSet::default(),
                             indexed_tx_graph::ChangeSet::from(index_changeset),
-                        )));
-                        db.commit()?;
+                        )))?;
                     }
 
                     // We don't want other callers/threads to use this address while we're using it
@@ -627,10 +625,10 @@ where
                     // We know the tx is at least unconfirmed now. Note if persisting here fails,
                     // it's not a big deal since we can always find it again form
                     // blockchain.
-                    db.lock().unwrap().stage(C::from((
+                    db.lock().unwrap().stage_and_commit(C::from((
                         local_chain::ChangeSet::default(),
                         keychain_changeset,
-                    )));
+                    )))?;
                     Ok(())
                 }
                 Err(e) => {
@@ -646,14 +644,14 @@ where
 }
 
 #[allow(clippy::type_complexity)]
-pub fn init<'m, CS: clap::Subcommand, S: clap::Args, C>(
-    db_magic: &'m [u8],
+pub fn init<CS: clap::Subcommand, S: clap::Args, C>(
+    db_magic: &[u8],
     db_default_path: &str,
 ) -> anyhow::Result<(
     Args<CS, S>,
     KeyMap,
     KeychainTxOutIndex<Keychain>,
-    Mutex<Database<'m, C>>,
+    Mutex<Database<C>>,
     C,
 )>
 where
@@ -681,7 +679,7 @@ where
         index.add_keychain(Keychain::Internal, internal_descriptor);
     }
 
-    let mut db_backend = match Store::<'m, C>::open_or_create_new(db_magic, &args.db_path) {
+    let mut db_backend = match Store::<C>::open_or_create_new(db_magic, &args.db_path) {
         Ok(db_backend) => db_backend,
         // we cannot return `err` directly as it has lifetime `'m`
         Err(err) => return Err(anyhow::anyhow!("failed to init db backend: {:?}", err)),

example-crates/example_electrum/src/main.rs

@@ -122,7 +122,7 @@ fn main() -> anyhow::Result<()> {
     let electrum_cmd = match &args.command {
         example_cli::Commands::ChainSpecific(electrum_cmd) => electrum_cmd,
         general_cmd => {
-            let res = example_cli::handle_commands(
+            return example_cli::handle_commands(
                 &graph,
                 &db,
                 &chain,
@@ -135,9 +135,6 @@ fn main() -> anyhow::Result<()> {
                 },
                 general_cmd.clone(),
             );
-
-            db.lock().unwrap().commit()?;
-            return res;
         }
     };
 

example-crates/example_esplora/src/main.rs

@@ -125,7 +125,7 @@ fn main() -> anyhow::Result<()> {
         example_cli::Commands::ChainSpecific(esplora_cmd) => esplora_cmd,
         // These are general commands handled by example_cli. Execute the cmd and return.
         general_cmd => {
-            let res = example_cli::handle_commands(
+            return example_cli::handle_commands(
                 &graph,
                 &db,
                 &chain,
@@ -140,9 +140,6 @@ fn main() -> anyhow::Result<()> {
                 },
                 general_cmd.clone(),
             );
-
-            db.lock().unwrap().commit()?;
-            return res;
         }
     };