Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
205 changes: 204 additions & 1 deletion packages/rs-platform-wallet-ffi/src/persistence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ use crate::wallet_registration_persistence::AccountAddressPoolFFI;
use crate::wallet_restore_types::{
AccountSpecFFI, AccountTypeTagFFI, ContactProfileRestoreEntryFFI, IdentityKeyRestoreFFI,
IdentityRestoreEntryFFI, LoadWalletListFreeFn, PaymentRestoreEntryFFI,
ProviderSpecialTxRestoreEntryFFI, StandardAccountTypeTagFFI, UnresolvedAssetLockTxRecordFFI,
ProviderSpecialTxRestoreEntryFFI, StandardAccountTypeTagFFI, UnconfirmedOutgoingTxRecordFFI,
UnresolvedAssetLockTxRecordFFI,
UtxoRestoreEntryFFI, WalletRestoreEntryFFI,
};
use dpp::address_funds::PlatformAddress;
Expand Down Expand Up @@ -4852,6 +4853,45 @@ impl Drop for LoadGuard {
}
}

/// Put a batch of unconfirmed outgoing sends into replay order.
///
/// `first_seen` establishes the baseline, but the host records it in whole
/// seconds, so two sends a moment apart share one and their relative order is
/// undefined. A dependency pass then moves any send that spends another send
/// in the same batch behind it — replaying a child first leaves it with no
/// input to spend, so it is discarded as irrelevant and that send's replay is
/// silently lost.
fn order_unconfirmed_outgoing(
mut decoded: Vec<(u64, dashcore::blockdata::transaction::Transaction)>,
) -> Vec<dashcore::blockdata::transaction::Transaction> {
decoded.sort_by_key(|(first_seen, _)| *first_seen);

let in_batch: std::collections::HashSet<_> =
decoded.iter().map(|(_, tx)| tx.txid()).collect();
let mut emitted: std::collections::HashSet<_> = std::collections::HashSet::new();
let mut ordered = Vec::with_capacity(decoded.len());
let mut queue: std::collections::VecDeque<_> = decoded.into_iter().collect();
// Bounded: a full lap with nothing emitted means the remainder depends on
// itself, which valid transactions cannot do. Emit in `first_seen` order
// rather than spin.
let mut passed_over = 0usize;
while let Some((first_seen, tx)) = queue.pop_front() {
let waits_on_batch_peer = tx.input.iter().any(|input| {
let parent = input.previous_output.txid;
in_batch.contains(&parent) && !emitted.contains(&parent)
});
if waits_on_batch_peer && passed_over <= queue.len() {
queue.push_back((first_seen, tx));
passed_over += 1;
continue;
}
emitted.insert(tx.txid());
ordered.push(tx);
passed_over = 0;
}
ordered
}

