FFIManagedWalletInfo mutating FFIs silently lose writes when handle came from `wallet_manager_get_managed_wallet_info`

[xdustinface]

Summary

FFIManagedWalletInfo is obtained via two distinct paths with very different semantics, and the FFI type system cannot tell them apart. Mutating FFI functions work correctly only for one of the two paths. Callers pairing a manager-derived handle with a mutating function will get a successful return value while their writes are silently discarded.

Discovered via CodeRabbit review on #618, which flagged the same class of bug for the (newly added) managed_core_account_set_transaction_label. That specific instance is being fixed in PR #618. This issue tracks the broader pattern.

The two paths

1. Standalone — wallet_create_managed_wallet(wallet) in key-wallet-ffi/src/transaction_checking.rs:76. Builds a fresh ManagedWalletInfo from a wallet and hands ownership to the caller. The handle is the state, mutations persist for the handle's lifetime.

2. Detached snapshot — wallet_manager_get_managed_wallet_info(manager, wallet_id) in key-wallet-ffi/src/wallet_manager.rs:618. Acquires a read lock on the shared WalletManager, calls .cloned() on the matching ManagedWalletInfo, then double-clones into a fresh Box. The handle is fully detached from WalletManager::wallet_infos. Mutations do not flow back, and the snapshot is discarded on managed_wallet_info_free.

Both paths return the same opaque C type, so consumers (Swift, Kotlin, C) cannot distinguish them at the type level.

Affected mutating functions

These FFIs call FFIManagedWalletInfo::inner_mut() and mutate the underlying ManagedWalletInfo. All work correctly for path 1, all silently lose writes for path 2:

• managed_wallet_set_gap_limit (address_pool.rs:392)
• managed_wallet_generate_addresses_to_index (address_pool.rs:487)
• managed_wallet_mark_address_used (address_pool.rs:647)
• managed_wallet_get_next_bip44_receive_address (managed_wallet.rs:54), mutates the pool as it advances the cursor
• managed_wallet_get_next_bip44_change_address (managed_wallet.rs:147), same
• managed_wallet_get_bip_44_external_address_range (managed_wallet.rs:242), if it advances the cursor
• managed_wallet_get_bip_44_internal_address_range (managed_wallet.rs:394), same
• managed_wallet_check_transaction (transaction_checking.rs:111), may mutate pool and transaction maps during matching

All of these return success in either mode. There is no way for a caller to detect that their write was dropped.

Why it is a footgun

• Identical C signature, different runtime semantics.
• "Successful" return (true / FFIErrorCode::Success) even when the write never reaches shared storage.
• Arc::get_mut / inner_mut guards are vacuous on a freshly cloned value (refcount always 1), so they cannot catch the mistake.
• A client app that uses the manager for multi-wallet orchestration and naively pairs it with these setters will see data loss that only manifests on the next re-fetch, typically across app sessions.

Possible directions

1. Type-level split: introduce two distinct opaque types, e.g. FFIManagedWalletInfoRef (read-only, returned by the manager) and FFIManagedWalletInfoOwned (standalone, returned by wallet_create_managed_wallet). Make all mutating FFIs accept only FFIManagedWalletInfoOwned. Add manager-routed equivalents (wallet_manager_set_gap_limit, etc.) for writes that must hit shared state.

2. Route all mutations through the manager: deprecate the detached-snapshot path for writes entirely. Anything that needs to mutate wallet state must go through wallet_manager_* functions that acquire the write lock on Arc<RwLock<WalletManager>> and operate on get_wallet_info_mut. The existing pattern in wallet_manager_process_transaction (wallet_manager.rs:~791) is the model.

3. Minimum: document and audit: mark wallet_manager_get_managed_wallet_info as returning a read-only snapshot, document that mutating FFIs must not be paired with it, and audit call sites of each mutating function for correctness.

Option 2 is probably the cleanest long term but requires new FFI surface. Option 1 is enforceable at the C type level but introduces a breaking API change. Option 3 is a stopgap that leaves the footgun but documents it.

Related

• PR #618: narrow fix for the managed_core_account_set_transaction_label instance of this bug.
• CodeRabbit comment: feat: add wallet_manager_set_transaction_label FFI function dashpay/rust-dashcore#618 (comment)