fix(ckbtc): use scope guard to prevent double minting (#3930)

XC-254

The loop over processable_utxos may keep looping and eventually panic
when exceeding the instruction limit if there is a large number of utxos
and all of them have a value less than check_fee. If it does panic, then
any previous state modification will not be persisted which spells
trouble if a previous utxo has already been minted. Although this is a
very unlikely scenario, it does still present a non-zero risk.

The fix is to use a scope guard that will be triggered under this
situation and update the utxo to a special status to prevent it from
being minted again.

rs/bitcoin/ckbtc/minter/ckbtc_minter.did

@@ -399,6 +399,10 @@ type EventType = variant {
         utxo : Utxo;
         account : Account;
     };
+    checked_utxo_mint_unknown : record {
+        utxo : Utxo;
+        account : Account;
+    };
     ignored_utxo : record { utxo: Utxo; };
     suspended_utxo : record { utxo: Utxo; account: Account; reason: SuspendedReason };
     retrieve_btc_kyt_failed : record {

rs/bitcoin/ckbtc/minter/src/dashboard.rs

@@ -128,6 +128,20 @@ pub fn build_dashboard(account_to_utxos_start: u64) -> Vec<u8> {
                     </thead>
                     <tbody>{}</tbody>
                 </table>
+                <h3>Mint status unknown utxos</h3>
+                <table>
+                    <thead>
+                        <tr>
+                            <th>Txid</th>
+                            <th>Vout</th>
+                            <th>Height</th>
+                            <th>Value (BTC)</th>
+                        </tr>
+                    </thead>
+                    <tbody>
+                        {}
+                    </tbody>
+                </table>
                 <h3>Quarantined utxos</h3>
                 <table>
                     <thead>
@@ -182,6 +196,7 @@ pub fn build_dashboard(account_to_utxos_start: u64) -> Vec<u8> {
         build_submitted_transactions(s),
         build_finalized_requests(s),
         build_unconfirmed_change(s),
+        build_mint_unknown_utxos(s),
         build_quarantined_utxos(s),
         build_ignored_utxos(s),
         build_account_to_utxos_table(s, account_to_utxos_start, DEFAULT_PAGE_SIZE),
@@ -439,6 +454,27 @@ pub fn build_finalized_requests(s: &CkBtcMinterState) -> String {
     })
 }
 
+pub fn build_mint_unknown_utxos(s: &CkBtcMinterState) -> String {
+    with_utf8_buffer(|buf| {
+        for utxo in s.mint_status_unknown_utxos() {
+            writeln!(
+                buf,
+                "<tr>
+                    <td>{}</td>
+                    <td>{}</td>
+                    <td>{}</td>
+                    <td>{}</td>
+                </tr>",
+                txid_link(s, &utxo.outpoint.txid),
+                utxo.outpoint.vout,
+                utxo.height,
+                DisplayAmount(utxo.value)
+            )
+            .unwrap()
+        }
+    })
+}
+
 pub fn build_quarantined_utxos(s: &CkBtcMinterState) -> String {
     with_utf8_buffer(|buf| {
         for utxo in s.quarantined_utxos() {

rs/bitcoin/ckbtc/minter/src/metrics.rs

@@ -271,6 +271,12 @@ pub fn encode_metrics(
         "Total number of suspended UTXOs due to being marked as tainted.",
     )?;
 
+    metrics.encode_gauge(
+        "ckbtc_minter_mint_status_unknown_utxos_count",
+        state::read_state(|s| s.mint_status_unknown_utxos().count()) as f64,
+        "Total number of UTXOs with unknown mint status.",
+    )?;
+
     let mut histogram_vec = metrics.histogram_vec(
         "ckbtc_minter_update_calls_latency",
         "The latency of ckBTC minter `update_balance` calls in milliseconds.",

rs/bitcoin/ckbtc/minter/src/state.rs

@@ -249,20 +249,8 @@ pub enum UtxoCheckStatus {
     Clean,
     /// The UTXO in question is tainted.
     Tainted,
-}
-
-impl UtxoCheckStatus {
-    pub fn from_clean_flag(clean: bool) -> Self {
-        if clean {
-            Self::Clean
-        } else {
-            Self::Tainted
-        }
-    }
-
-    pub fn is_clean(self) -> bool {
-        self == Self::Clean
-    }
+    /// The UTXO is clean but minting failed.
+    CleanButMintUnknown,
 }
 
 /// Relevant data for a checked UTXO. The UUID and `kyt_provider` are kept for
@@ -1062,6 +1050,24 @@ impl CkBtcMinterState {
         }
     }
 
+    /// Marks the given UTXO as successfully checked but minting failed.
+    fn mark_utxo_checked_mint_unknown(&mut self, utxo: Utxo, account: &Account) {
+        // It should have already been removed from suspended_utxos
+        debug_assert_eq!(
+            self.suspended_utxos.contains_utxo(&utxo, account),
+            (None, None),
+            "BUG: UTXO was still suspended and cannot be marked as mint unknown"
+        );
+        self.checked_utxos.insert(
+            utxo,
+            CheckedUtxo {
+                uuid: None,
+                status: UtxoCheckStatus::CleanButMintUnknown,
+                kyt_provider: None,
+            },
+        );
+    }
+
     /// Marks the given UTXO as successfully checked.
     fn mark_utxo_checked_v2(&mut self, utxo: Utxo, account: &Account) {
         self.suspended_utxos.remove(account, &utxo);
@@ -1277,6 +1283,16 @@ impl CkBtcMinterState {
             SuspendedReason::Quarantined => Some(u),
         })
     }
+
+    pub fn mint_status_unknown_utxos(&self) -> impl Iterator<Item = &Utxo> {
+        self.checked_utxos.iter().filter_map(|(utxo, checked)| {
+            if checked.status == UtxoCheckStatus::CleanButMintUnknown {
+                Some(utxo)
+            } else {
+                None
+            }
+        })
+    }
 }
 
 #[derive(Eq, PartialEq, Debug, Default)]

rs/bitcoin/ckbtc/minter/src/state/audit.rs

@@ -115,6 +115,22 @@ pub fn mark_utxo_checked<R: CanisterRuntime>(
     state.mark_utxo_checked_v2(utxo, &account);
 }
 
+pub fn mark_utxo_checked_mint_unknown<R: CanisterRuntime>(
+    state: &mut CkBtcMinterState,
+    utxo: Utxo,
+    account: Account,
+    runtime: &R,
+) {
+    record_event(
+        EventType::CheckedUtxoMintUnknown {
+            utxo: utxo.clone(),
+            account,
+        },
+        runtime,
+    );
+    state.mark_utxo_checked_mint_unknown(utxo, &account);
+}
+
 pub fn quarantine_utxo<R: CanisterRuntime>(
     state: &mut CkBtcMinterState,
     utxo: Utxo,

rs/bitcoin/ckbtc/minter/src/state/eventlog.rs

@@ -3,7 +3,7 @@ use crate::lifecycle::upgrade::UpgradeArgs;
 use crate::state::invariants::CheckInvariants;
 use crate::state::{
     ChangeOutput, CkBtcMinterState, FinalizedBtcRetrieval, FinalizedStatus, Overdraft,
-    RetrieveBtcRequest, SubmittedBtcTransaction, SuspendedReason, UtxoCheckStatus,
+    RetrieveBtcRequest, SubmittedBtcTransaction, SuspendedReason,
 };
 use crate::state::{ReimburseDepositTask, ReimbursedDeposit, ReimbursementReason};
 use candid::Principal;
@@ -202,6 +202,10 @@ mod event {
             /// The mint block on the ledger.
             mint_block_index: u64,
         },
+
+        /// Indicates an UTXO is checked to be clean and pre-mint
+        #[serde(rename = "checked_utxo_mint_unknown")]
+        CheckedUtxoMintUnknown { account: Account, utxo: Utxo },
     }
 }
 
@@ -366,18 +370,17 @@ pub fn replay<I: CheckInvariants>(
                 uuid,
                 clean,
                 kyt_provider,
-            } => match UtxoCheckStatus::from_clean_flag(clean) {
-                UtxoCheckStatus::Clean => {
+            } => {
+                if clean {
                     state.mark_utxo_checked(
                         utxo,
                         if uuid.is_empty() { None } else { Some(uuid) },
                         kyt_provider,
                     );
-                }
-                UtxoCheckStatus::Tainted => {
+                } else {
                     state.discard_utxo_without_account(utxo, SuspendedReason::Quarantined);
                 }
-            },
+            }
             EventType::CheckedUtxoV2 { utxo, account } => {
                 state.mark_utxo_checked_v2(utxo, &account);
             }
@@ -442,6 +445,9 @@ pub fn replay<I: CheckInvariants>(
                     },
                 );
             }
+            EventType::CheckedUtxoMintUnknown { utxo, account } => {
+                state.mark_utxo_checked_mint_unknown(utxo, &account);
+            }
         }
     }
 

