Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
14 changes: 12 additions & 2 deletions packages/rs-platform-wallet-ffi/src/identity_top_up.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,21 @@ pub unsafe extern "C" fn platform_wallet_top_up_from_addresses_with_signer(

let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| {
let identity_wallet = wallet.identity().clone();
let platform_wallet = wallet.platform().clone();
block_on_worker(async move {
let address_signer: &VTableSigner = unsafe { &*(signer_addr as *const VTableSigner) };
identity_wallet
let (address_infos, new_balance) = identity_wallet
.top_up_from_addresses(&identity_id, input_map, address_signer, None)
.await
.await?;
// Reconcile the spent platform-address balances from the proof so
// the wallet's displayed balance and next input selection reflect
// the spend. Resolves spent addresses via the address provider, so
// it covers addresses restored from disk that are no longer in a
// live derived pool.
platform_wallet
.apply_top_up_reconciliation(&address_infos)
.await;
Comment thread
shumkov marked this conversation as resolved.
Ok::<_, platform_wallet::PlatformWalletError>(new_balance)
})
});
let result = unwrap_option_or_return!(option);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ use dash_sdk::platform::transition::top_up_identity_from_addresses::TopUpIdentit
use dpp::address_funds::PlatformAddress;
use dpp::fee::Credits;

use dash_sdk::query_types::AddressInfos;

use crate::error::PlatformWalletError;

