Skip to content

fix(platform-wallet): snapshot identity cross-check + typed persister-load errors (#3980 follow-ups) #3984

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lklimek merged 6 commits into from
Jul 2, 2026
+303 −25
Merged

fix(platform-wallet): snapshot identity cross-check + typed persister-load errors (#3980 follow-ups) #3984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e66fbf5
fix(platform-wallet): preserve persisted wallet state through seedles…
QuantumExplorer Jul 2, 2026
51cd9c9
docs(platform-wallet): add normative persistence architecture to README
QuantumExplorer Jul 2, 2026
9879a9e
fix(platform-wallet): distinguish wrong-row snapshots with SnapshotId…
lklimek Jul 2, 2026
8999eea
test(platform-wallet): cover snapshot-mismatch skip alongside a healt…
lklimek Jul 2, 2026
161d8b8
feat(platform-wallet): propagate typed persister-load errors
lklimek Jul 2, 2026
40806d6
merge: retarget onto squashed #3980 base (feat/platform-wallet-rehydr…
lklimek Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/rs-platform-wallet-ffi/src/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,10 @@ pub const LOAD_SKIP_REASON_MALFORMED_XPUB: u32 = 101;
/// `reason_code`: any other structural decode / projection failure on
/// the persisted row.
pub const LOAD_SKIP_REASON_DECODE_ERROR: u32 = 102;
/// `reason_code`: the carried managed-info snapshot does not describe its
/// persisted row (wallet_id/network differ, or its account set diverges
/// from the row's account manifest) — a wrong-row snapshot.
pub const LOAD_SKIP_REASON_SNAPSHOT_IDENTITY_MISMATCH: u32 = 103;
/// `reason_code`: an unrecognized `CorruptKind` — forward-compat
/// fallback until this crate maps a newly added corrupt-row family.
pub const LOAD_SKIP_REASON_CORRUPT_OTHER: u32 = 199;
Expand All @@ -203,6 +207,7 @@ pub struct SkippedWalletFFI {
/// constants: [`LOAD_SKIP_REASON_MISSING_MANIFEST`] (100),
/// [`LOAD_SKIP_REASON_MALFORMED_XPUB`] (101),
/// [`LOAD_SKIP_REASON_DECODE_ERROR`] (102),
/// [`LOAD_SKIP_REASON_SNAPSHOT_IDENTITY_MISMATCH`] (103),
/// [`LOAD_SKIP_REASON_CORRUPT_OTHER`] (199), or
/// [`LOAD_SKIP_REASON_OTHER`] (200). No secret material is ever
/// carried.
Expand Down Expand Up @@ -234,6 +239,7 @@ fn skip_reason_code(reason: &platform_wallet::SkipReason) -> u32 {
platform_wallet::SkipReason::CorruptPersistedRow { kind } => match kind {
CorruptKind::MissingManifest => LOAD_SKIP_REASON_MISSING_MANIFEST,
CorruptKind::MalformedXpub => LOAD_SKIP_REASON_MALFORMED_XPUB,
CorruptKind::SnapshotIdentityMismatch => LOAD_SKIP_REASON_SNAPSHOT_IDENTITY_MISMATCH,
CorruptKind::DecodeError(_) => LOAD_SKIP_REASON_DECODE_ERROR,
// `CorruptKind` is #[non_exhaustive]; a future variant maps to a
// generic corrupt-row code until this mapping is extended.
Expand Down Expand Up @@ -582,6 +588,7 @@ mod tests {
assert_eq!(LOAD_SKIP_REASON_MISSING_MANIFEST, 100);
assert_eq!(LOAD_SKIP_REASON_MALFORMED_XPUB, 101);
assert_eq!(LOAD_SKIP_REASON_DECODE_ERROR, 102);
assert_eq!(LOAD_SKIP_REASON_SNAPSHOT_IDENTITY_MISMATCH, 103);
assert_eq!(LOAD_SKIP_REASON_CORRUPT_OTHER, 199);
assert_eq!(LOAD_SKIP_REASON_OTHER, 200);
}
Expand All @@ -600,6 +607,10 @@ mod tests {
skip_reason_code(&corrupt(CorruptKind::MalformedXpub)),
LOAD_SKIP_REASON_MALFORMED_XPUB
);
assert_eq!(
skip_reason_code(&corrupt(CorruptKind::SnapshotIdentityMismatch)),
LOAD_SKIP_REASON_SNAPSHOT_IDENTITY_MISMATCH
);
assert_eq!(
skip_reason_code(&corrupt(CorruptKind::DecodeError("boom".into()))),
LOAD_SKIP_REASON_DECODE_ERROR
Expand Down
12 changes: 12 additions & 0 deletions packages/rs-platform-wallet/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,18 @@ pub enum PlatformWalletError {
#[error("Wallet creation failed: {0}")]
WalletCreation(String),

/// The persister failed to load the client start state during
/// rehydration. Carries the typed [`PersistenceError`] so callers keep
/// its retry classification (`is_transient()` /
/// [`PersistenceErrorKind`]) instead of a flattened string — a
/// transient backend hiccup (e.g. `SQLITE_BUSY`) stays distinguishable
/// from a permanent failure and can be retried.
///
/// [`PersistenceError`]: crate::changeset::PersistenceError
/// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind
#[error("failed to load persisted client state: {0}")]
PersisterLoad(#[from] crate::changeset::PersistenceError),

/// The persisted wallet has UTXOs to restore but no funds-bearing
/// account in its reconstructed account collection to hold them.
/// Fail-closed rather than reconstructing a silent zero balance —
Expand Down
72 changes: 54 additions & 18 deletions packages/rs-platform-wallet/src/manager/load.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,7 @@ impl<P: PlatformWalletPersistence + 'static> PlatformWalletManager<P> {
// not here — drop the snapshot at this entry point.
#[cfg(feature = "shielded")]
shielded: _,
} = self.persister.load().map_err(|e| {
PlatformWalletError::WalletCreation(format!(
"Failed to load persisted client state: {}",
e
))
})?;
} = self.persister.load()?;

let persister_dyn: Arc<dyn PlatformWalletPersistence> = Arc::clone(&self.persister) as _;

Expand Down Expand Up @@ -169,19 +164,17 @@ impl<P: PlatformWalletPersistence + 'static> PlatformWalletManager<P> {
// skips a second eager gap-window derivation.
Some(info) => {
let mut info = *info;
// The snapshot must describe this row's wallet; a
// mismatch is a corrupt row, skipped like any other
// structural failure.
if info.wallet_id != expected_wallet_id || info.network != network {
// The snapshot must describe this row's wallet and its
// account set must agree with the manifest that built
// the watch-only wallet above. Either mismatch is a
// wrong-row snapshot — skipped like any structural
// failure, kept distinct from unreadable bytes.
if info.wallet_id != expected_wallet_id
|| info.network != network
|| !snapshot_accounts_match_manifest(&info, &account_manifest)
{
let reason = SkipReason::CorruptPersistedRow {
kind: CorruptKind::DecodeError(format!(
"managed-info snapshot (wallet {}, network {:?}) does not \
match its row (wallet {}, network {:?})",
hex::encode(info.wallet_id),
info.network,
hex::encode(expected_wallet_id),
network,
)),
kind: CorruptKind::SnapshotIdentityMismatch,
};
outcome.skipped.push((expected_wallet_id, reason.clone()));
self.event_manager
Expand Down Expand Up @@ -328,3 +321,46 @@ impl<P: PlatformWalletPersistence + 'static> PlatformWalletManager<P> {
Ok(outcome)
}
}

/// Whether the snapshot's account set matches the row's account manifest.
///
/// The manifest is the account-set oracle used to build the watch-only
/// wallet; a snapshot carrying a different set of account types describes
/// a different wallet and must not be consumed.
///
/// The manifest is enumerated from `Wallet::all_accounts` (ECDSA-only:
/// carries `PlatformPayment`, omits the BLS `ProviderOperatorKeys` /
/// EdDSA `ProviderPlatformKeys`); the snapshot from
/// `ManagedWalletInfo::all_managed_accounts` (the mirror: carries the
/// BLS/EdDSA provider keys, omits `PlatformPayment`). Comparison is
/// restricted to the families both enumerations can carry so this known
/// asymmetry never rejects a legitimate snapshot.
fn snapshot_accounts_match_manifest(
info: &ManagedWalletInfo,
manifest: &[crate::changeset::AccountRegistrationEntry],
) -> bool {
use key_wallet::account::AccountType;
use std::collections::BTreeSet;

fn comparable(t: &AccountType) -> bool {
!matches!(
t,
AccountType::ProviderOperatorKeys
| AccountType::ProviderPlatformKeys
| AccountType::PlatformPayment { .. }
)
}

let manifest_types: BTreeSet<AccountType> = manifest
.iter()
.map(|e| e.account_type)
.filter(comparable)
.collect();
let snapshot_types: BTreeSet<AccountType> = info
.all_managed_accounts()
.iter()
.map(|a| a.managed_account_type().to_account_type())
.filter(comparable)
.collect();
manifest_types == snapshot_types
}
Comment on lines +338 to +366

@thepastaclaw thepastaclaw Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Snapshot/manifest cross-check has defense-in-depth gaps in filtered families and empty-set case

snapshot_accounts_match_manifest compares only the set of AccountType variants after filtering out ProviderOperatorKeys, ProviderPlatformKeys, and PlatformPayment { .. } (line 345-352). The filter is justified — those are the known enumeration asymmetries between Wallet::all_accounts and ManagedWalletInfo::all_managed_accounts — but three residual gaps remain in the very scenario this function was added to defend against (a wrong-row / tampered snapshot):

  1. xpubs are not compared. info.wallet_id is a persisted field, not a re-derivation. A row with matching wallet_id/network/account-type set but attacker-controlled per-account xpubs will pass, and wallet_info = *info then consumes those pools into the SPV watch set while build_watch_only_wallet uses the manifest's xpubs — divergent detection/attribution surfaces without any signing-side failure.
  2. Asymmetric filtering hides side-specific extras. PlatformPayment is filtered on the manifest side (where it can appear) and ProviderOperatorKeys/ProviderPlatformKeys on the snapshot side (where they can appear). A snapshot with an extra ProviderOperatorKeys the manifest wouldn't attest to, or a manifest with an extra PlatformPayment the snapshot wouldn't attest to, is invisible because each is filtered on exactly the side that would have caught it.
  3. Vacuous empty-set match. If a row's manifest reduces to only filtered families (e.g. a single PlatformPayment entry — build_watch_only_wallet only requires non-empty) and the snapshot's comparable set is also empty, ∅ == ∅ returns true and any wrong-row snapshot with wallet_id/network set to match sails through. Unlikely on real wallets (which always carry a Standard account), but silently defeats the guard when it occurs.

All three sit behind the documented trust boundary (the persister is trusted as-is, tracked follow-up for cryptographic manifest binding to wallet_id), so none is a new hole opened by this PR — but each undermines the specific narrowing this PR claims to add. Consider requiring at least one comparable account on both sides, comparing per-account xpubs (or a hash) for the shared families, and rejecting snapshot-only families that aren't in the manifest.

source: ['claude']

@thepastaclaw thepastaclaw Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still present at 40806d6.

11 changes: 11 additions & 0 deletions packages/rs-platform-wallet/src/manager/load_outcome.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ pub enum CorruptKind {
/// One or more manifest `account_xpub` bytes failed to parse as a
/// well-formed extended public key.
MalformedXpub,
/// The carried [`ManagedWalletInfo`] snapshot does not describe the
/// persisted row it is attached to: its `wallet_id`/`network` differ
/// from the row, or its account set diverges from the row's account
/// manifest. This is a wrong-row/structurally-inconsistent snapshot —
/// distinct from unreadable bytes ([`Self::DecodeError`]).
///
/// [`ManagedWalletInfo`]: key_wallet::wallet::managed_wallet_info::ManagedWalletInfo
SnapshotIdentityMismatch,
/// Any other structural decode / projection failure surfaced by the
/// persister. The string is a structural projection — never a raw
/// row byte slice or a hex-encoded key.
Expand All @@ -53,6 +61,9 @@ impl std::fmt::Display for CorruptKind {
match self {
Self::MissingManifest => f.write_str("missing account manifest"),
Self::MalformedXpub => f.write_str("malformed account xpub"),
Self::SnapshotIdentityMismatch => {
f.write_str("snapshot does not match its persisted row")
}
Comment on lines 44 to +66

@thepastaclaw thepastaclaw Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: SnapshotIdentityMismatch collapses three distinct failure modes into one undifferentiated variant

The new SnapshotIdentityMismatch variant is a dataless enum tag, and load.rs:172-175 sets it uniformly for all three branches of the || guard: wallet_id mismatch, network mismatch, or account-set divergence. Before this PR, a wallet_id/network mismatch produced a formatted DecodeError string that named exactly which field diverged and with what values, readable directly from the on_wallet_skipped_on_load log line. Now the FFI log (manager.rs:429-432, unchanged) records only wallet_id + the static string "snapshot does not match its persisted row". An operator triaging a skipped row from logs can no longer tell whether wallet_id, network, or the account set tripped the guard without reproducing locally. The variant-vs-string trade-off is otherwise fine (variants are match-able and stable) — worth carrying at least a small discriminant enum (WalletId/Network/AccountSet) so the check that failed remains observable without leaking snapshot bytes.

source: ['claude']

@thepastaclaw thepastaclaw Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still present at 40806d6.

Comment on lines 44 to +66

@thepastaclaw thepastaclaw Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: SnapshotIdentityMismatch collapses three failure modes and loses diagnostic granularity

Carried forward from the prior review (161d8b8) — verified byte-identical at HEAD. CorruptKind::SnapshotIdentityMismatch is a dataless variant (line 52) and load.rs:172-183 sets it uniformly across all three branches of the || guard: wallet_id mismatch, network mismatch, and account-set divergence. Before this PR, a wallet_id/network mismatch produced a formatted DecodeError string naming exactly which field diverged and its two values. Now the Display impl (lines 64-66) always renders the identical static string "snapshot does not match its persisted row", and the FFI log (rs-platform-wallet-ffi/src/manager.rs) records only wallet_id + this static string. An operator triaging a skipped row from logs alone can no longer tell whether wallet_id, network, or the account set tripped the guard without reproducing locally.

The enum-over-string design is otherwise the right call (match-able and does not leak snapshot bytes) — carrying a small discriminant sub-enum (WalletId/Network/AccountSet) as data on the variant would preserve which check failed without regressing on the secrecy goal.

source: ['claude']

Self::DecodeError(s) => write!(f, "decode error: {s}"),
}
}
Expand Down
Loading
Loading