key-wallet: `mark_instant_send_utxos` can downgrade a mined record to `InstantSend`, erasing its block height — ChainLock promotion then skips it forever

[romchornyi]

Expected Behavior

An InstantSend lock applied to a transaction the wallet already holds as InBlock or InChainLockedBlock (or has already finalized) keeps the block context and its BlockInfo, the same way check_core_transaction already treats it. A later ChainLock at or above that height promotes the record to InChainLockedBlock.

Current Behavior

WalletInfoInterface::mark_instant_send_utxos overwrites the record's context unconditionally:

rust-dashcore/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs

Lines 564 to 582 in 0da39eb

	fn mark_instant_send_utxos(&mut self, txid: &Txid, lock: &InstantLock) -> bool {
	if !self.instant_send_locks.insert(*txid) {
	return false;
	}
	let mut any_changed = false;
	// Kept for the sweep below: it needs the locked transaction's inputs,
	// and this signature carries only its txid.
	let mut locked_transaction = None;
	for mut account in self.accounts.all_accounts_mut() {
	if account.mark_utxos_instant_send(txid) {
	any_changed = true;
	}
	if let Some(record) = account.transactions_mut().get_mut(txid) {
	record.update_context(TransactionContext::InstantSend(lock.clone()));
	any_changed = true;
	if locked_transaction.is_none() {
	locked_transaction = Some(record.transaction.clone());
	}
	}

and TransactionRecord::update_context is a plain assignment with no ordering guard:

rust-dashcore/key-wallet/src/managed_account/transaction_record.rs

Lines 182 to 184 in 0da39eb

	pub fn update_context(&mut self, context: TransactionContext) {
	self.context = context;
	}

TransactionContext::InstantSend(InstantLock) carries no BlockInfo, so the record's height() becomes None:

rust-dashcore/key-wallet/src/transaction_checking/transaction_context.rs

Lines 68 to 74 in 0da39eb

	Mempool,
	/// Transaction is in the mempool with an InstantSend lock
	InstantSend(InstantLock),
	/// Transaction is in a block at the given height
	InBlock(BlockInfo),
	/// Transaction is in a chain-locked block at the given height
	InChainLockedBlock(BlockInfo),

apply_chain_lock only promotes InBlock records and intentionally skips InstantSend, so a downgraded record is never promoted again, however far the chain advances:

rust-dashcore/key-wallet/src/managed_account/managed_core_keys_account.rs

Lines 156 to 196 in 0da39eb

	/// Promote any `InBlock` records at height `<= cl_height` to
	/// [`TransactionContext::InChainLockedBlock`] and return their txids.
	///
	/// Under the default `keep-finalized-transactions=OFF` feature
	/// configuration the promoted records are immediately dropped via
	/// [`Self::drop_finalized_transaction`], with their txids retained
	/// only in `finalized_txids`. With the feature on the records stay
	/// in `transactions` with the updated context.
	///
	/// Idempotent: records already in `InChainLockedBlock` or already
	/// dropped to `finalized_txids` are not revisited and do not appear
	/// in the result. Records still in `Mempool` or `InstantSend`
	/// context are intentionally skipped, since chainlock-driven
	/// promotion only applies to records that have already been mined.
	pub(crate) fn apply_chain_lock(&mut self, cl_height: CoreBlockHeight) -> Vec<Txid> {
	let candidates: Vec<(Txid, BlockInfo)> = self
	.transactions
	.iter()
	.filter_map(|(txid, record)| match &record.context {
	TransactionContext::InBlock(info) if info.height() <= cl_height => {
	Some((*txid, *info))
	}
	_ => None,
	})
	.collect();
	let mut promoted = Vec::with_capacity(candidates.len());
	for (txid, info) in candidates {
	if let Some(record) = self.transactions.get_mut(&txid) {
	record.update_context(TransactionContext::InChainLockedBlock(info));
	promoted.push(txid);
	}
	}
	#[cfg(not(feature = "keep-finalized-transactions"))]
	for txid in &promoted {
	self.drop_finalized_transaction(txid);
	}
	promoted
	}

The same transition through check_core_transaction is already guarded — an IS lock on a transaction that is finalized or is_confirmed() is ignored ("Only accept IS transitions for unconfirmed transactions"):

rust-dashcore/key-wallet/src/transaction_checking/wallet_checker.rs

Lines 191 to 215 in 0da39eb

	if !is_new {
	// IS lock on a transaction that is already confirmed is stale — ignore
	if context.is_instant_send() {
	if !self.instant_send_locks.insert(txid) {
	return result;
	}
	// Only accept IS transitions for unconfirmed transactions.
	// A chainlocked tx may have had its full record dropped
	// under the default feature config — `transaction_is_finalized`
	// catches that case via `finalized_txids` and the in-map
	// record check covers `InBlock`.
	let already_confirmed = result.affected_accounts.iter().any(|am| {
	let Some(account) =
	self.accounts.get_by_account_type_match(&am.account_type_match)
	else {
	return false;
	};
	if account.transaction_is_finalized(&txid) {
	return true;
	}
	account.transactions().get(&txid).is_some_and(|r| r.is_confirmed())
	});
	if already_confirmed {
	return result;
	}

mark_instant_send_utxos has no equivalent guard. It is reached from WalletManager::process_instant_send_lock:

rust-dashcore/key-wallet-manager/src/process_block.rs

Lines 417 to 434 in 0da39eb

	fn process_instant_send_lock(&mut self, instant_lock: InstantLock) {
	let txid = instant_lock.txid;
	// `mark_instant_send_utxos` recomputes balances internally when any
	// UTXO is newly marked, so we have to snapshot per-account balances
	// up front to surface the diff afterwards.
	let mut prior_account_balances: BTreeMap<
	WalletId,
	BTreeMap<AccountType, WalletCoreBalance>,
	> = self.wallet_infos.iter().map(|(id, info)| (*id, info.account_balances())).collect();
	let mut affected_wallets = Vec::new();
	for (wallet_id, info) in self.wallet_infos.iter_mut() {
	if info.mark_instant_send_utxos(&txid, &instant_lock) {
	info.update_balance();
	affected_wallets.push(*wallet_id);
	}
	}

which dash-spv's mempool manager calls only while the transaction is still in its mempool map; otherwise the lock is parked in pending_is_locks and later re-applied through the guarded check_core_transaction path:

rust-dashcore/dash-spv/src/sync/mempool/manager.rs

Lines 608 to 626 in 0da39eb

	let instant_lock_opt = if let Some(tx) = self.transactions.get_mut(&txid) {
	tx.is_instant_send = true;
	tracing::debug!("Marked mempool tx {} as InstantSend-locked", txid);
	Some(instant_lock)
	} else if self.pending_is_locks.len() < MAX_PENDING_IS_LOCKS {
	self.pending_is_locks.insert(txid, (instant_lock, Instant::now()));
	tracing::debug!("IS lock arrived before tx {}, remembering for later", txid);
	None
	} else {
	tracing::warn!(
	"Pending IS locks at capacity ({}), dropping IS lock for {}",
	MAX_PENDING_IS_LOCKS,
	txid
	);
	None
	};
	if let Some(lock) = instant_lock_opt {
	let mut wallet = self.wallet.write().await;
	wallet.process_instant_send_lock(lock);

So in steady state the unguarded overwrite is not a routine path. It is reachable in the ordering window between the wallet applying a block that contains the transaction and the mempool manager evicting it (remove_confirmed, driven by SyncEvent::BlockProcessed) — an IS lock that arrives in that window reaches mark_instant_send_utxos for an already-mined record. Any future caller of the public trait method inherits the same hazard.

Visible symptom downstream: ChainLockProcessed with no locked transactions for the affected record, and consumers that need a mined height for it (for example rs-platform-wallet's asset-lock ChainLock proof) wait for a promotion that cannot happen.

Possible Solution

Make mark_instant_send_utxos mirror the guard in wallet_checker.rs (L202-L212): skip the context update when account.transaction_is_finalized(txid) or record.is_confirmed(). Whether the UTXOs should still be flagged IS-locked and the conflict sweep still run in that case is a maintainer call; the sweep is harmless for a mined transaction.

A more general option is a guard inside update_context that refuses to move from a block context back to Mempool / InstantSend; that would also cover the re-broadcast re-sighting downgrade described in dashpay/platform#4238, but touches more callers. Keeping the InstantLock on a mined record is the reverse-direction problem in #763 and can be solved together.

Wallets already in this state are repaired on the consumer side (dashpay/platform, ticket 32167); this change stops new records from entering it.

Steps to Reproduce

Unit-level, no network needed:

1. Record a wallet-owned transaction with TransactionContext::InBlock(BlockInfo { height: H, .. }).
2. Apply its InstantLock — in a key-wallet-manager unit test via process_instant_send_lock (private; see event_tests.rs for an in-crate caller), or by calling the WalletInfoInterface::mark_instant_send_utxos trait method directly.
3. Observe: the record's context is InstantSend(_) and height() is None.
4. Call apply_chain_lock(H + 10): the record is not in the promoted list and stays InstantSend.

For comparison, feeding the same lock through process_mempool_transaction(&tx, Some(lock)) (→ check_core_transaction) leaves the record InBlock at height H.

Context

• Found while triaging Dash Wallet iOS support ticket 32167: a Core→Shielded transfer whose asset lock can never get a ChainLock proof, because the funding record is InstantSend with no height. Whether this overwrite is how that wallet got there is not proven; the code allows it only through the ordering window described above, so there is no routine path to look for.
• Nothing open changes this path: fix(key-wallet, dash-spv): re-emit late knowledge — record corrections, durable rescans, address-pool repair #979 and the other open key-wallet PRs do not touch mark_instant_send_utxos or update_context.
• Related: Found-021: TransactionRecord::update_context silently drops InstantLock on InBlock/InChainLockedBlock promotion #763 (reverse direction — promotion to a block context drops the InstantLock), Asset-lock ChainLock finality wait has no per-record promotion and no cancellation — stuck locks unrecoverable, app shutdown hangs platform#4238 (asset-lock ChainLock wait with no per-record promotion).

Your Environment

• rust-dashcore dev @ 0da39ebfd716fb47e11a4ac1bdd9a68da27ead16; identical for the cited files at e4208c90786a6854bd498315bcb571ef24182c15, the revision pinned by dashpay/platform v4.2-dev.
• Steps verified with the unit test below, added to key-wallet-manager/src/event_tests.rs (reuses its setup_manager_with_wallet, create_tx_paying_to, make_block, dummy_instant_lock helpers). cargo test -p key-wallet-manager --lib verify_ -- --nocapture at 0da39ebf (the run also printed the context after each step; those println!s are omitted from the code below):

after block: ctx=block 2000 height=Some(2000)
after process_instant_send_lock: ctx=instant send height=None
ChainLockProcessed locked_transactions: [{}]
after apply_chain_lock(2010): ctx=instant send height=None
after process_mempool_transaction(tx, Some(lock)): ctx=block 2000 height=Some(2000)
test result: ok. 2 passed; 0 failed

Test code

fn bip44_context(
    manager: &WalletManager<ManagedWalletInfo>,
    wallet_id: &WalletId,
    txid: &Txid,
) -> Option<(TransactionContext, Option<u32>)> {
    manager
        .get_wallet_info(wallet_id)
        .unwrap()
        .accounts
        .standard_bip44_accounts
        .get(&0)
        .unwrap()
        .transactions()
        .get(txid)
        .map(|r| (r.context.clone(), r.height()))
}

#[tokio::test]
async fn verify_late_is_lock_downgrades_mined_record() {
    let (mut manager, wallet_id, addr) = setup_manager_with_wallet();
    let wallets = BTreeSet::from([wallet_id]);
    let tx = create_tx_paying_to(&addr, 0x67);
    let block = make_block(vec![tx.clone()], 0x67, 1_500);
    manager.process_block_for_wallets(&block, block.block_hash(), 2_000, &wallets).await;
    let (ctx, h) = bip44_context(&manager, &wallet_id, &tx.txid()).expect("record");
    assert!(matches!(ctx, TransactionContext::InBlock(_)));
    assert_eq!(h, Some(2_000));

    manager.process_instant_send_lock(dummy_instant_lock(tx.txid()));
    let (ctx, h) = bip44_context(&manager, &wallet_id, &tx.txid()).expect("record");
    assert!(matches!(ctx, TransactionContext::InstantSend(_)));
    assert_eq!(h, None);

    manager.apply_chain_lock(ChainLock::dummy(2_010));
    let (ctx, _) = bip44_context(&manager, &wallet_id, &tx.txid()).expect("record still stored");
    assert!(matches!(ctx, TransactionContext::InstantSend(_)));
}

#[tokio::test]
async fn verify_check_core_transaction_path_keeps_block_context() {
    let (mut manager, wallet_id, addr) = setup_manager_with_wallet();
    let wallets = BTreeSet::from([wallet_id]);
    let tx = create_tx_paying_to(&addr, 0x68);
    let block = make_block(vec![tx.clone()], 0x68, 1_500);
    manager.process_block_for_wallets(&block, block.block_hash(), 2_000, &wallets).await;

    manager.process_mempool_transaction(&tx, Some(dummy_instant_lock(tx.txid()))).await;
    let (ctx, h) = bip44_context(&manager, &wallet_id, &tx.txid()).expect("record");
    assert!(matches!(ctx, TransactionContext::InBlock(_)));
    assert_eq!(h, Some(2_000));
}