Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -781,6 +796,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 @@ -807,6 +871,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
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use crate::wallet::persister::WalletPersister;

use super::provider::PlatformPaymentAddressProvider;

use dash_sdk::query_types::AddressInfos;

/// Platform address wallet providing DIP-17 platform payment address functionality.
#[derive(Clone)]
pub struct PlatformAddressWallet {
Expand Down Expand Up @@ -157,6 +159,83 @@ impl PlatformAddressWallet {
Ok(())
}

/// Reconcile platform-address balances after an identity
/// top-up-from-addresses, from the proof-attested `address_infos` the SDK
/// returns. Each spent address's `(account_index, address_index)` is
/// resolved from the persisted provider state — covering addresses
/// restored from disk that are no longer in a live derived pool — its
/// in-memory balance is set to the proof's post-spend value, and a
/// `PlatformAddressChangeSet` is persisted so the displayed balance and
/// the next input selection both reflect on-chain reality.
///
/// Without this, the local balances stay frozen at their pre-top-up
/// values: the wallet keeps displaying a stale "Platform Balance" and the
/// next top-up over-selects the now-drained addresses, which Drive
/// rejects with "Insufficient combined address balances".
///
/// Locks are released before persisting (the persistence backend runs its
/// callbacks inline), and errors are logged rather than propagated —
/// Platform already accepted the top-up, and a later sync reconciles.
pub async fn apply_top_up_reconciliation(&self, address_infos: &AddressInfos) {
// Resolve spent addresses to balance entries under the provider read
// lock, then drop it.
let entries = {
let guard = self.provider.read().await;
match guard
.as_ref()
.and_then(|p| p.per_wallet_state(&self.wallet_id))
{
Some(state) => {
let per_account: Vec<_> = state
.iter()
.map(|(idx, acct)| (*idx, acct.addresses()))
.collect();
super::provider::build_top_up_balance_entries(
self.wallet_id,
&per_account,
address_infos,
)
}
None => Vec::new(),
}
};
if entries.is_empty() {
return;
}
// Apply the proof-attested post-spend balances in memory, then drop
// the write lock.
{
let mut wm = self.wallet_manager.write().await;
if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) {
for entry in &entries {
if let Some(account) = info
.core_wallet
.platform_payment_managed_account_at_index_mut(entry.account_index)
{
account.set_address_credit_balance(
entry.address,
entry.funds.balance,
None,
);
}
}
}
}
Comment on lines +222 to +240

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: Reconciliation updates ManagedPlatformAccount but leaves the live provider's found cache stale

apply_top_up_reconciliation writes the proof-attested post-spend balance into ManagedPlatformAccount.address_balances via set_address_credit_balance and persists a changeset — this correctly fixes the two consumers the PR targets (displayed balance and auto_select_inputs, both of which read ManagedPlatformAccount.address_balances).

However, the parallel copy of this data in PlatformPaymentAddressProvider.per_wallet[wallet_id][account_index].found (a BTreeMap<PlatformP2PKHAddress, AddressFunds>) is never updated. That map is what current_balances() hands back to the SDK to seed the next incremental BLAST sync pass. The normal sync path keeps both maps in lockstep (on_address_found in provider.rs updates the scratch found map AND calls account.set_address_credit_balance in the same callback), but this new top-up path only does the second half.

Until the next full sync round rebuilds the provider's found entry (or the process restarts and reloads from the just-persisted changeset), the live in-memory provider keeps seeding the stale pre-spend balance for that address within the current session — the same two-copies-of-truth inconsistency the PR is trying to eliminate, one layer up.

source: ['claude']

// Persist with no locks held.
let cs = crate::PlatformAddressChangeSet {
addresses: entries,
..Default::default()
};
if let Err(e) = self.persister.store(cs.into()) {
tracing::error!(
error = %e,
"Failed to persist top-up platform-address reconciliation; \
in-memory balances are updated but durable rows stay stale \
until the next platform-address sync"
);
}
}

/// Get the network from the SDK.
pub fn network(&self) -> key_wallet::Network {
self.sdk.network
Expand Down
Loading