Skip to content

fix: correct confirmations for UTXOs in wallet FFI #306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xdustinface wants to merge 1 commit into
base: v0.42-dev
Choose a base branch
from
+7 −5
Open

fix: correct confirmations for UTXOs in wallet FFI #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions key-wallet-ffi/src/utxo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,9 @@ pub unsafe extern "C" fn managed_wallet_get_utxos(
// Get script bytes
let script_bytes = utxo.txout.script_pubkey.as_bytes().to_vec();

// Calculate confirmations (0 if unconfirmed)
let confirmations = if utxo.is_confirmed {
1
let current_height = managed_info.inner().synced_height();
let confirmations = if utxo.is_confirmed && utxo.height > 0 {
current_height.saturating_sub(utxo.height) + 1
} else {
0
};
Comment on lines +122 to 127
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical bug: Incorrect confirmation count when current_height < utxo.height.

The calculation current_height.saturating_sub(utxo.height) + 1 returns 1 confirmation when current_height < utxo.height, which is incorrect. When the synced height hasn't caught up to the UTXO's block height (e.g., during initial sync or reorg), the code should return 0 confirmations, not 1.

The existing pattern in immature_transaction.rs:120-126 correctly checks current_height >= self.height before adding 1.

🔎 Proposed fix to match the established pattern 
 let current_height = managed_info.inner().synced_height();
-let confirmations = if utxo.is_confirmed && utxo.height > 0 {
-    current_height.saturating_sub(utxo.height) + 1
+let confirmations = if utxo.is_confirmed && utxo.height > 0 && current_height >= utxo.height {
+    (current_height - utxo.height) + 1
 } else {
     0
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let current_height = managed_info.inner().synced_height();
let confirmations = if utxo.is_confirmed && utxo.height > 0 {
current_height.saturating_sub(utxo.height) + 1
} else {
0
};
let current_height = managed_info.inner().synced_height();
let confirmations = if utxo.is_confirmed && utxo.height > 0 && current_height >= utxo.height {
(current_height - utxo.height) + 1
} else {
0
};
🤖 Prompt for AI Agents 
In key-wallet-ffi/src/utxo.rs around lines 122 to 127, the confirmation
calculation uses current_height.saturating_sub(utxo.height) + 1 which yields 1
when current_height < utxo.height; change the logic to first require
current_height >= utxo.height before computing confirmations (e.g., if
utxo.is_confirmed && current_height >= utxo.height { current_height -
utxo.height + 1 } else { 0 }) so that heights not yet reached return 0
confirmations.

Expand Down
6 changes: 4 additions & 2 deletions key-wallet-ffi/src/utxo_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ mod utxo_tests {
managed_info.accounts.insert(bip44_account);

let ffi_managed_info = Box::into_raw(Box::new(FFIManagedWalletInfo::new(managed_info)));

unsafe { (*ffi_managed_info).inner_mut() }.update_synced_height(300);
let result = unsafe {
managed_wallet_get_utxos(&*ffi_managed_info, &mut utxos_out, &mut count_out, error)
};
Expand All @@ -253,19 +253,21 @@ mod utxo_tests {
assert_eq!(utxos[0].vout, 0);
assert_eq!(utxos[0].amount, 50000);
assert_eq!(utxos[0].height, 100);
assert_eq!(utxos[0].confirmations, 1);
assert_eq!(utxos[0].confirmations, 201);

// Check second UTXO
assert_eq!(utxos[1].txid[0], 1);
assert_eq!(utxos[1].vout, 1);
assert_eq!(utxos[1].amount, 100000);
assert_eq!(utxos[1].height, 101);
assert_eq!(utxos[1].confirmations, 200);

// Check third UTXO
assert_eq!(utxos[2].txid[0], 2);
assert_eq!(utxos[2].vout, 2);
assert_eq!(utxos[2].amount, 150000);
assert_eq!(utxos[2].height, 102);
assert_eq!(utxos[2].confirmations, 199);
Comment on lines +256 to +270
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add test coverage for edge cases in confirmation calculation.

The updated confirmation values (201, 200, 199) correctly reflect the calculation based on synced_height(300) and UTXO heights (100, 101, 102). However, the test suite is missing coverage for important edge cases:

  1. Unconfirmed UTXOs (is_confirmed = false) should return 0 confirmations
  2. UTXOs with height = 0 should return 0 confirmations
  3. When current_height < utxo.height (e.g., during sync or reorg) should return 0 confirmations
  4. Boundary case: current_height = utxo.height should return 1 confirmation

Based on coding guidelines, consider adding property-based tests for confirmation invariants and covering mainnet/testnet configurations.

🤖 Prompt for AI Agents 
In key-wallet-ffi/src/utxo_tests.rs around lines 256 to 270, the existing
assertions check normal confirmation counts but lack edge-case coverage; add
unit tests that (1) assert confirmations == 0 when is_confirmed == false, (2)
assert confirmations == 0 when utxo.height == 0, (3) assert confirmations == 0
when current_height < utxo.height (simulate reorg/sync), and (4) assert
confirmations == 1 when current_height == utxo.height; additionally add a
property-based test to validate confirmation invariants across random heights
and a small matrix to run the same cases under mainnet and testnet
configurations to ensure behavior is consistent.

}

// Clean up
Expand Down