Skip to content

refactor: use BlockInfo in TransactionRecord #580

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

Closed
xdustinface wants to merge 1 commit into from
Closed

refactor: use BlockInfo in TransactionRecord #580

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
4 changes: 2 additions & 2 deletions key-wallet-ffi/FFI_API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,7 @@ Get the parent wallet ID of a managed account Note: ManagedAccount doesn't stor
#### `managed_wallet_check_transaction`

```c
managed_wallet_check_transaction(managed_wallet: *mut FFIManagedWalletInfo, wallet: *mut FFIWallet, tx_bytes: *const u8, tx_len: usize, context_type: FFITransactionContext, block_height: c_uint, block_hash: *const u8, // 32 bytes if not null timestamp: u64, update_state: bool, result_out: *mut FFITransactionCheckResult, error: *mut FFIError,) -> bool
managed_wallet_check_transaction(managed_wallet: *mut FFIManagedWalletInfo, wallet: *mut FFIWallet, tx_bytes: *const u8, tx_len: usize, context_type: FFITransactionContext, block_info: FFIBlockInfo, update_state: bool, result_out: *mut FFITransactionCheckResult, error: *mut FFIError,) -> bool
```

**Description:**
Expand Down Expand Up @@ -1300,7 +1300,7 @@ Build and sign a transaction using the wallet's managed info This is the recomm
#### `wallet_check_transaction`

```c
wallet_check_transaction(wallet: *mut FFIWallet, tx_bytes: *const u8, tx_len: usize, context_type: FFITransactionContext, block_height: u32, block_hash: *const u8, // 32 bytes if not null timestamp: u64, update_state: bool, result_out: *mut FFITransactionCheckResult, error: *mut FFIError,) -> bool
wallet_check_transaction(wallet: *mut FFIWallet, tx_bytes: *const u8, tx_len: usize, context_type: FFITransactionContext, block_info: FFIBlockInfo, update_state: bool, result_out: *mut FFITransactionCheckResult, error: *mut FFIError,) -> bool
```

**Description:**
Expand Down
25 changes: 6 additions & 19 deletions key-wallet-ffi/src/managed_account.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use dashcore::hashes::Hash;