/// Reconstruct an external-signable [`Wallet`] + matching start-state
/// bucket from a single `WalletRestoreEntryFFI`. The mnemonic / seed
/// stays in the host's keychain; signing requests route back through
Expand Down Expand Up @@ -5482,11 +5522,71 @@ fn build_wallet_start_state(
// was interrupted by an app kill can resume from the latest
// status without rebroadcasting.
let unused_asset_locks = build_unused_asset_locks(entry)?;

// Decode the sends the host still holds as unconfirmed. Decode
// only: applying the spend needs `check_core_transaction`, which is
// async and wants the `Wallet` and the `ManagedWalletInfo`
// together, so the replay happens at the async boundary in
// `manager::load::load_from_persistor`. See
// `ClientWalletStartState::unconfirmed_outgoing_txs`.
//
// Ordered so a parent send is replayed before a child that spends its
// change — a child applied first finds its input absent and is dropped
// as irrelevant, silently losing that send's replay.
//
// `first_seen` alone cannot express this: the host stores it in whole
// seconds, and two sends a moment apart share one. So the `first_seen`
// sort only establishes a stable starting order, and a dependency pass
// then moves any send that spends another send in the same batch behind
// it.
let unconfirmed_outgoing_txs = {
use dashcore::consensus::Decodable;
let recs: &[UnconfirmedOutgoingTxRecordFFI] = if entry
.unconfirmed_outgoing_tx_records
.is_null()
|| entry.unconfirmed_outgoing_tx_records_count == 0
{
&[]
} else {
unsafe {
slice::from_raw_parts(
entry.unconfirmed_outgoing_tx_records,
entry.unconfirmed_outgoing_tx_records_count,
)
}
};
let mut decoded: Vec<(u64, dashcore::blockdata::transaction::Transaction)> =
Vec::with_capacity(recs.len());
let mut dropped_decode = 0usize;
for rec in recs {
if rec.tx_bytes.is_null() || rec.tx_bytes_len == 0 {
dropped_decode += 1;
continue;
}
let bytes = unsafe { slice::from_raw_parts(rec.tx_bytes, rec.tx_bytes_len) };
match dashcore::blockdata::transaction::Transaction::consensus_decode(
&mut &bytes[..],
) {
Ok(tx) => decoded.push((rec.first_seen, tx)),
Err(_) => dropped_decode += 1,
Comment thread
romchornyi marked this conversation as resolved.
Outdated
}
}
if dropped_decode > 0 {
tracing::warn!(
wallet_id = %hex::encode(entry.wallet_id),
dropped_decode,
"load: unconfirmed outgoing tx records failed to decode"
);
}
order_unconfirmed_outgoing(decoded)
};

let wallet_state = ClientWalletStartState {
wallet,
wallet_info,
identity_manager,
unused_asset_locks,
unconfirmed_outgoing_txs,
};

let platform_address_state = if per_account.is_empty()
Expand Down Expand Up @@ -6658,6 +6758,109 @@ mod tests {
//! restoration loops that don't need the full FFI plumbing —
//! exercising the in-memory mutation against synthetic input.

mod unconfirmed_outgoing_order {
use super::super::order_unconfirmed_outgoing;
use dashcore::blockdata::transaction::Transaction;
use dashcore::{OutPoint, ScriptBuf, TxIn, TxOut};

fn tx_spending(parents: &[(dashcore::Txid, u32)], value: u64) -> Transaction {
Transaction {
version: 2,
lock_time: 0,
input: parents
.iter()
.map(|(txid, vout)| TxIn {
previous_output: OutPoint {
txid: *txid,
vout: *vout,
},
script_sig: ScriptBuf::new(),
sequence: 0xffff_ffff,
witness: Default::default(),
})
.collect(),
output: vec![TxOut {
value,
script_pubkey: ScriptBuf::new(),
}],
special_transaction_payload: None,
}
}

fn root(value: u64) -> Transaction {
tx_spending(
&[(
"0000000000000000000000000000000000000000000000000000000000000001"
.parse()
.expect("static txid"),
0,
)],
value,
)
}

/// The host stores `first_seen` in whole seconds, so a parent and the
/// child spending its change can share one. Replaying the child first
/// leaves it with no input and it is dropped as irrelevant — that
/// send's replay is then silently lost, which is the whole failure
/// this ordering exists to prevent.
#[test]
fn a_child_sharing_its_parents_second_is_replayed_after_it() {
let parent = root(50_000);
let child = tx_spending(&[(parent.txid(), 0)], 40_000);

// Child offered first, identical timestamps: nothing but the
// dependency pass can separate them.
let ordered = order_unconfirmed_outgoing(vec![
(1_700_000_000, child.clone()),
(1_700_000_000, parent.clone()),
]);

assert_eq!(
ordered.iter().map(|tx| tx.txid()).collect::<Vec<_>>(),
vec![parent.txid(), child.txid()],
"the parent must be replayed before the child that spends it"
);
}

/// A chain of three, offered fully reversed and all in one second.
#[test]
fn a_reversed_chain_is_restored_to_dependency_order() {
let a = root(90_000);
let b = tx_spending(&[(a.txid(), 0)], 80_000);
let c = tx_spending(&[(b.txid(), 0)], 70_000);

let ordered = order_unconfirmed_outgoing(vec![
(1_700_000_000, c.clone()),
(1_700_000_000, b.clone()),
(1_700_000_000, a.clone()),
]);

assert_eq!(
ordered.iter().map(|tx| tx.txid()).collect::<Vec<_>>(),
vec![a.txid(), b.txid(), c.txid()]
);
}

/// Sends that do not depend on each other keep the order `first_seen`
/// gave them — the dependency pass must not reshuffle the baseline.
#[test]
fn independent_sends_keep_their_first_seen_order() {
let older = root(10_000);
let newer = root(20_000);

let ordered = order_unconfirmed_outgoing(vec![
(1_700_000_050, newer.clone()),
(1_700_000_000, older.clone()),
]);

assert_eq!(
ordered.iter().map(|tx| tx.txid()).collect::<Vec<_>>(),
vec![older.txid(), newer.txid()]
);
}
}

use super::*;

// --- persists_durably: the fail-closed durability attestation ---
Expand Down
27 changes: 27 additions & 0 deletions packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,21 @@ pub struct ProviderSpecialTxRestoreEntryFFI {
pub first_seen: u64,
}

/// One outgoing transaction the host still holds as unconfirmed,
/// replayed at load so its spend effect survives a restart.
#[repr(C)]
pub struct UnconfirmedOutgoingTxRecordFFI {
/// Consensus-encoded transaction body, the same wire format
/// `dashcore::consensus::encode::serialize` produces. Swift-owned
/// for the callback window; freed by `LoadWalletListFreeFn`.
pub tx_bytes: *mut u8,
pub tx_bytes_len: usize,
/// Host's `firstSeen` for the row, in seconds. The load path
/// replays in ascending order so a parent send is applied before a
/// child that spends its change.
pub first_seen: u64,
}

/// Per-wallet entry returned by `on_load_wallet_list_fn`.
///
/// `accounts` points to a contiguous array of length `accounts_count`.
Expand Down Expand Up @@ -644,6 +659,16 @@ pub struct WalletRestoreEntryFFI {
/// unresolved asset locks.
pub unresolved_asset_lock_tx_records: *const UnresolvedAssetLockTxRecordFFI,
pub unresolved_asset_lock_tx_records_count: usize,
/// Outgoing transactions the host still holds as unconfirmed
/// (mempool context, no block height), oldest `first_seen` first.
///
/// Replayed at load through the ordinary mempool check so their
/// spend effect is restored — see
/// [`UnconfirmedOutgoingTxRecordFFI`]. `null` / `0` when the wallet
/// has none. Each entry's `tx_bytes` buffer is Swift-owned and
/// freed by `LoadWalletListFreeFn`.
pub unconfirmed_outgoing_tx_records: *const UnconfirmedOutgoingTxRecordFFI,
pub unconfirmed_outgoing_tx_records_count: usize,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 Blocking: Preserve the wallet restore FFI layout or version the ABI

WalletRestoreEntryFFI is a #[repr(C)] struct shared across the FFI boundary, but the new unconfirmed_outgoing_tx_records fields were inserted before the existing provider, address-pool, and chain-lock fields. This changes both the offsets and the size/array stride of the callback structure. A host compiled against the previous layout will have its provider pointer and subsequent fields read at the wrong Rust offsets, and the new Rust code can read beyond the old allocation. Initializing the new fields to null and zero only protects hosts rebuilt against the new definition; it does not make an older binary pass null/zero for fields that did not exist. Preserve the legacy callback structure and add replay data through a separately versioned callback/API, or add an explicit structure size/version handshake and refuse to read fields beyond the supplied size. Add a legacy-layout compatibility test.

source: gpt-6-astra (phase2-reviewer: general, ffi-engineer)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8fc9ee95c5, though not the way you proposed — worth saying why.

The offset shift was a real defect and there was no reason for it: the two fields went in ahead of provider_special_txs, core_address_pools and last_applied_chain_lock_bytes and moved all three. They are now at the end of the struct, so every existing field sits exactly where it did, and the struct carries a note that additions belong there.

I did not take the stronger remedy — freezing the callback struct and carrying replay data over a separately versioned channel. That protects against an old host binary meeting new Rust, and I do not think this repo can reach that state: DashSDKFFI.xcframework is a build_ios.sh artifact rather than something checked in, and the Swift package, the JNI bindings and the Rust side are always built from a single checkout. If there is a distribution path where the two halves can diverge that I am not seeing, say so and I will build the versioned channel — I would rather be corrected than guess about an ABI.

On the deterministic resend-lifecycle coverage: agreed, and it is the more valuable of the two findings. The current loader test would still pass with the generation check or the payment guard removed, which is precisely the logic that needed two rounds of review to get right. It needs a controllable TransactionBroadcaster and synchronisation barriers — real work rather than a quick addition. I would rather do it properly as a follow-up, alongside the eligibility-predicate move already agreed with CodeRabbit, than bolt on something shallow here. Tell me if you would rather it blocked this PR and I will do it now.

🤖 Reviewed with Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Still applies (re-reviewed at 8fc9ee95): The latest commit fixes the offset shift for existing fields by appending the replay fields, but an old producer still supplies an allocation that is shorter than the new Rust struct and there is no size/version check before reading the appended fields. The original ABI-safety issue therefore remains for mixed-version binaries.

/// Persisted provider special transactions (ProRegTx / ProUpServTx /
/// ProUpRegTx / ProUpRevTx) re-staged onto the wallet's provider-key
/// accounts so rust-dashcore #876 retention keeps them resident and
Expand Down Expand Up @@ -702,6 +727,8 @@ impl Default for WalletRestoreEntryFFI {
tracked_asset_locks_count: 0,
unresolved_asset_lock_tx_records: std::ptr::null(),
unresolved_asset_lock_tx_records_count: 0,
unconfirmed_outgoing_tx_records: std::ptr::null(),
unconfirmed_outgoing_tx_records_count: 0,
provider_special_txs: std::ptr::null(),
provider_special_txs_count: 0,
core_address_pools: std::ptr::null(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::collections::BTreeMap;

use crate::changeset::identity_manager_start_state::IdentityManagerStartState;
use crate::wallet::asset_lock::tracked::TrackedAssetLock;
use dashcore::OutPoint;
use dashcore::{OutPoint, Transaction};
use key_wallet::wallet::ManagedWalletInfo;
use key_wallet::Wallet;

Expand All @@ -33,4 +33,31 @@ pub struct ClientWalletStartState {
/// Asset locks that have not yet been consumed by an identity
/// registration / top-up, keyed by account index → outpoint.
pub unused_asset_locks: BTreeMap<u32, BTreeMap<OutPoint, TrackedAssetLock>>,
/// Outgoing transactions the host still has as unconfirmed, in
/// `first_seen` order (a parent send precedes a child that spends
/// its change).
///
/// These are decoded here but deliberately NOT applied here: the
/// spend has to travel through the normal
/// `check_core_transaction(.., Mempool, ..)` path so `update_utxos`
/// runs — dropping the input from `utxos` and recording it in
/// `spent_outpoints`. That call is async and needs the `Wallet` and
/// the `ManagedWalletInfo` together, so the replay happens at the
/// async boundary in
/// [`load_from_persistor`](crate::manager::load), not while this
/// snapshot is being built.
///
/// # Why the replay exists
///
/// The spend effect of an unconfirmed outgoing transaction is never
/// persisted: `isSpent` deliberately stays `false` on the input row
/// until the spending transaction reaches a block, because a
/// mempool-only sighting is reversible by eviction. A running app is
/// still correct — the effect lives in memory. Across a restart it
/// used to be recovered only by re-observing the transaction on the
/// network, which is impossible for one that never got there: the
/// input came back as spendable and the balance re-counted it,
/// permanently. Replaying the record at load restores exactly the
/// state the live process held.
pub unconfirmed_outgoing_txs: Vec<Transaction>,
}
Loading
Loading