From 088d4aa5d6ae9ed132e863cd05f3c11e5b3b8dd3 Mon Sep 17 00:00:00 2001 From: Bartosz Rozwarski Date: Fri, 4 Sep 2026 12:13:28 +0200 Subject: [PATCH 1/3] docs(platform-wallet): make comments explain the decision, not the PR history Comments across this crate had accumulated a running account of how the code got here: issue and PR numbers, review-round tags, "this used to do X, now it does Y", and notes on which revert a line had to survive. That history is already in git blame, and in a comment it crowds out the thing a reader actually needs, which is why the code is shaped this way. Rewrite those comments in the present tense so each one states the invariant it guards, the failure mode it prevents, or why the obvious alternative is wrong. Where a paragraph carried only history and no surviving rationale, drop it. Two examples of what this preserves rather than deletes. The broadcast input fence in `reservations.rs` no longer recounts the five bounds it was given across review rounds; it states why neither a block height nor a monotonic deadline is evidence that a signed transaction is dead. The `log` dependency in `Cargo.toml` no longer explains where the line must sit to survive a three-way merge; it says the JNI layer routes `log` to Android logcat, which is why the dependency exists at all. Kept: open-work trackers (`TODO(platform#3040)`, the rust-dashcore#916 follow-up in `spv/runtime.rs`), external spec references, and phrases like "no longer" where they describe runtime state rather than project history. Comments only. No code, identifier, string literal, log message, or test name changed; verified by filtering the diff to comment lines. `cargo check --all-features --tests` and `cargo fmt --check` both pass. Co-Authored-By: Claude Opus 5 --- packages/rs-platform-wallet/Cargo.toml | 12 +- .../rs-platform-wallet/src/address_paths.rs | 2 +- .../src/changeset/changeset.rs | 27 +- .../src/changeset/core_bridge.rs | 104 +++--- .../src/changeset/identity_scan_state.rs | 2 +- .../src/changeset/traits.rs | 5 +- packages/rs-platform-wallet/src/error.rs | 39 ++- .../src/manager/accessors.rs | 9 +- .../rs-platform-wallet/src/manager/load.rs | 21 +- .../rs-platform-wallet/src/manager/mod.rs | 68 ++-- .../src/manager/platform_address_sync.rs | 17 +- .../rs-platform-wallet/src/manager/startup.rs | 67 ++-- .../src/manager/wallet_lifecycle.rs | 39 ++- .../src/masternode/record.rs | 2 +- .../rs-platform-wallet/src/spv/runtime.rs | 2 +- .../rs-platform-wallet/src/wallet/apply.rs | 14 +- .../src/wallet/asset_lock/build.rs | 46 ++- .../src/wallet/asset_lock/manager.rs | 7 +- .../src/wallet/asset_lock/orchestration.rs | 11 +- .../src/wallet/asset_lock/sync/proof.rs | 10 +- .../src/wallet/asset_lock/sync/recovery.rs | 29 +- .../src/wallet/asset_lock/sync/tracking.rs | 6 +- .../src/wallet/core/balance_handler.rs | 2 +- .../src/wallet/core/broadcast.rs | 164 +++++----- .../src/wallet/core/generation.rs | 300 ++++++++---------- .../src/wallet/core/sign_message.rs | 6 +- .../src/wallet/core/spend_observer.rs | 44 ++- .../src/wallet/core/transaction.rs | 25 +- .../src/wallet/core/wallet.rs | 9 +- .../src/wallet/identity/crypto/dip14.rs | 6 +- .../src/wallet/identity/crypto/validation.rs | 2 +- .../identity/network/contact_requests.rs | 11 +- .../src/wallet/identity/network/contacts.rs | 13 +- .../src/wallet/identity/network/contract.rs | 2 +- .../src/wallet/identity/network/discovery.rs | 37 +-- .../src/wallet/identity/network/document.rs | 19 +- .../src/wallet/identity/network/dpns.rs | 3 +- .../identity/network/dpns_marketplace.rs | 82 +++-- .../src/wallet/identity/network/invitation.rs | 4 +- .../src/wallet/identity/network/payments.rs | 138 ++++---- .../src/wallet/identity/network/profile.rs | 4 +- .../network/register_from_addresses.rs | 2 +- .../wallet/identity/network/registration.rs | 11 +- .../wallet/identity/network/seed_binding.rs | 20 +- .../wallet/identity/network/tokens/burn.rs | 3 +- .../wallet/identity/network/tokens/claim.rs | 3 +- .../network/tokens/destroy_frozen_funds.rs | 3 +- .../wallet/identity/network/tokens/freeze.rs | 3 +- .../wallet/identity/network/tokens/mint.rs | 3 +- .../wallet/identity/network/tokens/pause.rs | 3 +- .../identity/network/tokens/purchase.rs | 3 +- .../wallet/identity/network/tokens/resume.rs | 3 +- .../identity/network/tokens/set_price.rs | 3 +- .../identity/network/tokens/transfer.rs | 3 +- .../identity/network/tokens/unfreeze.rs | 3 +- .../identity/network/tokens/update_config.rs | 3 +- .../src/wallet/identity/network/transfer.rs | 3 +- .../identity/network/transfer_to_addresses.rs | 3 +- .../src/wallet/identity/network/withdrawal.rs | 3 +- .../managed_identity/contact_requests.rs | 4 +- .../state/managed_identity/identity_ops.rs | 4 +- .../identity/state/managed_identity/mod.rs | 4 +- .../wallet/identity/state/manager/apply.rs | 2 +- .../identity/state/manager/lifecycle.rs | 10 +- .../src/wallet/identity/state/manager/mod.rs | 2 +- .../src/wallet/masternode_withdrawal.rs | 4 +- .../fund_from_asset_lock.rs | 16 +- .../src/wallet/platform_addresses/provider.rs | 14 +- .../src/wallet/platform_addresses/sync.rs | 4 +- .../src/wallet/platform_addresses/transfer.rs | 4 +- .../src/wallet/platform_addresses/wallet.rs | 3 +- .../wallet/platform_addresses/withdrawal.rs | 22 +- .../src/wallet/platform_wallet.rs | 21 +- .../src/wallet/platform_wallet_traits.rs | 9 +- .../src/wallet/provider_key_at_index.rs | 8 +- .../src/wallet/reservations.rs | 37 +-- .../src/wallet/shielded/coordinator.rs | 2 +- .../src/wallet/shielded/file_store.rs | 27 +- .../src/wallet/shielded/operations.rs | 5 +- .../src/wallet/signed_payment_registry.rs | 42 ++- 80 files changed, 801 insertions(+), 931 deletions(-) diff --git a/packages/rs-platform-wallet/Cargo.toml b/packages/rs-platform-wallet/Cargo.toml index 20899d523a3..4f30d42932c 100644 --- a/packages/rs-platform-wallet/Cargo.toml +++ b/packages/rs-platform-wallet/Cargo.toml @@ -56,16 +56,8 @@ image = { version = "0.25", default-features = false, features = ["png", "jpeg", # Security zeroize = "1" -# `log` facade. `changeset/core_bridge.rs` emits watermark-freeze breadcrumbs -# through `log` so they reach Android logcat (the JNI layer installs -# `android_logger` as the global `log` logger, tag `DashSDK`; the Kotlin SDK's -# only `tracing` subscriber writes to stdout, which Android discards). -# Declared here — deliberately NOT inside the `tracing` block above — so this -# crate owns the dependency independently of the encrypted-txMetadata change -# (#4277) that first introduced a `log` line: that change was reverted on -# v4.2-dev (#4279), and a `log` line living in that reverted region gets -# dropped by the 3-way merge, leaving the `log::` calls in core_bridge.rs -# undeclared (E0433). Keeping it in this untouched region makes it survive. +# `log` facade: the JNI layer routes `log` to Android logcat, while `tracing` +# output is discarded there. log = "0.4" # Shielded pool (optional, behind `shielded` feature) diff --git a/packages/rs-platform-wallet/src/address_paths.rs b/packages/rs-platform-wallet/src/address_paths.rs index 83c86e3aec0..10914cf1379 100644 --- a/packages/rs-platform-wallet/src/address_paths.rs +++ b/packages/rs-platform-wallet/src/address_paths.rs @@ -44,7 +44,7 @@ use crate::DerivedAddress; /// Render the BIP32 derivation path for a `DerivedAddress` event /// payload. See module-level docs for the path layout rules. pub fn derivation_path_for_derived_address(derived: &DerivedAddress) -> Option { - // `Address` no longer exposes its network directly — its base58/bech32 + // `Address` does not expose its network directly — its base58/bech32 // prefix is ambiguous across testnet/devnet/regtest. The derivation path // only distinguishes mainnet (coin type `5'`) from everything else (`1'`), // so probe for mainnet and fall back to testnet otherwise. diff --git a/packages/rs-platform-wallet/src/changeset/changeset.rs b/packages/rs-platform-wallet/src/changeset/changeset.rs index d36fcfdc76b..2ff19d8235e 100644 --- a/packages/rs-platform-wallet/src/changeset/changeset.rs +++ b/packages/rs-platform-wallet/src/changeset/changeset.rs @@ -13,12 +13,11 @@ //! key-wallet lives in dedicated sub-changesets: identities, contacts, //! platform addresses, asset locks, and token balances. //! -//! Earlier revisions of this file used `key_wallet::changeset::WalletChangeSet` -//! verbatim in the `core` field. That upstream type was deleted in favour -//! of an event-bus model (see PR #696 in rust-dashcore). Platform-wallet -//! subscribes to the event bus, projects each event into a `CoreChangeSet`, -//! and routes it through this changeset's `core` slot — keeping the -//! per-domain merge / apply shape downstream consumers already know. +//! key-wallet exposes core wallet changes as an event bus rather than a +//! changeset type of its own. Platform-wallet subscribes to that bus, +//! projects each event into a `CoreChangeSet`, and routes it through this +//! changeset's `core` slot — so every domain, core included, shares one +//! merge / apply shape downstream consumers can rely on. use std::collections::{BTreeMap, BTreeSet}; @@ -87,7 +86,7 @@ use crate::wallet::identity::{ #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct CoreChangeSet { /// Transaction records produced by this batch — one WALLET-LEVEL - /// record per txid (dashpay/platform#4387). + /// record per txid. /// /// Includes records first stored (`TransactionDetected`, /// `BlockProcessed.inserted`), records whose context advanced @@ -265,8 +264,8 @@ impl HighestUsedIndexes { } } -/// Fold same-txid [`TransactionRecord`]s into ONE wallet-level record — -/// the dashpay/platform#4387 fix at the batch seam. +/// Fold same-txid [`TransactionRecord`]s into ONE wallet-level record +/// at the batch seam. /// /// Upstream `check_core_transaction` emits one record PER MATCHED ACCOUNT /// for a single transaction, each carrying only its account's slice @@ -447,7 +446,7 @@ pub(crate) fn fold_same_txid_records(records: &mut Vec) { /// /// One linear pass over each side per merge — the adapter's drain calls /// merge once per buffered event, so this deliberately avoids the -/// full-vec re-fold a `fold_same_txid_records` call here used to cost. +/// full-vec re-fold a `fold_same_txid_records` call here would cost. fn coalesce_newest_wins( existing: &mut Vec, incoming: Vec, @@ -495,7 +494,7 @@ fn context_rank(context: &key_wallet::transaction_checking::TransactionContext) impl Merge for CoreChangeSet { fn merge(&mut self, other: Self) { - // Records: coalesce by txid, NEWEST-WINS (dashpay/platform#4387). + // Records: coalesce by txid, NEWEST-WINS. // // The event bridge already folded each event's per-account // slices into one wallet-level record per txid (see @@ -1468,7 +1467,7 @@ impl Merge for DpnsNameStateChangeSet { /// Per-(identity, token) balance changes emitted by /// [`crate::manager::identity_sync::IdentitySyncManager::sync_now`]. /// -/// The watch list itself is no longer changeset-replicated — it lives +/// The watch list itself is not changeset-replicated — it lives /// purely in the manager's in-memory cache. Persistence carries only /// the post-sync balance updates and tombstones. #[derive(Debug, Clone, Default, PartialEq)] @@ -1894,8 +1893,8 @@ pub struct PlatformWalletChangeSet { /// carries: no persister vtable has a slot for this field yet, so on /// hosts that have not adopted it the verdict is process-lifetime only. /// Within a process it still redirects a second bring-up, and a partial - /// scan is now retried inside its own launch — but closing - /// dashpay/platform#4365 across launches needs the host slot. + /// scan is retried inside its own launch — but honouring the verdict + /// across launches needs the host slot. pub identity_scan_state: Option, /// Per-account registration entries emitted at registration / on /// later `add_account` calls. See [`AccountRegistrationEntry`] for diff --git a/packages/rs-platform-wallet/src/changeset/core_bridge.rs b/packages/rs-platform-wallet/src/changeset/core_bridge.rs index f9b7f491977..74e97dee742 100644 --- a/packages/rs-platform-wallet/src/changeset/core_bridge.rs +++ b/packages/rs-platform-wallet/src/changeset/core_bridge.rs @@ -15,7 +15,7 @@ //! consumer deliberately does NOT use that broadcast: under a heavy SPV //! catch-up the broadcast ring overflows (`RecvError::Lagged`) and drops the //! record/watermark events, which let the durable sync height outrun the -//! rows it implies and freeze forever (dashpay/platform#4069). The unbounded +//! rows it implies and freeze forever. The unbounded //! persistence channel can never `Lagged`, so that freeze cannot occur. //! //! # Why a single consumer, not per-wallet @@ -61,18 +61,16 @@ use crate::wallet::platform_wallet::PlatformWalletInfo; /// Maximum number of `WalletEvent`s folded into a single /// `persister.store(..)` round-trip by [`run_wallet_event_adapter`]. /// -/// # Why batch at all (dashpay/platform#4069 follow-up) +/// # Why batch at all /// /// This adapter's per-event cost is overwhelmingly the persister call: on /// Android that is a JNI hop into a Room transaction (milliseconds), while /// projecting a `WalletEvent` into a [`CoreChangeSet`] is microseconds. -/// Storing one event per `store()` therefore pinned the drain rate at +/// Storing one event per `store()` would therefore pin the drain rate at /// roughly the *store* rate — low hundreds per second — which is far below /// what a historical SPV catch-up emits. Draining the lossless persistence -/// channel one store at a time let its backlog grow without bound during a -/// catch-up (and, on the old bounded broadcast this consumer used to read, -/// overflowed the ring and froze the watermark — the root cause the -/// dedicated unbounded channel removes). +/// channel one store at a time would let its backlog grow without bound +/// during a catch-up. /// /// Folding every event *already buffered* in the channel into one changeset /// per wallet collapses a burst of N events into a single store, so the @@ -87,10 +85,9 @@ use crate::wallet::platform_wallet::PlatformWalletInfo; /// starving the cancellation branch of the select below. const ADAPTER_STORE_BATCH_LIMIT: usize = 512; -/// Session fault state for the durable-watermark guard -/// (dashpay/platform#4069). +/// Session fault state for the durable-watermark guard. /// -/// Now that the persistence channel is a lossless unbounded `mpsc`, two things +/// Because the persistence channel is a lossless unbounded `mpsc`, two things /// fault a wallet, and both name the wallets they hit, so a sibling whose rows /// are still landing atomically keeps advancing — freezing it too would force a /// redundant rescan of a wallet that never lost a row. @@ -248,7 +245,7 @@ where /// [`freeze_synced_height_if_faulted`] helper — is directly testable /// (drive a real `mpsc::UnboundedSender`, inject a probe persister). /// -/// # Lossless persistence channel (dashpay/platform#4069) +/// # Lossless persistence channel /// /// The upstream `WalletManager` publishes `WalletEvent`s to this consumer /// over a dedicated, **unbounded** `mpsc` persistence channel (taken once @@ -260,16 +257,15 @@ where /// subscribe-before-publish race: an `mpsc::UnboundedReceiver` buffers events /// sent before the task's first poll rather than dropping them. /// -/// This closes the historical freeze: previously this consumer read the -/// manager's *bounded* broadcast ring, and during a historical SPV catch-up -/// the manager processed blocks far faster than this single-threaded adapter -/// could drain them through the (slow, JNI + Room) persister, so the ring -/// overflowed and `recv()` returned `Lagged` — the dropped events being -/// exactly the record/UTXO/spent-marker events, while the bare -/// `SyncHeightAdvanced` watermark kept flowing and advanced the persisted -/// `syncedHeight` past blocks whose rows never reached disk. The durable -/// watermark then outran its rows and the guard below latched it frozen -/// forever. With the lossless channel that path no longer exists. +/// The manager's *bounded* broadcast ring is the wrong transport here: during +/// a historical SPV catch-up the manager processes blocks far faster than +/// this single-threaded adapter can drain them through the (slow, JNI + Room) +/// persister, so the ring overflows and `recv()` returns `Lagged` — the +/// dropped events being exactly the record/UTXO/spent-marker events, while +/// the bare `SyncHeightAdvanced` watermark keeps flowing and advances the +/// persisted `syncedHeight` past blocks whose rows never reach disk. The +/// durable watermark then outruns its rows and the guard below latches it +/// frozen forever. With the lossless channel that path cannot occur. /// /// # Durable-watermark guard (fail-closed backstop) /// @@ -567,7 +563,7 @@ async fn run_wallet_event_adapter

( /// 1. Apply [`freeze_synced_height_if_faulted`] *after* the fold, so a /// `synced_height` that entered via `Merge` is stripped just like a /// standalone one (otherwise folding would smuggle a watermark past the -/// guard and reintroduce dashpay/platform#4069). +/// guard and let the durable watermark outrun its rows). /// 2. Record `frozen` from the height the batch *proposed*, captured before the /// guard strips it. /// 3. Record `persisted` only from the `Ok` arm of `store()`. A rejected store @@ -680,7 +676,7 @@ where diag } -/// Durable-watermark guard for dashpay/platform#4069. +/// Durable-watermark guard. /// /// When a wallet has faulted this session — its `store()` was rejected, or a /// commit panic left the batch's outcome unknown — its persisted @@ -793,11 +789,11 @@ async fn build_core_changeset( // Live mempool matching emits ONE event per matched // account, each carrying only that account's slice — and // nothing marks a transaction's last slice. Folding - // whatever slices happened to share an adapter drain made - // the persisted row depend on scheduling: a drain that - // caught one slice stored that slice as the wallet's row - // (the dashpay/platform#4387 bug, reintroduced - // nondeterministically). The MANAGER, not the drain, is + // whatever slices happen to share an adapter drain would + // make the persisted row depend on scheduling: a drain + // that catches one slice would store that slice as the + // wallet's row, nondeterministically. The MANAGER, not + // the drain, is // the boundary where a transaction's slices are complete: // by the time this event is projected the manager already // holds every so-far-matched account's record for the @@ -905,8 +901,7 @@ async fn build_core_changeset( // wallet-level `records` copy: one block can insert // SEVERAL per-account records for one transaction (a // multi-account spend), and the txid-keyed row needs the - // one wallet-level record (dashpay/platform#4387 — see - // fold_same_txid_records). + // one wallet-level record (see fold_same_txid_records). cs.account_records.extend( inserted .iter() @@ -963,10 +958,10 @@ async fn build_core_changeset( // `ChainLockProcessed` fires every time the wallet's // `last_applied_chain_lock` advances, // even when no record was promoted — so a quiescent wallet's - // boundary advance is no longer invisible to this bridge. - // The earlier `TransactionsChainlocked`-only signal had a + // boundary advance is never invisible to this bridge. + // A `TransactionsChainlocked`-only signal would leave a // gap on the "metadata advanced but per-account empty" - // path; the new event closes it deterministically. + // path; this event closes it deterministically. CoreChangeSet { last_applied_chain_lock: Some(chain_lock.clone()), ..CoreChangeSet::default() @@ -1372,8 +1367,7 @@ fn derive_spent_utxos(record: &TransactionRecord) -> Vec { /// [`spent_outpoints`], which drives the in-broadcast fence's release. The two /// consumers must not be able to disagree about which inputs count: the fence /// releases an outpoint precisely when the wallet treats it as spent, so a -/// divergence would either strand a fence forever or drop one early -/// (`dashpay/platform#4309`). +/// divergence would either strand a fence forever or drop one early. /// /// [`InputDetail`]: key_wallet::managed_account::transaction_record::InputDetail fn spent_outpoint( @@ -1656,7 +1650,7 @@ mod contact_watch_only_projection_tests { } } - /// dashpay/platform#4387: a multi-account spend's per-account records + /// A multi-account spend's per-account records /// must fold into ONE wallet-level row. Models the S22 field sweep in /// miniature: the BIP44 slice spends 2.0, the receival slice spends /// 0.62 with 0.005 change — the persisted row must carry the summed @@ -2767,7 +2761,7 @@ mod tests { } } - /// dashpay/platform#4069: while persistence is healthy the sync + /// While persistence is healthy the sync /// watermark flows through untouched. #[test] fn healthy_persistence_keeps_synced_height() { @@ -2781,7 +2775,7 @@ mod tests { assert_eq!(core.last_processed_height, Some(300)); } - /// dashpay/platform#4069: once persistence has faulted, the durable + /// Once persistence has faulted, the durable /// watermark is frozen (`synced_height` stripped) so it can't outrun /// the rows — but ONLY `synced_height` is dropped; every other field /// (here `last_processed_height`, standing in for records/UTXO @@ -2805,9 +2799,9 @@ mod tests { ); } - /// dashpay/platform#4069 (per-wallet fault scoping): a `store()` + /// Per-wallet fault scoping: a `store()` /// rejection freezes ONLY the named wallet; a sibling keeps advancing. - /// (The old global `broadcast::Lagged` latch is gone — the lossless + /// (There is no global `Lagged` latch — the lossless /// unbounded persistence channel can never lag.) #[test] fn fault_state_scopes_store_rejection_per_wallet() { @@ -2828,7 +2822,7 @@ mod tests { ); } - // ── Adapter-loop integration tests (dashpay/platform#4069) ── + // ── Adapter-loop integration tests ── // // These drive `run_wallet_event_adapter` with a real lossless // `mpsc::UnboundedSender` and a probe persister so the LOOP — not just @@ -2870,8 +2864,8 @@ mod tests { obs: UnboundedSender, fail_once: Mutex>, /// Wallets whose NEXT `store()` panics instead of returning. Models a - /// backend that dies mid-write — the case that used to unwind the whole - /// adapter task and now surfaces as a `JoinError`. + /// backend that dies mid-write — which surfaces as a `JoinError` + /// instead of unwinding the whole adapter task. panic_once: Mutex>, /// Held closed to keep a `store()` call parked. The SQLite backend /// commits a real transaction per call, so a slow disk parks the caller @@ -2988,8 +2982,8 @@ mod tests { /// (`DEFAULT_WALLET_EVENT_CAPACITY` == 1000) is delivered losslessly over /// the unbounded persistence channel, so the fault latch never trips and /// the durable watermark advances all the way to the tip of the - /// catch-up. On the old broadcast this burst would `Lagged` and freeze the - /// watermark forever (dashpay/platform#4069). + /// catch-up. On a bounded broadcast this burst would `Lagged` and freeze + /// the watermark forever. #[tokio::test] async fn lossless_burst_never_freezes_and_watermark_reaches_tip() { const BURST: u32 = 3000; // >> the old broadcast ring (1000) @@ -3286,8 +3280,8 @@ mod tests { /// watermark advance by killing the writer outright. `spawn_blocking` /// turns that into a recoverable `JoinError` — and merely logging it would /// let the NEXT batch persist a higher `synced_height` for a wallet whose - /// earlier rows may never have landed, which is exactly the hole - /// dashpay/platform#4069 closed. + /// earlier rows may never have landed, which is exactly the hole the + /// durable-watermark guard exists to close. #[tokio::test] async fn a_panicking_commit_freezes_the_batch_wallets() { let wallet_id = [0xEEu8; 32]; @@ -3551,8 +3545,8 @@ mod tests { /// standalone or folded together with a record. The freeze is applied /// after the fold, so a `synced_height` that entered via `Merge` is /// stripped just like a standalone one; otherwise folding would smuggle - /// the watermark past the guard and reintroduce dashpay/platform#4069 - /// (durable watermark outrunning the rows it implies). + /// the watermark past the guard (durable watermark outrunning the rows + /// it implies). #[tokio::test] async fn watermark_is_still_stripped_after_a_fault() { let wallet_id = [9u8; 32]; @@ -3974,15 +3968,15 @@ mod tests { assert!(!observed.rejected); } - // ── Batch-diagnostic reporting (dashpay/platform#4290 review) ── + // ── Batch-diagnostic reporting ── // // The per-drain `wallet-event batch: ...` line is read off a mainnet // tester's logcat to answer "is the durable watermark advancing?", so what // it reports is a tested property, not a comment. // - // The regression these lock down: the diagnostic used to fold - // `core.synced_height` into `synced_height_persisted` BEFORE calling - // `persister.store(...)`. A rejected store therefore logged + // The invariant these lock down: `core.synced_height` folds into + // `synced_height_persisted` only AFTER `persister.store(...)` accepts. + // Folding it before the call would make a rejected store log // `synced_height_persisted=Some(h)` in the very same drain that faulted the // wallet *because* height `h`'s rows were not accepted — an internally // contradictory trace that points a diagnosis at the wrong subsystem. @@ -4050,7 +4044,7 @@ mod tests { .contains("synced_height_persisted=Some(500)")); } - /// REGRESSION (PR #4290 review): a REJECTED `store()` must never be + /// Invariant: a REJECTED `store()` must never be /// reported as persisted. #[test] fn rejected_store_is_not_reported_as_persisted() { @@ -4248,7 +4242,7 @@ mod tests { /// A wallet that entered the drain already faulted and whose store is /// rejected AGAIN counts once, not twice: `faulted` is a wallet count and - /// must never exceed `wallets` (#4315 review finding 30c2e8e95003). + /// must never exceed `wallets`. #[test] fn repeat_rejection_of_a_faulted_wallet_counts_once() { let wallet_id = [9u8; 32]; diff --git a/packages/rs-platform-wallet/src/changeset/identity_scan_state.rs b/packages/rs-platform-wallet/src/changeset/identity_scan_state.rs index 40eca834175..d07b34ff391 100644 --- a/packages/rs-platform-wallet/src/changeset/identity_scan_state.rs +++ b/packages/rs-platform-wallet/src/changeset/identity_scan_state.rs @@ -25,7 +25,7 @@ /// once the process exited. An identity at the unanswered index then stayed /// invisible for the life of the installation, along with all of its contacts /// — a silent, permanent gap whose only symptom is a missing identity and -/// DPNS name after a restore. See dashpay/platform#4365. +/// DPNS name after a restore. /// /// This is that missing fact. `complete` is stored rather than derived from /// `failed_indices` because the two ways a scan can end early are different: diff --git a/packages/rs-platform-wallet/src/changeset/traits.rs b/packages/rs-platform-wallet/src/changeset/traits.rs index 60d98195ba9..647f4d7ed33 100644 --- a/packages/rs-platform-wallet/src/changeset/traits.rs +++ b/packages/rs-platform-wallet/src/changeset/traits.rs @@ -305,9 +305,8 @@ pub trait PlatformWalletPersistence: Send + Sync { /// // TODO: wallet-less / global objects (the `WalletId::default()` / // `[0u8; 32]` sentinel scope for parentless or global metadata) are - // not yet expressible through `flush`. Hosts that previously called - // the now-removed `commit_writes` should call `flush` per wallet - // instead; a sentinel-scope flush path is still to be designed. + // not yet expressible through `flush`. Hosts call `flush` per wallet; + // a sentinel-scope flush path is still to be designed. fn flush(&self, wallet_id: WalletId) -> Result<(), PersistenceError>; /// Replace the persisted tracked-masternode set for `network` with diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index d24412b3007..d3ab952c93d 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -55,8 +55,8 @@ pub enum PlatformWalletError { /// A gap-limit scan ended empty with at least one index left unanswered. /// Distinct from an empty success: it means "we do not know", so the /// caller must retry rather than record that the seed owns no identity. - /// Both outcomes used to arrive as `Ok(vec![])`, which is how a transient - /// DAPI failure right after restore-from-seed became a whole session + /// Collapsing both outcomes into `Ok(vec![])` would let a transient + /// DAPI failure right after restore-from-seed become a whole session /// without an identity. /// /// "Retry" is the contract, not a promise that the cause is transient — a @@ -122,16 +122,14 @@ pub enum PlatformWalletError { /// still provably unswept. The generation's pending-spend fence /// ([`WalletGeneration`](crate::wallet::core::WalletGeneration)) is NOT /// swept with either: it has no bound of its own and is released by the - /// wallet OBSERVING the outpoint spent, and by nothing else - /// (`dashpay/platform#4309`). + /// wallet OBSERVING the outpoint spent, and by nothing else. /// - /// So an earlier promise made here — that the reservation TTL reconciles an - /// ambiguous outcome — no longer holds and was never sound: elapsed time is - /// not evidence about the transaction, which stays valid and relayable no - /// matter how long the wait. The build refusal that follows a `MaybeSent` - /// is [`Self::InputMidBroadcast`], and it stands until a spend is observed - /// — this wallet's own transaction landing, or a conflicting one taking the - /// outpoint. + /// The reservation TTL does NOT reconcile an ambiguous outcome: elapsed + /// time is not evidence about the transaction, which stays valid and + /// relayable no matter how long the wait. The build refusal that follows + /// a `MaybeSent` is [`Self::InputMidBroadcast`], and it stands until a + /// spend is observed — this wallet's own transaction landing, or a + /// conflicting one taking the outpoint. /// /// Removing the wallet and re-creating it under the same id does NOT end /// the refusal: the fence map is keyed by wallet id and handed to the @@ -203,11 +201,11 @@ pub enum PlatformWalletError { /// [`Self::AssetLockTransaction`] string: the refusal says nothing wrong /// about the request itself — the same intent can be re-attempted once /// the conflict resolves (see below for what "resolves" requires) — and - /// telling it apart from a genuine build failure previously meant - /// substring-matching prose (`message.contains("mid-broadcast")`, which - /// the tests did too). All three selection choke points — the + /// telling it apart from a genuine build failure must not require + /// substring-matching prose (`message.contains("mid-broadcast")`). + /// All three selection choke points — the /// finalized-transaction build, the contact-payment build and the - /// asset-lock build — now return this one variant. + /// asset-lock build — return this one variant. /// /// # Retrying the INTENT requires reconciling the fenced transaction first /// @@ -231,8 +229,7 @@ pub enum PlatformWalletError { /// Reaching a caller at all is the uncommon path: a fenced input is /// normally still reserved and never offered to selection. This fires only /// in the window after key-wallet's reservation TTL swept that dispatch's - /// reservation, which is exactly what the fence exists to cover - /// (`dashpay/platform#4309`). + /// reservation, which is exactly what the fence exists to cover. #[error( "selected input {outpoint} is mid-broadcast by an in-flight dispatch; \ retry after it completes" @@ -505,7 +502,7 @@ pub enum PlatformWalletError { /// Asset-lock coin selection came up short, so a host (and ultimately the /// wallet UI) can render a precise shortfall instead of a stringly-typed - /// "Insufficient funds" message (dashpay/platform#4073). + /// "Insufficient funds" message. /// /// What `available` covers depends on the build's funding form. An /// exact-amount build funds from a POOLED source list — the default @@ -1177,7 +1174,7 @@ pub fn promote_document_trade_error_or( /// error, yet is deliberately kept free of any dependency on the FFI crate. /// The two definitions are pinned byte-identical by a compile-time assertion in /// `platform-wallet-ffi` (`src/error.rs`), so any drift is a build failure -/// rather than a silent code-31 regression (dashpay/platform#4183 review). +/// rather than a silent code-31 regression. pub const SIGNER_KEY_UNAVAILABLE_PREFIX: &str = "signer_error:key_unavailable: "; /// Preserve a structured `SigningKeyUnavailable` signer failure through an @@ -1199,7 +1196,7 @@ pub const SIGNER_KEY_UNAVAILABLE_PREFIX: &str = "signer_error:key_unavailable: " /// The check is **structural and position-0 only** (the marker must start the /// nested `ProtocolError::Generic` payload); it is never a substring sniff of /// the rendered error, so a foreign signer that merely mentions the token is -/// not misrouted into key repair (dashpay/platform#4183 review). This mirrors +/// not misrouted into key repair. This mirrors /// the guarded restore already performed by the FFI conversion. pub fn preserve_signer_key_unavailable_or( error: dash_sdk::Error, @@ -1258,7 +1255,7 @@ mod signer_key_unavailable_tests { /// The marker only counts at position 0: a generic error that merely /// mentions it mid-message is wrapped, never preserved as the typed - /// key-unavailable shape (dashpay/platform#4183 review). + /// key-unavailable shape. #[test] fn substring_marker_is_not_preserved() { let error = dash_sdk::Error::Protocol(dpp::ProtocolError::Generic(format!( diff --git a/packages/rs-platform-wallet/src/manager/accessors.rs b/packages/rs-platform-wallet/src/manager/accessors.rs index 0f77a3a4b70..f63c2f3eecf 100644 --- a/packages/rs-platform-wallet/src/manager/accessors.rs +++ b/packages/rs-platform-wallet/src/manager/accessors.rs @@ -161,11 +161,10 @@ pub struct TrackedAssetLockSnapshot { /// Snapshot of the per-account metadata for a single account. /// -/// `is_watch_only` and `custom_name` were dropped after upstream -/// removed both from `ManagedCoreFundsAccount` / `ManagedCoreKeysAccount`. -/// Watch-only is now a wallet-level property (read off `Wallet.wallet_type`) -/// and `AccountMetadata` no longer exists. Re-add fields here only if -/// the upstream variants gain them again. +/// Carries no `is_watch_only` or `custom_name`: upstream's +/// `ManagedCoreFundsAccount` / `ManagedCoreKeysAccount` have neither, and +/// watch-only is a wallet-level property (read off `Wallet.wallet_type`). +/// Add such fields here only if the upstream variants gain them. #[derive(Debug, Clone, Copy)] pub struct AccountMetadataSnapshot { pub total_transactions: u64, diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index a8669115513..c44d9cdb277 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -91,10 +91,9 @@ impl PlatformWalletManager

{ // (below) and key this generation's in-broadcast fence map by it. let wallet_id = wallet.compute_wallet_id(); - // The fence map is per WALLET, not per generation - // (`dashpay/platform#4309`, review round 8). On a first load the - // registry is empty and this is a fresh map; a re-load — or a load - // that follows a removal — inherits whatever pending spends the + // The fence map is per WALLET, not per generation. On a first load + // the registry is empty and this is a fresh map; a re-load — or a + // load that follows a removal — inherits whatever pending spends the // previous generation under this id left standing, rather than // handing the restored UTXOs back unprotected. // @@ -478,8 +477,8 @@ mod idempotent_load_tests { /// The app re-activates its per-network manager on every SDK emission, /// which re-runs `load_from_persistor` against a manager that already /// holds the persisted wallet. The second (and every later) call must - /// be a no-op `Ok(())` — NOT the `WalletExists`-wrapped - /// `WalletCreation` error that used to crash the app on the main + /// be a no-op `Ok(())` — NOT a `WalletExists`-wrapped + /// `WalletCreation` error, which crashes the app on the main /// thread. Exactly one wallet stays registered across the calls. #[tokio::test] async fn repeated_load_from_persistor_is_idempotent() { @@ -500,9 +499,9 @@ mod idempotent_load_tests { "first load must register exactly the persisted wallet" ); - // Re-hydrating with the wallet already present used to surface - // `Failed to register persisted wallet in WalletManager: Wallet - // already exists`. It must now be a silent no-op. + // Re-hydrating with the wallet already present must be a silent + // no-op, not `Failed to register persisted wallet in WalletManager: + // Wallet already exists`. manager .load_from_persistor() .await @@ -519,8 +518,8 @@ mod idempotent_load_tests { ); } - /// `dashpay/platform#4309`-adjacent lifecycle hazard: a rollback must not - /// remove a registration it did not make. + /// Lifecycle hazard: a rollback must not remove a registration it did not + /// make. /// /// The interleaving: this load publishes generation G1 under an id, a /// concurrent `remove_wallet` frees that id, a registration publishes G2 diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index d1ea58404d3..e95c712c338 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -427,7 +427,7 @@ pub struct PlatformWalletManager { /// cancellation token; [`shutdown`](Self::shutdown) cancels, joins, and /// reports per-worker terminal status. pub(super) registry: Arc>, - /// Host-visible hard sync-fault latch (dashpay/platform#4069). Set + /// Host-visible hard sync-fault latch. Set /// (and never cleared for this manager instance's lifetime) by the /// wallet-event adapter the first time it freezes a durable watermark /// after a persistence `store()` rejection — the one remaining fault @@ -438,21 +438,21 @@ pub struct PlatformWalletManager { pub(super) sync_fault: Arc, /// Per-WALLET in-broadcast fence maps, handed to every /// [`WalletGeneration`](crate::wallet::core::WalletGeneration) registered - /// under each id (`dashpay/platform#4309`, review round 8). + /// under each id. /// /// A fence describes a signed transaction that may be live on the network. - /// That fact outlives the wallet *instance* that dispatched it: removing a - /// wallet and re-creating it under the same id used to mint a generation - /// with an empty map, so the re-created wallet restored the persisted UTXO + /// That fact outlives the wallet *instance* that dispatched it: if removing + /// a wallet and re-creating it under the same id minted a generation with + /// an empty map, the re-created wallet would restore the persisted UTXO /// with nothing holding it — not the fence, not key-wallet's memory-only /// reservation — and could sign a conflicting spend of an outpoint the - /// original transaction still spends. Keying the map here instead makes the + /// original transaction still spends. Keying the map here makes the /// replacement inherit it. /// /// **Deliberately never pruned.** A removed wallet's entry stays, because a /// removal is exactly when the protection must survive; dropping it on - /// removal would restore the bug for the recreate-after-remove path this - /// exists to close. Growth is bounded by the number of distinct wallet ids + /// removal would reopen the recreate-after-remove hazard this exists to + /// close. Growth is bounded by the number of distinct wallet ids /// this process has registered, and each entry reaps its own cleared rows /// on read. /// @@ -476,12 +476,11 @@ impl PlatformWalletManager

{ ) -> Self { // Take the manager's lossless, unbounded persistence receiver BEFORE // the manager is wrapped in the shared `Arc` and handed to any - // producer. Unlike the old broadcast subscription, an - // `mpsc::UnboundedReceiver` buffers events emitted during startup - // rather than dropping them, so there is no subscribe-before-publish - // race and — being unbounded — it can never `Lagged` and freeze the - // durable sync watermark (dashpay/platform#4069). The receiver is - // taken here, once, and moved into the adapter task below. + // producer. A broadcast subscription would drop events emitted during + // startup; an `mpsc::UnboundedReceiver` buffers them instead, so + // there is no subscribe-before-publish race and — being unbounded — + // it can never `Lagged` and freeze the durable sync watermark. The + // receiver is taken here, once, and moved into the adapter task below. let mut wallet_manager_inner = WalletManager::new(sdk.network); let event_receiver = wallet_manager_inner .take_persistence_receiver() @@ -495,7 +494,7 @@ impl PlatformWalletManager

{ // handles for a clean, panic-aware shutdown join. let registry = ThreadRegistry::::new(); - // Host-visible hard sync-fault latch (dashpay/platform#4069). The + // Host-visible hard sync-fault latch. The // adapter raises it the first time it freezes a durable watermark. let sync_fault = Arc::new(std::sync::atomic::AtomicBool::new(false)); @@ -522,7 +521,7 @@ impl PlatformWalletManager

{ let balance_handler = Arc::new(BalanceUpdateHandler::new(Arc::clone(&wallets))); // SpendObservationHandler releases in-broadcast input fences when the // wallet observes the fenced outpoints spent — the evidence that ends - // the fence a dispatch installs (`dashpay/platform#4309`). It takes the + // the fence a dispatch installs. It takes the // same `wallets` map, and for the same lock reason as the balance // handler: the event fires inside SPV's block-processing write section, // so the generation cannot be resolved through the wallet-manager lock. @@ -615,7 +614,7 @@ impl PlatformWalletManager

{ /// manager mints for a wallet is built from this, so a generation that /// replaces another under the same id inherits its pending-spend fences — /// see the [`in_broadcast_fences`](Self#structfield.in_broadcast_fences) - /// field docs (`dashpay/platform#4309`). + /// field docs. pub(super) fn in_broadcast_fences_for( &self, wallet_id: &WalletId, @@ -630,13 +629,13 @@ impl PlatformWalletManager

{ } /// Whether the wallet-event adapter has frozen a durable sync - /// watermark this manager's lifetime (dashpay/platform#4069). + /// watermark this manager's lifetime. /// /// Returns `true` once — and stays `true` for THIS manager instance's /// lifetime (a destroyed-and-recreated manager starts unlatched) — - /// after a persistence `store()` was rejected, the one remaining fault - /// trigger: the lossless persistence channel cannot drop or lag events, - /// so the old broadcast-lag trigger no longer exists. A latch means the + /// after a persistence `store()` was rejected, the only fault trigger: + /// the lossless persistence channel cannot drop or lag events. A latch + /// means the /// persisted `syncedHeight` is deliberately held behind the chain tip /// for the affected wallet and a rescan is pending on the next launch. /// Integrators poll this to @@ -760,18 +759,17 @@ impl PlatformWalletManager

{ /// /// # The missing-coordinator case is an ERROR, not a silent no-op /// - /// This used to `Ok(())` when `shielded_coordinator()` was `None`, - /// treating "no coordinator" as "nothing to clear". That masked the exact - /// on-device failure this fix targets: the host taps Clear on a manager - /// whose coordinator is **not installed on this instance** — e.g. an SDK - /// rebuild handed the host a fresh `PlatformWalletManager` whose - /// `configure_shielded` never ran (or ran on a different instance than the - /// one currently syncing). The quiesce runs (sync loop stops), the call - /// returns `Ok`, and the host then wipes its own Room/SwiftData rows — - /// while the **on-disk commitment tree is never touched** (file mtime - /// unchanged on device, no `reset_commitment_tree` call). The next bind - /// reloads the still-full tree + its persisted watermark and re-freezes - /// everything. + /// Returning `Ok(())` when `shielded_coordinator()` is `None` — treating + /// "no coordinator" as "nothing to clear" — masks a real on-device + /// failure: the host taps Clear on a manager whose coordinator is **not + /// installed on this instance** — e.g. an SDK rebuild handed the host a + /// fresh `PlatformWalletManager` whose `configure_shielded` never ran (or + /// ran on a different instance than the one currently syncing). The + /// quiesce runs (sync loop stops), the call returns `Ok`, and the host + /// then wipes its own Room/SwiftData rows — while the **on-disk + /// commitment tree is never touched** (file mtime unchanged on device, no + /// `reset_commitment_tree` call). The next bind reloads the still-full + /// tree + its persisted watermark and re-freezes everything. /// /// The FFI only exposes this call behind a bound, shielded-enabled host /// surface (the "Clear" button), so reaching it with no coordinator is a @@ -1095,8 +1093,8 @@ mod tests { } /// The constructor must register [`SpendObservationHandler`] on the event - /// fan-out, over the LIVE wallets map (`dashpay/platform#4309`, review - /// round 6): a spend-bearing wallet event dispatched through the manager's + /// fan-out, over the LIVE wallets map: a spend-bearing wallet event + /// dispatched through the manager's /// own `event_manager` must release a registered wallet's in-broadcast /// fence. Dropping the handler from the constructor's handler list — the /// accidental-omission regression this pins — fails the final assertion, diff --git a/packages/rs-platform-wallet/src/manager/platform_address_sync.rs b/packages/rs-platform-wallet/src/manager/platform_address_sync.rs index f8e7cefb127..d0a5bb2eabb 100644 --- a/packages/rs-platform-wallet/src/manager/platform_address_sync.rs +++ b/packages/rs-platform-wallet/src/manager/platform_address_sync.rs @@ -1,9 +1,8 @@ //! Periodic platform-address balance sync coordinator. //! -//! Mirrors what iOS used to do in `PlatformBalanceSyncService`: run -//! [`PlatformAddressWallet::sync_balances`] for every registered wallet -//! on a fixed cadence, and emit a summary event so UI and persistence -//! layers can react. +//! Runs [`PlatformAddressWallet::sync_balances`] for every registered +//! wallet on a fixed cadence, and emits a summary event so UI and +//! persistence layers can react. //! //! Not auto-started. Call [`PlatformAddressSyncManager::start`] once the //! wallets are registered and the SPV runtime is up. @@ -32,7 +31,7 @@ use crate::manager::{ use crate::wallet::platform_wallet::WalletId; use crate::wallet::PlatformWallet; -/// Default cadence — matches the 15s BLAST loop we previously ran in Swift. +/// Default cadence. pub const DEFAULT_SYNC_INTERVAL_SECS: u64 = 15; /// Outcome of syncing a single wallet in a pass. @@ -718,10 +717,10 @@ mod tests { } /// `sync_wallet` is a second entry point into the same per-wallet - /// state, so it must observe the same admission as `sync_now` — it - /// used to bypass both the `is_syncing` slot and the gate entirely, - /// which let a per-wallet sync take a wallet's provider lock and - /// persist a fresh watermark right after a reset cleared it. + /// state, so it must observe the same admission as `sync_now` — + /// bypassing the `is_syncing` slot or the gate would let a per-wallet + /// sync take a wallet's provider lock and persist a fresh watermark + /// right after a reset cleared it. #[tokio::test] async fn sync_wallet_is_refused_while_admission_is_shut() { let (mgr, _counter) = make_manager(); diff --git a/packages/rs-platform-wallet/src/manager/startup.rs b/packages/rs-platform-wallet/src/manager/startup.rs index 168512f4d40..7d19ee1a959 100644 --- a/packages/rs-platform-wallet/src/manager/startup.rs +++ b/packages/rs-platform-wallet/src/manager/startup.rs @@ -222,9 +222,9 @@ pub enum WalletStartupStatus { /// The distinction from [`Self::Ready`] is the whole point: an identity /// hiding at an unanswered index is invisible to everything that consults /// local state, so calling this launch `Ready` promises an identity set - /// that was never established. That is #4365's exact shape, one level up — - /// the wallet has *an* identity, so the warm-launch shortcut and every - /// tally signal read clean while a second identity stays lost. + /// that was never established. The lost-second-identity shape recurs one + /// level up: the wallet has *an* identity, so the warm-launch shortcut + /// and every tally signal read clean while a second identity stays lost. /// /// Not terminal: the verdict stays on record, so the next launch re-opens /// the question instead of taking the shortcut. Nothing about the contact @@ -242,10 +242,9 @@ impl WalletStartupStatus { /// the failure is local and will still be there next time, or the scan /// answered everything it probed. /// - /// This is the distinction platform#4352 made expressible: before it, "no - /// identity exists" and "we never got through" both arrived as an empty - /// success, so clients either retried a proven-empty scan forever or cached - /// a network failure as fact. + /// The distinction matters: if "no identity exists" and "we never got + /// through" both arrived as an empty success, clients would either retry a + /// proven-empty scan forever or cache a network failure as fact. pub fn discovery_worth_retrying(self) -> bool { matches!(self, Self::PartialNoIdentity | Self::IdentityScanIncomplete) } @@ -412,12 +411,10 @@ impl StartupTally { /// identity there is nothing to have drained. pub(crate) fn status(&self) -> WalletStartupStatus { // Both of these say "the identity question is still open", so neither - // may decide the verdict once an identity is known. That used to be - // structurally impossible — discovery ran only when nothing was on - // file, and every branch that found something returned early — but a - // rescan forced by an incomplete prior scan reaches them with an - // identity already recorded, and reporting *that* launch as - // `DiscoveryFailed` would hide a sync and drain that both ran. + // may decide the verdict once an identity is known. A rescan forced + // by an incomplete prior scan reaches them with an identity already + // recorded, and reporting *that* launch as `DiscoveryFailed` would + // hide a sync and drain that both ran. // // A local fault outranks unreachability: both leave the question open, // but only this one tells the client not to bother asking again. @@ -451,11 +448,11 @@ impl StartupTally { // Last, and deliberately so: every check above describes work this // launch did, while this one describes an identity set the wallet is // on record as not having fully established. Ranking it here is what - // makes the fix additive — the only run whose status changes is the - // one that used to come back `Ready`, which is precisely the run that - // was lying. Everything else keeps the status a client already - // handles, and reads `identity_scan_incomplete` on the outcome if it - // cares. + // keeps the check additive — the only run it reclassifies is the one + // that would otherwise come back `Ready`, which is precisely the run + // that would be lying. Everything else keeps the status a client + // already handles, and reads `identity_scan_incomplete` on the outcome + // if it cares. // // `Ready` is the promise that a contact payment has everything it // needs. An unanswered index can hide a whole identity from every @@ -550,10 +547,10 @@ impl PlatformWalletManager // scan it does not need — unless the scan that produced those // identities is on record as having left indices unanswered, in // which case "we already have one" is not evidence that we have - // them all. A wallet whose second identity was hidden by a failed - // probe used to stay that way for the life of the installation, - // because this shortcut is the only thing that would have looked - // again (dashpay/platform#4365). + // them all. Without that exception a wallet whose second identity + // was hidden by a failed probe stays that way for the life of the + // installation, because this shortcut is the only thing that would + // look again. // // Only a recorded incomplete scan re-opens the question. An absent // verdict keeps the shortcut, so hosts that do not persist it are @@ -783,8 +780,8 @@ impl PlatformWalletManager /// An `Ok` result ends the loop whether or not it found anything: Platform /// answered, and an empty answer is a proof of absence that rescanning /// cannot overturn. Only [`PlatformWalletError::IdentityDiscoveryIncomplete`] - /// — the error platform#4352 introduced for a scan that never got through - /// — is worth another attempt. + /// — the error for a scan that never got through — is worth another + /// attempt. async fn discover_identity_with_backoff( &self, wallet_id: &WalletId, @@ -863,9 +860,9 @@ impl PlatformWalletManager // prefix of the index space and answered the rest of it not at // all, which is exactly the state a later launch must not // mistake for a settled identity set. Without this the - // budget-expiry path reproduces #4365 in its own right — it - // consults local state, finds the sighting that was persisted - // before cancellation, and records a warm launch. + // budget-expiry path hides a second identity in its own right + // — it consults local state, finds the sighting that was + // persisted before cancellation, and records a warm launch. self.record_identity_scan_cut_off(wallet_id).await; // Sightings persist incrementally, so an abandoned scan may // still have folded an identity in before it was cut off. @@ -1022,8 +1019,8 @@ mod tests { ); } - /// The opposite case, and the reason the distinction is expressible at all - /// (platform#4352): never reaching Platform is not evidence of absence. + /// The opposite case, and the reason the distinction exists at all: never + /// reaching Platform is not evidence of absence. #[test] fn unreachable_discovery_is_not_settled() { let mut tally = StartupTally::default(); @@ -1213,11 +1210,11 @@ mod tests { /// the rescan failing is not nothing either: it means the scan gap that /// forced it is still there. /// - /// This test previously asserted `Ready` for the unreachable half, pinning - /// the very defect the `identity_scan_incomplete` signal exists to close — - /// a launch that knows its identity set is partial reporting the status - /// that promises it is complete. Both halves keep their real subject (the - /// identity must not be re-opened) and now assert the gap is reported. + /// Asserting `Ready` for the unreachable half would pin the very defect + /// the `identity_scan_incomplete` signal exists to close — a launch that + /// knows its identity set is partial reporting the status that promises it + /// is complete. Both halves keep their real subject (the identity must not + /// be re-opened) and assert the gap is reported. #[test] fn a_failed_rescan_reports_the_scan_gap_without_reopening_the_identity() { // The scenario the name describes: the prior verdict said incomplete, @@ -1617,7 +1614,7 @@ mod tests { assert!(!report.is_complete()); assert!(report.is_fully_degraded()); - // The back-compat return shape can no longer render this as success. + // The back-compat return shape must not render this as success. let err = wallet .identity() .dashpay() diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index 270d9ff6aa2..c844c1b9e7b 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -173,7 +173,7 @@ impl PlatformWalletManager

{ // downstream `walletId`-keyed structure network-correct by // construction — no per-network disambiguation needed in the // persistence layer, and network-blind child tables (UTXOs, - // asset locks, platform addresses) can no longer cross-feed + // asset locks, platform addresses) cannot cross-feed // between a mnemonic's per-network wallets. The watch-only // restore path (`Wallet::new_external_signable`) reuses the // persisted id verbatim, so it stays self-consistent across @@ -217,9 +217,8 @@ impl PlatformWalletManager

{ // A registration under an id a previous generation held is exactly the // remove-and-recreate case: that generation's pending-spend fences // protect signed transactions that are still valid and still relayable, - // so the replacement inherits them rather than starting clean - // (`dashpay/platform#4309`, review round 8). A first registration finds - // no entry and gets an empty map, as before. + // so the replacement inherits them rather than starting clean. A first + // registration finds no entry and gets an empty map. let registration_wallet_id = wallet.compute_wallet_id(); let generation = Arc::new(WalletGeneration::with_fences( self.in_broadcast_fences_for(®istration_wallet_id), @@ -668,8 +667,7 @@ impl PlatformWalletManager

{ /// inside it, `CoreWallet::is_same_generation` passes for a removed /// generation (a removed generation matches itself), and the reservation age /// guard is disabled once `last_processed_height` returns `None`. So a - /// payment for a wallet the host already deleted reaches the network - /// (`dashpay/platform#4185`). + /// payment for a wallet the host already deleted reaches the network. /// /// Taking the gate *inside* this method rather than leaving it to the caller /// is deliberate: `PlatformWalletManager` is public and `SignedPaymentRegistry` @@ -714,7 +712,7 @@ impl PlatformWalletManager

{ /// The `Arc` validated under the gate is therefore retained, /// and the public-map entry is removed only while it still names that same /// generation. Both maps, the returned handle and the `tear_down` argument - /// are then all that one generation (`dashpay/platform#4185`). The one + /// are then all that one generation. The one /// remaining id-keyed step is the shielded coordinator detach below, which /// has no generation concept at all; a generation that has just been /// registered has not run `bind_shielded` yet, so it holds no coordinator @@ -1138,25 +1136,25 @@ mod register_wallet_duplicate_tests { ); } - /// `dashpay/platform#4309`, REVIEW ROUND 8 — PENDING-SPEND PROTECTION MUST - /// SURVIVE WALLET RECREATION. + /// PENDING-SPEND PROTECTION MUST SURVIVE WALLET RECREATION. /// - /// The in-broadcast fence used to live in the `WalletGeneration` itself, so - /// it was not merely process-local but *generation*-local. Removing a wallet - /// and re-creating it under the same id mints a fresh generation, and the - /// fence map went with the old one — while the signed transaction it was - /// protecting stays perfectly valid and can still be relayed by a DAPI - /// endpoint or a peer that retained it. The re-created wallet restored the - /// persisted UTXO with neither the fence nor key-wallet's memory-only - /// reservation holding it, and could sign a conflicting spend of the very - /// same outpoint. + /// If the in-broadcast fence lived in the `WalletGeneration` itself, it + /// would be not merely process-local but *generation*-local. Removing a + /// wallet and re-creating it under the same id mints a fresh generation, + /// and the fence map would go with the old one — while the signed + /// transaction it protects stays perfectly valid and can still be relayed + /// by a DAPI endpoint or a peer that retained it. The re-created wallet + /// would restore the persisted UTXO with neither the fence nor key-wallet's + /// memory-only reservation holding it, and could sign a conflicting spend + /// of the very same outpoint. /// /// Fences are therefore keyed by WALLET, not by generation: a generation /// that replaces another under the same id inherits its predecessor's /// pending-spend fences, and they are retired by the same evidence as ever — /// an observed spend — not by the replacement. /// - /// Red before the fix: the re-created wallet reported no conflict at all. + /// Without wallet-keyed fences the re-created wallet reports no conflict + /// at all. #[tokio::test] async fn a_recreated_wallet_inherits_the_pending_fences_of_the_generation_it_replaces() { use dashcore::hashes::Hash; @@ -1297,8 +1295,7 @@ mod register_wallet_duplicate_tests { } } -/// Removal versus a same-id re-registration that lands *during* the removal -/// (`dashpay/platform#4185` review). +/// Removal versus a same-id re-registration that lands *during* the removal. /// /// The invariant: `remove_wallet_with_teardown` removes, returns and tears down /// exactly the wallet generation it validated under that generation's lifecycle diff --git a/packages/rs-platform-wallet/src/masternode/record.rs b/packages/rs-platform-wallet/src/masternode/record.rs index aa183ed0da5..ef49abb979d 100644 --- a/packages/rs-platform-wallet/src/masternode/record.rs +++ b/packages/rs-platform-wallet/src/masternode/record.rs @@ -579,7 +579,7 @@ mod tests { assert!(mn.owner_key_hash.is_some()); assert!(mn.voting_key_hash.is_some()); assert!(mn.collateral.is_some()); - // #4116 key-ownership extraction: operator BLS key + payout script + // Key-ownership extraction: operator BLS key + payout script // are lifted; the legacy (v1) fixture is a regular MN so it has no // platform node id. assert!( diff --git a/packages/rs-platform-wallet/src/spv/runtime.rs b/packages/rs-platform-wallet/src/spv/runtime.rs index 4839b891e7b..2eb2701f130 100644 --- a/packages/rs-platform-wallet/src/spv/runtime.rs +++ b/packages/rs-platform-wallet/src/spv/runtime.rs @@ -765,7 +765,7 @@ mod shutdown_tests { /// host callbacks) and report SPV non-clean. /// /// Without the post-abort deadline this test hangs: that is exactly the - /// hang that reached the FFI's `destroy` before this fix. + /// hang that would reach the FFI's `destroy`. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn spv_task_surviving_abort_is_returned_for_reparking() { let (started_tx, started_rx) = tokio::sync::oneshot::channel(); diff --git a/packages/rs-platform-wallet/src/wallet/apply.rs b/packages/rs-platform-wallet/src/wallet/apply.rs index 58fe2b063fc..abd89c534ec 100644 --- a/packages/rs-platform-wallet/src/wallet/apply.rs +++ b/packages/rs-platform-wallet/src/wallet/apply.rs @@ -141,7 +141,7 @@ impl PlatformWalletInfo { // not through changeset replay. The core field on `cs` is // therefore informational here and intentionally not // applied; we drop it explicitly so future readers don't - // expect a re-application path that no longer exists. + // expect a re-application path that does not exist. drop(core); // 2. Identities. @@ -774,7 +774,7 @@ mod tests { /// Token-balance changesets are accepted by `apply_changeset` for /// shape compatibility but are not replayed onto - /// `PlatformWalletInfo` (which no longer has token_balances / + /// `PlatformWalletInfo` (which has no token_balances / /// token_watched fields). The canonical balance cache lives on /// `IdentitySyncManager` and is rebuilt by the next sync pass; the /// FFI persister surfaces the upserts/tombstones to the Swift side @@ -1396,9 +1396,8 @@ mod tests { assert_eq!(restored.identity.revision(), 5); } - /// Reviewer #6d: contact tombstone for a present (non-orphan) owner - /// must drop the matching pending request — happy-path coverage - /// previously only existed via the orphan-skip test. + /// A contact tombstone for a present (non-orphan) owner + /// must drop the matching pending request. #[test] fn apply_contact_tombstone_drops_pending_for_present_owner() { let mut wallet = build_test_wallet(); @@ -1855,10 +1854,9 @@ mod tests { }); // Token balance changesets are accepted for shape compat but - // no longer drive `PlatformWalletInfo` state — the manager + // do not drive `PlatformWalletInfo` state — the manager // owns the balance cache. Include one anyway to confirm the - // double-apply still works once the field has been replaced - // with a `drop`. + // double-apply still works while the field is simply dropped. let mut tok_cs = TokenBalanceChangeSet::default(); let token = Identifier::from([8u8; 32]); tok_cs.balances.insert((identity, token), 42); diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index 3b24cec9b40..87ee04d46a8 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -65,9 +65,9 @@ impl AssetLockManager { /// `DerivationPath` is what the caller hands back to the same /// `signer` when the credit output is later consumed on Platform. /// - /// Exact-amount form — the historical entry point, now **pooled**: it funds - /// from [`ASSET_LOCK_FUNDING_SOURCES`] (BIP44 + BIP32 + every DashPay - /// contact-receiving account), so the lock no longer needs its whole amount + /// Exact-amount form, **pooled**: it funds from + /// [`ASSET_LOCK_FUNDING_SOURCES`] (BIP44 + BIP32 + every DashPay + /// contact-receiving account), so the lock does not need its whole amount /// sitting in one account and change returns to BIP44. The /// funding-parameterized form is /// [`Self::build_asset_lock_transaction_with_funding`]. @@ -187,8 +187,7 @@ impl AssetLockManager { /// that gap, and a competing build can then sweep and re-reserve this very /// input, find no fence, pass its own copy of the check, and complete — /// after which this build's already-signed asset lock still goes to the wire - /// against an input reassigned to another payment (`dashpay/platform#4309`, - /// review round 7). + /// against an input reassigned to another payment. /// /// The returned pin closes that. The CALLER OWNS ITS SETTLEMENT and must /// account for every exit: [`InBroadcastPin::settle_released`] on a @@ -341,8 +340,8 @@ impl AssetLockManager { // // `build_asset_lock_with_signer` always returns the `Public` // variant. The `Private` arm would only come from the soft- - // wallet `build_asset_lock` path which we no longer call from - // platform-wallet — defensively bail if it appears. + // wallet `build_asset_lock` path, which platform-wallet does not + // call — defensively bail if it appears. use key_wallet::wallet::managed_wallet_info::asset_lock_builder::AssetLockCreditKeys; let path = match result.keys { AssetLockCreditKeys::Public(mut keys) => { @@ -950,7 +949,7 @@ impl AssetLockManager { // `in_broadcast_pin` fences those inputs from the moment they were // reserved — installed under the build's own write guard, so no // competing build can sweep and re-reserve them across the durability - // gate and the broadcast await below (`dashpay/platform#4309`). Every + // gate and the broadcast await below. Every // exit from here on settles it: released on the aborts that never // reach the broadcaster and on a definitive rejection, left pending // otherwise. @@ -997,9 +996,8 @@ impl AssetLockManager { // transaction to protect: release it alongside the reservation // — but AFTER the cleanup, never before it. The cleanup awaits // the manager read lock, and an input that is unfenced while - // still reserved-or-reusable is exactly the window review round - // 8 closed on the contact-send path - // (`dashpay/platform#4309`). This site's release is + // still reserved-or-reusable is exactly the window the + // contact-send path closes. This site's release is // owner-guarded by `reservation_token`, so a newer build's // reservation cannot be clobbered here even so; the ordering is // uniform across every settle-with-cleanup site rather than @@ -1010,7 +1008,7 @@ impl AssetLockManager { // released on drop: the abort is established and nothing was // sent, so a pending-spend settle there would fence inputs no // observed spend could ever clear — same shape as the - // contact-send rejection arm (`dashpay/platform#4309`). + // contact-send rejection arm. let mut in_broadcast_pin = in_broadcast_pin; in_broadcast_pin.settle_released_on_drop(); crate::wallet::reservations::release_reservation_after_rejected_broadcast( @@ -1064,7 +1062,7 @@ impl AssetLockManager { // leaving it would block the retry the release exists to enable. // It comes down AFTER the cleanup, not before — see the // drain-floor branch above for why every settle-with-cleanup - // site keeps that order (`dashpay/platform#4309`, round 8) — + // site keeps that order — // and the released verdict is recorded BEFORE the cleanup's // first await, so a cancellation inside it settles released // rather than opening an uncleanable pending-spend fence over @@ -1162,9 +1160,8 @@ impl AssetLockManager { // Provably nothing on the wire and the row is gone: free the // fence with the reservation so the rebuild can reselect — // the fence coming down LAST, after the cleanup await, so - // the input is never unfenced while still reusable - // (`dashpay/platform#4309`, round 8; see the drain-floor - // branch for the full window). + // the input is never unfenced while still reusable (see + // the drain-floor branch for the full window). // // The released verdict IS established now — rejected AND // unresumable — so it is recorded before the cleanup's @@ -1279,7 +1276,7 @@ impl AssetLockManager { /// Map a key-wallet [`AssetLockError`] to a [`PlatformWalletError`], promoting /// every coin-selection shortfall shape to the typed /// [`PlatformWalletError::AssetLockInsufficientFunds`] so callers get one -/// structured shortfall contract (dashpay/platform#4073) instead of a string +/// structured shortfall contract instead of a string /// they must pattern-match: /// /// - `BuilderError::InsufficientFunds` / `SelectionError::InsufficientFunds` @@ -1364,7 +1361,7 @@ mod tests { /// The zero-spendable-candidate selection error must surface the SAME /// typed shortfall as a partial shortfall (not the generic string form), /// so hosts stay on one structured path; and a partial shortfall must - /// still carry its own exact amounts (dashpay/platform#4073). + /// still carry its own exact amounts. #[test] fn coin_selection_shortfalls_map_to_typed_insufficient_funds() { use super::{map_builder_error, AssetLockError, BuilderError, SelectionError}; @@ -1552,14 +1549,13 @@ mod tests { *utxos.keys().next().expect("one utxo") } - /// `dashpay/platform#4309`, REVIEW ROUND 7 — THE ASSET-LOCK BUILD'S OWN - /// FENCE. + /// THE ASSET-LOCK BUILD'S OWN FENCE. /// - /// The build's conflict check stopped it from CONSUMING an input another - /// dispatch had fenced. It did not fence the selection it had just made, so + /// The build's conflict check stops it from CONSUMING an input another + /// dispatch has fenced. Without a fence on the selection it has just made, /// everything between the check and the direct `broadcaster.broadcast(&tx)` /// — the pool durability gate, the `Built` tracking write, and the await - /// itself — ran with no pin on those inputs. + /// itself — would run with no pin on those inputs. /// /// 1. A funded asset lock builds, signs, releases the manager guard, and /// SUSPENDS inside the broadcaster before submission. @@ -1568,8 +1564,8 @@ mod tests { /// 3. A competing asset-lock build runs. There is exactly one spendable /// UTXO, so it selects the same input the parked lock already spends. /// - /// Before the fix step 3 SUCCEEDED and returned a second signed asset lock - /// against that input. It must now be refused with `InputMidBroadcast`. + /// Step 3 must be refused with `InputMidBroadcast` rather than returning a + /// second signed asset lock against that input. /// /// The two builds run through two `AssetLockManager`s over ONE shared /// wallet manager. That is not a workaround for the per-manager diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs index b9c810b6d26..e5eb64ba719 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs @@ -52,11 +52,8 @@ pub struct AssetLockManager { /// queue their own `AssetLockChangeSet`s into the changeset flush /// boundary without round-tripping through the parent wallet. /// - /// Item 8 sub-step 1a: previously mutations returned - /// `AssetLockChangeSet` and callers (including - /// `create_funded_asset_lock_proof` itself) dropped them with - /// `let _cs = ...`. Every emitted changeset now flows straight - /// into `queue_persist` here. + /// Invariant: no mutation drops its `AssetLockChangeSet` — every + /// emitted changeset flows straight into `queue_persist` here. pub(super) persister: WalletPersister, /// Serializes the funding-index-critical section of /// [`broadcast_funded_asset_lock`](Self::broadcast_funded_asset_lock) — diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs index f4056957dea..75512084f8b 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs @@ -88,10 +88,9 @@ pub(crate) const CL_FALLBACK_TIMEOUT: Duration = Duration::from_secs(180); /// /// On expiry the reconciliation still returns the typed /// [`PlatformWalletError::AssetLockAlreadyConsumed`] — the code-24 signal -/// hosts branch on — having simply failed to attach the chain proof. That -/// matches the pre-#4357 behavior (typed error, no proof retained) while -/// keeping #4357's proof retention whenever the ChainLock is reachable -/// inside the bound. +/// hosts branch on — having simply failed to attach the chain proof. The +/// typed error is what matters to the host; the proof is retained only +/// when the ChainLock is reachable inside the bound. pub(crate) const RECONCILIATION_CHAIN_LOCK_TIMEOUT: Duration = Duration::from_secs(180); /// Bounded proof wait applied after a resume re-broadcast came back @@ -108,8 +107,8 @@ pub(crate) const RECONCILIATION_CHAIN_LOCK_TIMEOUT: Duration = Duration::from_se /// /// Sized to comfortably cover a ChainLock (~2.5 min) so a transaction that /// really was accepted still resolves inside the bound; on expiry the -/// caller gets `TransactionBroadcastUnconfirmed`, which is what the -/// pre-#4367 code returned immediately. +/// caller gets `TransactionBroadcastUnconfirmed`, the same verdict an +/// immediate give-up would report. pub(crate) const UNCONFIRMED_BROADCAST_PROOF_TIMEOUT: Duration = Duration::from_secs(180); /// Delay between retries when Platform rejected with CL-height-too-low. diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs index 6d4b674d965..d606aca555f 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs @@ -368,9 +368,8 @@ impl AssetLockManager { loop { // Arm the `Notify` future BEFORE the state check, closing - // the missed-wakeup race in dashpay/platform#3641 - // (Found-008): `notify_waiters()` only wakes already- - // registered waiters and does NOT store a permit, so a + // the missed-wakeup race: `notify_waiters()` only wakes + // already-registered waiters and does NOT store a permit, so a // CL/IS event arriving in the gap between "no proof yet" // and the `.await` below would be discarded and we'd // sleep until `FinalityTimeout`. Calling `enable()` on @@ -473,9 +472,8 @@ impl AssetLockManager { loop { iter += 1; // Arm the `Notify` future BEFORE the state check, closing - // the missed-wakeup race in dashpay/platform#3641 - // (Found-008): `notify_waiters()` only wakes already- - // registered waiters and does NOT store a permit, so an + // the missed-wakeup race: `notify_waiters()` only wakes + // already-registered waiters and does NOT store a permit, so an // IS/CL event arriving in the gap between "no proof yet" // and the `.await` below would be discarded and we'd // sleep until `FinalityTimeout`. Calling `enable()` on diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs index e0a88a2e6f0..4f61b6e9988 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs @@ -77,7 +77,7 @@ impl AssetLockManager { // Phase 2 (no lock held): resolve status. The persister // fallback's I/O (synchronous lookup, possibly an FFI - // callback into a SwiftData query) is no longer serialized + // callback into a SwiftData query) is not serialized // behind the wallet-manager write lock. let (status, proof) = match proof { Some(ref p) => { @@ -777,8 +777,8 @@ impl AssetLockManager { // SPV broadcaster only reaches `Rejected` on `NotConnected`. // So the advance above cannot be read as evidence the tx is // live, and the expiry of the bounded wait that follows it is - // translated back into the `TransactionBroadcastUnconfirmed` - // the caller used to get immediately. + // translated back into the same `TransactionBroadcastUnconfirmed` + // an immediate failure reports. // // A DEFINITE `Rejected` is scoped to the attempt that // produced it, exactly as on the `Broadcast` arm below: with @@ -1290,11 +1290,10 @@ impl AssetLockManager { /// `key_wallet::signer::Signer` when later consuming the credit /// output on Platform. /// - /// Previously this method derived the actual private key from the - /// wallet's root xpriv; that path is no longer reachable for - /// `ExternalSignable` wallets (the root key isn't in-process) and - /// the signer-based architecture doesn't need it — the signer - /// owns derivation end-to-end. + /// The private key itself is never derived here: the wallet's root + /// xpriv is not in-process for `ExternalSignable` wallets, and the + /// signer-based architecture doesn't need it — the signer owns + /// derivation end-to-end. async fn rederive_credit_output_path( &self, lock: &TrackedAssetLock, @@ -2620,9 +2619,9 @@ mod tests { /// Promotion is EVICTION under the default /// `keep-finalized-transactions = OFF` build: `apply_chain_lock` drops /// the record it has just promoted and keeps only its txid in the - /// account's finalized set. A snapshot that asked the record alone - /// therefore questioned the one place finality no longer lives, and - /// condemned a locally final lock on the strength of a sibling the same + /// account's finalized set. A snapshot that asks the record alone + /// therefore questions the one place finality no longer lives, and + /// condemns a locally final lock on the strength of a sibling the same /// chainlock never buried. The chainlock here is applied for real — /// the funding transaction is filed in a block below the lock height and /// promoted by the wallet's own pass — so the eviction is the wallet's, @@ -3531,12 +3530,12 @@ mod tests { /// classifies every failure that way, and the SPV broadcaster reaches /// `Rejected` only on `NotConnected`. So advancing to `Broadcast` and /// then waiting with `wait_for_proof(None)` — which is what the three - /// `resume_asset_lock(.., None)` production call sites do — turned a - /// broadcast failure that used to surface in ~30s into a wait that never - /// ends, because no proof can arrive for a tx that was never accepted. + /// `resume_asset_lock(.., None)` production call sites do — would turn a + /// broadcast failure into a wait that never ends, because no proof can + /// arrive for a tx that was never accepted. /// /// `start_paused` auto-advances the substituted bound, so this asserts - /// termination *and* that the caller gets the pre-#4367 typed error back. + /// termination *and* that the caller gets the typed error back. #[tokio::test(start_paused = true)] async fn unbounded_resume_of_an_ambiguous_rebroadcast_terminates() { let (error, status) = resume_lock_at( diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs index 9a08a5930d9..04c53fbb36b 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs @@ -91,9 +91,9 @@ impl AssetLockManager { // or zero connected peers), never the ORIGINAL broadcast that moved the // row to `Broadcast` in an earlier process — so it is not evidence that // the transaction is absent from the network, and removing the row on it - // deleted tracking for possibly-mined asset locks during ordinary offline - // relaunches. `resume_asset_lock` now surfaces the typed error and leaves - // the row untouched. + // would delete tracking for possibly-mined asset locks during ordinary + // offline relaunches. `resume_asset_lock` surfaces the typed error and + // leaves the row untouched. /// Mark a tracked asset lock as /// [`Consumed`](AssetLockStatus::Consumed) after a successful diff --git a/packages/rs-platform-wallet/src/wallet/core/balance_handler.rs b/packages/rs-platform-wallet/src/wallet/core/balance_handler.rs index abc626d55b5..1fb87a0d1de 100644 --- a/packages/rs-platform-wallet/src/wallet/core/balance_handler.rs +++ b/packages/rs-platform-wallet/src/wallet/core/balance_handler.rs @@ -70,7 +70,7 @@ impl EventHandler for BalanceUpdateHandler { }; // Wait-free snapshot of the wallets map; cannot fail or block, so - // a lifecycle write can no longer cost a snapshot. A wallet absent + // a lifecycle write cannot cost a snapshot. A wallet absent // from the snapshot is one still inside its creation window: it is // registered in the inner manager (and therefore SPV-visible) // several `.await`s before it is published here. Both creation diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 8db36c69c75..1879f18346c 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -65,7 +65,7 @@ impl CoreWallet { /// inputs leave this wallet's selectable set within milliseconds; /// `DapiBroadcaster::broadcast` only awaits `sdk.execute` and injects /// nothing, so on that path the inputs are still selectable while the - /// transaction is in flight (`dashpay/platform#4309`). So: + /// transaction is in flight. So: /// /// * **Definitive pre-send rejection** (`BroadcastError::Rejected`) — the /// transaction provably did not reach the network. The fence is dropped @@ -79,16 +79,15 @@ impl CoreWallet { /// /// # Why the fence waits for an observation instead of a height bound /// - /// Three earlier revisions bounded the pending-spend phase at - /// `last_processed_height + N` and disagreed only about where to sample the - /// height — before the await, after it, after it under a still-held guard. - /// Every one of them can be consumed by a routine historical catch-up: the - /// wallet advances that height by thousands of blocks in seconds, and those - /// blocks were mined BEFORE this transaction was submitted, so they are not - /// evidence that it has been seen or dropped. On the `DapiBroadcaster` path - /// — which returns from `sdk.execute` without injecting anything into local - /// wallet state — the input then becomes reselectable while the transaction - /// is in flight (`dashpay/platform#4309`, review round 5). + /// A pending-spend phase bounded at `last_processed_height + N` is unsound + /// wherever the height is sampled — before the await, after it, after it + /// under a still-held guard. Any such bound can be consumed by a routine + /// historical catch-up: the wallet advances that height by thousands of + /// blocks in seconds, and those blocks were mined BEFORE this transaction + /// was submitted, so they are not evidence that it has been seen or + /// dropped. On the `DapiBroadcaster` path — which returns from + /// `sdk.execute` without injecting anything into local wallet state — the + /// input then becomes reselectable while the transaction is in flight. /// /// The release condition is therefore evidence, not elapsed chain: the /// outpoint is freed when the wallet sees it spent. That is a fact about @@ -97,11 +96,11 @@ impl CoreWallet { /// pipeline, DAPI when the transaction is relayed back or lands in a block. /// /// There is NO backstop timeout behind that, and deliberately so. A - /// one-hour monotonic deadline used to sit here as a liveness valve; a - /// clock catch-up cannot fast-forward is still not evidence about this - /// transaction, and once it lapsed the next build could sign a conflicting - /// spend of inputs the original might still take (`dashpay/platform#4309`, - /// review round 7). A transaction the wallet never observes at all — evicted + /// monotonic deadline as a liveness valve would not help: a clock catch-up + /// cannot fast-forward is still not evidence about this transaction, and + /// once it lapsed the next build could sign a conflicting spend of inputs + /// the original might still take. A transaction the wallet never observes + /// at all — evicted /// for fee, conflicted away unseen — therefore holds its inputs for the rest /// of the process. That is the correct trade: those are exactly the inputs a /// possibly-live signed transaction spends. See the @@ -109,16 +108,16 @@ impl CoreWallet { /// and for the two liveness shapes that may shorten the wait without /// weakening it. /// - /// # Why there is no post-await manager guard any more + /// # Why there is no post-await manager guard /// - /// Round 4 of this review added one: the fence's height had to be sampled - /// and installed inside a single manager read guard, or a writer queued - /// behind it could advance the clock in between and the fence would land - /// already lapsed. With no clock to sample at all there is nothing for a - /// height writer to interleave with — the settle sets a flag inside the - /// `in_broadcast` critical section. So it needs no manager lock, and this - /// method now touches the wallet-manager lock exactly once, before the - /// send, which also removes a lock acquisition from every dispatch. + /// A height-bounded fence would need one: its height would have to be + /// sampled and installed inside a single manager read guard, or a writer + /// queued behind it could advance the clock in between and the fence + /// would land already lapsed. With no clock to sample at all there is + /// nothing for a height writer to interleave with — the settle sets a flag + /// inside the `in_broadcast` critical section. So it needs no manager + /// lock, and this method touches the wallet-manager lock exactly once, + /// before the send, which also spares every dispatch a lock acquisition. /// /// A wallet no longer in the manager skips the pin (there is no /// registered generation to fence builds on — they cannot fund from a @@ -162,7 +161,7 @@ impl CoreWallet { // the dispatching future being cancelled, or an unwind, mid-`broadcast`. // Neither says anything about whether the transaction reached the // network, and freeing the inputs there lets an immediate reselection - // double-spend a transaction already on the wire (`dashpay/platform#4309`). + // double-spend a transaction already on the wire. // // Only a definitive pre-send rejection proves nothing was sent, so it is // the one outcome that releases. An ambiguous `MaybeSent` stays fenced. @@ -194,7 +193,7 @@ impl CoreWallet { /// broadcast is `.await`ed, and during that await key-wallet's TTL sweep can /// reclaim this build's reservation and a concurrent build re-reserve the /// same inputs under a new token. Releasing by outpoint alone would then - /// free that other build's inputs (the `dashpay/platform#4185` double-spend + /// free that other build's inputs (the release/re-reserve double-spend /// window); presenting the token frees only inputs this build still owns. /// /// # Reservation age guard @@ -335,7 +334,7 @@ impl CoreWallet { /// used by the immediate send path, this takes an [`AccountTypePreference`] /// so it ALSO reconciles a CoinJoin-funded deferred payment — one whose /// `build_signed`/`finalize` reserved the selected inputs but which has no - /// `StandardAccountType`, and which previously kept its reservation held + /// `StandardAccountType`, and whose reservation would otherwise stay held /// until the TTL backstop. /// /// The release delegates to @@ -345,8 +344,8 @@ impl CoreWallet { /// reservation freed by this token) AND — via `token` — only on inputs this /// build still owns. The deferred registry can hold the reservation across a /// long build→broadcast gap, so a TTL sweep re-reserving the same inputs - /// under a new token is a real risk; the owner guard closes the - /// `dashpay/platform#4185` release/re-reserve race. + /// under a new token is a real risk; the owner guard closes that + /// release/re-reserve race. /// /// `accounts` are the concrete accounts that contributed the transaction's /// inputs (`SignedCoreTransaction::funding_accounts`) — a pooled send spans @@ -844,8 +843,7 @@ mod tests { /// therefore exercises the whole handler path — the variant gate /// (`observing_wallet`), the projection (`observed_spends`), the /// wallets-map `try_read`, the wallet-id lookup, and the selected - /// generation's release — not a shortcut to `observe_spent` - /// (`dashpay/platform#4309`, review round 6). + /// generation's release — not a shortcut to `observe_spent`. fn observe_via_event_handler( core: &CoreWallet, event: key_wallet_manager::WalletEvent, @@ -862,10 +860,9 @@ mod tests { /// Assert that `result` is the typed in-broadcast conflict, and return the /// outpoint it names. /// - /// The tests used to spell this `message.contains("mid-broadcast")`, which - /// is exactly the substring-matching the typed - /// `PlatformWalletError::InputMidBroadcast` variant removes - /// (`dashpay/platform#4309`, review round 5 suggestion). + /// Matching the typed `PlatformWalletError::InputMidBroadcast` variant, + /// never `message.contains("mid-broadcast")`: substring-matching prose is + /// exactly what the typed variant exists to make unnecessary. fn expect_mid_broadcast( result: Result, context: &str, @@ -938,13 +935,11 @@ mod tests { ); // The dispatching pin has now lifted — and the input is STILL not - // selectable. This assertion has been through three revisions of - // `dashpay/platform#4309`: it originally asserted the input was free - // again (the bug), then that it was fenced until a height-anchored - // bound. It now holds regardless of the chain clock, because the fence - // is waiting for an observed spend that this mock manager — which runs - // no mempool pipeline, exactly like the `DapiBroadcaster` path — never - // produces. + // selectable. Asserting that it is free again would be the bug, and a + // height-anchored bound would only defer it. The assertion holds + // regardless of the chain clock, because the fence is waiting for an + // observed spend that this mock manager — which runs no mempool + // pipeline, exactly like the `DapiBroadcaster` path — never produces. let still_fenced = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; expect_mid_broadcast( @@ -954,7 +949,7 @@ mod tests { ); } - /// `dashpay/platform#4309`, THE ROUND-5 BLOCKER, VERBATIM. + /// THE HISTORICAL-CATCH-UP HAZARD, STATED DIRECTLY. /// /// > after [the guard is released], a synchronization writer queued during /// > the short critical section — or ordinary catch-up completing before @@ -963,13 +958,13 @@ mod tests { /// > mined BEFORE the transaction was submitted, so they provide no /// > evidence that the submitted transaction has been observed or dropped. /// - /// This is the test that FAILS on every prior revision of this PR. Each of - /// them installed `pending_until = + IN_BROADCAST_FENCE_BLOCKS` - /// and reaped the fence once `last_processed_height` reached it; the - /// catch-up below clears that bound by a wide margin no matter which height - /// was sampled — pre-await, post-await, or post-await under a held guard — - /// so all three leave the input reselectable here while the transaction is - /// on the network. + /// This is the test that FAILS for any height-bounded fence. An + /// implementation that installs `pending_until = + N` and + /// reaps the fence once `last_processed_height` reaches it loses here: the + /// catch-up below clears that bound by a wide margin no matter which + /// height is sampled — pre-await, post-await, or post-await under a held + /// guard — so every such variant leaves the input reselectable while the + /// transaction is on the network. /// /// The broadcaster is `AlwaysOk`: the transaction is ACCEPTED, so it is /// certainly on the wire. The manager runs no mempool pipeline, which is @@ -997,8 +992,8 @@ mod tests { // Historical catch-up. Not a few blocks past some bound — a whole // month of blocks, all of them mined long before this transaction was // submitted, applied in the instant between the dispatch returning and - // the next build. This is the ordinary mobile resync, and it is what - // consumed every height-anchored bound this PR previously shipped. + // the next build. This is the ordinary mobile resync, and it consumes + // any height-anchored bound. let caught_up = stamped + 17_000; advance_processed_height(&core, caught_up).await; @@ -1139,25 +1134,24 @@ mod tests { core.abandon_transaction(&after).await; } - /// `dashpay/platform#4309` — A WALLETS-MAP WRITE IN FLIGHT MUST NOT COST - /// A SPEND OBSERVATION. + /// A WALLETS-MAP WRITE IN FLIGHT MUST NOT COST A SPEND OBSERVATION. /// - /// `SpendObservationHandler::on_wallet_event` is synchronous, so while the - /// wallets map was a `tokio::sync::RwLock` it could only probe with - /// `try_read` — a probe that fails while wallet registration/removal holds - /// the write lock. For a DAPI-path dispatch the observation lost that way - /// can be the only spend-bearing event the wallet ever gets: InstantLock - /// promotions carry no record here by design, and an evicted or - /// never-confirmed transaction produces no inserted `BlockProcessed` - /// record. With no deadline behind the pending-spend fence, one moment of - /// lock contention left the input fenced for the manager's lifetime even - /// though the wallet HAD observed it spent. + /// `SpendObservationHandler::on_wallet_event` is synchronous, so over a + /// `tokio::sync::RwLock` wallets map it could only probe with `try_read` — + /// a probe that fails while wallet registration/removal holds the write + /// lock. For a DAPI-path dispatch the observation lost that way can be the + /// only spend-bearing event the wallet ever gets: InstantLock promotions + /// carry no record here by design, and an evicted or never-confirmed + /// transaction produces no inserted `BlockProcessed` record. With no + /// deadline behind the pending-spend fence, one moment of lock contention + /// would leave the input fenced for the manager's lifetime even though the + /// wallet HAD observed it spent. /// - /// The map is now an `ArcSwap`, so the read cannot fail and the handler + /// The map is an `ArcSwap`, so the read cannot fail and the handler /// applies every observation at delivery — no deferral queue, no window - /// to lose it in. The closest reachable analogue of the old contention is - /// a lifecycle writer parked mid-`rcu`, which this test pins open across - /// the delivery: the fence must clear anyway, before that writer commits. + /// to lose it in. The closest reachable analogue of that contention is a + /// lifecycle writer parked mid-`rcu`, which this test pins open across the + /// delivery: the fence must clear anyway, before that writer commits. #[tokio::test] async fn a_wallets_map_write_in_flight_does_not_cost_a_spend_observation() { let (core, signer, outputs) = funded_core_wallet( @@ -1233,7 +1227,7 @@ mod tests { } /// The handler releases ONLY the generation registered under the event's - /// wallet id (`dashpay/platform#4309`, review round 6). Two fenced wallets + /// wallet id. Two fenced wallets /// share ONE wallets map — the production shape — and: an event naming a /// wallet id registered NOWHERE releases neither fence, and wallet A's own /// spend event releases A's fence while B's stands. A handler that routed @@ -1319,25 +1313,25 @@ mod tests { ); } - /// `dashpay/platform#4309`, REVIEW ROUND 7 — THE END-TO-END REGRESSION. + /// THE END-TO-END ELAPSED-TIME REGRESSION. /// - /// The pending-spend phase used to expire one hour after the dispatch - /// settled, on a monotonic clock. The clock was the right kind — catch-up + /// A pending-spend phase that expires one hour after the dispatch settles, + /// on a monotonic clock, is unsound. The clock is the right kind — catch-up /// cannot move it — but a deadline of ANY kind is the wrong instrument: the /// signed transaction stays valid, and an hour passing proves nothing about /// whether a peer retained it. A DAPI endpoint that accepts the transaction /// while withholding it from the network, or an app backgrounded past the - /// deadline, was enough. With key-wallet's reservation also swept by - /// catch-up, the next build then re-selected the input and SIGNED A + /// deadline, is enough. With key-wallet's reservation also swept by + /// catch-up, the next build then re-selects the input and SIGNS A /// CONFLICTING TRANSACTION over a spend that might still land. /// /// This drives that exact sequence through the real send path: accept the /// transaction (`AlwaysOk`, and this manager runs no mempool pipeline — the /// `DapiBroadcaster` shape, so nothing observes the spend), run catch-up far /// past key-wallet's reservation TTL, bring due every timeout the fence might - /// carry, and build again. On the deadline-bearing revision that second build - /// SUCCEEDED and returned a second signed transaction spending the same - /// input. It must now be refused, and released only by the observed spend. + /// carry, and build again. Under a deadline-bearing fence that second build + /// SUCCEEDS and returns a second signed transaction spending the same + /// input. It must be refused, and released only by the observed spend. #[tokio::test] async fn an_elapsed_deadline_cannot_retire_the_fence_a_spend_still_needs() { let (core, signer, outputs) = funded_core_wallet( @@ -1367,7 +1361,7 @@ mod tests { ); // Now let every elapsed-time release the fence might carry come due — - // the hour of wall clock the old backstop waited out. + // the hour of wall clock a monotonic backstop would wait out. assert!( core.generation().test_elapse_time_based_release(&fenced), "the accepted dispatch must be in the pending-spend phase" @@ -1392,19 +1386,19 @@ mod tests { core.abandon_transaction(&after).await; } - /// `dashpay/platform#4309`, the CANCELLATION path. + /// The CANCELLATION path. /// /// A caller wrapping the send in `timeout`/`select!` drops the dispatching /// future mid-`broadcast`. That path reaches neither the release nor any /// return value, and cancellation proves nothing: DAPI may have delivered /// the request while awaiting its response, SPV may have dispatched to /// peers while awaiting an echo or IS-lock. So the fence must survive it — - /// and, unlike in earlier revisions, it needs no special case to do so: - /// `Drop` sets the same flag the normal path does, so a cancelled dispatch - /// settles exactly like a returning one. + /// and it needs no special case to do so: `Drop` sets the same flag the + /// normal path does, so a cancelled dispatch settles exactly like a + /// returning one. /// - /// Catch-up runs far past any bound a previous revision would have - /// installed before the abort. + /// Catch-up runs far past any height bound a fence could have installed + /// before the abort. #[tokio::test] async fn cancelled_dispatch_keeps_its_fence_across_catch_up() { let entered = Arc::new(tokio::sync::Barrier::new(2)); diff --git a/packages/rs-platform-wallet/src/wallet/core/generation.rs b/packages/rs-platform-wallet/src/wallet/core/generation.rs index fcef75d518a..5224985f838 100644 --- a/packages/rs-platform-wallet/src/wallet/core/generation.rs +++ b/packages/rs-platform-wallet/src/wallet/core/generation.rs @@ -31,7 +31,7 @@ use super::balance::WalletBalance; /// through different locks: teardown would take one gate, an in-flight payment /// would hold the other, and the exclusion would silently vanish with nothing to /// fail. Keeping them in one `Arc` makes that divergence unrepresentable — -/// same generation is the same gate, by construction (`dashpay/platform#4185`). +/// same generation is the same gate, by construction. /// /// [`Deref`] to [`WalletBalance`] keeps every existing lock-free balance read /// (`generation.confirmed()`, `info.balance.locked()`, …) working unchanged. @@ -102,35 +102,31 @@ pub struct WalletGeneration { /// accepted response and an ambiguous `MaybeSent` return with the input still /// selectable here while the transaction is in flight. Dropping the fence at /// dispatch return would therefore reopen, on the DAPI path, exactly the - /// sweep + re-select race the pin was added to close - /// (`dashpay/platform#4309`). + /// sweep + re-select race the pin exists to close. /// /// # The pending-spend phase ends on EVIDENCE, and on nothing else /// /// **The invariant: no quantity that merely ELAPSES may retire this - /// phase.** Not chain height, and not wall-clock time either. Four earlier - /// revisions violated it — three bounded the phase at `height + N` blocks - /// and argued only about *which* height to anchor on (the pre-send check's, - /// a post-await sample, a post-await sample installed under one manager - /// guard); the fourth replaced that with a one-hour monotonic deadline. The - /// height forms were unsound because `last_processed_height` is not a clock - /// during catch-up: the wallet can advance it by thousands of blocks in - /// seconds, and every one of those blocks was mined BEFORE the transaction - /// was submitted, so an ordinary historical sync consumed the whole - /// interval (`dashpay/platform#4309`, review round 5). - /// - /// The monotonic deadline fixed the wrong half of that. Making the clock - /// unfast-forwardable does not make elapsed time evidence, and the fence - /// needs evidence: a signed transaction does not become invalid by getting - /// older, and no amount of waiting proves no peer retained it. A malicious - /// or isolated DAPI endpoint can accept the transaction while withholding - /// it from this wallet and from the network, and a mobile wallet can sit - /// backgrounded far longer than any deadline worth setting. Once the - /// deadline lapses and catch-up has also swept key-wallet's reservation, - /// the next build prunes the fence and signs a CONFLICTING transaction — - /// and the retained original can still be broadcast afterwards, so either - /// user intent can win the double-spend race (`dashpay/platform#4309`, - /// review round 7). + /// phase.** Not chain height, and not wall-clock time either. A bound of + /// `height + N` blocks is unsound whichever height it anchors on (the + /// pre-send check's, a post-await sample, a post-await sample installed + /// under one manager guard), because `last_processed_height` is not a + /// clock during catch-up: the wallet can advance it by thousands of blocks + /// in seconds, and every one of those blocks was mined BEFORE the + /// transaction was submitted, so an ordinary historical sync consumes the + /// whole interval. + /// + /// A monotonic wall-clock deadline fixes the wrong half of that. Making + /// the clock unfast-forwardable does not make elapsed time evidence, and + /// the fence needs evidence: a signed transaction does not become invalid + /// by getting older, and no amount of waiting proves no peer retained it. + /// A malicious or isolated DAPI endpoint can accept the transaction while + /// withholding it from this wallet and from the network, and a mobile + /// wallet can sit backgrounded far longer than any deadline worth setting. + /// Once such a deadline lapses and catch-up has also swept key-wallet's + /// reservation, the next build prunes the fence and signs a CONFLICTING + /// transaction — and the retained original can still be broadcast + /// afterwards, so either user intent can win the double-spend race. /// /// So there is no deadline at all. The pending-spend phase is released by /// exactly one thing — the wallet observing the outpoint spent, which is @@ -194,8 +190,7 @@ pub struct WalletGeneration { /// [`PlatformWalletManager`](crate::PlatformWalletManager) keys by /// `wallet_id` and hands to every generation registered under that id, so /// removing a wallet and re-creating it under the same id inherits the - /// pending spends rather than starting clean - /// (`dashpay/platform#4309`, review round 8). + /// pending spends rather than starting clean. /// /// The balance and the lifecycle gate above genuinely describe *this* /// instance, and must not cross a recreation. A fence does not: it @@ -241,8 +236,7 @@ impl std::fmt::Debug for SettleBoundaryHook { /// Owning the map here rather than inside `WalletGeneration` is what lets a /// pending spend outlive the instance that dispatched it: a remove-and-recreate /// under the same id mints a fresh generation but hands it this same `Arc`, so -/// the still-valid signed transaction's inputs stay fenced -/// (`dashpay/platform#4309`, review round 8). See the +/// the still-valid signed transaction's inputs stay fenced. See the /// `WalletGeneration::in_broadcast` field docs for the full argument. /// /// # Not yet durable @@ -283,8 +277,8 @@ struct InBroadcastFence { /// /// A plain flag, deliberately: not a deadline, not a height, not anything /// that can come due. Only [`WalletGeneration::observe_spent`] clears it. - /// See the `WalletGeneration::in_broadcast` field docs for why every bound - /// tried here — three chain-derived, one monotonic — was unsound. + /// See the `WalletGeneration::in_broadcast` field docs for why no bound — + /// chain-derived or monotonic — is sound here. pending: bool, /// The wallet has OBSERVED this outpoint spent /// ([`WalletGeneration::observe_spent`]). Retires the pending-spend phase @@ -299,11 +293,10 @@ struct InBroadcastFence { impl InBroadcastFence { /// Whether this fence still blocks re-selection. /// - /// Takes NO clock of any kind — no height, and (since review round 7) no + /// Takes NO clock of any kind — no height, and no /// [`Instant`](std::time::Instant) either. A fence is held while a dispatch /// is in flight or its transaction may be on the network, and is released - /// only by evidence ([`WalletGeneration::observe_spent`]). Nothing elapses - /// (`dashpay/platform#4309`). + /// only by evidence ([`WalletGeneration::observe_spent`]). Nothing elapses. fn blocks(&self) -> bool { self.dispatching > 0 || self.pending } @@ -314,10 +307,9 @@ impl InBroadcastFence { /// phase must not be undone by a slower concurrent dispatch of the same /// transaction settling afterwards. /// - /// Idempotent, and there is nothing left to order between two concurrent - /// dispatches of the same transaction. This used to install a deadline and - /// take care never to SHORTEN an existing one so both dispatches stayed - /// covered; with no deadline, one flag covers both by construction. + /// Idempotent, and there is nothing to order between two concurrent + /// dispatches of the same transaction: with no deadline that a later + /// settle could SHORTEN, one flag covers both by construction. fn open_pending(&mut self) { if self.observed_spent { return; @@ -346,7 +338,7 @@ impl InBroadcastFence { /// is dropped — see [`WalletGeneration::pin_in_broadcast`]. /// /// The INITIAL value is the least-informed one: a pin that learns nothing -/// before it drops must fence (`dashpay/platform#4309`). +/// before it drops must fence. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] enum PendingSpendSettle { /// The transaction may be on the network — the broadcaster returned @@ -355,12 +347,12 @@ enum PendingSpendSettle { /// Both open the pending-spend phase, which then waits for an observed /// spend. /// - /// The two cases need no distinction any more. When the phase carried a - /// height-derived bound they did: a cancelled dispatch had no post-await - /// sample to anchor on, so it had to fence unanchored and borrow a later - /// selection's clock. With no bound to anchor there is nothing to sample, - /// and a `Drop` that can reach neither a lock nor an await settles - /// identically to a normal return. + /// The two cases need no distinction. A height-derived bound would need + /// one — a cancelled dispatch has no post-await sample to anchor on, so it + /// would have to fence unanchored and borrow a later selection's clock — + /// but with no bound to anchor there is nothing to sample, and a `Drop` + /// that can reach neither a lock nor an await settles identically to a + /// normal return. #[default] Pending, /// A definitive pre-send rejection — the one outcome that proves the @@ -393,7 +385,7 @@ impl WalletGeneration { /// The balance and the lifecycle gate are per generation — they describe /// *this* instance. The fence map is not: it describes signed transactions /// that may be live on the network, and those outlive the instance that - /// dispatched them (`dashpay/platform#4309`, review round 8). See the + /// dispatched them. See the /// [`in_broadcast`](Self#structfield.in_broadcast) field docs. pub(crate) fn with_fences(fences: Arc) -> Self { Self { @@ -440,8 +432,7 @@ impl WalletGeneration { /// Exclusive against every payment operation on this generation. Removal /// holds it across BOTH the manager removal and the deferred-state sweep, so /// the two are one linearization point rather than two steps with a window - /// between them that a retained handle could broadcast through - /// (`dashpay/platform#4185`). + /// between them that a retained handle could broadcast through. /// /// Acquiring it waits for payment operations that have already entered their /// liveness-check/publish section. It does **not** wait for an operation @@ -492,13 +483,13 @@ impl WalletGeneration { /// that follows it. Chain height cannot bound this fence at all: catch-up /// advances it over blocks mined before the transaction was ever submitted, /// so any `height + N` bound can be consumed by an ordinary historical sync - /// without a single piece of evidence about the dispatch - /// (`dashpay/platform#4309`). The pending-spend phase ends when the wallet - /// OBSERVES the outpoint spent ([`observe_spent`](Self::observe_spent)), - /// and there is no fallback bound of any other kind either — a wall clock - /// the chain cannot move is still not evidence about this transaction - /// (review round 7). Accepting no clock at either end makes the - /// mis-anchoring unrepresentable rather than merely corrected. + /// without a single piece of evidence about the dispatch. The + /// pending-spend phase ends when the wallet OBSERVES the outpoint spent + /// ([`observe_spent`](Self::observe_spent)), and there is no fallback + /// bound of any other kind either — a wall clock the chain cannot move is + /// still not evidence about this transaction. Accepting no clock at either + /// end makes the mis-anchoring unrepresentable rather than merely + /// corrected. /// /// Callers pin on the generation currently REGISTERED in the manager /// (`PlatformWalletInfo::generation`), the same object the build-side @@ -521,8 +512,7 @@ impl WalletGeneration { outpoints, // Fenced by default: a pin that learns nothing before it drops must // still hold the inputs. Only a definitive pre-send rejection - // narrows this. See the `InBroadcastPin` type docs - // (`dashpay/platform#4309`). + // narrows this. See the `InBroadcastPin` type docs. settle: PendingSpendSettle::Pending, } } @@ -542,14 +532,14 @@ impl WalletGeneration { /// /// # No height parameter, deliberately /// - /// This used to take the caller's `last_processed_height` and reap every - /// fence the chain had advanced past. That is the defect: during catch-up - /// the wallet advances that height over blocks mined BEFORE the dispatch, - /// so an ordinary historical sync completing between a dispatch and this - /// call could retire a fence protecting a transaction that had just gone to - /// the network (`dashpay/platform#4309`, review round 5). The fence now - /// answers to observed spends ALONE — no chain clock, and no wall clock - /// either (review round 7) — so this call retires nothing by consulting it. + /// Taking the caller's `last_processed_height` and reaping every fence the + /// chain had advanced past would be a defect: during catch-up the wallet + /// advances that height over blocks mined BEFORE the dispatch, so an + /// ordinary historical sync completing between a dispatch and this call + /// could retire a fence protecting a transaction that had just gone to the + /// network. The fence answers to observed spends ALONE — no chain clock, + /// and no wall clock either — so this call retires nothing by consulting + /// it. /// /// Cleared entries are reaped here rather than by a timer: this is the only /// place the fence is consulted, so pruning on read keeps the map free of @@ -629,11 +619,10 @@ impl WalletGeneration { /// under a single `in_broadcast` lock acquisition. There is no clock to /// read and no guard to release in between, so no observer can catch this /// outpoint in the torn state — `dispatching` already lifted, pending-spend - /// not yet open — that would make it briefly selectable. Earlier revisions - /// sampled a `last_processed_height` from the wallet-manager lock and had - /// to hold that guard across the install to get the same property - /// (`dashpay/platform#4309`, review round 4); setting a flag needs no guard - /// at all. + /// not yet open — that would make it briefly selectable. Sampling a + /// `last_processed_height` from the wallet-manager lock would require + /// holding that guard across the install to get the same property; + /// setting a flag needs no guard at all. /// /// [`Self::settle_boundary_hook`] fires at exactly that midpoint under /// `cfg(test)` — after the first outpoint's dispatching hold is lifted and @@ -641,8 +630,7 @@ impl WalletGeneration { /// merely after the lock is acquired. A hook that fired on lock /// acquisition would be satisfied by the first half of a split /// implementation too; fired here, only a critical section that spans - /// both halves keeps the boundary unobservable - /// (`dashpay/platform#4309`, review round 6). + /// both halves keeps the boundary unobservable. fn unpin_in_broadcast(&self, outpoints: &[OutPoint], settle: PendingSpendSettle) { let mut pinned = self.in_broadcast_lock(); for outpoint in outpoints { @@ -692,16 +680,17 @@ impl WalletGeneration { /// pending-spend phase has no deadline to bring due, so this call mutates /// nothing at all and only answers "is this outpoint pending?". /// - /// Kept — and kept callable — because it is the harness the round-7 - /// regressions are written against, and it means the same thing in both - /// designs: *let every timeout this fence might have expire, then look*. - /// Against the deadline-bearing implementation the same call retired the - /// fence and the next [`in_broadcast_conflict`](Self::in_broadcast_conflict) - /// handed the input back for re-selection; against this one the fence - /// stands until an observed spend. Those two outcomes are exactly what + /// Kept — and kept callable — because it is the harness the elapsed-time + /// regressions are written against, and it means the same thing whether + /// or not a fence carries a deadline: *let every timeout this fence might + /// have expire, then look*. Against a deadline-bearing implementation the + /// same call retires the fence and the next + /// [`in_broadcast_conflict`](Self::in_broadcast_conflict) hands the input + /// back for re-selection; against this one the fence stands until an + /// observed spend. Those two outcomes are exactly what /// `the_pending_fence_outlives_any_elapsed_deadline` and /// `an_elapsed_deadline_cannot_retire_the_fence_a_spend_still_needs` - /// discriminate (`dashpay/platform#4309`). + /// discriminate. #[cfg(test)] pub(crate) fn test_elapse_time_based_release(&self, outpoint: &OutPoint) -> bool { self.in_broadcast_lock() @@ -715,14 +704,13 @@ impl WalletGeneration { /// phase is opened — the torn state itself. /// /// The test-only synchronization hook that makes the handoff regression - /// DETERMINISTIC (`dashpay/platform#4309`, review round 5 suggestion). The - /// previous regression parked a writer and hoped the scheduler granted it - /// the lock inside a window a handful of instructions wide, so it stayed - /// green against the pre-fix code. With this hook the observer is run at - /// the midpoint by construction, and what it can see there is the whole + /// DETERMINISTIC. A regression that parks a writer and hopes the scheduler + /// grants it the lock inside a window a handful of instructions wide stays + /// green against a split transition. With this hook the observer is run + /// at the midpoint by construction, and what it can see there is the whole /// assertion. /// - /// The firing point matters (round 6): fired on lock ACQUISITION, the + /// The firing point matters: fired on lock ACQUISITION, the /// observation would complete before any fence was touched, so an /// implementation that split the decrement and the pending install into /// separate critical sections — the regression under test — would satisfy @@ -748,8 +736,7 @@ impl WalletGeneration { /// observer that simply waits for the lock always sees the finished state /// and can never tell whether it was granted mid-transition or after it. /// Probing with `try_lock` turns "held" into an observable outcome, which is - /// exactly the invariant the deterministic handoff regression asserts - /// (`dashpay/platform#4309`, review round 5). + /// exactly the invariant the deterministic handoff regression asserts. #[cfg(test)] pub(crate) fn try_probe_in_broadcast(&self, outpoint: &OutPoint) -> InBroadcastProbe { match self.in_broadcast.fences.try_lock() { @@ -811,32 +798,31 @@ pub(crate) enum InBroadcastProbe { /// rejection, the one outcome that PROVES the transaction is not on the wire — /// frees them outright. /// -/// The default is deliberately the conservative one (`dashpay/platform#4309`). -/// This guard's drop runs on paths that carry no information about whether the -/// transaction was sent: the dispatching future cancelled mid-await, an unwind, -/// or a suspension inside the broadcaster before submission. Treating those -/// like a rejection — the previous behaviour — frees inputs that may already be -/// spent on the network, so an immediate reselection double-spends them. Absence +/// The default is deliberately the conservative one. This guard's drop runs +/// on paths that carry no information about whether the transaction was sent: +/// the dispatching future cancelled mid-await, an unwind, or a suspension +/// inside the broadcaster before submission. Treating those like a rejection +/// frees inputs that may already be spent on the network, so an immediate +/// reselection double-spends them. Absence /// of evidence that a send happened is not evidence that it did not, so the /// fence must survive every exit except the one that proves otherwise. /// /// # The pending-spend phase waits for evidence, not for a bound to run out /// -/// Fencing by default is only half of it. Three earlier revisions paired that -/// default with a `last_processed_height + N` bound and argued about where to -/// sample the height; all three could be consumed by an ordinary historical -/// catch-up, because those elapsed blocks were mined before the transaction was -/// submitted and say nothing about it (`dashpay/platform#4309`, review round 5). -/// A fourth swapped the height for a one-hour monotonic deadline, which fails -/// the same way for the same reason: a signed transaction does not expire, so -/// an hour of a clock no one can fast-forward is still not evidence that its -/// inputs are safe to spend again (review round 7). +/// Fencing by default is only half of it. Pairing that default with a +/// `last_processed_height + N` bound is unsound wherever the height is sampled: +/// an ordinary historical catch-up consumes the bound, because those elapsed +/// blocks were mined before the transaction was submitted and say nothing +/// about it. A one-hour monotonic deadline fails the same way for the same +/// reason: a signed transaction does not expire, so an hour of a clock no one +/// can fast-forward is still not evidence that its inputs are safe to spend +/// again. /// /// The pending-spend phase ends when the wallet OBSERVES the outpoint spent /// ([`WalletGeneration::observe_spent`]), and nothing else ends it. That is /// readable without a lock, a guard or an await, so EVERY exit — normal return, /// cancellation, unwind — settles the same way. The cancellation path needs no -/// special case at all any more. +/// special case at all. pub(crate) struct InBroadcastPin { generation: Arc, outpoints: Vec, @@ -853,16 +839,13 @@ impl InBroadcastPin { /// /// # Why this takes no height, and no guard /// - /// It used to take a `last_processed_height` sampled after the broadcaster - /// returned, from a wallet-manager guard the caller had to keep held across - /// the call so no writer could advance the clock between the sample and the - /// install. A later revision swapped that height for a monotonic - /// `Instant::now` deadline read inside the `in_broadcast` critical section. - /// - /// Both are gone, and so is the bound they computed. What this installs is - /// a plain flag: there is no clock to sample, so no guard to hold and no - /// window to protect. The phase it opens ends on an observed spend - /// (`dashpay/platform#4309`). + /// A `last_processed_height` sampled after the broadcaster returned would + /// need a wallet-manager guard held across the call so no writer could + /// advance the clock between the sample and the install; a monotonic + /// `Instant::now` deadline read inside the `in_broadcast` critical section + /// would still be a bound that merely elapses. What this installs is a + /// plain flag: there is no clock to sample, so no guard to hold and no + /// window to protect. The phase it opens ends on an observed spend. /// /// # This is equivalent to just dropping the pin /// @@ -904,9 +887,9 @@ impl InBroadcastPin { /// same two shapes `settle_released` names), synchronously, BEFORE the /// first `.await` of any cleanup that must still run under the raised /// fence. The pin stays live, so its dispatching hold keeps the outpoints - /// fenced through that cleanup — the round-8 ordering is untouched — but - /// every exit after this call, the cleanup future being dropped mid-await - /// included, now settles the fence as released. + /// fenced through that cleanup, but every exit after this call, the + /// cleanup future being dropped mid-await included, settles the fence as + /// released. /// /// Without it, a rejection arm that awaited its reservation cleanup /// before settling would, on cancellation inside that await, drop the pin @@ -914,7 +897,7 @@ impl InBroadcastPin { /// the outcome is unknown; here the outcome is proven — nothing reached /// the network — so the pending-spend fence it opened could never be /// cleared by an observed spend and held the inputs for the manager's - /// lifetime (`dashpay/platform#4309`). + /// lifetime. /// /// Once recorded, the verdict is FINAL for this pin: rejection is /// established by the broadcaster's definitive answer (or by the @@ -1010,9 +993,9 @@ mod tests { ); } - /// `dashpay/platform#4309`: dropping a pin WITHOUT a definitive rejection - /// keeps the fence. This is the cancellation / unwind / suspension path, - /// none of which proves the transaction failed to reach the network. + /// Dropping a pin WITHOUT a definitive rejection keeps the fence. This is + /// the cancellation / unwind / suspension path, none of which proves the + /// transaction failed to reach the network. #[test] fn dropping_an_unreleased_pin_keeps_the_fence() { let generation = Arc::new(WalletGeneration::new()); @@ -1028,11 +1011,10 @@ mod tests { ); } - /// `dashpay/platform#4309`: once a definitive pre-send failure is - /// ESTABLISHED, recording it on the pin makes every later exit settle - /// released — the cancellation-safe half of `settle_released`. A - /// rejection arm awaits its reservation cleanup under the still-raised - /// fence (round-8 ordering); if that future is dropped inside the await, + /// Once a definitive pre-send failure is ESTABLISHED, recording it on the + /// pin makes every later exit settle released — the cancellation-safe half + /// of `settle_released`. A rejection arm awaits its reservation cleanup + /// under the still-raised fence; if that future is dropped inside the await, /// the pin must not fall back to its pending default and fence a /// transaction proven never sent — nothing could ever observe that spend, /// so nothing could ever clear the fence. @@ -1066,16 +1048,15 @@ mod tests { ); } - /// THE HEADLINE PROPERTY (`dashpay/platform#4309`, review round 5). + /// THE HEADLINE PROPERTY. /// /// A pending-spend fence is not consulted against chain height at all, so - /// no amount of catch-up can retire it. Previous revisions bounded the - /// fence at `height + IN_BROADCAST_FENCE_BLOCKS` and every one of them lost - /// the fence to a historical sync that advanced the clock past the bound - /// over blocks mined BEFORE the dispatch. + /// no amount of catch-up can retire it. A fence bounded at `height + N` + /// loses to a historical sync that advances the clock past the bound over + /// blocks mined BEFORE the dispatch. /// - /// `in_broadcast_conflict` no longer takes a height, so this test states - /// the property the only way it can still be stated: the fence survives + /// `in_broadcast_conflict` takes no height, so this test states the + /// property the only way it can be stated: the fence survives /// unboundedly many consultations and any amount of elapsed chain, and only /// an observation clears it. #[test] @@ -1193,20 +1174,20 @@ mod tests { ); } - /// `dashpay/platform#4309`, REVIEW ROUND 7 — THE UNIT-LEVEL REGRESSION. + /// THE UNIT-LEVEL ELAPSED-TIME REGRESSION. /// - /// The pending-spend fence used to carry a one-hour monotonic deadline, and - /// `in_broadcast_conflict` retired the fence on that deadline ALONE. Elapsed - /// time is not evidence: the signed transaction is still valid, and nothing - /// about an hour passing proves no peer retained it. A withholding DAPI - /// endpoint or an hour-backgrounded app was therefore enough to hand the - /// input back to the next build, which would sign a conflicting transaction - /// over inputs the original might still spend. + /// A pending-spend fence carrying a one-hour monotonic deadline, with + /// `in_broadcast_conflict` retiring the fence on that deadline ALONE, is + /// unsound. Elapsed time is not evidence: the signed transaction is still + /// valid, and nothing about an hour passing proves no peer retained it. A + /// withholding DAPI endpoint or an hour-backgrounded app is then enough to + /// hand the input back to the next build, which signs a conflicting + /// transaction over inputs the original might still spend. /// /// `test_elapse_time_based_release` means "let every timeout this fence - /// might carry come due, then tell me whether it is pending". Against the - /// deadline-bearing implementation it retired the fence and the assertion - /// below failed; against this one there is no deadline to bring due, so the + /// might carry come due, then tell me whether it is pending". Against a + /// deadline-bearing implementation it retires the fence and the assertion + /// below fails; against this one there is no deadline to bring due, so the /// fence stands and only the observed spend at the end releases it. #[test] fn the_pending_fence_outlives_any_elapsed_deadline() { @@ -1281,9 +1262,8 @@ mod tests { } /// Two dispatches of the same transaction settling in sequence: the second - /// settle must never UNDO the fence the first installed. This used to be a - /// statement about deadlines never being shortened; with no deadline the - /// property is simply that the phase stays open. + /// settle must never UNDO the fence the first installed. With no deadline + /// to shorten, the property is simply that the phase stays open. #[test] fn a_second_settle_does_not_undo_the_first_fence() { let generation = Arc::new(WalletGeneration::new()); @@ -1309,13 +1289,12 @@ mod tests { } /// Fences are per WALLET, and a generation that replaces another under the - /// same id INHERITS them (`dashpay/platform#4309`, review round 8). + /// same id INHERITS them. /// - /// This test used to assert the opposite — that a re-created wallet got a - /// fresh map — and that was the bug: the map went with the old generation - /// while the transaction it protected stayed valid and relayable, so the - /// replacement could sign a conflicting spend of the same outpoint. The - /// end-to-end round trip through the manager is + /// A re-created wallet that got a fresh map would be the bug: the map would + /// go with the old generation while the transaction it protected stayed + /// valid and relayable, so the replacement could sign a conflicting spend + /// of the same outpoint. The end-to-end round trip through the manager is /// `a_recreated_wallet_inherits_the_pending_fences_of_the_generation_it_replaces`; /// this pins the mechanism. #[test] @@ -1410,24 +1389,23 @@ mod tests { ); } - /// `dashpay/platform#4309`, REVIEW ROUND 5 SUGGESTION: THE - /// DISPATCHING→PENDING HANDOFF REGRESSION, MADE DETERMINISTIC. + /// THE DISPATCHING→PENDING HANDOFF REGRESSION, MADE DETERMINISTIC. /// /// The transition lifts the dispatching hold and opens the pending-spend /// phase. If those two ever land in separate critical sections, an observer /// in between sees the outpoint held by NOTHING and a build can select an /// input whose transaction may be on the wire. /// - /// The previous regression parked a manager writer and hoped the scheduler - /// granted it the lock inside a window a handful of instructions wide. It - /// did not reliably do so — the reviewer showed it stays green against the - /// pre-fix code — so it proved nothing. + /// A regression that parks a manager writer and hopes the scheduler grants + /// it the lock inside a window a handful of instructions wide does not + /// reliably get it — it stays green against a split transition — so it + /// proves nothing. /// /// This one is deterministic. [`WalletGeneration::on_next_settle_boundary`] /// runs the observer AT the midpoint by construction — after the /// dispatching hold is lifted, before the pending phase opens, so the /// probe lands inside the torn state itself rather than before any fence - /// was touched (round 6) — and the settling thread BLOCKS until the + /// was touched — and the settling thread BLOCKS until the /// observer has published what it saw, so there is no race to lose. The /// observer probes with `try_lock` /// ([`WalletGeneration::try_probe_in_broadcast`]) rather than blocking, diff --git a/packages/rs-platform-wallet/src/wallet/core/sign_message.rs b/packages/rs-platform-wallet/src/wallet/core/sign_message.rs index dc9bc1d2b12..65321a9d099 100644 --- a/packages/rs-platform-wallet/src/wallet/core/sign_message.rs +++ b/packages/rs-platform-wallet/src/wallet/core/sign_message.rs @@ -232,7 +232,7 @@ impl CoreWallet { // reserved machine marker a signer stamps at POSITION 0 of its own // rendering (`MnemonicResolverCoreSigner::NotFound` in production), so // key unavailability is recognized by that position-0 check — never a - // substring sniff (#4183 review) — and it must happen BEFORE the + // substring sniff — and it must happen BEFORE the // "signer rejected the digest at {path}: " context is prepended, which // would push the marker mid-string where no permitted check can see it. let (signature, public_key) = signer @@ -641,8 +641,8 @@ mod tests { /// The marker only counts at position 0 of the signer's rendering: a /// mid-string mention stays `MessageSigningFailed`, never key-unavailable — - /// promoting it would be the substring sniff #4183's review rejected, and - /// would misroute a generic failure into the host's key repair. + /// promoting it would be a substring sniff, and would misroute a generic + /// failure into the host's key repair. #[tokio::test] async fn mid_string_marker_is_not_promoted_during_message_signing() { let (wm, wallet_id, _, address) = diff --git a/packages/rs-platform-wallet/src/wallet/core/spend_observer.rs b/packages/rs-platform-wallet/src/wallet/core/spend_observer.rs index ba9151db56a..57810c5dfd2 100644 --- a/packages/rs-platform-wallet/src/wallet/core/spend_observer.rs +++ b/packages/rs-platform-wallet/src/wallet/core/spend_observer.rs @@ -28,12 +28,11 @@ use crate::wallet::PlatformWallet; /// its inputs fenced, because the broadcaster's return says "this may be on the /// network", not "this wallet has seen the spend" — and on the /// `DapiBroadcaster` path the two are far apart, since `sdk.execute` injects -/// nothing into local wallet state. Something has to end that fence, and three -/// earlier revisions tried to end it on elapsed `last_processed_height`. That -/// cannot work: catch-up advances the chain clock over blocks mined *before* -/// the transaction was submitted, so an ordinary historical sync retires a -/// fence without a shred of evidence about the transaction it was protecting -/// (`dashpay/platform#4309`). +/// nothing into local wallet state. Something has to end that fence, and +/// elapsed `last_processed_height` cannot be it: catch-up advances the chain +/// clock over blocks mined *before* the transaction was submitted, so an +/// ordinary historical sync would retire a fence without a shred of evidence +/// about the transaction it was protecting. /// /// So the fence ends on the observation instead, and this handler is where the /// observation arrives. @@ -76,17 +75,16 @@ use crate::wallet::PlatformWallet; /// block. Releasing the fence then takes only the generation's `in_broadcast` /// `std::sync::Mutex` for a few hash operations and never awaits. /// -/// The infallibility is what retires the deferral this handler used to need. -/// While the map was a `tokio::sync::RwLock`, a `try_read` losing to a -/// lifecycle writer had to queue the observation for the next delivered -/// event — dropping it was unacceptable, since `TransactionDetected` can be -/// the ONLY spend-bearing event a dispatch ever produces (InstantLock -/// promotions carry no record here by design, and an evicted or -/// never-confirmed transaction inserts no `BlockProcessed` record) and the -/// pending-spend fence has no deadline behind it, so one lost observation -/// fenced an input for the manager's lifetime (`dashpay/platform#4309`). With -/// a read that cannot fail there is no such window and nothing to queue: every -/// observation is applied at delivery. +/// The infallibility is what makes a deferral queue unnecessary here. Over a +/// `tokio::sync::RwLock` map, a `try_read` losing to a lifecycle writer would +/// have to queue the observation for the next delivered event — dropping it +/// is unacceptable, since `TransactionDetected` can be the ONLY spend-bearing +/// event a dispatch ever produces (InstantLock promotions carry no record +/// here by design, and an evicted or never-confirmed transaction inserts no +/// `BlockProcessed` record) and the pending-spend fence has no deadline +/// behind it, so one lost observation would fence an input for the manager's +/// lifetime. With a read that cannot fail there is no such window and nothing +/// to queue: every observation is applied at delivery. /// /// One outcome stays terminal, deliberately: a wallet id that resolves to no /// entry in the map is unregistered — a resolution, not contention, with @@ -169,7 +167,7 @@ pub(crate) fn observed_spends(event: &WalletEvent) -> Vec { // Finality promotions of records the wallet already holds, and a bare // watermark advance. No new spend in any of them — and note that the // watermark is precisely the "chain moved" signal that must NOT touch - // a fence (`dashpay/platform#4309`). + // a fence. WalletEvent::TransactionInstantLocked { .. } | WalletEvent::ChainLockProcessed { .. } | WalletEvent::SyncHeightAdvanced { .. } => Vec::new(), @@ -180,8 +178,7 @@ pub(crate) fn observed_spends(event: &WalletEvent) -> Vec { mod tests { //! Cover the projection — which events count as observing a spend, and //! which outpoints they yield. That decision IS the fence's release - //! condition (`dashpay/platform#4309`), so it is pinned here rather than - //! only exercised end to end. + //! condition, so it is pinned here rather than only exercised end to end. use dashcore::hashes::Hash; use dashcore::{ @@ -335,10 +332,9 @@ mod tests { /// THE VARIANT THAT MUST NEVER TOUCH A FENCE. /// /// `SyncHeightAdvanced` is the bare "the chain moved" watermark, and it is - /// precisely the signal three earlier revisions of this fix let retire a - /// fence — via a `last_processed_height + N` bound rather than directly, - /// but with the same effect. It reports no spend and must stay that way - /// (`dashpay/platform#4309`). + /// precisely the signal a `last_processed_height + N` bound would let + /// retire a fence — indirectly, but with the same effect. It reports no + /// spend and must stay that way. #[test] fn chain_progress_alone_reports_no_spend() { let event = WalletEvent::SyncHeightAdvanced { diff --git a/packages/rs-platform-wallet/src/wallet/core/transaction.rs b/packages/rs-platform-wallet/src/wallet/core/transaction.rs index e587d85a671..a3a21337db2 100644 --- a/packages/rs-platform-wallet/src/wallet/core/transaction.rs +++ b/packages/rs-platform-wallet/src/wallet/core/transaction.rs @@ -100,7 +100,7 @@ pub struct SignedCoreTransaction { /// releases the reservation *owner-guarded*: after this build's inputs may /// have been swept by key-wallet's TTL and re-reserved by a concurrent build /// under a new token, releasing by outpoint alone would free that other - /// build's inputs (the `dashpay/platform#4185` double-spend window). + /// build's inputs (the release/re-reserve double-spend window). /// [`ManagedCoreFundsAccount::release_reservation_if_owner`] releases only /// inputs still owned by this token, closing that window. reservation_token: Option, @@ -117,8 +117,7 @@ pub struct SignedCoreTransaction { /// so a caller cannot finalize through wallet A and then register/broadcast /// through an unrelated wallet B — the registry would otherwise treat B as /// the owner, submit A's transaction through B's broadcaster, and run B's - /// cleanup while A's real reservation leaked until its TTL - /// (`dashpay/platform#4185`). + /// cleanup while A's real reservation leaked until its TTL. origin_generation: Arc, } @@ -172,7 +171,7 @@ impl SignedCoreTransaction { /// ownership: `SignedCoreTransaction` is deliberately not `Clone`, so a /// finalize yields exactly one ownership object and the registry can be /// handed it exactly once — a caller cannot mint two live tokens that name - /// the same held reservation (`dashpay/platform#4185`). The transaction, + /// the same held reservation. The transaction, /// funding account, and reservation height are derived here, not supplied /// independently by the caller. pub(crate) fn into_registered_parts(self) -> RegisteredPaymentParts { @@ -363,7 +362,7 @@ impl CoreWallet { // interleaving. The token rides in `SignedCoreTransaction` so a // later abandon or rejected broadcast releases *only* the inputs // this build still owns, even if a TTL sweep re-reserved them under - // a new token meanwhile (`dashpay/platform#4185`). + // a new token meanwhile. let mut builder = builder.set_current_height(height); // Accounts that took on this build's reservation bookkeeping, in // funding order — i.e. the ones a failure path must release. Under @@ -465,8 +464,7 @@ impl CoreWallet { // request itself is sound and can be re-attempted once the fenced // dispatch's outcome is reconciled (see the variant docs for the // duplicate-payment hazard in "retry unchanged"), and callers - // should not have to substring-match prose to tell it apart - // (`dashpay/platform#4309`). + // should not have to substring-match prose to tell it apart. if let Some(outpoint) = info.generation.in_broadcast_conflict(&unsigned) { release_all!(offered_accounts, info.core_wallet.accounts, &unsigned); return Err(PlatformWalletError::InputMidBroadcast { outpoint }); @@ -588,7 +586,7 @@ impl CoreWallet { reservation_token, // Capture the finalizing wallet's generation identity so the // deferred registry can refuse to bind this payment to any other - // wallet (`dashpay/platform#4185`). + // wallet. origin_generation: Arc::clone(self.generation()), }) } @@ -648,7 +646,7 @@ impl CoreWallet { /// (`SignedCoreTransaction::reservation_token`). When present the release is /// *owner-guarded* — it frees only inputs still owned by that token, so a /// reservation key-wallet's TTL swept and a concurrent build re-took is left - /// untouched (`dashpay/platform#4185`). When `None` (the build reserved + /// untouched. When `None` (the build reserved /// nothing) it falls back to the unconditional by-outpoint release; that /// path is never reached for a funded finalize, which always reserves. pub(crate) async fn release_transaction_reservation( @@ -918,12 +916,13 @@ mod tests { ) } - /// `reservation_only` end to end IN THIS WORKSPACE. key-wallet #994 covers - /// `add_funding_reservation_only` on its own side, but nothing here proved + /// `reservation_only` end to end IN THIS WORKSPACE. key-wallet covers + /// `add_funding_reservation_only` on its own side, but only this proves /// the flag survives the crossing: it travels an FFI struct field, a /// finalizer bool, and a key-wallet call, and a refactor that drops it - /// anywhere along that path leaves every test in this repo green while - /// silently reintroducing the >500-input build this branch exists to fix. + /// anywhere along that path leaves every other test in this repo green + /// while silently readmitting the >500-input build the flag exists to + /// prevent. /// /// The account holds two UTXOs; the builder is seeded with exactly one. /// Under the flag the build must spend that one and nothing else — and a diff --git a/packages/rs-platform-wallet/src/wallet/core/wallet.rs b/packages/rs-platform-wallet/src/wallet/core/wallet.rs index 29442935d4c..bdc0470445e 100644 --- a/packages/rs-platform-wallet/src/wallet/core/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/core/wallet.rs @@ -86,8 +86,8 @@ impl CoreWallet { /// /// This is the single generation identity shared by BOTH deferred-payment /// paths — the registry-token path - /// ([`SignedPaymentRegistry`](crate::SignedPaymentRegistry), `dashpay/platform#4185`) - /// and the V2 finalized-transaction handle path (`dashpay/platform#4196`) — + /// ([`SignedPaymentRegistry`](crate::SignedPaymentRegistry)) + /// and the V2 finalized-transaction handle path — /// so neither acts on a re-created wallet's `ReservationSet` while an old /// handle still names the old generation. pub fn is_same_generation( @@ -118,8 +118,7 @@ impl CoreWallet { /// transactions to the network must hold this across both its /// [`is_current_generation`](Self::is_current_generation) check and the /// action that check authorizes. Without it the check is a bare - /// point-in-time observation and a teardown can complete in the gap - /// (`dashpay/platform#4185`). + /// point-in-time observation and a teardown can complete in the gap. /// /// Scoped to this generation, so holding it across a slow SPV send blocks /// only this wallet's teardown — never an unrelated wallet's. @@ -395,7 +394,7 @@ impl CoreWallet { /// be atomic against a concurrent teardown take /// [`generation_payment_guard`](Self::generation_payment_guard) around the /// check and the action it gates; on its own this is a point-in-time - /// observation (`dashpay/platform#4185`). + /// observation. pub async fn is_current_generation(&self) -> bool { let wm = self.wallet_manager.read().await; wm.get_wallet_info(&self.wallet_id) diff --git a/packages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rs b/packages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rs index 799c86917cf..9145403be8f 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rs @@ -434,9 +434,9 @@ mod tests { // the 107-byte DIP-14 serialization (ends in a Normal256 child) and // encrypts to 128 bytes — failing the contract's maxItems: 96. // - // The earlier `account_xpub.encode()` producer emitted the 107-byte - // form (107 != 69); this assertion pins the byte-exact compact layout - // so a revert to `encode()` is caught. + // `account_xpub.encode()` would emit the 107-byte form (107 != 69); + // this assertion pins the byte-exact compact layout so a switch to + // `encode()` is caught. let wallet = test_wallet(Network::Testnet); let (sender, recipient) = test_identifiers(); diff --git a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs index 0110458e026..4b3d58c4bca 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs @@ -482,7 +482,7 @@ mod tests { fn test_sender_wrong_purpose() { // VOTING, not AUTHENTICATION: the legacy dashj cohort pairs an // AUTHENTICATION sender key with an AUTHENTICATION recipient key and - // is now accepted on receive, so AUTHENTICATION no longer exercises + // is accepted on receive, so AUTHENTICATION does not exercise // the sender-side rejection this test is about. let sender = make_identity(vec![make_key(0, KeyType::ECDSA_SECP256K1, Purpose::VOTING)]); let recipient = make_identity(vec![make_key( diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs index a9a6fa64e6c..ca7cd2797ed 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs @@ -1217,7 +1217,7 @@ fn count_account_build_ops(queue: &[crate::changeset::PendingContactCrypto]) -> /// on the wallet. That is right for a recurring background sweep and wrong for /// anything that treats the pass as a precondition, because the two endings it /// collapses are opposites — "Platform answered, and there is nothing new" and -/// "Platform answered nobody, so we do not know". Both used to arrive as +/// "Platform answered nobody, so we do not know". Both arrive as /// `Ok(vec![])`. /// /// The distinction matters most at startup, where a completed pass is the @@ -2318,11 +2318,10 @@ impl DashPayView<'_, B> { // // Deciding here means a MIXED failure — our key // purpose-rejected and the contact's key hard-faulted — - // now leaves the entry queued where the composed validator - // would have marked the channel broken. Deliberate: see + // leaves the entry queued where a composed validator would + // mark the channel broken. Deliberate: see // `validate_recipient_key`. Marking broken is unappealable - // by the user, and the retry it avoids no longer costs a - // fetch. + // by the user, and the retry it avoids costs no fetch. let our_identity = { let wm = self.wallet_manager.read().await; wm.get_wallet_info(&self.wallet_id) @@ -5469,7 +5468,7 @@ mod contact_info_provider_tests { const PHRASE: &str = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; - /// MUST-FIX (security review): the contactInfo seal/open the signer produces + /// Invariant: the contactInfo seal/open the signer produces /// must be byte-identical to the resident `derive_contact_info_keys` AT THE /// REAL identity-auth root path — not an arbitrary path. contactInfo is /// self-encrypted (no counterparty round-trip), so a wrong root silently diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs index 821bfe9e4d9..a19288f4128 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs @@ -48,10 +48,10 @@ fn dashpay_account_registration_changeset( /// retry on a poisoned channel). /// - **Transient** leaves the channel intact so the next sync sweep retries. /// -/// (In the seedless model this method no longer derives the ECDH scalar — the -/// caller passes a signer-derived `shared_key` — so a "key material -/// unavailable" classification no longer arises here; that DEFER decision now -/// lives at the drain's provider call.) +/// (This method does not derive the ECDH scalar — the caller passes a +/// signer-derived `shared_key` — so a "key material unavailable" +/// classification cannot arise here; that DEFER decision lives at the drain's +/// provider call.) /// /// [`register_external_contact_account`]: IdentityWallet::register_external_contact_account #[derive(Debug)] @@ -269,9 +269,8 @@ impl DashPayView<'_, B> { /// /// Used by the SPV / backend task layer to classify observed /// transaction outputs as DashPay incoming payments from a - /// specific contact — replaces the redundant - /// `dashpay_address_mappings` reverse-lookup table the UI - /// layer used to maintain. The authoritative state is already + /// specific contact. No separate reverse-lookup table is needed + /// in the UI layer: the authoritative state is already /// tracked by `register_contact_account`, which inserts the /// account into the wallet's `ManagedAccountCollection` so /// key-wallet manages the address pool (derivation + gap limit diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contract.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contract.rs index 02ffbef33d8..11a42a37ab0 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contract.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contract.rs @@ -8,7 +8,7 @@ //! / tokens / DashPay / identity key derivation / identity //! registration belongs in the `platform-wallet` crate." //! -//! Mirrors the post-#3541 identity-flow shape: +//! Mirrors the identity-flow shape: //! - Library function takes a `Signer` reference //! so the FFI's external `KeychainSigner` trampoline can route //! signing back to Swift / Keychain without crossing seed bytes. diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs index 93dbfe1ff5b..d18f1fb5cd4 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -384,12 +384,13 @@ impl IdentityWallet { // the verdict below. Every `?` in here is a LOCAL fault — a wallet that // left the manager, a persistence write that failed — not a probe that // went unanswered, and each of them abandons the scan part-way through - // the index space. Returning straight out left no verdict at all, and - // "unknown" is what keeps the warm-launch shortcut: the next launch saw - // the identities this scan had already folded in, took the shortcut, - // and never looked at the indices it never reached. That is #4365's - // shape on the local-fault path, so the error is carried out to the - // publish below rather than thrown from the middle of the walk. + // the index space. Returning straight out would leave no verdict at + // all, and "unknown" is what keeps the warm-launch shortcut: the next + // launch sees the identities this scan already folded in, takes the + // shortcut, and never looks at the indices it never reached. That is + // the missed-identity shape on the local-fault path, so the error is + // carried out to the publish below rather than thrown from the middle + // of the walk. let scan_outcome: Result<(), PlatformWalletError> = async { while tally.should_continue(gap_limit) { // Derive the MASTER auth pubkey hash for this identity index @@ -687,9 +688,9 @@ impl IdentityWallet { /// /// A scan answers "which identities does this seed own", and there are three /// endings, not two: it found some, it confirmed there are none, or it never -/// got an answer. The third used to be reported as the second — a failed probe -/// incremented the same miss counter as an empty index, so a scan that reached -/// no one at all returned "this seed owns no identity". Callers cannot retry +/// got an answer. The third must not be reported as the second: if a failed +/// probe incremented the same miss counter as an empty index, a scan that +/// reached no one at all would return "this seed owns no identity". Callers cannot retry /// what they were told is a definitive answer, so a few seconds of network /// trouble after restore-from-seed cost a whole session's DashPay state. /// @@ -1139,10 +1140,10 @@ mod tests { assert!(!tally.is_trustworthy()); } - /// The #4365 shape: an identity at index 0, no answer at index 1. The - /// scan is trustworthy — its findings are real — and it is NOT complete, - /// and those are different questions. Reporting only the first is what let - /// an identity at the unanswered index stay hidden for the life of an + /// The missed-identity shape: an identity at index 0, no answer at index + /// 1. The scan is trustworthy — its findings are real — and it is NOT + /// complete, and those are different questions. Reporting only the first + /// lets an identity at the unanswered index stay hidden for the life of an /// installation. #[test] fn a_scan_that_found_something_despite_a_failed_probe_is_trustworthy_but_incomplete() { @@ -1348,11 +1349,11 @@ mod tests { /// probe hash from resident key material, and this wallet is /// external-signable (its seed lives outside the manager), so the derive /// fails on the first index. That is one of the `?` early returns above - /// `publish_scan_verdict`, and before this fix every one of them returned - /// without publishing anything at all: the previous verdict stood, and a - /// verdict that says "complete" is exactly what keeps the warm-launch - /// shortcut armed. The wallet would then trust an index space this scan - /// abandoned — #4365's shape reached from the local-fault side. + /// `publish_scan_verdict`; if any of them returned without publishing + /// anything at all, the previous verdict would stand, and a verdict that + /// says "complete" is exactly what keeps the warm-launch shortcut armed. + /// The wallet would then trust an index space this scan abandoned — the + /// missed-identity shape reached from the local-fault side. #[tokio::test] async fn a_local_fault_mid_scan_replaces_a_stale_complete_verdict() { use crate::changeset::IdentityScanStateEntry; diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/document.rs b/packages/rs-platform-wallet/src/wallet/identity/network/document.rs index be3a76caa1f..c0409a1e3f5 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/document.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/document.rs @@ -10,7 +10,7 @@ //! `platform-wallet` crate"; the Swift side only renders the form, //! marshals the values, and persists the confirmed document. //! -//! Mirrors the post-#3541 identity-flow shape: +//! Mirrors the identity-flow shape: //! - The library function takes a `Signer` //! reference so the FFI's external `KeychainSigner` trampoline can //! route signing back to Swift / Keychain without crossing seed @@ -310,8 +310,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the // FFI boundary can still restore code 31; only genuine - // operation failures get stringified into `InvalidIdentityData` - // (dashpay/platform#4183 review). + // operation failures get stringified into `InvalidIdentityData`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to put document to platform: {e}" @@ -538,8 +537,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the // FFI boundary can still restore code 31; only genuine - // operation failures get stringified into `InvalidIdentityData` - // (dashpay/platform#4183 review). + // operation failures get stringified into `InvalidIdentityData`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to replace document: {e}" @@ -591,8 +589,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the // FFI boundary can still restore code 31; only genuine - // operation failures get stringified into `InvalidIdentityData` - // (dashpay/platform#4183 review). + // operation failures get stringified into `InvalidIdentityData`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to delete document: {e}" @@ -655,8 +652,7 @@ impl IdentityWallet { // Typed trade rejections (not-for-sale / price-changed / // insufficient credits) and the structured key-unavailable // signer failure survive; only genuine operation failures - // get stringified into `InvalidIdentityData` - // (dashpay/platform#4183 review). + // get stringified into `InvalidIdentityData`. crate::error::promote_document_trade_error_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to transfer document: {e}" @@ -718,8 +714,7 @@ impl IdentityWallet { .map_err(|e| { // Typed trade rejections and the structured key-unavailable // signer failure survive; only genuine operation failures - // get stringified into `InvalidIdentityData` - // (dashpay/platform#4183 review). + // get stringified into `InvalidIdentityData`. crate::error::promote_document_trade_error_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to set document price: {e}" @@ -789,7 +784,7 @@ impl IdentityWallet { // behind the wallet's pre-flight — and the structured // key-unavailable signer failure survive; only genuine // operation failures get stringified into - // `InvalidIdentityData` (dashpay/platform#4183 review). + // `InvalidIdentityData`. crate::error::promote_document_trade_error_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to purchase document: {e}" diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/dpns.rs b/packages/rs-platform-wallet/src/wallet/identity/network/dpns.rs index 5e6476c1af3..86f0b809bb2 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/dpns.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/dpns.rs @@ -246,8 +246,7 @@ impl IdentityWallet { let result = self.sdk.register_dpns_name(input).await.map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `InvalidIdentityData` - // (dashpay/platform#4183 review). + // failures get stringified into `InvalidIdentityData`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to register DPNS name '{}': {}", diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/dpns_marketplace.rs b/packages/rs-platform-wallet/src/wallet/identity/network/dpns_marketplace.rs index 11df0c62951..05f66240859 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/dpns_marketplace.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/dpns_marketplace.rs @@ -2620,11 +2620,11 @@ mod tests { ); } - /// THE REGRESSION. First pass after a process restart: the in-memory - /// snapshot is empty (exactly what the load path produces) but the - /// durable mirror still holds the row, so the departure recovers the - /// `document_id` its removal delta needs. Before the fix this - /// returned `None` and the mirror row was orphaned forever. + /// First pass after a process restart: the in-memory snapshot is + /// empty (exactly what the load path produces) but the durable + /// mirror still holds the row, so the departure recovers the + /// `document_id` its removal delta needs. Answering `None` here + /// would orphan the mirror row forever. #[test] fn departed_document_id_falls_back_to_the_persisted_row_after_a_restart() { let document_id = Identifier::from([0x33; 32]); @@ -2781,15 +2781,14 @@ mod tests { ); } - /// THE ROUND-3 REGRESSION for the departure path. Platform CONFIRMS - /// the name is gone (the mock answers the domain query with an empty - /// document set), which is the branch that resolves the departure, - /// drops the identity's label, and emits the removal delta. When the - /// persistence lookup for the `document_id` FAILED rather than - /// answering "no row", the old code could not tell the two apart: - /// resolution carried on with no id, the label — the only trigger - /// for future departure detection — was removed, and the durable row - /// was orphaned for good. + /// The departure path. Platform CONFIRMS the name is gone (the mock + /// answers the domain query with an empty document set), which is + /// the branch that resolves the departure, drops the identity's + /// label, and emits the removal delta. A persistence lookup for the + /// `document_id` that FAILS must not be mistaken for one answering + /// "no row": resolving with no id would remove the label — the only + /// trigger for future departure detection — and orphan the durable + /// row for good. /// /// The first assertion block establishes that this mock really does /// take the confirmed-absent branch, so the retention assertion that @@ -2869,16 +2868,16 @@ mod tests { assert_eq!(healed.summary.document_id, Some(document_id)); } - /// THE ROUND-4 REGRESSION. A NON-retryable persistence error + /// A NON-retryable persistence error /// (`Fatal` / `Constraint` / `LockPoisoned`) cannot be retried into /// success, so it must not park this identity's departure queue — /// but it does not establish that no durable row exists, either. - /// The old arm degraded it to "no previous id" and carried on; with - /// Platform confirming the document absent, that RESOLVED the - /// departure, dropped the label — the only trigger for future - /// departure detection — and reported a successful sync, leaving - /// any persisted row orphaned for good. It must instead be a - /// terminal per-item FAILURE: no retry, no label drop, no deltas. + /// Degrading it to "no previous id" and carrying on would, with + /// Platform confirming the document absent, RESOLVE the departure, + /// drop the label — the only trigger for future departure + /// detection — and report a successful sync, leaving any persisted + /// row orphaned for good. It must instead be a terminal per-item + /// FAILURE: no retry, no label drop, no deltas. #[tokio::test] async fn resolve_departed_name_fails_terminally_when_the_persistence_error_is_not_retryable() { let document_id = Identifier::from([0xA4; 32]); @@ -2978,15 +2977,15 @@ mod tests { ); } - /// THE ROUND-5 REGRESSION. DPNS domain documents are deletable, and - /// a label can be re-registered under a fresh document id: persisted - /// document A (the identity's own row) was deleted and an unrelated - /// identity registered document B under the same normalized label. - /// The domain query answers with B, whose history never departs the - /// wallet identity, so classification yields no sale status. The old - /// code then reported and removed B — the replacement owner's - /// document, never a row of this departure — while the identity's - /// durable row A survived with no label left to ever trigger its + /// DPNS domain documents are deletable, and a label can be + /// re-registered under a fresh document id: persisted document A + /// (the identity's own row) was deleted and an unrelated identity + /// registered document B under the same normalized label. The + /// domain query answers with B, whose history never departs the + /// wallet identity, so classification yields no sale status. + /// Reporting and removing B — the replacement owner's document, + /// never a row of this departure — would leave the identity's + /// durable row A with no label left to ever trigger its /// reconciliation. The removal delta must target the RECOVERED prior /// incarnation A and leave the replacement B untouched. #[tokio::test] @@ -3159,18 +3158,17 @@ mod tests { }) } - /// THE ROUND-6 REGRESSION (successor to the round-5 one above): the - /// re-registered replacement itself passed through this wallet - /// identity and departed. Persisted document A (the identity's own - /// row) was deleted, the label was re-registered as B, and B was - /// acquired by this identity and transferred away — all before this - /// sync pass. `classify_departure(B)` correctly yields - /// `Transferred`, but the old code wrote B's historical entry with - /// `remove_document_id: None`; the caller then dropped the label — - /// the only trigger that would ever revisit the durable row — - /// leaving recovered row A persisted as `Owned` forever. B's - /// classified entry and A's retirement must land in the same - /// changeset. + /// Successor to the case above: the re-registered replacement + /// itself passed through this wallet identity and departed. + /// Persisted document A (the identity's own row) was deleted, the + /// label was re-registered as B, and B was acquired by this identity + /// and transferred away — all before this sync pass. + /// `classify_departure(B)` correctly yields `Transferred`, but + /// writing B's historical entry with `remove_document_id: None` + /// would let the caller drop the label — the only trigger that + /// would ever revisit the durable row — leaving recovered row A + /// persisted as `Owned` forever. B's classified entry and A's + /// retirement must land in the same changeset. #[tokio::test] async fn resolve_departed_name_retires_the_prior_incarnation_when_the_replacement_also_departed( ) { diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs b/packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs index c44eecb6685..0027446e5ba 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs @@ -274,12 +274,12 @@ impl IdentityWallet { // `ASSET_LOCK_FUNDING_SOURCES` — the BIP44 and BIP32 accounts at // `funding_account_index` plus every DashPay contact-receiving account // — so an invitation can be funded from a balance spread across - // accounts, without the sweep-then-lock hop that used to be required. + // accounts, with no sweep-then-lock hop. // `identity_index` is unused for the // `IdentityInvitation` funding type. Only the broadcast half runs here — // the proof wait is deferred until AFTER the invitation record below is // durably persisted, so an interruption during the (potentially long) - // proof wait can no longer orphan the funded lock from the reclaim UI. + // proof wait cannot orphan the funded lock from the reclaim UI. let (path, out_point) = self .asset_locks .broadcast_funded_asset_lock( diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index d59c3250390..43e001ba0ad 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -1218,17 +1218,15 @@ impl DashPayView<'_, B> { let current_height = info.core_wallet.synced_height(); - // Pool the same funding set as a plain send (#4329): BIP44 + - // BIP32 + every DashPay receiving account. Pinning this path to - // BIP44 alone was the reason a wallet whose balance had moved into + // Pool the same funding set as a plain send: BIP44 + BIP32 + + // every DashPay receiving account. Pinning this path to BIP44 + // alone would make a wallet whose balance has moved into // contact-receiving accounts hit "Insufficient funds" on a screen - // showing plenty — the exact symptom #4329 fixed for the core send - // path, which this path never picked up (it only took that PR's - // `set_funding` → `add_funding` rename). + // showing plenty. // // Order is load-bearing: BIP44 is offered first, and the builder // takes the change address from the first funding source, so - // change keeps returning to BIP44 as before. CoinJoin stays out by + // change keeps returning to BIP44. CoinJoin stays out by // construction — spending mixed outputs alongside transparent ones // links them and undoes the mixing — and so do the contact // *external* accounts, which hold the counterparty's xpub and no @@ -1239,7 +1237,7 @@ impl DashPayView<'_, B> { .add_output(&payment_address, amount_duffs); // Derivation paths for every offered UTXO, since the signer closure - // below can no longer resolve them from one account. + // below cannot resolve them from one account. let mut funding_paths: std::collections::HashMap< dashcore::Address, key_wallet::bip32::DerivationPath, @@ -1373,8 +1371,7 @@ impl DashPayView<'_, B> { // this very input, finds no fence on it, passes its own copy of the // check above, and completes — after which THIS future resumes and // puts its already-signed transaction on the wire against an input - // reassigned to another payment (`dashpay/platform#4309`, review - // round 7). + // reassigned to another payment. // // So the pin is installed under the guard that just proved the // reservation is ours, making check-and-pin one atomic step, and it @@ -1432,8 +1429,8 @@ impl DashPayView<'_, B> { // The pin installed under the build guard is held across this await — // that is the whole point of it — and settled on the way out. Only a // definitive rejection releases the inputs; every other outcome leaves - // the pending-spend fence standing until the wallet observes the spend - // (`dashpay/platform#4309`). A cancellation or unwind inside `broadcast` + // the pending-spend fence standing until the wallet observes the spend. + // A cancellation or unwind inside `broadcast` // reaches neither arm and settles as pending through // `InBroadcastPin::drop`, which is the conservative direction. let broadcast_result = match self.broadcaster.broadcast(&tx).await { @@ -1452,25 +1449,24 @@ impl DashPayView<'_, B> { // fence inputs of a transaction proven never sent — a fence no // observed spend could ever clear, held for the manager's // lifetime since the pending phase carries no deadline by - // design (`dashpay/platform#4309`). + // design. let mut in_broadcast_pin = in_broadcast_pin; in_broadcast_pin.settle_released_on_drop(); // // ORDER MATTERS — the cleanup runs FIRST, under the still-live - // fence, and only then does the pin come down - // (`dashpay/platform#4309`, review round 8). The cleanup is an + // fence, and only then does the pin come down. The cleanup is an // `.await`: it must re-acquire the wallet-manager read lock, and // on this path it carries NO reservation token, so it performs an // unconditional `release_reservation`. Releasing the fence first - // opened a window in which this input was neither fenced nor — - // once catch-up had swept the build's reservation — reserved. A - // build already queued on the manager write lock could take it in - // that window, pass the now-absent conflict check, and drop the - // lock with its external signer still pending (finalized builds - // install no pin until broadcast); the unconditional cleanup then - // deleted THAT build's newer reservation, and a second - // finalization could reserve and sign the same input — two live - // conflicting handles. + // would open a window in which this input is neither fenced nor + // — once catch-up has swept the build's reservation — reserved. + // A build already queued on the manager write lock could take it + // in that window, pass the now-absent conflict check, and drop + // the lock with its external signer still pending (finalized + // builds install no pin until broadcast); the unconditional + // cleanup would then delete THAT build's newer reservation, and + // a second finalization could reserve and sign the same input — + // two live conflicting handles. // // With the fence held across the cleanup there is no such window: // a queued build that runs first meets the fence and rolls back @@ -5286,7 +5282,7 @@ mod tests { /// * It pins the deliberate mixed-failure policy change: purpose-rejected /// on our side wins, and the entry stays recoverable. /// - /// Drained twice, because the cost this PR removes is per sweep, not once. + /// Drained twice, because the cost avoided is per sweep, not once. #[tokio::test] async fn unaccepted_recipient_purpose_never_fetches_and_stays_recoverable() { use crate::changeset::{PendingContactCrypto, PendingContactCryptoOp}; @@ -6223,13 +6219,11 @@ mod tests { } /// A contact payment funds from a DashPay **receiving** account when BIP44 - /// alone cannot cover it — the pooled funding set a plain send has used - /// since #4329. + /// alone cannot cover it — the same pooled funding set a plain send uses. /// - /// This path kept its BIP44-only pin through that PR (it took only the - /// `set_funding` → `add_funding` rename), so a wallet whose balance had - /// moved into contact-receiving accounts saw the funds in its total and got - /// `Insufficient funds` trying to pay a contact. Reported from mainnet + /// A BIP44-only pin on this path lets a wallet whose balance has moved + /// into contact-receiving accounts see the funds in its total and still + /// get `Insufficient funds` trying to pay a contact. Observed on mainnet /// after 8 successful contact payments drained BIP44: `available 41505, /// required 100000`, on a screen showing plenty. /// @@ -6784,14 +6778,13 @@ mod tests { } } - /// `dashpay/platform#4309`, REVIEW ROUND 7 — THE CONTACT-PAYMENT BUILD'S - /// OWN FENCE. + /// THE CONTACT-PAYMENT BUILD'S OWN FENCE. /// - /// The build's conflict check stopped it from CONSUMING an input another - /// dispatch had fenced. It did not fence the selection it had just made, so + /// The build's conflict check stops it from CONSUMING an input another + /// dispatch has fenced. Without a fence on the selection it has just made, /// the stretch after the manager write guard drops — the durability store - /// and `broadcaster.broadcast(&tx)` — ran with no pin at all. This test - /// drives the resulting race end to end: + /// and `broadcaster.broadcast(&tx)` — would run with no pin at all. This + /// test drives the resulting race end to end: /// /// 1. A contact payment builds, signs, releases the guard, and SUSPENDS /// inside the broadcaster before submission. @@ -6802,10 +6795,10 @@ mod tests { /// UTXO, so it selects the same input the parked transaction already /// spends. /// - /// Before the fix step 3 SUCCEEDED — it found no fence (the parked build - /// never installed one), passed its own conflict check, and returned a - /// second signed transaction against the same input, which the resuming - /// original then raced on the wire. It must now be refused. + /// Without the build's own fence step 3 would SUCCEED — it would find no + /// fence (the parked build installed none), pass its own conflict check, + /// and return a second signed transaction against the same input, which + /// the resuming original then races on the wire. It must be refused. #[tokio::test] async fn a_suspended_contact_payment_fences_its_inputs_against_a_competing_build() { use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; @@ -6881,20 +6874,20 @@ mod tests { ); } - /// `dashpay/platform#4309`, REVIEW ROUND 8 — THE FENCE MUST OUTLIVE THE - /// REJECTED-BROADCAST RESERVATION CLEANUP. + /// THE FENCE MUST OUTLIVE THE REJECTED-BROADCAST RESERVATION CLEANUP. /// - /// The definitive-rejection arm used to drop the fence FIRST and only then - /// await `release_reservation_after_rejected_broadcast`. That cleanup is - /// token-less on this path, so it performs an UNCONDITIONAL - /// `release_reservation`, and it can only run after re-acquiring the - /// wallet-manager read lock — an await. In that window the input was - /// neither fenced nor (once catch-up had swept it) reserved, so a build - /// already queued on the manager write lock could reserve it, pass the - /// now-absent conflict check, and drop the lock with an external signer - /// still pending. The unconditional cleanup then deleted THAT build's - /// newer reservation, leaving the outpoint free for a second finalization - /// to reserve and sign — two fresh conflicting handles over one input. + /// If the definitive-rejection arm dropped the fence FIRST and only then + /// awaited `release_reservation_after_rejected_broadcast`, there would be + /// a window: that cleanup is token-less on this path, so it performs an + /// UNCONDITIONAL `release_reservation`, and it can only run after + /// re-acquiring the wallet-manager read lock — an await. In that window + /// the input is neither fenced nor (once catch-up has swept it) reserved, + /// so a build already queued on the manager write lock could reserve it, + /// pass the now-absent conflict check, and drop the lock with an external + /// signer still pending. The unconditional cleanup would then delete THAT + /// build's newer reservation, leaving the outpoint free for a second + /// finalization to reserve and sign — two fresh conflicting handles over + /// one input. /// /// The invariant that closes it: the fence stays up THROUGH the cleanup and /// comes down only after it. A queued build that runs first then meets a @@ -6903,10 +6896,10 @@ mod tests { /// Driven here by holding the wallet-manager WRITE lock across the /// broadcaster's rejection. The cleanup needs the READ lock, so it cannot /// complete while the test holds the write side — which makes the assertion - /// an invariant rather than a race: with the fix the fence CANNOT be gone at - /// this observation point, because the only code that releases it runs after - /// a cleanup that is provably still blocked. Before the fix the release ran - /// synchronously the instant `broadcast` returned, so the fence was gone. + /// an invariant rather than a race: the fence CANNOT be gone at this + /// observation point, because the only code that releases it runs after a + /// cleanup that is provably still blocked. A release that ran synchronously + /// the instant `broadcast` returned would already have taken it down. #[tokio::test] async fn the_contact_send_fence_outlives_its_rejected_broadcast_reservation_cleanup() { use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; @@ -6985,28 +6978,29 @@ mod tests { ); } - /// `dashpay/platform#4309` — CANCELLATION DURING THE REJECTED-BROADCAST - /// CLEANUP MUST NOT LEAVE A PERMANENT FENCE. + /// CANCELLATION DURING THE REJECTED-BROADCAST CLEANUP MUST NOT LEAVE A + /// PERMANENT FENCE. /// /// After `broadcast()` definitively returns `Rejected`, the send awaits /// the token-less reservation cleanup under the still-raised fence (the - /// round-8 ordering, proven by the sibling test above). The pin used to - /// carry its DEFAULT pending-on-drop verdict through that await, so - /// cancelling the send future while the cleanup waited on the manager - /// lock dropped the pin as `Pending`: a pending-spend fence over the - /// inputs of a transaction PROVEN never sent. No spend of it can ever be - /// observed, and the pending phase has no deadline by design, so the - /// outpoint stayed fenced for the manager's lifetime. + /// ordering proven by the sibling test above). Were the pin to carry its + /// DEFAULT pending-on-drop verdict through that await, cancelling the send + /// future while the cleanup waits on the manager lock would drop the pin + /// as `Pending`: a pending-spend fence over the inputs of a transaction + /// PROVEN never sent. No spend of it can ever be observed, and the pending + /// phase has no deadline by design, so the outpoint would stay fenced for + /// the manager's lifetime. /// - /// The rejection verdict is now recorded on the pin synchronously, before - /// the cleanup's first await gives cancellation its first opportunity, so - /// a drop ANYWHERE afterwards settles the fence as released. + /// The rejection verdict is therefore recorded on the pin synchronously, + /// before the cleanup's first await gives cancellation its first + /// opportunity, so a drop ANYWHERE afterwards settles the fence as + /// released. /// /// The test drives the send future by hand (noop waker) so every step is /// deterministic: park it inside the broadcaster, pin the cleanup behind /// a held manager WRITE lock, poll the rejection through to the cleanup - /// await, then DROP the future there — the cancellation the finding - /// describes — and require the outpoint to be left unfenced. + /// await, then DROP the future there — the cancellation described above + /// — and require the outpoint to be left unfenced. #[tokio::test] async fn cancelling_the_rejected_broadcast_cleanup_leaves_no_fence() { use std::task::{Context, Poll, Waker}; diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/profile.rs b/packages/rs-platform-wallet/src/wallet/identity/network/profile.rs index e4c041d2724..47f35c8a020 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/profile.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/profile.rs @@ -161,8 +161,8 @@ impl DashPayView<'_, B> { properties.insert("avatarFingerprint".to_string(), Value::Bytes(fp.to_vec())); } - // 4. Look up identity + signing key. We no longer need the - // identity_index — the signer is supplied externally. + // 4. Look up identity + signing key. The identity_index is not + // needed here — the signer is supplied externally. let signing_key = { let wm = self.wallet_manager.read().await; let info = wm diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs b/packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs index a9059e70030..1ddb7137e7f 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs @@ -111,7 +111,7 @@ impl IdentityWallet { // Preserve a structured key-unavailable signer failure (from the // identity signer) so the FFI boundary can still restore code 31; // otherwise fall through to the existing nonce-promotion / - // stringifying wrapper unchanged (dashpay/platform#4183 review). + // stringifying wrapper unchanged. crate::error::preserve_signer_key_unavailable_or(e, |e| { crate::error::promote_address_nonce_error(&e).unwrap_or_else(|| { PlatformWalletError::InvalidIdentityData(format!( diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/registration.rs b/packages/rs-platform-wallet/src/wallet/identity/network/registration.rs index d1a8c965880..9cdb0e59977 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/registration.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/registration.rs @@ -572,12 +572,11 @@ impl IdentityWallet { // Helpers // --------------------------------------------------------------------------- -// `find_tracked_unproven_lock` was removed when -// `PlatformWalletError::FinalityTimeout` was widened to carry the full -// `OutPoint` (previously only the `Txid`). The IS→CL fallback now reads -// the outpoint directly off the error payload — no BTreeMap walk by -// `(funding_type, identity_index)` is needed, which also closes the -// non-determinism gap when multiple unproven locks shared that key. +// `PlatformWalletError::FinalityTimeout` carries the full `OutPoint`, so +// the IS→CL fallback reads the outpoint directly off the error payload — +// no BTreeMap walk by `(funding_type, identity_index)` is needed, which +// also closes the non-determinism gap when multiple unproven locks share +// that key. // --------------------------------------------------------------------------- // Tests diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs b/packages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs index 6634264f848..10cc57c35c2 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs @@ -1003,9 +1003,9 @@ mod tests { // The gate in front of the drain. // // `verify_seed_binds` above proves the check itself is right. These prove - // the drain cannot run without it — the property that matters, because the - // FFI entry point every JNI client binds to used to call the drains - // directly and skip the check entirely. + // the drain cannot run without it — the property that matters for the FFI + // entry point every JNI client binds to, which must not reach the drains + // directly and skip the check. // ----------------------------------------------------------------------- /// A different valid BIP-39 vector: the mis-mapped Keychain slot. @@ -1261,8 +1261,8 @@ mod tests { } } - /// The defect: a payment made through a wrong-seed provider used to drain - /// first and fail second. The drain runs `RegisterReceiving` against + /// The failure mode: a payment made through a wrong-seed provider that + /// drains first and fails second. The drain runs `RegisterReceiving` against /// whatever seed the provider resolves, and `register_contact_account` /// keys its existence check on `(index, us, them)` rather than the xpub — /// so the wrong account is written once, never revisited, and the wallet @@ -1679,11 +1679,11 @@ mod tests { // // Both gated passes take a deadline, and the startup sequence hands one // down precisely so no Platform-wallet step can hold Core SPV past its - // budget. The check itself used to sit outside it: the provider is the - // host's Keychain / Keystore, and a host that never answers held the whole - // launch. An already-spent deadline was worse than useless — it still paid - // for a Keychain derivation before the drain it guards would have stopped - // at its first entry. + // budget. The check itself must sit inside it: the provider is the host's + // Keychain / Keystore, and a host that never answers would hold the whole + // launch. An already-spent deadline must stop the check too, or it still + // pays for a Keychain derivation before the drain it guards would have + // stopped at its first entry. // // Bounding it is safe in a way bounding the drains is not: the check // derives a public key and compares it, committing nothing on the way, so diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/burn.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/burn.rs index 699892d1c1e..80968d1d274 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/burn.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/burn.rs @@ -80,8 +80,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token burn failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/claim.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/claim.rs index 81a3449e9ae..06a1d2b94f9 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/claim.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/claim.rs @@ -76,8 +76,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token claim failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/destroy_frozen_funds.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/destroy_frozen_funds.rs index d3defd7b1c6..e1730f3a0cd 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/destroy_frozen_funds.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/destroy_frozen_funds.rs @@ -90,8 +90,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token destroy frozen funds failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/freeze.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/freeze.rs index 80b3f4426d3..85ffe4cff90 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/freeze.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/freeze.rs @@ -83,8 +83,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token freeze failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/mint.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/mint.rs index 7e47d7aee30..b4298a97f34 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/mint.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/mint.rs @@ -136,8 +136,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token mint failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/pause.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/pause.rs index b99f37cc3c1..4772e8c2bcd 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/pause.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/pause.rs @@ -82,8 +82,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token pause failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/purchase.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/purchase.rs index 6005515d98c..4344f27a144 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/purchase.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/purchase.rs @@ -81,8 +81,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token purchase failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/resume.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/resume.rs index 0629f9a477e..e787f0cea47 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/resume.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/resume.rs @@ -81,8 +81,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token resume failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/set_price.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/set_price.rs index eb2a420d1d6..7a6c2473c4d 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/set_price.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/set_price.rs @@ -106,8 +106,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token set price failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/transfer.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/transfer.rs index ff9a708006b..d516eb79c12 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/transfer.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/transfer.rs @@ -85,8 +85,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token transfer failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/unfreeze.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/unfreeze.rs index d2c44595563..52a86c38834 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/unfreeze.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/unfreeze.rs @@ -85,8 +85,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token unfreeze failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/update_config.rs b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/update_config.rs index 1db8497a95c..61a6c1b6403 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/tokens/update_config.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/tokens/update_config.rs @@ -89,8 +89,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the FFI // boundary can still restore code 31; only genuine operation - // failures get stringified into `TokenError` - // (dashpay/platform#4183 review). + // failures get stringified into `TokenError`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::TokenError(format!("Token config update failed: {}", e)) }) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/transfer.rs b/packages/rs-platform-wallet/src/wallet/identity/network/transfer.rs index 10a503684d1..b12b81ed1a2 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/transfer.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/transfer.rs @@ -109,8 +109,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the // FFI boundary can still restore code 31; only genuine - // operation failures get stringified into `InvalidIdentityData` - // (dashpay/platform#4183 review). + // operation failures get stringified into `InvalidIdentityData`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to transfer credits: {}", diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/transfer_to_addresses.rs b/packages/rs-platform-wallet/src/wallet/identity/network/transfer_to_addresses.rs index 54e3b234831..cbc00326e2a 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/transfer_to_addresses.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/transfer_to_addresses.rs @@ -112,8 +112,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the // FFI boundary can still restore code 31; only genuine - // operation failures get stringified into `InvalidIdentityData` - // (dashpay/platform#4183 review). + // operation failures get stringified into `InvalidIdentityData`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to transfer credits to addresses: {}", diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/withdrawal.rs b/packages/rs-platform-wallet/src/wallet/identity/network/withdrawal.rs index 939c3797cbd..ddc8fb0e287 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/withdrawal.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/withdrawal.rs @@ -107,8 +107,7 @@ impl IdentityWallet { .map_err(|e| { // Preserve a structured key-unavailable signer failure so the // FFI boundary can still restore code 31; only genuine - // operation failures get stringified into `InvalidIdentityData` - // (dashpay/platform#4183 review). + // operation failures get stringified into `InvalidIdentityData`. crate::error::preserve_signer_key_unavailable_or(e, |e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to withdraw credits: {}", diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs b/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs index 1a955ce78b2..a5eb645450e 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs @@ -2055,8 +2055,8 @@ mod tests { } /// Per-element `apply_ignored_sender` over a fresh identity reproduces a - /// wholesale set assign — the equivalence the replay/restore paths (which - /// previously assigned or extended the whole `BTreeSet`) rely on. + /// wholesale set assign — the equivalence the replay/restore paths rely + /// on. #[test] fn apply_ignored_sender_loop_equals_wholesale_assign() { let persisted: std::collections::BTreeSet = [ diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs b/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs index 8e4b98bc635..0ecb4fe2c81 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs @@ -42,8 +42,8 @@ impl ManagedIdentity { /// [`IdentityKeyEntry`] upsert per registered public key on this /// identity. /// - /// Private-key bytes / derivation breadcrumbs no longer ride along - /// here — `ManagedIdentity` doesn't carry `key_storage` anymore, + /// Private-key bytes / derivation breadcrumbs do not ride along + /// here — `ManagedIdentity` carries no `key_storage`, /// so every emitted entry has `wallet_id == None` and /// `derivation_indices == None`. Callers that need the /// breadcrumb (e.g. registration / discovery) emit a dedicated diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rs b/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rs index 1dc5f651ddd..c1afd616375 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rs @@ -46,8 +46,8 @@ pub struct ManagedIdentity { /// subsequent operations (signing, ECDH) can derive the correct keys. /// /// `Some(idx)` when this identity lives in a wallet's bucket — `idx` is - /// the inner BTreeMap key. `None` for out-of-wallet identities (formerly - /// "watched"); they have no HD-derivation context. + /// the inner BTreeMap key. `None` for out-of-wallet identities; they + /// have no HD-derivation context. pub identity_index: Option, /// Last block time when balance was updated for this identity diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs b/packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs index 0080effaa3c..7317eb129cf 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs @@ -165,7 +165,7 @@ impl IdentityManager { /// /// Layers the public-key record into the DPP `Identity`'s /// `public_keys` map (overwriting any existing slot with the same - /// `KeyID`). Private-key data is no longer kept on + /// `KeyID`). Private-key data is not kept on /// `ManagedIdentity` (it lives in the iOS Keychain on the client /// side); the `(wallet_id, derivation_indices)` breadcrumb on the /// entry tells the client how to re-derive the scalar. diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs b/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs index 33bbe80e4b8..e326bf31409 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs @@ -94,12 +94,10 @@ impl IdentityManager { /// Add an identity to the out-of-wallet (observed read-only) bucket. /// - /// Replaces the previous `add_watched_identity` — we no longer keep - /// a separate `WatchedIdentity` type; observed identities are just - /// `ManagedIdentity` rows with `wallet_id == None` and an empty - /// public-key map. If an identity with the same id is already in - /// either bucket this is a no-op (matches the old `add_watched_identity` - /// idempotency contract). + /// There is no separate `WatchedIdentity` type; observed identities + /// are just `ManagedIdentity` rows with `wallet_id == None` and an + /// empty public-key map. If an identity with the same id is already + /// in either bucket this is a no-op. pub fn add_out_of_wallet_identity( &mut self, identity: Identity, diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/manager/mod.rs b/packages/rs-platform-wallet/src/wallet/identity/state/manager/mod.rs index 7b967f669ed..32237f583e4 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/manager/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/manager/mod.rs @@ -419,7 +419,7 @@ mod tests { assert!(manager.location_index().is_empty()); } - /// The cross-launch half of dashpay/platform#4365: an incomplete scan + /// The cross-launch half of the missed-identity shape: an incomplete scan /// verdict restored from the start state must still say "incomplete", or /// the next launch takes the warm shortcut over an identity set that was /// never fully probed. diff --git a/packages/rs-platform-wallet/src/wallet/masternode_withdrawal.rs b/packages/rs-platform-wallet/src/wallet/masternode_withdrawal.rs index f510e104d61..4ecf07f3b14 100644 --- a/packages/rs-platform-wallet/src/wallet/masternode_withdrawal.rs +++ b/packages/rs-platform-wallet/src/wallet/masternode_withdrawal.rs @@ -389,8 +389,8 @@ impl PlatformWallet { /// `expected_hash160`, sign with `signer` at `path` through /// [`DerivedKeyIdentitySigner`] (which re-checks the derived key against /// `expected_hash160` before emitting a signature), broadcast, and wait for -/// the proved balance. Error semantics are unchanged from #4451: definitive -/// rejections stay retryable, ambiguous outcomes are +/// the proved balance. Error semantics: definitive rejections stay +/// retryable, ambiguous outcomes are /// [`PlatformWalletError::MasternodeWithdrawalUnconfirmed`]. #[allow(clippy::too_many_arguments)] pub(crate) async fn execute_masternode_withdrawal( diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs index 25c5a2c8833..1738c5c3592 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs @@ -80,10 +80,10 @@ impl PlatformAddressWallet { /// [`remainder_fee_strategy`], because the only address that can /// legitimately absorb the fee is the remainder output and its /// consensus index is a property of that map's ordering, not of - /// any caller's list order. Every binding that computed the index - /// from its own list order silently mis-targeted the fee whenever - /// the remainder was not also first lexicographically, so the - /// index is no longer the caller's to supply. This mirrors the C + /// any caller's list order. A binding that computes the index + /// from its own list order silently mis-targets the fee whenever + /// the remainder is not also first lexicographically, so the + /// index is not the caller's to supply. This mirrors the C /// ABI, where `fee_strategy` / `fee_strategy_count` are likewise /// still accepted and ignored. /// @@ -1151,10 +1151,10 @@ mod tests { /// Both arrangements are pinned, because the hazard is asymmetric: /// a caller computing the index from its own array order happens to /// be right whenever the remainder is also first lexicographically, - /// and silently wrong otherwise. The second case below is the one - /// that used to misfire — with a third-party payee in the set it - /// charges the fee to the payee's explicit amount instead of the - /// sender's change. + /// and silently wrong otherwise. The second case below is the one a + /// list-order index gets wrong — with a third-party payee in the set + /// it would charge the fee to the payee's explicit amount instead of + /// the sender's change. #[test] fn remainder_fee_strategy_targets_the_remainder_output() { let alice = p2pkh(0x0A); diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs index 3ce68738197..6eb140ab58b 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs @@ -1390,8 +1390,8 @@ mod tests { /// `sync_finished` must drop an address proven absent this pass from /// the committed `found` map so it stops seeding the next pass and - /// `current_balances()` no longer yields it. This is the core of the - /// stale-balance-after-chain-reset fix. + /// `current_balances()` no longer yields it. This is what keeps a stale + /// balance from surviving a chain reset. #[tokio::test] async fn sync_finished_removes_absent_from_committed_found() { let addr = p2pkh(1); @@ -1775,11 +1775,11 @@ mod tests { /// ADDR-09, credit side: a committed credit is pinned at the proof /// height (`AddressFunds::as_of_height`), which is what stops the /// sync's delta replay from re-applying the on-chain `AddToCredits` - /// on top of the just-committed absolute — so the seam no longer - /// needs to invalidate the incremental watermark (the old gate, - /// which forced a full rescan yet could not protect the rescan - /// itself from the same replay). The fast incremental cadence is - /// preserved for every flow. + /// on top of the just-committed absolute — so the seam does not + /// need to invalidate the incremental watermark (a forced full + /// rescan could not protect the rescan itself from the same + /// replay). The fast incremental cadence is preserved for every + /// flow. #[tokio::test] async fn reconcile_pins_committed_credit_and_keeps_watermark() { use dash_sdk::query_types::AddressInfo; diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs index b2ead28309e..9eb908ae41e 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs @@ -222,8 +222,8 @@ mod tests { /// An address that previously carried a cached balance and is proven /// absent this pass must produce a single zeroed entry (balance 0, - /// nonce 0). This is the chain-reset bug: without it the stale - /// balance is never written back to 0. + /// nonce 0). Without it a stale balance is never written back to 0 + /// after a chain reset. #[test] fn absent_previously_funded_address_emits_zero_entry() { let mut before = BTreeMap::new(); diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs index 38db2fd492f..3d5fb91113d 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs @@ -394,8 +394,8 @@ impl PlatformAddressWallet { }; // Lower fee_strategy AFTER augmentation so indexes resolve against - // the FINAL outputs map. Lowering before would reintroduce the - // misrouting bug this wrapper exists to prevent. + // the FINAL outputs map. Lowering before would cause the + // misrouting this wrapper exists to prevent. let indexed_fee_strategy = fee_strategy.to_indexed(&inputs_for_resolve, &final_outputs)?; // Replicate the Auto path's ReduceOutput fee-headroom guard so callers diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index 582f6fd2e87..3774e48e203 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -542,8 +542,7 @@ impl PlatformAddressWallet { /// /// Equivalent to [`initialize`]: the unified provider is rebuilt /// from the current account set in the wallet manager. The name - /// is kept for API continuity with call sites that used to add - /// per-account providers. + /// is kept for API continuity with per-account-provider call sites. pub async fn add_provider(&self, _account_index: u32) -> Result<(), PlatformWalletError> { self.initialize().await; Ok(()) diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs index e1fef220d44..1b75bfd33df 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs @@ -1040,11 +1040,10 @@ mod tests { // the fee-source input at `balance − fee`. These tests pin that invariant // — "every per-input requested amount ≤ that input's balance" — so a // future change to `reserve_withdrawal_fee_on_largest_input` can't - // reintroduce an over-request. The doubled-balance ADDR-04 repro was a - // WRONG INPUT to this function (a stale/doubled cached balance was passed - // in); `plan_withdrawal` now sources balances from the same on-chain - // `AddressInfo::fetch_many` proof the spend re-checks, so the values fed - // here are the authoritative ones. + // reintroduce an over-request. A stale/doubled cached balance would be a + // WRONG INPUT to this function; `plan_withdrawal` sources balances from + // the same on-chain `AddressInfo::fetch_many` proof the spend re-checks, + // so the values fed here are the authoritative ones. /// Assert the plan is spendable: for every input, the planned withdraw /// amount is ≤ the balance that input was selected with. `balances` maps @@ -1209,13 +1208,12 @@ mod tests { /// SDK's `AddressInfo::fetch_many` proof query returns (the on-chain truth the /// spend re-checks), NOT from the wallet's cached `address_credit_balance`. /// -/// This is the behavior the ADDR-04 fix actually changes; the pure-function -/// tests above can't reach it because they call -/// `reserve_withdrawal_fee_on_largest_input` directly with balances already -/// chosen. Here we deliberately make the cache DISAGREE with the chain (a -/// doubled/stale cached balance for one address) and assert the plan follows -/// the chain. A future regression that reintroduced a cache read (e.g. -/// `.unwrap_or_else(|| cached_balance)`) would fail this test. +/// This is the behavior the pure-function tests above can't reach, because +/// they call `reserve_withdrawal_fee_on_largest_input` directly with balances +/// already chosen. Here we deliberately make the cache DISAGREE with the +/// chain (a doubled/stale cached balance for one address) and assert the plan +/// follows the chain. A future regression that reintroduced a cache read +/// (e.g. `.unwrap_or_else(|| cached_balance)`) would fail this test. #[cfg(test)] mod plan_withdrawal_seam_tests { use std::collections::BTreeSet; diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs index c1ae06ba180..dad43b97310 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs @@ -380,10 +380,9 @@ impl PlatformWallet { /// Access the identity wallet. /// - /// Covers both identity-lifecycle and DashPay-contract operations — - /// these used to be split across `identity()` / `dashpay()`, but the - /// two facades were merged (the underlying `ManagedIdentity` state - /// was already shared between them). Keeps the single `SpvBroadcaster` + /// Covers both identity-lifecycle and DashPay-contract operations in + /// one facade, since the underlying `ManagedIdentity` state is shared + /// between them. Keeps the single `SpvBroadcaster` /// specialization the rest of this wallet uses. pub fn identity(&self) -> &IdentityWallet { &self.identity @@ -1001,10 +1000,10 @@ impl PlatformWallet { // and per-subwallet store state is purged only for accounts // this registration DROPS or re-keys — accounts that remain // bound with the same viewing key keep their in-memory notes - // and watermark. A re-bind racing an in-flight sync pass can - // therefore no longer wipe the pass's results (the former - // unregister-then-register cycle here purged the whole - // wallet behind the pass's store lock and then restored a + // and watermark. A re-bind racing an in-flight sync pass + // therefore cannot wipe the pass's results (an + // unregister-then-register cycle here would purge the whole + // wallet behind the pass's store lock and then restore a // pre-pass snapshot — the "note discovered by sync is // unspendable until app restart" / "every pass rescans from // 0" failure). Registration also runs BEFORE the restore so @@ -1394,7 +1393,7 @@ impl PlatformWallet { /// address (`"dash1…"` / `"tdash1…"`). Parsed via /// `PlatformAddress::from_bech32m_string`; the recipient's HRP is /// verified against the wallet's network HRP class here, since the - /// network-agnostic decoder no longer enforces it. `seed` supplies + /// network-agnostic decoder does not enforce it. `seed` supplies /// the transient spend authority (see /// [`shielded_transfer_to`](Self::shielded_transfer_to)). #[cfg(feature = "shielded")] @@ -2084,8 +2083,8 @@ mod check_recipient_hrp_tests { #[test] fn devnet_address_into_devnet_wallet_is_accepted() { - // The paloma regression: a devnet `tdash1…` recipient must be - // accepted by a devnet wallet (it was previously mis-rejected as + // A devnet `tdash1…` recipient must be + // accepted by a devnet wallet (not mis-rejected as // Testnet). let addr = recipient(dashcore::Network::Devnet); assert!(addr.starts_with("tdash1")); diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs index 49ed828d228..02d052359de 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs @@ -87,11 +87,10 @@ impl WalletInfoInterface for PlatformWalletInfo { self.core_wallet.birth_height() } - // `first_loaded_at` / `set_first_loaded_at` were dropped from - // `WalletInfoInterface` upstream and have no backing methods on - // `ManagedWalletInfo` anymore. The field still exists on - // `WalletMetadata` but is read/written directly there; the trait - // surface no longer requires delegating accessors here. + // `first_loaded_at` lives on `WalletMetadata` and is read/written + // directly there; `WalletInfoInterface` has no accessors for it and + // `ManagedWalletInfo` has no backing methods, so nothing is + // delegated here. fn update_last_synced(&mut self, timestamp: u64) { self.core_wallet.update_last_synced(timestamp); diff --git a/packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs b/packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs index 629a7c574cc..33acecb64d3 100644 --- a/packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs +++ b/packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs @@ -406,10 +406,10 @@ impl PlatformWallet { /// `include_private` additionally requests the raw private scalar. /// /// The derivation delegates to key-wallet's gate-free provider-key - /// entry points (rust-dashcore #881) so every per-index key is - /// byte-identical to what `Wallet::from_mnemonic` account creation - /// produces; it never feeds a secp256k1 child scalar into a - /// BLS/Ed25519 master (the pre-#879 hybrid). + /// entry points so every per-index key is byte-identical to what + /// `Wallet::from_mnemonic` account creation produces; it never feeds a + /// secp256k1 child scalar into a BLS/Ed25519 master, which would yield + /// keys that differ from the ones the wallet's own accounts hold. /// /// # Errors /// - [`PlatformWalletError::AddressNotFound`] if this wallet has no diff --git a/packages/rs-platform-wallet/src/wallet/reservations.rs b/packages/rs-platform-wallet/src/wallet/reservations.rs index 3ee46a7a59f..fb4ddd7676b 100644 --- a/packages/rs-platform-wallet/src/wallet/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/reservations.rs @@ -13,8 +13,8 @@ //! that sweep is NOT what makes the ambiguous case safe. The inputs of a //! transaction that may be on the network are held by the generation's //! pending-spend fence, which the TTL does not touch and which no elapsed -//! quantity retires — only an observed spend does -//! (`dashpay/platform#4309`). Reservation cleanup here and fence settlement in +//! quantity retires — only an observed spend does. +//! Reservation cleanup here and fence settlement in //! the caller are two separate obligations; see //! [`release_reservation_after_rejected_broadcast`] for the order they must run //! in. @@ -62,16 +62,10 @@ pub(crate) const RESERVATION_MAX_AGE_BLOCKS: u32 = 20; // THERE IS DELIBERATELY NO TIMEOUT CONSTANT FOR THE BROADCAST INPUT FENCE. // -// An `IN_BROADCAST_FENCE_ORPHAN_TIMEOUT` used to live here: one hour on a -// monotonic `Instant`, after which the pending-spend phase of -// `WalletGeneration::pin_in_broadcast` released an outpoint the wallet had -// never observed spent. It was the fifth bound this fence was given and the -// fifth to be unsound (`dashpay/platform#4309` — three height-anchored forms in -// rounds 2-4, the monotonic one in rounds 5-6, all removed in round 7). -// -// The monotonic clock did fix what the height-anchored bounds got wrong — -// catch-up cannot fast-forward it. It did not fix the actual defect, which is -// that ELAPSED TIME IS NOT EVIDENCE. A signed transaction does not become +// Neither a height-anchored bound nor a monotonic-clock deadline is sound +// here. A height bound can be fast-forwarded by catch-up. A monotonic clock +// cannot, but it shares the real defect: ELAPSED TIME IS NOT EVIDENCE. A +// signed transaction does not become // invalid by getting older, and waiting does not prove no peer retained it: a // withholding DAPI endpoint can accept the transaction while keeping it off the // network, and a backgrounded mobile wallet can outlast any deadline worth @@ -121,11 +115,10 @@ pub(crate) const RESERVATION_MAX_AGE_BLOCKS: u32 = 20; /// `SignedPaymentError::WalletRemoved` before sampling the height, its /// `reconcile_removed_entry` release is itself generation-bound and no-ops on a /// missing wallet, and the finalized-transaction handle path runs after the -/// FFI layer's generation-identity check. The earlier claim that "the -/// wallet-mismatch / account-lookup paths already reject those cases" was wrong -/// for the registry broadcast path — `is_same_generation` compares handles (a -/// removed generation matches itself) and that path performs no account lookup -/// at all (`dashpay/platform#4185`). +/// FFI layer's generation-identity check. The wallet-mismatch and +/// account-lookup paths are not enough on their own for the registry broadcast +/// path: `is_same_generation` compares handles (a removed generation matches +/// itself) and that path performs no account lookup at all. pub(crate) fn reservation_expired(registered_height: u32, current_height: Option) -> bool { match current_height { Some(current) => current.saturating_sub(registered_height) >= RESERVATION_MAX_AGE_BLOCKS, @@ -206,12 +199,12 @@ pub(crate) async fn broadcast_releasing_on_rejection { /// key-wallet's TTL may sweep its reservation and a concurrent build /// re-reserve the same inputs under a new token before this entry is /// broadcast or released. Presenting this token to the owner-guarded release - /// frees only inputs still owned by this build, never the other build's - /// (`dashpay/platform#4185`). + /// frees only inputs still owned by this build, never the other build's. funding_reservation_token: Option, } @@ -290,7 +287,7 @@ impl SignedPaymentRegistry { /// `signed` is **consumed**, which is what enforces unique reservation /// ownership: `SignedCoreTransaction` is not `Clone`, so a single finalize /// can be registered at most once — there is no way to mint two live tokens - /// that name the same held reservation (`dashpay/platform#4185`). The built + /// that name the same held reservation. The built /// transaction, the funding account, the mandatory reservation height /// (`SignedCoreTransaction::reservation_height` — captured inside the /// funding critical section before the potentially-slow external signer ran, @@ -319,8 +316,8 @@ impl SignedPaymentRegistry { /// before its first poll and silently drop the consumed `signed` — and its /// held reservation — without inserting it. An `async fn` here would only /// move `signed` into a future whose body runs on the first poll; dropping - /// that future before polling would leak the reservation to key-wallet's TTL - /// (`dashpay/platform#4185`). Callers invoke it directly. + /// that future before polling would leak the reservation to key-wallet's TTL. + /// Callers invoke it directly. /// /// # Liveness is the caller's obligation /// @@ -416,8 +413,7 @@ impl SignedPaymentRegistry { // `StaleToken`), or it stays live until we leave. Shared, so concurrent // payments — on this generation and on every other — are unaffected, and // scoped per generation, so holding it across the network send below - // blocks only THIS wallet's teardown rather than every wallet's - // (`dashpay/platform#4185`). + // blocks only THIS wallet's teardown rather than every wallet's. // // Taking `current`'s gate rather than the entry's is sound because the // only path that proceeds past the check below is one where @@ -456,7 +452,7 @@ impl SignedPaymentRegistry { // handle broadcasts a removed wallet's payment onto the network, and the // teardown sweep cannot stop it: the sweep and the removal are one // linearization point, but a broadcast that entered the gate first is - // outside it (`dashpay/platform#4185`). + // outside it. // // The entry is already removed, so we drop it WITHOUT releasing — the // reservation ceased to exist with the generation, and a release by @@ -600,8 +596,8 @@ impl SignedPaymentRegistry { /// caller holds that generation's exclusive lifecycle gate across BOTH. /// Sweeping without it leaves two windows a payment operation slips through — /// a broadcast between the removal and this sweep still finds its entry, and - /// an in-flight finalizer registers a fresh token *after* this sweep has run - /// (`dashpay/platform#4185`). This function cannot take the gate itself: it + /// an in-flight finalizer registers a fresh token *after* this sweep has run. + /// This function cannot take the gate itself: it /// is synchronous, and the removal it must be atomic with is `async`. /// /// [`PlatformWalletManager::remove_wallet_with_teardown`](crate::PlatformWalletManager::remove_wallet_with_teardown) @@ -1139,7 +1135,7 @@ mod tests { ); } - /// Regression for `dashpay/platform#4185` blocker: registration must bind the + /// Invariant: registration must bind the /// token to the SAME wallet generation the payment was finalized against, not /// to a separately-supplied wallet. Registering a payment finalized through /// wallet A through an unrelated wallet B is refused up front with @@ -1314,9 +1310,9 @@ mod tests { // NOTE: the former `concurrent_registers_yield_distinct_tokens` test // registered sixteen clones of ONE reserved transaction to probe the token // allocator. That is exactly the duplicate-capability pattern unique - // ownership now forbids: `register` consumes a non-`Clone` + // ownership forbids: `register` consumes a non-`Clone` // `SignedCoreTransaction`, so a single reservation can be registered at most - // once (`dashpay/platform#4185`). Token distinctness is guaranteed by + // once. Token distinctness is guaranteed by // construction (the `AtomicU64` allocator), and concurrent consumption is // covered by `concurrent_broadcasts_serialize_to_one_send`. @@ -1566,11 +1562,9 @@ mod tests { ); } - // NOTE: the former `release_entries_for_wallet_frees_the_reservation` test - // is removed with the `release_entries_for_wallet` method it exercised. - // Destroying wrapper aliases no longer releases deferred-payment tokens: a - // wrapper handle does not own the payment, so its destruction must leave the - // token live and broadcastable (`dashpay/platform#4185`, blocker 2). Token + // NOTE: destroying wrapper aliases must not release deferred-payment + // tokens: a wrapper handle does not own the payment, so its destruction + // must leave the token live and broadcastable. Token // reservations are reconciled by the payment owner (explicit // broadcast/release) or dropped at actual generation teardown // (`remove_entries_for_wallet`). @@ -1827,7 +1821,7 @@ mod tests { .release_reservation(tx); } - /// Owner-guarded release regression (`dashpay/platform#4185`): a rejected + /// Owner-guarded release: a rejected /// deferred broadcast must free ONLY the inputs its own build still owns. If /// key-wallet's TTL swept this build's reservation and a concurrent build /// re-reserved the same outpoint under a new token, the rejection's release From ad2118e206c65bebf9e3aecd80c73d9a6c1d4628 Mon Sep 17 00:00:00 2001 From: Bartosz Rozwarski Date: Fri, 4 Sep 2026 12:33:51 +0200 Subject: [PATCH 2/3] docs(platform-wallet): correct two inaccurate claims found in review Both statements predate this branch and were carried forward while the surrounding comments were rewritten. Since the point of the branch is comments that tell the truth about the code, fix them here. `add_out_of_wallet_identity` claimed observed identities have an empty public-key map. `ManagedIdentity` has no key map of its own and `new_out_of_wallet` stores the supplied `Identity` unchanged, so an identity fetched from Platform keeps its public keys. State what actually distinguishes the bucket: `wallet_id` and `identity_index` are both `None`, which is what forces signing and derivation callers to handle it. `derive_provider_key_at_index` claimed the derivation delegates to key-wallet's gate-free provider-key entry points. That holds for the BLS operator and Ed25519 platform-node kinds only. secp256k1 has no such entry point, as the inline comment in the owner/voting arm already says: that arm reads the public side off the account xpub and derives the private side through `ExtendedPrivKey` and the account path. Co-Authored-By: Claude Opus 5 --- .../wallet/identity/state/manager/lifecycle.rs | 9 ++++++--- .../src/wallet/provider_key_at_index.rs | 15 ++++++++++----- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs b/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs index e326bf31409..c40ca540b42 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs @@ -95,9 +95,12 @@ impl IdentityManager { /// Add an identity to the out-of-wallet (observed read-only) bucket. /// /// There is no separate `WatchedIdentity` type; observed identities - /// are just `ManagedIdentity` rows with `wallet_id == None` and an - /// empty public-key map. If an identity with the same id is already - /// in either bucket this is a no-op. + /// are just `ManagedIdentity` rows with `wallet_id == None` and + /// `identity_index == None`, which is what forces signing and + /// derivation callers to handle them explicitly. The `Identity` is + /// stored exactly as supplied — an observed identity fetched from + /// Platform keeps the public keys it came with. If an identity with + /// the same id is already in either bucket this is a no-op. pub fn add_out_of_wallet_identity( &mut self, identity: Identity, diff --git a/packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs b/packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs index 33acecb64d3..9044fc23a5b 100644 --- a/packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs +++ b/packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs @@ -405,11 +405,16 @@ impl PlatformWallet { /// [`wallet_seed_bytes`](key_wallet::wallet::Wallet::wallet_seed_bytes). /// `include_private` additionally requests the raw private scalar. /// - /// The derivation delegates to key-wallet's gate-free provider-key - /// entry points so every per-index key is byte-identical to what - /// `Wallet::from_mnemonic` account creation produces; it never feeds a - /// secp256k1 child scalar into a BLS/Ed25519 master, which would yield - /// keys that differ from the ones the wallet's own accounts hold. + /// The BLS operator and Ed25519 platform-node kinds delegate to + /// key-wallet's gate-free provider-key entry points, which consume the + /// raw seed directly. secp256k1 has no such entry point, so the owner + /// and voting kinds take the public side off the account xpub and + /// derive the private side inline: raw seed to master xpriv to the + /// account's own DIP-3 path to a non-hardened child at `index`. Either + /// route yields a key byte-identical to what `Wallet::from_mnemonic` + /// account creation produces, and neither feeds a secp256k1 child + /// scalar into a BLS/Ed25519 master, which would yield keys that + /// differ from the ones the wallet's own accounts hold. /// /// # Errors /// - [`PlatformWalletError::AddressNotFound`] if this wallet has no From e6a59f9964312721b888c02b7da2cb50dfb8fac8 Mon Sep 17 00:00:00 2001 From: Bartosz Rozwarski Date: Fri, 4 Sep 2026 12:38:13 +0200 Subject: [PATCH 3/3] docs(platform-wallet): repair two comments this branch carried forward broken Both defects predate the branch and survived the cleanup pass unnoticed. `remove_wallet` said the two maps, the returned handle and the `tear_down` argument "are then all that one generation", which has no verb. They name that generation. Rewrapped the paragraph, which had been left ragged after an issue reference was removed from the middle of it. The `AssetLockManager::persister` field doc pointed at `queue_persist`, which is a method on `PlatformWallet`, not on this manager. The manager queues through `queue_asset_lock_changeset`, which is what stores the changeset through this handle. Co-Authored-By: Claude Opus 5 --- .../rs-platform-wallet/src/manager/wallet_lifecycle.rs | 9 ++++----- .../rs-platform-wallet/src/wallet/asset_lock/manager.rs | 3 ++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index c844c1b9e7b..2de7cc831fa 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -712,11 +712,10 @@ impl PlatformWalletManager

{ /// The `Arc` validated under the gate is therefore retained, /// and the public-map entry is removed only while it still names that same /// generation. Both maps, the returned handle and the `tear_down` argument - /// are then all that one generation. The one - /// remaining id-keyed step is the shielded coordinator detach below, which - /// has no generation concept at all; a generation that has just been - /// registered has not run `bind_shielded` yet, so it holds no coordinator - /// entry to detach. + /// then all name that one generation. The one remaining id-keyed step is + /// the shielded coordinator detach below, which has no generation concept + /// at all; a generation that has just been registered has not run + /// `bind_shielded` yet, so it holds no coordinator entry to detach. /// /// The inner-manager removal needs no such check: G1 can only leave /// `wallet_manager` through this method (which requires G1's gate, held here) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs index e5eb64ba719..86e6143d141 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs @@ -53,7 +53,8 @@ pub struct AssetLockManager { /// boundary without round-tripping through the parent wallet. /// /// Invariant: no mutation drops its `AssetLockChangeSet` — every - /// emitted changeset flows straight into `queue_persist` here. + /// emitted changeset goes straight to `queue_asset_lock_changeset`, + /// which stores it through this handle. pub(super) persister: WalletPersister, /// Serializes the funding-index-critical section of /// [`broadcast_funded_asset_lock`](Self::broadcast_funded_asset_lock) —