Merge pull request fedimint#6176 from dpc/24-10-16-more-efficient-oob…

…-refund

 chore: claim oob refunds with a single state machine

fedimint-client/src/sm/executor.rs

@@ -228,18 +228,20 @@ impl Executor {
             if let Some(module_context) =
                 self.inner.module_contexts.get(&state.module_instance_id())
             {
-                let context = self
+                if let Some(context) = self
                     .inner
                     .state
                     .read()
-                    .unwrap()
+                    .expect("locking failed")
                     .gen_context(&state)
-                    .expect("executor should be running at this point");
-
-                if state.is_terminal(module_context, &context) {
-                    return Err(AddStateMachinesError::Other(anyhow!(
+                {
+                    if state.is_terminal(module_context, &context) {
+                        return Err(AddStateMachinesError::Other(anyhow!(
                         "State is already terminal, adding it to the executor doesn't make sense."
                     )));
+                    }
+                } else {
+                    warn!(target: LOG_CLIENT_REACTOR, "Executor should be running at this point");
                 }
             }
 

fedimint-core/src/db/mod.rs

@@ -572,6 +572,7 @@ impl Database {
                         warn!(
                             target: LOG_DB,
                             curr_attempts,
+                            ?err,
                             "Database commit failed in an autocommit block - terminating"
                         );
                         return Err(AutocommitError::CommitFailed {
@@ -585,6 +586,7 @@ impl Database {
                     warn!(
                         target: LOG_DB,
                         curr_attempts,
+                        %err,
                         delay_ms = %delay,
                         "Database commit failed in an autocommit block - retrying"
                     );

modules/fedimint-dummy-client/src/db.rs

@@ -79,6 +79,8 @@ pub(crate) fn get_v1_migrated_state(
     let decoders = ModuleDecoderRegistry::default();
     let dummy_sm_variant = u16::consensus_decode(cursor, &decoders)?;
 
+    // We are only migrating the type of one of the variants, so we do nothing on
+    // other discriminants.
     if dummy_sm_variant != 5 {
         return Ok(None);
     }

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

@@ -1,14 +1,18 @@
+use std::io::Cursor;
+
 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::module::registry::ModuleDecoderRegistry;
 use fedimint_core::{impl_db_lookup, impl_db_record, Amount};
 use fedimint_mint_common::Nonce;
 use serde::Serialize;
 use strum_macros::EnumIter;
 
 use crate::backup::recovery::MintRecoveryState;
-use crate::SpendableNoteUndecoded;
+use crate::oob::{MintOOBStateMachine, MintOOBStateMachineV1, MintOOBStates, MintOOBStatesV1};
+use crate::{MintClientStateMachines, SpendableNoteUndecoded};
 
 #[repr(u8)]
 #[derive(Clone, EnumIter, Debug)]
@@ -110,3 +114,32 @@ pub async fn migrate_to_v1(
 
     Ok(None)
 }
+
+/// Maps all `Unreachable` states in the state machine to `OutputDone`
+pub(crate) fn migrate_state_to_v2(
+    operation_id: OperationId,
+    cursor: &mut Cursor<&[u8]>,
+) -> anyhow::Result<Option<(Vec<u8>, OperationId)>> {
+    let decoders = ModuleDecoderRegistry::default();
+
+    let mint_client_state_machine_variant = u16::consensus_decode(cursor, &decoders)?;
+
+    let bytes = match mint_client_state_machine_variant {
+        2 => {
+            let old_state = MintOOBStateMachineV1::consensus_decode(cursor, &decoders)?;
+
+            let new_state = match old_state.state {
+                MintOOBStatesV1::Created(created) => MintOOBStates::Created(created),
+                MintOOBStatesV1::UserRefund(refund) => MintOOBStates::UserRefund(refund),
+                MintOOBStatesV1::TimeoutRefund(refund) => MintOOBStates::TimeoutRefund(refund),
+            };
+            MintClientStateMachines::OOB(MintOOBStateMachine {
+                operation_id: old_state.operation_id,
+                state: new_state,
+            })
+            .consensus_encode_to_vec()
+        }
+        _ => return Ok(None),
+    };
+    Ok(Some((bytes, operation_id)))
+}

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

@@ -163,7 +163,7 @@ impl MintInputStateCreated {
 
 #[derive(Debug, Clone, Eq, PartialEq, Hash, Decodable, Encodable)]
 pub struct MintInputStateRefund {
-    refund_txid: TransactionId,
+    pub refund_txid: TransactionId,
 }
 
 impl MintInputStateRefund {

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

@@ -35,9 +35,11 @@ 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::{migrate_to_v1, DbKeyPrefix, NoteKeyPrefix, RecoveryFinalizedKey};
+use client_db::{
+    migrate_state_to_v2, migrate_to_v1, DbKeyPrefix, NoteKeyPrefix, RecoveryFinalizedKey,
+};
 use event::NoteSpent;
-use fedimint_client::db::ClientMigrationFn;
+use fedimint_client::db::{migrate_state, ClientMigrationFn};
 use fedimint_client::module::init::{
     ClientModuleInit, ClientModuleInitArgs, ClientModuleRecoverArgs,
 };
@@ -78,6 +80,7 @@ use fedimint_mint_common::config::MintClientConfig;
 pub use fedimint_mint_common::*;
 use futures::{pin_mut, StreamExt};
 use hex::ToHex;
+use oob::MintOOBStatesCreatedMulti;
 use serde::{Deserialize, Serialize};
 use strum::IntoEnumIterator;
 use tbs::{AggregatePublicKey, Signature};
@@ -92,7 +95,7 @@ use crate::client_db::{
 use crate::input::{
     MintInputCommon, MintInputStateCreated, MintInputStateMachine, MintInputStates,
 };
-use crate::oob::{MintOOBStateMachine, MintOOBStates, MintOOBStatesCreated};
+use crate::oob::{MintOOBStateMachine, MintOOBStates};
 use crate::output::{
     MintOutputCommon, MintOutputStateMachine, MintOutputStates, MintOutputStatesCreated,
     NoteIssuanceRequest,
@@ -610,6 +613,9 @@ impl ClientModuleInit for MintClientInit {
         migrations.insert(DatabaseVersion(0), |dbtx, _, _| {
             Box::pin(migrate_to_v1(dbtx))
         });
+        migrations.insert(DatabaseVersion(1), |_, active_states, inactive_states| {
+            Box::pin(async { migrate_state(active_states, inactive_states, migrate_state_to_v2) })
+        });
 
         migrations
     }
@@ -1305,18 +1311,13 @@ impl MintClientModule {
             MintClientModule::delete_spendable_note(&self.client_ctx, dbtx, amount, note).await;
         }
 
-        let mut state_machines = Vec::new();
-
-        for (amount, spendable_note) in selected_notes.clone().into_iter_items() {
-            state_machines.push(MintClientStateMachines::OOB(MintOOBStateMachine {
-                operation_id,
-                state: MintOOBStates::Created(MintOOBStatesCreated {
-                    amount,
-                    spendable_note,
-                    timeout: fedimint_core::time::now() + try_cancel_after,
-                }),
-            }));
-        }
+        let state_machines = vec![MintClientStateMachines::OOB(MintOOBStateMachine {
+            operation_id,
+            state: MintOOBStates::CreatedMulti(MintOOBStatesCreatedMulti {
+                spendable_notes: selected_notes.clone().into_iter_items().collect(),
+                timeout: fedimint_core::time::now() + try_cancel_after,
+            }),
+        })];
 
         Ok((operation_id, state_machines, selected_notes))
     }
@@ -1334,13 +1335,17 @@ impl MintClientModule {
                     match state.state {
                         MintOOBStates::TimeoutRefund(refund) => Some(SpendOOBRefund {
                             user_triggered: false,
-                            transaction_id: refund.refund_txid,
+                            transaction_ids: vec![refund.refund_txid],
                         }),
                         MintOOBStates::UserRefund(refund) => Some(SpendOOBRefund {
                             user_triggered: true,
-                            transaction_id: refund.refund_txid,
+                            transaction_ids: vec![refund.refund_txid],
+                        }),
+                        MintOOBStates::UserRefundMulti(refund) => Some(SpendOOBRefund {
+                            user_triggered: true,
+                            transaction_ids: vec![refund.refund_txid],
                         }),
-                        MintOOBStates::Created(_) => None,
+                        MintOOBStates::Created(_) | MintOOBStates::CreatedMulti(_) => None,
                     }
                 }),
         )
@@ -1771,33 +1776,45 @@ impl MintClientModule {
 
                     if refund.user_triggered {
                         yield SpendOOBState::UserCanceledProcessing;
+                    }
 
-                        match client_ctx
-                            .transaction_updates(operation_id)
-                            .await
-                            .await_tx_accepted(refund.transaction_id)
-                            .await
-                        {
-                            Ok(()) => {
-                                yield SpendOOBState::UserCanceledSuccess;
-                            },
-                            Err(_) => {
-                                yield SpendOOBState::UserCanceledFailure;
-                            }
-                        }
-                    } else {
-                        match client_ctx
+                    let mut success = true;
+
+                    for txid in refund.transaction_ids {
+                        debug!(
+                            target: LOG_CLIENT_MODULE_MINT,
+                            %txid,
+                            operation_id=%operation_id.fmt_short(),
+                            "Waiting for oob refund txid"
+                        );
+                        if client_ctx
                             .transaction_updates(operation_id)
                             .await
-                            .await_tx_accepted(refund.transaction_id)
-                            .await
-                        {
-                            Ok(()) => {
-                                yield SpendOOBState::Refunded;
-                            },
-                            Err(_) => {
-                                yield SpendOOBState::Success;
+                            .await_tx_accepted(txid)
+                            .await.is_err() {
+                                success = false;
                             }
+                    }
+
+                    debug!(
+                        target: LOG_CLIENT_MODULE_MINT,
+                        operation_id=%operation_id.fmt_short(),
+                        %success,
+                        "Done waiting for all refund oob txids"
+                     );
+
+                    match (refund.user_triggered, success) {
+                        (true, true) => {
+                            yield SpendOOBState::UserCanceledSuccess;
+                        },
+                        (true, false) => {
+                            yield SpendOOBState::UserCanceledFailure;
+                        },
+                        (false, true) => {
+                            yield SpendOOBState::Refunded;
+                        },
+                        (false, false) => {
+                            yield SpendOOBState::Success;
                         }
                     }
                 }
@@ -1867,7 +1884,7 @@ pub fn spendable_notes_to_operation_id(
 #[derive(Debug, Serialize, Deserialize, Clone)]
 pub struct SpendOOBRefund {
     pub user_triggered: bool,
-    pub transaction_id: TransactionId,
+    pub transaction_ids: Vec<TransactionId>,
 }
 
 /// Defines a strategy for selecting e-cash notes given a specific target amount