bitcoind: make the rescan starting interface failible

This makes us more robust to races, where we'd crash previously

src/bitcoin/d/mod.rs

@@ -42,6 +42,7 @@ pub enum BitcoindError {
     InvalidVersion(u64),
     NetworkMismatch(String /*config*/, String /*bitcoind*/),
     MissingDescriptor,
+    AlreadyRescanning,
 }
 
 impl BitcoindError {
@@ -113,6 +114,9 @@ impl std::fmt::Display for BitcoindError {
             BitcoindError::MissingDescriptor => {
                 write!(f, "The watchonly wallet loaded on bitcoind does not have the main descriptor imported.")
             }
+            BitcoindError::AlreadyRescanning => {
+                write!(f, "A rescan is already ongoing for the watchonly wallet.")
+            }
         }
     }
 }
@@ -780,10 +784,15 @@ impl BitcoinD {
         Ok(())
     }
 
-    pub fn start_rescan(&self, desc: &MultipathDescriptor, timestamp: u32) {
+    pub fn start_rescan(
+        &self,
+        desc: &MultipathDescriptor,
+        timestamp: u32,
+    ) -> Result<(), BitcoindError> {
         // The wallet must not be already rescanning
-        // FIXME: don't assert, propagate an error instead.
-        assert!(self.rescan_progress().is_none());
+        if self.rescan_progress().is_some() {
+            return Err(BitcoindError::AlreadyRescanning);
+        }
 
         // Re-import the receive and change descriptors to the watchonly wallet for the purpose of
         // rescanning.
@@ -815,7 +824,7 @@ impl BitcoinD {
         // error and bitcoind will keep the new timestamps for the descriptors as if it had
         // successfully rescanned them.
         match self.make_noreply_request("importdescriptors", &params!(Json::Array(descriptors))) {
-            Ok(()) => {}
+            Ok(()) => Ok(()),
             Err(e) => {
                 // We checked at the beginning it was not already rescanning. If we get an error
                 // because of that it's just a spurious error from calling it multiple times in the
@@ -823,12 +832,9 @@ impl BitcoinD {
                 if !e.to_string().contains("is currently rescanning.") {
                     // However, if it's not an error because we triggered it twice due to a
                     // flickering connection fail as it shouldn't happen.
-                    // TODO: propagate the error back to the RPC command. There are too many
-                    // reasons this could fail that we shouldn't be crashing for that.
-                    panic!(
-                        "Unexpected error when starting rescan on bitcoind wallet: {}",
-                        e
-                    );
+                    Err(e)
+                } else {
+                    Ok(())
                 }
             }
         }

src/bitcoin/mod.rs

@@ -76,7 +76,11 @@ pub trait BitcoinInterface: Send {
 
     /// Trigger a rescan of the block chain for transactions related to this descriptor since
     /// the given date.
-    fn start_rescan(&self, desc: &descriptors::MultipathDescriptor, timestamp: u32);
+    fn start_rescan(
+        &self,
+        desc: &descriptors::MultipathDescriptor,
+        timestamp: u32,
+    ) -> Result<(), String>;
 
     /// Rescan progress percentage. Between 0 and 1.
     fn rescan_progress(&self) -> Option<f64>;
@@ -283,9 +287,14 @@ impl BitcoinInterface for d::BitcoinD {
         }
     }
 
-    fn start_rescan(&self, desc: &descriptors::MultipathDescriptor, timestamp: u32) {
+    fn start_rescan(
+        &self,
+        desc: &descriptors::MultipathDescriptor,
+        timestamp: u32,
+    ) -> Result<(), String> {
         // FIXME: in theory i think this could potentially fail to actually start the rescan.
-        self.start_rescan(desc, timestamp);
+        self.start_rescan(desc, timestamp)
+            .map_err(|e| e.to_string())
     }
 
     fn rescan_progress(&self) -> Option<f64> {
@@ -357,7 +366,11 @@ impl BitcoinInterface for sync::Arc<sync::Mutex<dyn BitcoinInterface + 'static>>
         self.lock().unwrap().broadcast_tx(tx)
     }
 
-    fn start_rescan(&self, desc: &descriptors::MultipathDescriptor, timestamp: u32) {
+    fn start_rescan(
+        &self,
+        desc: &descriptors::MultipathDescriptor,
+        timestamp: u32,
+    ) -> Result<(), String> {
         self.lock().unwrap().start_rescan(desc, timestamp)
     }
 

src/commands/mod.rs

@@ -61,6 +61,8 @@ pub enum CommandError {
     TxBroadcast(String),
     AlreadyRescanning,
     InsaneRescanTimestamp(u32),
+    /// An error that might occur in the racy rescan triggering logic.
+    RescanTrigger(String),
 }
 
 impl fmt::Display for CommandError {
@@ -92,6 +94,7 @@ impl fmt::Display for CommandError {
                 "There is already a rescan ongoing. Please wait for it to complete first."
             ),
             Self::InsaneRescanTimestamp(t) => write!(f, "Insane timestamp '{}'.", t),
+            Self::RescanTrigger(s) => write!(f, "Error while starting rescan: '{}'", s),
         }
     }
 }
@@ -521,7 +524,8 @@ impl DaemonControl {
         // rescanning. This could make us crash with the bitcoind backend if someone triggered a
         // rescan of the wallet just after we checked above and did now.
         self.bitcoin
-            .start_rescan(&self.config.main_descriptor, timestamp);
+            .start_rescan(&self.config.main_descriptor, timestamp)
+            .map_err(CommandError::RescanTrigger)?;
         db_conn.set_rescan(timestamp);
 
         Ok(())

src/jsonrpc/mod.rs

@@ -164,7 +164,8 @@ impl From<commands::CommandError> for Error {
             | commands::CommandError::AlreadyRescanning => {
                 Error::new(ErrorCode::InvalidParams, e.to_string())
             }
-            commands::CommandError::SanityCheckFailure(_) => {
+            commands::CommandError::SanityCheckFailure(_)
+            | commands::CommandError::RescanTrigger(..) => {
                 Error::new(ErrorCode::InternalError, e.to_string())
             }
             commands::CommandError::TxBroadcast(_) => {

src/testutils.rs

@@ -75,7 +75,7 @@ impl BitcoinInterface for DummyBitcoind {
         todo!()
     }
 
-    fn start_rescan(&self, _: &descriptors::MultipathDescriptor, _: u32) {
+    fn start_rescan(&self, _: &descriptors::MultipathDescriptor, _: u32) -> Result<(), String> {
         todo!()
     }