diff --git a/rs/bitcoin/ckbtc/minter/ckbtc_minter.did b/rs/bitcoin/ckbtc/minter/ckbtc_minter.did index 95dea05324a..24d1ee04a69 100644 --- a/rs/bitcoin/ckbtc/minter/ckbtc_minter.did +++ b/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 { diff --git a/rs/bitcoin/ckbtc/minter/src/dashboard.rs b/rs/bitcoin/ckbtc/minter/src/dashboard.rs index 46dea4720c1..346ca78a80e 100644 --- a/rs/bitcoin/ckbtc/minter/src/dashboard.rs +++ b/rs/bitcoin/ckbtc/minter/src/dashboard.rs @@ -128,6 +128,20 @@ pub fn build_dashboard(account_to_utxos_start: u64) -> Vec { {} +

Mint status unknown utxos

+ + + + + + + + + + + {} + +
TxidVoutHeightValue (BTC)

Quarantined utxos

@@ -182,6 +196,7 @@ pub fn build_dashboard(account_to_utxos_start: u64) -> Vec { 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, + " + + + + + ", + 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() { diff --git a/rs/bitcoin/ckbtc/minter/src/metrics.rs b/rs/bitcoin/ckbtc/minter/src/metrics.rs index 11a274477ed..03baedce85a 100644 --- a/rs/bitcoin/ckbtc/minter/src/metrics.rs +++ b/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.", diff --git a/rs/bitcoin/ckbtc/minter/src/state.rs b/rs/bitcoin/ckbtc/minter/src/state.rs index 8ae83035399..feddc12aee8 100644 --- a/rs/bitcoin/ckbtc/minter/src/state.rs +++ b/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 { + self.checked_utxos.iter().filter_map(|(utxo, checked)| { + if checked.status == UtxoCheckStatus::CleanButMintUnknown { + Some(utxo) + } else { + None + } + }) + } } #[derive(Eq, PartialEq, Debug, Default)] diff --git a/rs/bitcoin/ckbtc/minter/src/state/audit.rs b/rs/bitcoin/ckbtc/minter/src/state/audit.rs index 993f128bf68..48986842949 100644 --- a/rs/bitcoin/ckbtc/minter/src/state/audit.rs +++ b/rs/bitcoin/ckbtc/minter/src/state/audit.rs @@ -115,6 +115,22 @@ pub fn mark_utxo_checked( state.mark_utxo_checked_v2(utxo, &account); } +pub fn mark_utxo_checked_mint_unknown( + 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( state: &mut CkBtcMinterState, utxo: Utxo, diff --git a/rs/bitcoin/ckbtc/minter/src/state/eventlog.rs b/rs/bitcoin/ckbtc/minter/src/state/eventlog.rs index 3c834e7dee3..25d00394c08 100644 --- a/rs/bitcoin/ckbtc/minter/src/state/eventlog.rs +++ b/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( 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( }, ); } + EventType::CheckedUtxoMintUnknown { utxo, account } => { + state.mark_utxo_checked_mint_unknown(utxo, &account); + } } } diff --git a/rs/bitcoin/ckbtc/minter/src/updates/tests.rs b/rs/bitcoin/ckbtc/minter/src/updates/tests.rs index 26c9653a2a1..f1a147d8a5a 100644 --- a/rs/bitcoin/ckbtc/minter/src/updates/tests.rs +++ b/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(); diff --git a/rs/bitcoin/ckbtc/minter/src/updates/update_balance.rs b/rs/bitcoin/ckbtc/minter/src/updates/update_balance.rs index 183f8a359cb..12c18ba6d5e 100644 --- a/rs/bitcoin/ckbtc/minter/src/updates/update_balance.rs +++ b/rs/bitcoin/ckbtc/minter/src/updates/update_balance.rs @@ -265,21 +265,23 @@ pub async fn update_balance( 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()), @@ -287,6 +289,18 @@ pub async fn update_balance( 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( 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);
{}{}{}{}