use crate::address_pool::{FFIAddressPool, FFIAddressPoolType};
use crate::error::{FFIError, FFIErrorCode};
use crate::types::FFIAccountType;
use crate::types::{FFIAccountType, FFIBlockInfo};
use crate::wallet_manager::FFIWalletManager;
use crate::FFINetwork;
use key_wallet::account::account_collection::{DashpayAccountKey, PlatformPaymentAccountKey};
Expand Down Expand Up @@ -666,12 +666,8 @@ pub struct FFITransactionRecord {
pub txid: [u8; 32],
/// Net amount for this account (positive = received, negative = sent)
pub net_amount: i64,
/// Block height if confirmed, 0 if unconfirmed
pub height: u32,
/// Block hash if confirmed (32 bytes), all zeros if unconfirmed
pub block_hash: [u8; 32],
/// Unix timestamp
pub timestamp: u64,
/// Block info (zeroed if unconfirmed)
pub block_info: FFIBlockInfo,
/// Fee if known, 0 if unknown
pub fee: u64,
/// Whether this is our transaction
Expand Down Expand Up @@ -729,18 +725,9 @@ pub unsafe extern "C" fn managed_core_account_get_transactions(
// Copy net amount
ffi_record.net_amount = record.net_amount;

// Copy height (0 if unconfirmed)
ffi_record.height = record.height.unwrap_or(0);

// Copy block hash (zeros if unconfirmed)
if let Some(block_hash) = record.block_hash {
ffi_record.block_hash = block_hash.to_byte_array();
} else {
ffi_record.block_hash = [0u8; 32];
}

// Copy timestamp
ffi_record.timestamp = record.timestamp;
// Copy block info
ffi_record.block_info =
record.block_info.map(FFIBlockInfo::from).unwrap_or_else(FFIBlockInfo::empty);
Comment on lines +728 to +730
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 25, 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 a direct test for managed_core_account_get_transactions block export.

This function now emits FFIBlockInfo::empty() for mempool records and a populated FFIBlockInfo for confirmed ones, but this file still does not assert either behavior. A small unit test here would lock down the new boundary behavior.
As per coding guidelines, "Write unit tests for new functionality in Rust code".

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@key-wallet-ffi/src/managed_account.rs` around lines 728 - 730, Add a unit
test that calls managed_core_account_get_transactions and asserts the
FFIBlockInfo behavior for mempool vs confirmed records: create or mock one
mempool transaction record and one confirmed transaction record, call
managed_core_account_get_transactions (or the helper that produces ffi Record
structs), and assert that the mempool record's block_info equals
FFIBlockInfo::empty() while the confirmed record's block_info equals
FFIBlockInfo::from(...) with the expected populated fields; place the test in
managed_account.rs alongside other tests to lock down the new boundary behavior.


// Copy fee (0 if unknown)
ffi_record.fee = record.fee.unwrap_or(0);
Expand Down
22 changes: 5 additions & 17 deletions key-wallet-ffi/src/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoIn
use secp256k1::{Message, Secp256k1, SecretKey};

use crate::error::{FFIError, FFIErrorCode};
use crate::types::{block_info_from_ffi, FFINetwork, FFITransactionContext, FFIWallet};
use crate::types::{
transaction_context_from_ffi, FFIBlockInfo, FFINetwork, FFITransactionContext, FFIWallet,
};
use crate::FFIWalletManager;

// MARK: - Transaction Types
Expand Down Expand Up @@ -388,9 +390,7 @@ pub unsafe extern "C" fn wallet_check_transaction(
tx_bytes: *const u8,
tx_len: usize,
context_type: FFITransactionContext,
block_height: u32,
block_hash: *const u8, // 32 bytes if not null
timestamp: u64,
block_info: FFIBlockInfo,
update_state: bool,
result_out: *mut FFITransactionCheckResult,
error: *mut FFIError,
Expand Down Expand Up @@ -419,19 +419,7 @@ pub unsafe extern "C" fn wallet_check_transaction(
};

// Build the transaction context
use key_wallet::transaction_checking::TransactionContext;
let context = match context_type {
FFITransactionContext::Mempool => TransactionContext::Mempool,
FFITransactionContext::InBlock => {
let info = block_info_from_ffi(block_height, block_hash, timestamp);
TransactionContext::InBlock(info)
}
FFITransactionContext::InChainLockedBlock => {
let info = block_info_from_ffi(block_height, block_hash, timestamp);
TransactionContext::InChainLockedBlock(info)
}
FFITransactionContext::InstantSend => TransactionContext::InstantSend,
};
let context = transaction_context_from_ffi(context_type, &block_info);

// Create a ManagedWalletInfo from the wallet
use key_wallet::transaction_checking::WalletTransactionChecker;
Expand Down
21 changes: 4 additions & 17 deletions key-wallet-ffi/src/transaction_checking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ use std::slice;

use crate::error::{FFIError, FFIErrorCode};
use crate::managed_wallet::{managed_wallet_info_free, FFIManagedWalletInfo};
use crate::types::{block_info_from_ffi, FFITransactionContext, FFIWallet};
use crate::types::{transaction_context_from_ffi, FFIBlockInfo, FFITransactionContext, FFIWallet};
use dashcore::consensus::Decodable;
use dashcore::Transaction;
use key_wallet::transaction_checking::{
account_checker::CoreAccountTypeMatch, TransactionContext, WalletTransactionChecker,
account_checker::CoreAccountTypeMatch, WalletTransactionChecker,
};
use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;

Expand Down Expand Up @@ -112,9 +112,7 @@ pub unsafe extern "C" fn managed_wallet_check_transaction(
tx_bytes: *const u8,
tx_len: usize,
context_type: FFITransactionContext,
block_height: c_uint,
block_hash: *const u8, // 32 bytes if not null
timestamp: u64,
block_info: FFIBlockInfo,
update_state: bool,
result_out: *mut FFITransactionCheckResult,
error: *mut FFIError,
Expand All @@ -141,18 +139,7 @@ pub unsafe extern "C" fn managed_wallet_check_transaction(
};

// Build the transaction context
let context = match context_type {
FFITransactionContext::Mempool => TransactionContext::Mempool,
FFITransactionContext::InBlock => {
let info = block_info_from_ffi(block_height, block_hash, timestamp);
TransactionContext::InBlock(info)
}
FFITransactionContext::InChainLockedBlock => {
let info = block_info_from_ffi(block_height, block_hash, timestamp);
TransactionContext::InChainLockedBlock(info)
}
FFITransactionContext::InstantSend => TransactionContext::InstantSend,
};
let context = transaction_context_from_ffi(context_type, &block_info);

// Check the transaction - wallet is now required
if wallet.is_null() {
Expand Down
122 changes: 65 additions & 57 deletions key-wallet-ffi/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,61 @@
use dashcore::hashes::Hash;
use key_wallet::transaction_checking::{BlockInfo, TransactionContext};
use key_wallet::{Network, Wallet};
use std::os::raw::{c_char, c_uint};
use std::os::raw::c_char;
use std::sync::Arc;

/// Convert FFI block parameters to a `BlockInfo`.
///
/// # Safety
///
/// If `block_hash` is non-null it must point to 32 readable bytes.
pub(crate) unsafe fn block_info_from_ffi(
height: u32,
block_hash: *const u8,
timestamp: u64,
) -> BlockInfo {
let block_hash = if !block_hash.is_null() {
let hash_bytes = std::slice::from_raw_parts(block_hash, 32);
let mut arr = [0u8; 32];
arr.copy_from_slice(hash_bytes);
dashcore::BlockHash::from_byte_array(arr)
} else {
dashcore::BlockHash::all_zeros()
};
BlockInfo::new(height, block_hash, timestamp as u32)
/// FFI-compatible block metadata (height, hash, timestamp).
#[repr(C)]
#[derive(Debug, Clone, Copy)]
pub struct FFIBlockInfo {
/// Block height
pub height: u32,
/// Block hash (32 bytes)
pub block_hash: [u8; 32],
/// Unix timestamp
pub timestamp: u32,
}

impl FFIBlockInfo {
/// All-zeros placeholder used for unconfirmed contexts.
pub fn empty() -> Self {
Self {
height: 0,
block_hash: [0u8; 32],
timestamp: 0,
}
}

/// Convert to native `BlockInfo`.
pub fn to_block_info(&self) -> BlockInfo {
let block_hash = dashcore::BlockHash::from_byte_array(self.block_hash);
BlockInfo::new(self.height, block_hash, self.timestamp)
}
}

impl From<BlockInfo> for FFIBlockInfo {
fn from(info: BlockInfo) -> Self {
Self {
height: info.height(),
block_hash: info.block_hash().to_byte_array(),
timestamp: info.timestamp(),
}
}
}

/// Convert an `FFIBlockInfo` and context type to a native `TransactionContext`.
pub(crate) fn transaction_context_from_ffi(
context_type: FFITransactionContext,
block_info: &FFIBlockInfo,
) -> TransactionContext {
match context_type {
FFITransactionContext::Mempool => TransactionContext::Mempool,
FFITransactionContext::InstantSend => TransactionContext::InstantSend,
FFITransactionContext::InBlock => TransactionContext::InBlock(block_info.to_block_info()),
FFITransactionContext::InChainLockedBlock => {
TransactionContext::InChainLockedBlock(block_info.to_block_info())
}
}
Comment on lines +48 to +60
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Reject the zero placeholder for confirmed contexts.

FFIBlockInfo::empty() is the documented placeholder for mempool/instant-send, but transaction_context_from_ffi() will currently accept that same zeroed struct for InBlock / InChainLockedBlock and build a fake BlockInfo at height 0 with an all-zero hash. One bad FFI call can then mark a transaction as confirmed with invalid block metadata and corrupt wallet state. Returning an error here instead of an infallible TransactionContext is probably the cleanest fix.

As per coding guidelines, **/*-ffi/**/*.rs: Handle memory safety carefully at FFI boundaries.

Also applies to: 793-827

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@key-wallet-ffi/src/types.rs` around lines 48 - 60, The current
transaction_context_from_ffi function constructs confirmed contexts from a
zeroed FFIBlockInfo, which can create invalid BlockInfo; change
transaction_context_from_ffi(signature) to return a Result<TransactionContext,
<appropriate error type>> and validate that for FFITransactionContext::InBlock
and ::InChainLockedBlock the provided block_info is not the placeholder (use
FFIBlockInfo::empty() or equivalent check); if it is empty return an error
instead of creating TransactionContext::InBlock/::InChainLockedBlock with a fake
block, and update all callers to propagate or handle the Result accordingly to
avoid silently accepting zeroed block metadata at the FFI boundary.

}

/// FFI Network type (single network)
Expand Down Expand Up @@ -760,62 +793,37 @@ impl From<TransactionContext> for FFITransactionContext {
pub struct FFITransactionContextDetails {
/// The context type
pub context_type: FFITransactionContext,
/// Block height (0 for mempool)
pub height: c_uint,
/// Block hash (32 bytes, null for mempool or if unknown)
pub block_hash: *const u8,
/// Timestamp (0 if unknown)
pub timestamp: c_uint,
/// Block info (zeroed for mempool/instant-send contexts)
pub block_info: FFIBlockInfo,
}

impl FFITransactionContextDetails {
/// Create a mempool context
pub fn mempool() -> Self {
FFITransactionContextDetails {
Self {
context_type: FFITransactionContext::Mempool,
height: 0,
block_hash: std::ptr::null(),
timestamp: 0,
block_info: FFIBlockInfo::empty(),
}
}

/// Create an in-block context
pub fn in_block(height: c_uint, block_hash: *const u8, timestamp: c_uint) -> Self {
FFITransactionContextDetails {
pub fn in_block(block_info: FFIBlockInfo) -> Self {
Self {
context_type: FFITransactionContext::InBlock,
height,
block_hash,
timestamp,
block_info,
}
}

/// Create a chain-locked block context
pub fn in_chain_locked_block(height: c_uint, block_hash: *const u8, timestamp: c_uint) -> Self {
FFITransactionContextDetails {
pub fn in_chain_locked_block(block_info: FFIBlockInfo) -> Self {
Self {
context_type: FFITransactionContext::InChainLockedBlock,
height,
block_hash,
timestamp,
block_info,
}
}

/// Convert to the native TransactionContext
/// Convert to the native `TransactionContext`
pub fn to_transaction_context(&self) -> TransactionContext {
match self.context_type {
FFITransactionContext::Mempool => TransactionContext::Mempool,
FFITransactionContext::InBlock => {
let info = unsafe {
block_info_from_ffi(self.height, self.block_hash, self.timestamp as u64)
};
TransactionContext::InBlock(info)
}
FFITransactionContext::InChainLockedBlock => {
let info = unsafe {
block_info_from_ffi(self.height, self.block_hash, self.timestamp as u64)
};
TransactionContext::InChainLockedBlock(info)
}
FFITransactionContext::InstantSend => TransactionContext::InstantSend,
}
transaction_context_from_ffi(self.context_type, &self.block_info)
}
}
33 changes: 15 additions & 18 deletions key-wallet-ffi/src/wallet_manager_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,19 +627,14 @@ mod tests {
];

// Create transaction contexts for testing
let mempool_context = crate::types::FFITransactionContextDetails {
context_type: crate::types::FFITransactionContext::Mempool,
height: 0,
block_hash: ptr::null(),
timestamp: 0,
};
let mempool_context = crate::types::FFITransactionContextDetails::mempool();

let block_context = crate::types::FFITransactionContextDetails {
context_type: crate::types::FFITransactionContext::InBlock,
height: 100000,
block_hash: ptr::null(),
timestamp: 1234567890,
};
let block_context =
crate::types::FFITransactionContextDetails::in_block(crate::types::FFIBlockInfo {
height: 100000,
block_hash: [0u8; 32],
timestamp: 1234567890,
});

// Test processing a mempool transaction
let processed = unsafe {
Expand Down Expand Up @@ -672,12 +667,14 @@ mod tests {
assert_eq!(unsafe { (*error).code }, FFIErrorCode::InvalidInput);

// Test processing a chain-locked block transaction
let chain_locked_context = crate::types::FFITransactionContextDetails {
context_type: crate::types::FFITransactionContext::InChainLockedBlock,
height: 100000,
block_hash: ptr::null(),
timestamp: 1234567890,
};
let chain_locked_context =
crate::types::FFITransactionContextDetails::in_chain_locked_block(
crate::types::FFIBlockInfo {
height: 100000,
block_hash: [0u8; 32],
timestamp: 1234567890,
},
);
let processed = unsafe {
wallet_manager::wallet_manager_process_transaction(
manager,
Expand Down
18 changes: 5 additions & 13 deletions key-wallet/src/managed_account/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,13 +344,9 @@ impl ManagedCoreAccount {
value: output.value,
script_pubkey: output.script_pubkey.clone(),
};
let mut utxo = Utxo::new(
outpoint,
txout,
addr,
context.block_info().map(|i| i.height).unwrap_or(0),
tx.is_coin_base(),
);
let block_height = context.block_info().map_or(0, |info| info.height);
let mut utxo =
Utxo::new(outpoint, txout, addr, block_height, tx.is_coin_base());
utxo.is_confirmed = context.confirmed();
utxo.is_instantlocked =
matches!(context, TransactionContext::InstantSend);
Expand Down Expand Up @@ -393,8 +389,7 @@ impl ManagedCoreAccount {
if let Some(tx_record) = self.transactions.get_mut(&tx.txid()) {
if !tx_record.is_confirmed() {
if let Some(info) = context.block_info() {
tx_record.mark_confirmed(info.height, info.block_hash);
tx_record.timestamp = info.timestamp as u64;
tx_record.mark_confirmed(*info);
changed = true;
}
}
Expand All @@ -411,13 +406,10 @@ impl ManagedCoreAccount {
context: TransactionContext,
) {
let net_amount = account_match.received as i64 - account_match.sent as i64;
let block_info = context.block_info();
let tx_record = TransactionRecord {
transaction: tx.clone(),
txid: tx.txid(),
height: block_info.map(|i| i.height),
block_hash: block_info.map(|i| i.block_hash),
timestamp: block_info.map(|i| i.timestamp as u64).unwrap_or(0),
block_info: context.block_info().copied(),
net_amount,
fee: None,
label: None,
Expand Down
Loading
Loading