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..2de7cc831fa 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,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 (`dashpay/platform#4185`). 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) @@ -1138,25 +1135,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 +1294,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..86e6143d141 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,9 @@ 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 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) — 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..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 @@ -94,12 +94,13 @@ 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 + /// `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/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..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 (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). + /// 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 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