use super::*;
Expand All @@ -26,6 +28,15 @@ impl IdentityWallet {
/// Uses the `TopUpIdentityFromAddresses` SDK trait. Address nonces are
/// looked up automatically.
///
/// Returns the proof-attested post-spend `AddressInfos` alongside the new
/// identity balance. The caller MUST reconcile the spent platform-address
/// balances from the `AddressInfos` via
/// [`PlatformAddressWallet::apply_top_up_reconciliation`] — this method
/// only owns the identity-side balance update, because resolving a spent
/// address back to its derivation index needs the address provider, which
/// lives on the platform-address wallet (and covers addresses restored
/// from disk that are no longer in a live derived pool).
Comment on lines +31 to +38

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.

🟡 Suggestion: Two-call reconciliation contract enforced only by a doc comment, not the type system

top_up_from_addresses now returns (AddressInfos, Credits) with a doc comment stating the caller "MUST" call PlatformAddressWallet::apply_top_up_reconciliation. This is exactly the same implicit side-channel shape whose neglect caused the bug this PR fixes (the SDK-level AddressInfos were previously discarded as _address_infos at the call site). Nothing prevents a future caller — or a mechanical refactor — from pattern-matching let (_, new_balance) = ... and compiling cleanly, silently reintroducing the phantom-balance regression.

Today there is exactly one in-tree caller (rs-platform-wallet-ffi/src/identity_top_up.rs) which does call reconciliation correctly, so the risk is low right now. But the more robust shape would be for IdentityWallet::top_up_from_addresses to take the PlatformAddressWallet (or the provider handle) and perform reconciliation internally so the two operations can't be pulled apart. Not blocking, but worth folding into a follow-up refactor.

source: ['claude']

///
/// # Arguments
///
/// * `identity_id` - The identity to top up.
Expand All @@ -40,7 +51,7 @@ impl IdentityWallet {
inputs: BTreeMap<PlatformAddress, Credits>,
address_signer: &S,
settings: Option<PutSettings>,
) -> Result<Credits, PlatformWalletError> {
) -> Result<(AddressInfos, Credits), PlatformWalletError> {
let identity = {
let wm = self.wallet_manager.read().await;
let info = wm.get_wallet_info(&self.wallet_id).ok_or_else(|| {
Expand All @@ -55,7 +66,7 @@ impl IdentityWallet {
.ok_or(PlatformWalletError::IdentityNotFound(*identity_id))?
};

let (_address_infos, new_balance) = identity
let (address_infos, new_balance) = identity
.top_up_from_addresses(&self.sdk, inputs, address_signer, settings)
.await
.map_err(|e| {
Expand Down Expand Up @@ -89,6 +100,10 @@ impl IdentityWallet {
}
}

Ok(new_balance)
// The spent platform-address balances are reconciled by the caller via
// `PlatformAddressWallet::apply_top_up_reconciliation`, which has the
// address provider needed to map restored addresses back to their
// derivation index.
Ok((address_infos, new_balance))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,12 @@ use key_wallet_manager::WalletManager;

use crate::error::PlatformWalletError;
use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId};
use crate::PlatformAddressBalanceEntry;
use dash_sdk::platform::address_sync::{
AddressFunds, AddressIndex, AddressProvider, AddressSyncResult,
};
use dash_sdk::query_types::AddressInfos;
use dpp::address_funds::PlatformAddress;
use tokio::sync::RwLock;

/// DIP-17 address coordinates used as both the pending-bimap key and
Expand Down Expand Up @@ -202,6 +205,18 @@ pub(crate) struct PlatformPaymentAddressProvider {
}

impl PlatformPaymentAddressProvider {
/// The committed per-account index/balance state for one wallet, or
/// `None` if the provider doesn't cover it. Exposes the full persisted
/// `index <-> address` bijection — including addresses restored from
/// disk that are no longer in a live derived pool — so callers can map a
/// spent address back to its derivation index.
pub(crate) fn per_wallet_state(
&self,
wallet_id: &WalletId,
) -> Option<&PerWalletPlatformAddressState> {
self.per_wallet.get(wallet_id)
}

/// Build a provider covering every platform payment account on
/// each wallet in `wallet_ids`.
///
Expand Down Expand Up @@ -811,6 +826,55 @@ impl AddressProvider for PlatformPaymentAddressProvider {
}
}

/// Resolve each spent platform address in a top-up's proof-attested
/// `address_infos` to its `(account_index, address_index)` using the
/// per-account index bijections, and pair it with the proof's post-spend
/// balance + nonce. Resolving against the bijections — rather than the live
/// derived address pool — means addresses restored from disk (present in the
/// persisted index map but not in `ManagedPlatformAccount::addresses`) are
/// still reconciled; without this, a top-up that spends a restored cached row
/// would leave its stale balance behind, preserving the phantom balance.
///
/// Addresses the proof returns that the wallet doesn't own (no bijection
/// entry) are skipped. Pure and lock-free so the reconciliation is
/// unit-testable.
pub(crate) fn build_top_up_balance_entries(
wallet_id: WalletId,
per_account_addresses: &[(u32, &BiBTreeMap<AddressIndex, PlatformP2PKHAddress>)],
address_infos: &AddressInfos,
) -> Vec<PlatformAddressBalanceEntry> {
let mut entries = Vec::new();
for (addr, maybe_info) in address_infos.iter() {
let PlatformAddress::P2pkh(hash) = addr else {
continue;
};
Comment on lines +847 to +850

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.

💬 Nitpick: Non-P2PKH proof entries silently skipped without a diagnostic

let PlatformAddress::P2pkh(hash) = addr else { continue; } drops any non-P2PKH variant with no log. Every other unresolved-address branch this PR introduces (missing provider state, empty entries with non-empty address_infos, persist failure) emits a tracing::warn!/error! precisely to surface phantom-balance conditions. If PlatformAddress ever gains a non-P2PKH variant, a silent drop here would hide a real reconciliation gap. A one-line tracing::warn! on the else-branch keeps the fix's diagnosability posture consistent. Not blocking — today only P2PKH platform addresses exist.

source: ['claude']

let p2pkh = PlatformP2PKHAddress::new(*hash);
let funds = match maybe_info {
Some(ai) => AddressFunds {
balance: ai.balance,
nonce: ai.nonce,
},
None => AddressFunds {
balance: 0,
nonce: 0,
},
};
Comment on lines +852 to +861

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.

🟡 Suggestion: None AddressInfo silently fabricates balance=0/nonce=0 instead of skipping+logging like the sibling reconciliation path

build_top_up_balance_entries maps a None AddressInfo to AddressFunds { balance: 0, nonce: 0 } and unconditionally emits/persists an entry. The sibling fund_from_asset_lock.rs::write_address_balances_changeset (line 385) treats a None on the same AddressInfos proof shape as a protocol-contract violation, logs tracing::error!, and skips — precisely to avoid recording a fabricated 'credited 0' row.

For a just-spent input address, None should be effectively unreachable (Drive's set_balance_to_address_operations_v0 never deletes the entry, and every spent address had a real prior nonce/balance verified by ensure_address_balance). If it ever fires, it indicates a proof/query anomaly, not a legitimate zero-state — and this code silently:

  1. Overwrites the in-memory balance to 0 via set_address_credit_balance(addr, 0, None).
  2. Persists PlatformAddressBalanceEntry { funds: { balance: 0, nonce: 0 } }, which external persisters (e.g. evo-tool's platform_address_balances table, per wallet/apply.rs:272-276) treat as authoritative for both balance and nonce — regressing replay-protection state that other code (sync.rs's had_funds = f.nonce != 0 gate) actually reads.

This reintroduces a narrower version of the exact 'local diverges from on-chain silently' bug class this PR exists to fix. The PR description explicitly says it mirrors fund_from_asset_lock, so aligning the two paths is the right call. Not blocking because current code paths make the branch nearly unreachable, but cheap to close and it also has no test coverage today.

Suggested change
let funds = match maybe_info {
Some(ai) => AddressFunds {
balance: ai.balance,
nonce: ai.nonce,
},
None => AddressFunds {
balance: 0,
nonce: 0,
},
};
let funds = match maybe_info {
Some(ai) => AddressFunds {
balance: ai.balance,
nonce: ai.nonce,
},
None => {
tracing::error!(
address = %p2pkh,
"Platform proof returned None AddressInfo for a spent address; skipping reconciliation entry to avoid persisting balance=0/nonce=0 over real state"
);
continue;
}
};

source: ['claude']

for &(account_index, bimap) in per_account_addresses {
if let Some(&address_index) = bimap.get_by_right(&p2pkh) {
entries.push(PlatformAddressBalanceEntry {
wallet_id,
account_index,
address_index,
address: p2pkh,
funds,
});
break;
}
}
}
entries
}

#[cfg(test)]
mod tests {
use super::*;
Expand All @@ -837,6 +901,58 @@ mod tests {
AddressFunds { balance, nonce }
}

/// Regression for the top-up reconciliation: a spent platform address
/// that exists only in the persisted index bijection (restored from
/// disk — not in any live derived pool) must still be resolved to its
/// derivation index and recorded with the proof's post-spend balance.
/// The original fix scanned only the live pool, so it left restored
/// rows stale, preserving the phantom Platform Balance the PR targets.
#[test]
fn build_top_up_entries_resolves_restored_address_outside_live_pool() {
use dash_sdk::query_types::AddressInfo;

// Present in the persisted bijection at index 3, but NOT in a live
// derived address pool.
let restored = p2pkh(0x11);
let mut bimap: BiBTreeMap<AddressIndex, PlatformP2PKHAddress> = BiBTreeMap::new();
bimap.insert(3, restored);

// The top-up spent it; the proof attests post-spend balance 5,
// nonce bumped to 4.
let restored_addr = PlatformAddress::P2pkh([0x11; 20]);
let mut address_infos = AddressInfos::new();
address_infos.insert(
restored_addr,
Some(AddressInfo {
address: restored_addr,
nonce: 4,
balance: 5,
}),
);

let per_account: [(u32, &BiBTreeMap<AddressIndex, PlatformP2PKHAddress>); 1] =
[(ACCOUNT, &bimap)];
let entries = build_top_up_balance_entries(WALLET, &per_account, &address_infos);

assert_eq!(
entries.len(),
1,
"a restored spent address must still be reconciled"
);
let e = &entries[0];
assert_eq!(e.address, restored);
assert_eq!(e.account_index, ACCOUNT);
assert_eq!(
e.address_index, 3,
"index resolved from the persisted bijection, not the live pool"
);
assert_eq!(
e.funds.balance, 5,
"records the proof's post-spend balance, not a stale value"
);
assert_eq!(e.funds.nonce, 4, "records the bumped nonce");
}

/// Build a provider whose committed `per_wallet` tracks a single
/// account on `wallet_id` with one funded address (index 0), backed
/// by the supplied wallet manager.
Expand Down Expand Up @@ -1136,6 +1252,119 @@ mod tests {
);
}

/// Records every changeset handed to `store`, so a test can assert what
/// the reconciliation actually persisted.
#[derive(Default)]
struct CapturingPersister {
stored: std::sync::Mutex<Vec<crate::changeset::PlatformWalletChangeSet>>,
}

impl crate::changeset::PlatformWalletPersistence for CapturingPersister {
fn store(
&self,
_wallet_id: WalletId,
changeset: crate::changeset::PlatformWalletChangeSet,
) -> Result<(), crate::changeset::PersistenceError> {
self.stored.lock().expect("persister mutex").push(changeset);
Ok(())
}

fn flush(&self, _wallet_id: WalletId) -> Result<(), crate::changeset::PersistenceError> {
Ok(())
}

fn load(
&self,
) -> Result<crate::changeset::ClientStartState, crate::changeset::PersistenceError>
{
Ok(crate::changeset::ClientStartState::default())
}
}

/// Integration regression for the top-up reconciliation *contract* — not
/// just the pure entry builder. `apply_top_up_reconciliation` must build
/// AND **persist** a `PlatformAddressChangeSet` carrying the proof's
/// post-spend balance for a spent address resolved via the provider's
/// persisted state. The reported bug was the *missing persist* (the SDK's
/// `address_infos` were discarded), so this pins that `store` actually
/// fires with the decremented entry — a helper-only test would still pass
/// if `apply_top_up_reconciliation` stopped persisting.
#[tokio::test]
async fn apply_top_up_reconciliation_persists_decremented_balance() {
use crate::broadcaster::SpvBroadcaster;
use crate::events::PlatformEventManager;
use crate::spv::SpvRuntime;
use crate::wallet::asset_lock::manager::AssetLockManager;
use crate::wallet::persister::WalletPersister;
use crate::wallet::platform_addresses::PlatformAddressWallet;
use dash_sdk::query_types::AddressInfo;
use tokio::sync::Notify;

let recorder = Arc::new(CapturingPersister::default());

// Wallet wired to the capturing persister. The rest mirrors the
// short-circuit fixture — `apply_top_up_reconciliation` only touches
// provider / wallet_manager / persister.
let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk"));
let wallet_manager = Arc::new(RwLock::new(WalletManager::new(sdk.network)));
let persister = WalletPersister::new(WALLET, recorder.clone());
let event_manager = Arc::new(PlatformEventManager::new(Vec::new()));
let spv = Arc::new(SpvRuntime::new(Arc::clone(&wallet_manager), event_manager));
let broadcaster = Arc::new(SpvBroadcaster::new(spv));
let asset_locks = Arc::new(AssetLockManager::new(
Arc::clone(&sdk),
Arc::clone(&wallet_manager),
WALLET,
Arc::new(Notify::new()),
broadcaster,
persister.clone(),
));
let wallet = PlatformAddressWallet::new(
sdk,
Arc::clone(&wallet_manager),
WALLET,
asset_locks,
persister,
);

// The provider knows the spent address via its persisted bijection
// (pre-spend balance 100).
let addr = p2pkh(0x11);
let provider =
provider_tracking_address(Arc::clone(&wallet_manager), WALLET, addr, funds(100, 1));
*wallet.provider.write().await = Some(provider);

// The top-up spent it; the proof attests post-spend balance 5, nonce 4.
let spent = PlatformAddress::P2pkh([0x11; 20]);
let mut address_infos = AddressInfos::new();
address_infos.insert(
spent,
Some(AddressInfo {
address: spent,
nonce: 4,
balance: 5,
}),
);

wallet.apply_top_up_reconciliation(&address_infos).await;

// The reconciliation must have PERSISTED the decremented entry — the
// contract the original bug broke by discarding `address_infos`.
let stored = recorder.stored.lock().expect("persister mutex");
let entry = stored
.iter()
.filter_map(|cs| cs.platform_addresses.as_ref())
.flat_map(|pa| pa.addresses.iter())
.find(|e| e.address == addr)
.expect("a persisted platform-address entry for the spent address");
assert_eq!(entry.account_index, ACCOUNT);
assert_eq!(
entry.funds.balance, 5,
"persists the proof's post-spend balance, not the stale pre-spend value"
);
assert_eq!(entry.funds.nonce, 4, "persists the bumped nonce");
}

/// `reset_sync_state` must zero the incremental watermark AND drop
/// the cached `found` seed, so the next pass is a full rescan rather
/// than an incremental catch-up. This is the core of the platform
Expand Down
Loading
Loading