Merge pull request fedimint#6099 from dpc/24-09-27-recover-mint-test-…

…detect-burn

chore(client): during recovery detect burned ecash due to nonce reuse

modules/fedimint-mint-client/src/backup/recovery.rs

@@ -12,7 +12,7 @@ use fedimint_core::{
     apply, async_trait_maybe_send, Amount, NumPeersExt, OutPoint, PeerId, Tiered, TieredMulti,
 };
 use fedimint_derive_secret::DerivableSecret;
-use fedimint_logging::{LOG_CLIENT_MODULE_MINT, LOG_CLIENT_RECOVERY_MINT};
+use fedimint_logging::{LOG_CLIENT_MODULE_MINT, LOG_CLIENT_RECOVERY, LOG_CLIENT_RECOVERY_MINT};
 use fedimint_mint_common::{MintInput, MintOutput, Nonce};
 use serde::{Deserialize, Serialize};
 use tbs::{AggregatePublicKey, BlindedMessage, PublicKeyShare};
@@ -29,7 +29,7 @@ use crate::{MintClientInit, MintClientModule, MintClientStateMachines, NoteIndex
 
 #[derive(Clone, Debug)]
 pub struct MintRecovery {
-    state: MintRecoveryState,
+    state: MintRecoveryStateV0,
     secret: DerivableSecret,
     client_ctx: ClientContext<MintClientModule>,
 }
@@ -63,7 +63,7 @@ impl RecoveryFromHistory for MintRecovery {
 
         Ok((
             MintRecovery {
-                state: MintRecoveryState::from_backup(
+                state: MintRecoveryStateV0::from_backup(
                     snapshot,
                     100,
                     config.tbs_pks.clone(),
@@ -85,6 +85,14 @@ impl RecoveryFromHistory for MintRecovery {
         Ok(dbtx
             .get_value(&RecoveryStateKey)
             .await
+            .and_then(|(state, common)| {
+                if let MintRecoveryState::V1(state) = state {
+                    Some((state, common))
+                } else {
+                    warn!(target: LOG_CLIENT_RECOVERY, "Found unknown version recovery state. Ignoring");
+                    None
+                }
+            })
             .map(|(state, common)| {
                 (
                     MintRecovery {
@@ -102,8 +110,11 @@ impl RecoveryFromHistory for MintRecovery {
         dbtx: &mut DatabaseTransaction<'_>,
         common: &RecoveryFromHistoryCommon,
     ) {
-        dbtx.insert_entry(&RecoveryStateKey, &(self.state.clone(), common.clone()))
-            .await;
+        dbtx.insert_entry(
+            &RecoveryStateKey,
+            &(MintRecoveryState::V1(self.state.clone()), common.clone()),
+        )
+        .await;
     }
 
     async fn delete_dbtx(&self, dbtx: &mut DatabaseTransaction<'_>) {
@@ -151,7 +162,11 @@ impl RecoveryFromHistory for MintRecovery {
             .sum::<Amount>()
             + finalized.spendable_notes.total_amount();
 
-        info!(amount = %restored_amount, "Finalizing mint recovery");
+        info!(
+            amount = %restored_amount,
+            burned_total = %finalized.burned_total,
+            "Finalizing mint recovery"
+        );
 
         debug!(
             target: LOG_CLIENT_RECOVERY_MINT,
@@ -224,6 +239,8 @@ pub struct EcashRecoveryFinalState {
     pub unconfirmed_notes: Vec<(OutPoint, Amount, NoteIssuanceRequest)>,
     /// Note index to derive next note in a given amount tier
     pub next_note_idx: Tiered<NoteIndex>,
+    /// Total burned amount
+    pub burned_total: Amount,
 }
 
 /// Newtype over [`BlindedMessage`] to enable `Ord`
@@ -247,6 +264,16 @@ impl From<CompressedBlindedMessage> for BlindedMessage {
     }
 }
 
+#[derive(Debug, Clone, Decodable, Encodable)]
+pub enum MintRecoveryState {
+    V1(MintRecoveryStateV0),
+    #[encodable_default]
+    Default {
+        variant: u64,
+        bytes: Vec<u8>,
+    },
+}
+
 /// The state machine used for fast-forwarding backup from point when it was
 /// taken to the present time by following epoch history items from the time the
 /// snapshot was taken.
@@ -255,7 +282,7 @@ impl From<CompressedBlindedMessage> for BlindedMessage {
 /// valid consensus items from the epoch history between time taken (or even
 /// somewhat before it) and present time.
 #[derive(Clone, Eq, PartialEq, Decodable, Encodable, Serialize, Deserialize)]
-pub struct MintRecoveryState {
+pub struct MintRecoveryStateV0 {
     spendable_notes: BTreeMap<Nonce, (Amount, SpendableNote)>,
     /// Nonces that we track that are currently spendable.
     pending_outputs: BTreeMap<Nonce, (OutPoint, Amount, NoteIssuanceRequest)>,
@@ -266,6 +293,11 @@ pub struct MintRecoveryState {
     /// the pool is kept shared (so only one lookup is enough), and
     /// replenishment is done each time a note is consumed.
     pending_nonces: BTreeMap<CompressedBlindedMessage, (NoteIssuanceRequest, NoteIndex, Amount)>,
+    /// Nonces that we have already used. Used for detecting double-used nonces
+    /// (accidentally burning funds).
+    used_nonces: BTreeMap<CompressedBlindedMessage, (NoteIssuanceRequest, NoteIndex, Amount)>,
+    /// Total amount probably burned due to re-using nonces
+    burned_total: Amount,
     /// Tail of `pending`. `pending_notes` is filled by generating note with
     /// this index and incrementing it.
     next_pending_note_idx: Tiered<NoteIndex>,
@@ -288,17 +320,19 @@ pub struct MintRecoveryState {
     gap_limit: u64,
 }
 
-impl fmt::Debug for MintRecoveryState {
+impl fmt::Debug for MintRecoveryStateV0 {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.write_fmt(format_args!(
-            "MintRestoreInProgressState(pending_outputs: {}, pending_nonces: {})",
+            "MintRestoreInProgressState(pending_outputs: {}, pending_nonces: {}, used_nonces: {}, burned_total: {})",
             self.pending_outputs.len(),
-            self.pending_nonces.len()
+            self.pending_nonces.len(),
+            self.used_nonces.len(),
+            self.burned_total,
         ))
     }
 }
 
-impl MintRecoveryState {
+impl MintRecoveryStateV0 {
     pub fn from_backup(
         backup: EcashBackupV0,
         gap_limit: u64,
@@ -324,6 +358,8 @@ impl MintRecoveryState {
                 })
                 .collect(),
             pending_nonces: BTreeMap::default(),
+            used_nonces: BTreeMap::default(),
+            burned_total: Amount::ZERO,
             next_pending_note_idx: backup.next_note_idx.clone(),
             last_used_nonce_idx: backup
                 .next_note_idx
@@ -397,6 +433,18 @@ impl MintRecoveryState {
             }
         };
 
+        if let Some((_issuance_request, note_idx, amount)) =
+            self.used_nonces.get(&output.blind_nonce.0.into())
+        {
+            self.burned_total += *amount;
+            warn!(
+                target: LOG_CLIENT_RECOVERY_MINT,
+                %note_idx,
+                %amount,
+                burned_total = %self.burned_total,
+                "Detected reused nonce during recovery. This means client probably burned funds in the past."
+            );
+        }
         // There is nothing preventing other users from creating valid
         // transactions mining notes to our own blind nonce, possibly
         // even racing with us. Including amount in blind nonce
@@ -411,28 +459,32 @@ impl MintRecoveryState {
         // greedy no matter what and take what we can, and just report
         // anything suspicious.
 
-        if let Some((issuance_request, note_idx, pending_amount)) = self
-            .pending_nonces
-            .get(&output.blind_nonce.0.into())
-            .copied()
+        if let Some((issuance_request, note_idx, pending_amount)) =
+            self.pending_nonces.remove(&output.blind_nonce.0.into())
         {
             // the moment we see our blind nonce in the epoch history, correctly or
             // incorrectly used, we know that we must have used
             // already
             self.observe_nonce_idx_being_used(pending_amount, note_idx, secret);
 
             if pending_amount == output.amount {
-                assert!(self
-                    .pending_nonces
-                    .remove(&output.blind_nonce.0.into())
-                    .is_some());
+                self.used_nonces.insert(
+                    output.blind_nonce.0.into(),
+                    (issuance_request, note_idx, pending_amount),
+                );
 
                 self.pending_outputs.insert(
                     issuance_request.nonce(),
                     (out_point, output.amount, issuance_request),
                 );
             } else {
+                // put it back, incorrect amount
+                self.pending_nonces.insert(
+                    output.blind_nonce.0.into(),
+                    (issuance_request, note_idx, pending_amount),
+                );
                 warn!(
+                    target: LOG_CLIENT_RECOVERY_MINT,
                     output = ?out_point,
                     blind_nonce = ?output.blind_nonce.0,
                     expected_amount = %pending_amount,
@@ -488,6 +540,7 @@ impl MintRecoveryState {
                 .iter()
                 .map(|(amount, value)| (amount, value.next()))
                 .collect(),
+            burned_total: self.burned_total,
         }
     }
 }

modules/fedimint-mint-client/src/client_db.rs

@@ -1,5 +1,6 @@
 use fedimint_client::module::init::recovery::RecoveryFromHistoryCommon;
 use fedimint_core::core::OperationId;
+use fedimint_core::db::{DatabaseTransaction, IDatabaseTransactionOpsCoreTyped as _};
 use fedimint_core::encoding::{Decodable, Encodable};
 use fedimint_core::{impl_db_lookup, impl_db_record, Amount};
 use fedimint_mint_common::Nonce;
@@ -98,3 +99,14 @@ impl_db_lookup!(
     key = CancelledOOBSpendKey,
     query_prefix = CancelledOOBSpendKeyPrefix,
 );
+
+pub async fn migrate_to_v1(
+    dbtx: &mut DatabaseTransaction<'_>,
+) -> anyhow::Result<Option<(Vec<(Vec<u8>, OperationId)>, Vec<(Vec<u8>, OperationId)>)>> {
+    // between v0 and v1, we changed the format of `MintRecoveryState`, and instead
+    // of migrating it, we can just delete it, so the recovery will just start
+    // again, ignoring any existing state from before the migration
+    dbtx.remove_entry(&RecoveryStateKey).await;
+
+    Ok(None)
+}

modules/fedimint-mint-client/src/lib.rs

@@ -33,7 +33,8 @@ use async_stream::{stream, try_stream};
 use backup::recovery::MintRecovery;
 use base64::Engine as _;
 use bitcoin_hashes::{sha256, sha256t, Hash, HashEngine as BitcoinHashEngine};
-use client_db::{DbKeyPrefix, NoteKeyPrefix, RecoveryFinalizedKey};
+use client_db::{migrate_to_v1, DbKeyPrefix, NoteKeyPrefix, RecoveryFinalizedKey};
+use fedimint_client::db::ClientMigrationFn;
 use fedimint_client::module::init::{
     ClientModuleInit, ClientModuleInitArgs, ClientModuleRecoverArgs,
 };
@@ -509,7 +510,7 @@ pub struct MintClientInit;
 
 impl ModuleInit for MintClientInit {
     type Common = MintCommonInit;
-    const DATABASE_VERSION: DatabaseVersion = DatabaseVersion(0);
+    const DATABASE_VERSION: DatabaseVersion = DatabaseVersion(1);
 
     async fn dump_database(
         &self,
@@ -595,6 +596,15 @@ impl ClientModuleInit for MintClientInit {
         args.recover_from_history::<MintRecovery>(self, snapshot)
             .await
     }
+
+    fn get_database_migrations(&self) -> BTreeMap<DatabaseVersion, ClientMigrationFn> {
+        let mut migrations: BTreeMap<DatabaseVersion, ClientMigrationFn> = BTreeMap::new();
+        migrations.insert(DatabaseVersion(0), |dbtx, _, _| {
+            Box::pin(migrate_to_v1(dbtx))
+        });
+
+        migrations
+    }
 }
 
 /// The `MintClientModule` is responsible for handling e-cash minting

modules/fedimint-mint-tests/tests/tests.rs

@@ -381,11 +381,14 @@ mod fedimint_migration_tests {
     use fedimint_core::time::now;
     use fedimint_core::{secp256k1, Amount, OutPoint, Tiered, TieredMulti, TransactionId};
     use fedimint_logging::TracingSetup;
-    use fedimint_mint_client::backup::recovery::{MintRecovery, MintRecoveryState};
+    use fedimint_mint_client::backup::recovery::{
+        MintRecovery, MintRecoveryState, MintRecoveryStateV0,
+    };
     use fedimint_mint_client::backup::{EcashBackup, EcashBackupV0};
     use fedimint_mint_client::client_db::{
         CancelledOOBSpendKey, CancelledOOBSpendKeyPrefix, NextECashNoteIndexKey,
-        NextECashNoteIndexKeyPrefix, NoteKey, NoteKeyPrefix, RecoveryStateKey,
+        NextECashNoteIndexKeyPrefix, NoteKey, NoteKeyPrefix, RecoveryFinalizedKey,
+        RecoveryStateKey,
     };
     use fedimint_mint_client::output::NoteIssuanceRequest;
     use fedimint_mint_client::{MintClientInit, MintClientModule, NoteIndex, SpendableNote};
@@ -520,8 +523,13 @@ mod fedimint_migration_tests {
 
         let backup = create_ecash_backup_v0(spendable_note, secret.clone());
 
-        let mint_recovery_state =
-            MintRecoveryState::from_backup(backup, 10, tbs_pks, pub_key_shares, &secret);
+        let mint_recovery_state = MintRecoveryState::V1(MintRecoveryStateV0::from_backup(
+            backup,
+            10,
+            tbs_pks,
+            pub_key_shares,
+            &secret,
+        ));
 
         MintRecovery::store_finalized(&mut dbtx.to_ref_nc(), true).await;
         dbtx.insert_new_entry(
@@ -704,13 +712,13 @@ mod fedimint_migration_tests {
                         fedimint_mint_client::client_db::DbKeyPrefix::RecoveryState => {
                             let restore_state = dbtx.get_value(&RecoveryStateKey).await;
                             ensure!(
-                                restore_state.is_some(),
-                                "validate_migrations was not able to read any RecoveryState"
+                                restore_state.is_none(),
+                                "validate_migrations expect the restore state to get deleted"
                             );
                             info!("Validated RecoveryState");
                         }
                         fedimint_mint_client::client_db::DbKeyPrefix::RecoveryFinalized => {
-                            let recovery_finalized = dbtx.get_value(&RecoveryStateKey).await;
+                            let recovery_finalized = dbtx.get_value(&RecoveryFinalizedKey).await;
                             ensure!(
                                 recovery_finalized.is_some(),
                                 "validate_migrations was not able to read any RecoveryFinalized"