rs/bitcoin/ckbtc/minter/src/updates/tests.rs

@@ -214,6 +214,77 @@ mod update_balance {
         );
     }
 
+    #[tokio::test]
+    async fn should_not_evaluate_mint_unknown_utxo() {
+        init_state_with_ecdsa_public_key();
+        let account = ledger_account();
+        let mut runtime = MockCanisterRuntime::new();
+        mock_increasing_time(&mut runtime, NOW, Duration::from_secs(1));
+
+        // Create two utxos, first one is already checked but with unknown mint status.
+        let checked_but_mint_unknown_utxo = quarantined_utxo();
+        let utxo = crate::test_fixtures::utxo();
+        mutate_state(|s| {
+            audit::mark_utxo_checked_mint_unknown(
+                s,
+                checked_but_mint_unknown_utxo.clone(),
+                account,
+                &runtime,
+            );
+        });
+        let check_fee = read_state(|s| s.check_fee);
+        let minted_amount = utxo.value - check_fee;
+        let events_before: Vec<_> = storage::events().map(|event| event.payload).collect();
+
+        mock_get_utxos_for_account(
+            &mut runtime,
+            account,
+            vec![checked_but_mint_unknown_utxo.clone(), utxo.clone()],
+        );
+        expect_check_transaction_returning(
+            &mut runtime,
+            utxo.clone(),
+            CheckTransactionResponse::Passed,
+        );
+        runtime
+            .expect_mint_ckbtc()
+            .times(1)
+            .withf(move |amount, account_, _memo| amount == &minted_amount && account_ == &account)
+            .return_const(Ok(1));
+        mock_schedule_now_process_logic(&mut runtime);
+        let result = update_balance(
+            UpdateBalanceArgs {
+                owner: Some(account.owner),
+                subaccount: account.subaccount,
+            },
+            &runtime,
+        )
+        .await;
+
+        assert_eq!(suspended_utxo(&checked_but_mint_unknown_utxo), None);
+        assert_eq!(suspended_utxo(&utxo), None);
+        // Only the 2nd utxo is minted
+        assert_eq!(
+            result,
+            Ok(vec![UtxoStatus::Minted {
+                block_index: 1,
+                minted_amount,
+                utxo: utxo.clone()
+            }])
+        );
+        assert_has_new_events(
+            &events_before,
+            &[
+                checked_utxo_event(utxo.clone(), account),
+                EventType::ReceivedUtxos {
+                    mint_txid: Some(1),
+                    to_account: account,
+                    utxos: vec![utxo],
+                },
+            ],
+        );
+    }
+
     #[tokio::test]
     async fn should_observe_latency_metrics() {
         init_state_with_ecdsa_public_key();

rs/bitcoin/ckbtc/minter/src/updates/update_balance.rs

@@ -265,28 +265,42 @@ pub async fn update_balance<R: CanisterRuntime>(
             continue;
         }
         let status = check_utxo(&utxo, &args, runtime).await?;
-        mutate_state(|s| match status {
+        match status {
+            // Skip utxos that are already checked but has unknown mint status
+            UtxoCheckStatus::CleanButMintUnknown => continue,
             UtxoCheckStatus::Clean => {
-                state::audit::mark_utxo_checked(s, utxo.clone(), caller_account, runtime);
+                mutate_state(|s| {
+                    state::audit::mark_utxo_checked(s, utxo.clone(), caller_account, runtime)
+                });
             }
             UtxoCheckStatus::Tainted => {
-                state::audit::quarantine_utxo(s, utxo.clone(), caller_account, now, runtime);
-            }
-        });
-        match status {
-            UtxoCheckStatus::Tainted => {
+                mutate_state(|s| {
+                    state::audit::quarantine_utxo(s, utxo.clone(), caller_account, now, runtime)
+                });
                 utxo_statuses.push(UtxoStatus::Tainted(utxo.clone()));
                 continue;
             }
-            UtxoCheckStatus::Clean => {}
-        }
+        };
+
         let amount = utxo.value - check_fee;
         let memo = MintMemo::Convert {
             txid: Some(utxo.outpoint.txid.as_ref()),
             vout: Some(utxo.outpoint.vout),
             kyt_fee: Some(check_fee),
         };
 
+        // After the call to `mint_ckbtc` returns, in a very unlikely situation the
+        // execution may panic/trap without persisting state changes and then we will
+        // have no idea whether the mint actually succeeded or not. If this happens
+        // the use of the guard below will help set the utxo to `CleanButMintUnknown`
+        // status so that it will not be minted again. Utxos with this status will
+        // require manual intervention.
+        let guard = scopeguard::guard((utxo.clone(), caller_account), |(utxo, account)| {
+            mutate_state(|s| {
+                state::audit::mark_utxo_checked_mint_unknown(s, utxo, account, runtime)
+            });
+        });
+
         match runtime
             .mint_ckbtc(amount, caller_account, crate::memo::encode(&memo).into())
             .await
@@ -323,6 +337,10 @@ pub async fn update_balance<R: CanisterRuntime>(
                 utxo_statuses.push(UtxoStatus::Checked(utxo));
             }
         }
+        // Defuse the guard. Note that In case of a panic (either before or after this point)
+        // the defuse will not be effective (due to state rollback), and the guard that was
+        // setup before the `mint_ckbtc` async call will be invoked.
+        scopeguard::ScopeGuard::into_inner(guard);
     }
 
     schedule_now(TaskType::ProcessLogic, runtime);