Repository navigation
feat(platform-wallet)!: update an identity at a revision the caller names - #5487
romchornyi wants to merge 67 commits into
Conversation
An identity update carries the cached identity's revision plus one, read when the update starts. A caller that records the revision before it submits, to tell later whether the update can still land (Drive takes an identity update only at one above the identity's revision), cannot see that read, and a refresh of the cache in between moves it. `update_identity_with_external_signer_at_revision` takes the revision the transition must carry and fails with `IdentityRevisionMismatch` before signing when the cache would give it another one, so the recorded revision is the only one the update can land at. The existing entry point passes `None` and behaves as before. Exposed as `platform_wallet_update_identity_with_signer_at_revision` in the FFI (a new symbol, so the Kotlin JNI binding is untouched) and as `ManagedPlatformWallet.updateIdentity(..., revision:)` in Swift. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…landed at After a broadcast the cache took its own revision plus one. A refresh between the snapshot and that apply had already stored the landed revision, so the cache went one past the chain, and the next update asked for at that revision would be refused by Drive. It now takes the higher of what it holds and the transition's revision. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
⛔ Final review complete — 3 blocking finding(s) (commit d415e70) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification found no blocking defects: the revision check precedes signing and broadcast, and the cache revision reconciliation avoids incrementing an already-refreshed revision. Two regression-coverage suggestions and one FFI documentation nit remain. Current Rust wallet, Swift SDK, and Kotlin SDK checks pass; PR Hygiene remains pending, and no local builds or tests were run.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change adds revision preconditions and concurrent cache-update handling across Rust, C FFI, and Swift, but is contained and does not meet the large-or-intricate requirement for critical. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:267-270: Add regression coverage for post-confirmation revision reconciliation
The three new tests exercise only `update_revision`, not this separate cache reconciliation fix. Replacing this assignment with the previous `cached_revision + 1` would leave those tests passing while restoring the concurrent-refresh bug. Add coverage of the local-apply path for a submitted transition at revision 5 when the cache remains at 4, has refreshed to 5, or has advanced to 6, asserting final revisions of 5, 5, and 6 respectively. Exercise the production local-apply path through a mocked broadcast or an extracted helper used by that path.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:399-409: Exercise revision rejection through the public async method
The helper tests do not exercise `update_identity_with_external_signer_at_revision`, so they would still pass if the public method stopped forwarding the requested revision or moved validation after signing or broadcast. Add an async regression test using a cached identity, a recording signer, and an SDK mock with no network expectations; assert the structured mismatch fields, zero signer calls, and unchanged cached state. Cover both mismatch directions and assert `(expected, actual)` explicitly, including `(5, 4)` for the cache-behind case, which currently checks only the error variant. The crate already enables SDK mocks in its dev-dependencies, and `network/signing_key.rs` demonstrates local wallet construction with `SdkBuilder::new_mock()`.
In `packages/rs-platform-wallet-ffi/src/identity_update.rs`:
- [NITPICK] packages/rs-platform-wallet-ffi/src/identity_update.rs:327-334: Document the new FFI entry point's pointer safety contract
The new public unsafe entry point documents revision behavior but not the caller obligations for its raw pointers. Its shared implementation reads the identity buffer, constructs slices from the supplied arrays, reads nested public-key buffers, and borrows the signer across worker execution. Add a `# Safety` section specifying readable lengths and alignment for nonempty arrays and nested buffers, a valid signer handle from the SDK's signer constructors, and the required lifetimes through completion of the call. The neighboring credit-transfer entry point provides a useful model. This is a documentation gap, not an ownership-layer violation; the crate explicitly allows `clippy::missing_safety_doc`, so CI does not enforce it.
|
@coderabbitai review No review for |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
Bots are done — your move: post |
…te cache step - Move the post-broadcast local apply into `apply_landed_update` and test it: a transition at 5 leaves a cache at 4, 5 or 6 at 5, 5 and 6, for an add-only and a disable-only update. The revision step now runs for every landed update, not only one that adds or disables keys. - Test `update_identity_with_external_signer_at_revision` itself on the mock SDK: a mismatch in either direction returns the structured (expected, actual), never consults the signer and leaves the cache unchanged; at the matching revision the same call reaches the signer and the SDK. - Document the pointer, signer and threading contract of the identity update FFI entry points, and correct the stale cache note on `update_identity_with_external_signer`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The revision preflight is correctly ordered, the public-path tests cover both mismatch directions, and all three prior findings are fixed. Two blocking cache races remain: newer cached key state can be overwritten by an older landed transition payload, and an older refresh can overwrite the locally applied landed update. The Rust workspace test check was still in progress in the supplied CI snapshot.
🔴 2 blocking | 🟡 1 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Prevent a stale refresh from undoing the landed cache update
packages/rs-platform-wallet/src/wallet/identity/network/loading.rs:339-365
refresh_identity fetches outside the wallet-manager lock and then unconditionally assigns the fetched identity under the write lock. An older fetch can therefore complete after apply_landed_update: it may fetch revision 4, the update may land at revision 5 and update the cache, and the refresh can then restore revision 4 and the old key set. A subsequent revision-qualified update sees the wrong cached revision and may build a duplicate revision-5 transition instead of refusing or submitting the intended revision. Apply fetched identities monotonically against the current cached revision, preserving newer key and persistence state, or serialize refresh application with landed-update reconciliation. The same policy should cover the other fetched-identity writer in load_identity_by_index_inner.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)
🟡 Suggestion: Preserve revision mismatch as a typed FFI error
packages/rs-platform-wallet-ffi/src/error.rs:1125-1137
IdentityRevisionMismatch is the expected, pre-broadcast result of the new revision-qualified API, but the blanket FFI conversion falls through to ErrorUnknown. Mobile callers therefore cannot reliably distinguish an expected stale-cache/preflight mismatch from an SDK failure or another unmapped error without parsing the rendered message, and the expected/actual fields are lost as structured data. Append a dedicated FFI result code and mirror it in the Swift and Kotlin result-code mappings so callers can safely discard or reconcile the recorded revision.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The change is large and intricate and modifies identity update signing, key-handling, and broadcast behavior in packages/rs-platform-wallet/src/wallet/identity/network/update.rs and its FFI/Swift boundaries. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:289-297: Do not apply an older transition payload over newer cached identity state
`apply_landed_update` makes only the revision monotonic. It still applies `added_keys` and `disabled_ids` unconditionally, even when `cached_revision` is already greater than `transition_revision`. For example, an update at revision 5 can add key 3, a later update can land at revision 6 and disable key 3, and the cache can be refreshed to revision 6 before the revision-5 confirmation applies. The `add_key` call then restores the revision-5 active key and emits an upsert that can overwrite the newer persisted key row, so the wallet may select a disabled key for signing despite reporting revision 6. Apply the transition payload only when it is not older than the cached identity, or revision-guard each key mutation and its persistence changeset.
In `packages/rs-platform-wallet/src/wallet/identity/network/loading.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/loading.rs:339-365: Prevent a stale refresh from undoing the landed cache update
`refresh_identity` fetches outside the wallet-manager lock and then unconditionally assigns the fetched identity under the write lock. An older fetch can therefore complete after `apply_landed_update`: it may fetch revision 4, the update may land at revision 5 and update the cache, and the refresh can then restore revision 4 and the old key set. A subsequent revision-qualified update sees the wrong cached revision and may build a duplicate revision-5 transition instead of refusing or submitting the intended revision. Apply fetched identities monotonically against the current cached revision, preserving newer key and persistence state, or serialize refresh application with landed-update reconciliation. The same policy should cover the other fetched-identity writer in `load_identity_by_index_inner`.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:1125-1137: Preserve revision mismatch as a typed FFI error
`IdentityRevisionMismatch` is the expected, pre-broadcast result of the new revision-qualified API, but the blanket FFI conversion falls through to `ErrorUnknown`. Mobile callers therefore cannot reliably distinguish an expected stale-cache/preflight mismatch from an SDK failure or another unmapped error without parsing the rendered message, and the `expected`/`actual` fields are lost as structured data. Append a dedicated FFI result code and mirror it in the Swift and Kotlin result-code mappings so callers can safely discard or reconcile the recorded revision.
…let-update-at-revision
… and fetches A landed update whose transition is older than the cached identity (the cache was refreshed past it) no longer applies its added and disabled keys: the cache already holds them and what later updates did to them, and the payload's upserts could re-enable a key a newer update had disabled. A fetched identity at a lower revision than the cache's no longer replaces it, in refresh_identity and in the index loader. Such a fetch was answered before an update the cache already took, and could put the cache back to the pre-update revision and keys. The loader still adds its breadcrumbs, riding on the cache's newer keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sult code IdentityRevisionMismatch is the expected refusal of the revision-qualified update, but it reached hosts as ErrorUnknown, so they could only tell it from an SDK failure by its text. It now maps to ErrorIdentityRevisionMismatch (62; 61 is held by open #5395), mirrored as a typed error in Swift (identityRevisionMismatch) and Kotlin (PlatformWallet.IdentityRevisionMismatch), each with a pin test, and recorded in the error-code registry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… lands A landed update whose cache was refreshed to its revision or past it skipped the payload but stored nothing: a refresh stores no keys, so the added keys lost their derivation breadcrumbs and the disabled keys never reached the store. The touched keys are now stored from the cache's own copies (on-chain disabled_at included), with their breadcrumbs. A cache at exactly the transition's revision is treated the same way, since the refresh already holds the payload as Platform has it. The tests read what reached the store, and the fixture takes a store and an SDK for that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ch site Discovery layered the keys of a fetch onto a known identity without looking at the cache's revision, so an older discovery answer could still re-enable a key a newer update had disabled. It now carries its breadcrumbs on the cache's keys, through the same helper the index loader uses (ManagedIdentity::key_decisions_on_cached_keys). When a landed update meets a cache at or past its revision, a disabled key's row is stored without a breadcrumb, which keeps the one it holds instead of writing an unverified one; a store failure for disabled keys only is logged, as disable_keys does, and an added key missing from the cache is logged. The updateIdentity(revision:) doc names the typed identityRevisionMismatch error, and take_fetched_identity says what the revision does not order (key limits, the balance). Tests: the index loader keeps a newer cache in memory and in what it stores, and the refresh test tells a taken answer from a dropped one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…adcrumb When a landed update meets a cache at or past its revision, a disabled key is now stored with the breadcrumb disable_keys gives it (the wallet slot's), not none: the SQLite store replaces a key's whole record, and a buffered changeset merge keeps the last upsert, so a row stored without a breadcrumb loses the one it had. Whether a store failure is an error now follows whether the batch held an added key, not whether one was asked. Discovery takes a newer fetch of a known identity as the loader and refresh_identity do, so its revision moves with the keys it lays on, and it folds before persisting the snapshot. The tests share one capturing store and an armable failing one, and cover a store failure for disabled keys only and for an added key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Platform never removes a key, so a key in an older fetch that the cache lacks is on the identity: carrying the fetch's breadcrumbs on the cache's keys now keeps the fetch's copy of such a key instead of dropping it. The same holds for a landed update meeting a cache at its revision that lacks an added key: the update's copy is stored and taken into the cache, and a store failure for it is an error. Discovery leaves a known identity's revision and balance alone again, as before this PR, and only rebases an older fetch's breadcrumbs: taking an equal-revision fetch whole could put back an older balance under a newer balance watermark. The loader skips the fold for an identity it just added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a load's fetch is older than the cache, the loader returned the cache as it was before storing the fetch's keys, so a key the fetch had and the cache lacked went into the cache and the store but not into what the call returned. It now returns the cache after storing them. The fold rule lives in one method, ManagedIdentity::key_decisions_for_fetch, used by the loader and discovery; the wallet-slot breadcrumb comes from ManagedIdentity::wallet_slot, shared with disable_keys. A disabled key id the cache lacks is logged on the at-or-past path as disable_keys logs it, and the docs say an added key the cache lacks is taken from the update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
Thanks for the trim and for the table, that is exactly what I was after. Reviewed 693fd17 against d4750290 (merge-base). I ran cargo test -p platform-wallet --all-features --lib and cargo test -p platform-wallet-storage locally on this head: platform-wallet 1645 passed, platform-wallet-storage 554 + 5 integration passed, 0 failures.
Verdict
The revision pin is correct and well covered. The fold is a real fix for a reproduced problem and reads cleanly now that it's one rule (fold_fetched_key) shared by refresh, load and discovery. I have one thing I'd like fixed before merge, and it's the same thing the bot keeps flagging, just with a much smaller fix than it asks for. The rest is non-blocking.
Blocking: inflated legacy revision has no exit
The +1 bump in the old apply_landed_update (9532196a) is on master and v4.1-dev, so it's in shipped wallets. Your own table says the "fetch between snapshot and bump" race reproduces in a realistic flow. So some devices can hold revision = chain + 1 today. On base that heals on the next refresh. On this branch every fetch is now "older", the cache keeps its revision forever, and every update of that identity fails: unpinned builds at chain + 2 and Drive rejects it, pinned at chain + 1 fails locally with IdentityRevisionMismatch. For DashConnect that means that identity can never register another login key. Rare, but permanent, and this PR is what makes it permanent.
I don't think it needs the provenance work in #5542. Drive already tells us the truth: InvalidIdentityRevisionError carries current_revision, and the SDK surfaces it as dash_sdk::Error::StateTransitionBroadcastError { cause: Some(ConsensusError::StateError(StateError::InvalidIdentityRevisionError(e))) }. In update_identity_with_external_signer_at_revision, on that error: set the cached revision to e.current_revision(), store the scalar snapshot, and return the error (a dedicated variant would be nicer, but even the existing mapping is fine as long as the cache is corrected). ~25 lines, one test with the mock SDK returning that consensus error, and the lock is gone: the second attempt builds at the right revision. It also covers any future way the cache can get ahead, which the fold alone cannot.
If you'd rather not touch the error path in this PR, I'd accept a take_fetched_identity exception instead: a fetch whose proof height is above the cache's last_synced_keys_block_time by more than N blocks is taken whole regardless of revision. But the Drive-error route is simpler and authoritative.
Non-blocking
update.rs: every failure ofbroadcast_and_waitis mapped toInvalidIdentityData("Failed to broadcast …"), so a caller cannot tell "rejected before broadcast" from "broadcast, proof wait timed out". The doc comment now says "the update may have landed … refresh before the next update", which is the right advice, but the caller has to apply it to every error. A distinct variant for the post-broadcast failure would make the feature's own use case (record first, reconcile later) cleaner. Fine as a follow-up; worth an issue so it isn't lost.apply_landed_update, cache-ahead branch: an added key the cache lacks is stored with the inferred breadcrumb (identity_index, key.id()). Same inference as base and as the behind branch, so consistent, and #5540 owns the fix. Just noting it so nobody reads this branch as the verified path.store_fetched_identity(refresh) emits rows withwallet_id: None, derivation_indices: Noneand relies on every persister keeping a held breadcrumb. SQLite now does, Swift and Kotlin already did. The updatedIdentityKeyEntrydoc states the contract. Good; this was the one place a refresh could have made a wallet key watch-only.loading.rs/discovery.rs:add_identity_unstoredthen one changeset. If that store fails the identity is in memory with no rows and the next refresh only stores changed keys (#5542 item 2). Acceptable, base had the same gap in a different shape.- The bot's "Core-managed identities can't re-enable keys" finding is moot: masternode identities stay at revision 0, a revision-0 fetch over a revision-0 cache is
>=, taken whole, re-enable included. Your doc comment already says so. - The bot's "stale refresh undoes landed update via equal-keys" finding is also moot since
0fbfba3freverted that exception. Both can be dismissed. - JNI:
hasRevision: Boolean, revision: Longis a reasonable way to pass anOption<u64>; the negative check is right. SwiftwithExtendedLifetime(signer)fix is correct. - Tests:
lagging_fetch_tests.rsandsqlite_identity_key_restore.rsare the right kind of test for this change (real fetch paths, real SQLite,SQLITE_FULL). Nice.
On dismissing the two remaining bot findings
Finding 2 (#5542 item 5, SQLite can't clear a legacy inferred breadcrumb): dismiss. Cosmetic by your own analysis, pubkey binding refuses the signature, and #5540 / #5525 own it.
Finding 1 (#5542 item 1, inflated revision): don't dismiss, fix it with the Drive-error recovery above. It's the one case where this PR turns a self-healing bug into a permanent one, and the fix is small.
… for its revision A cache one past the chain, which a build before this branch could leave (a fetch answered between an update's snapshot and its bump), no longer heals from a fetch: every fetch comes back older and keeps the cache's revision, so every update of that identity would be refused for good. Drive refuses such an update with InvalidIdentityRevisionError, which carries the revision it holds. On that refusal the cached revision moves down to Drive's and is stored with the scalar snapshot, so the next update carries the right one. It is lowered only while the cache is still at the revision the refused transition was built from, and never raised: the error comes from one node without a proof, so a wrong value can get later updates refused until a fetch, but cannot lock the cache ahead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
|
@llbartekll thanks, done in 2b8ba90, the Drive-error route.
Finding 2 is answered and resolved as you suggested. I also added the post-broadcast error variant to #5542, so it isn't lost. The full set is 3220 pass, with |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Two blocking issues remain. The new recovery path persists a lower identity revision taken from an unauthenticated DAPI consensus-error payload, so a malicious or stale endpoint can roll back a confirmed cache and enable subsequent stale-key reconciliation. In addition, the current CI Kotlin/native build fails because the new extractor calls the removed consensus_error_of helper; the base branch now exposes dash_sdk::Error::consensus_error().
🔴 2 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The change is intricate and directly alters identity key-state reconciliation and derivation-metadata preservation in managed_identity/identity_ops.rs and sqlite/schema/identity_keys.rs, alongside revision-pinned signing and update handling in network/update.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:406-425: Do not lower the cached revision from an unauthenticated DAPI error
`take_rejected_revision` treats `rejection.current_revision()` as authoritative and persists it whenever the cache still equals the submitted transition's starting revision. That value is not proof-authenticated: `broadcast_and_wait` returns `StateTransitionBroadcastError` immediately when the wait response contains an error, before the quorum proof is verified, and the SDK also exposes consensus errors from transport/check-tx responses. A malicious or compromised DAPI can therefore report an arbitrarily low revision for the target identity; the wallet lowers its confirmed cache while retaining newer keys, and a subsequent older fetch can pass the lowered revision gate and replace or fold stale key state. Reconcile only from a proof-verified identity/finality result or retain the refusal as an error without mutating the cache. Add a regression starting with a correctly confirmed cache, an unverified lower-revision refusal, and an older fetch.
In `packages/rs-platform-wallet/src/error.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/error.rs:1291-1298: Use the SDK-owned consensus accessor in the revision matcher
The new `as_identity_revision_invalid` function calls `consensus_error_of(error)`, but current `v5.1-dev` removed that wallet-local helper in commit `033ecaee2f3badc8e04ff8be664062f33516e3e4` and moved the traversal to `dash_sdk::Error::consensus_error()`. The current Kotlin/native CI build fails with `E0425` for the missing function and an additional type mismatch at this match. Replace the call with `error.consensus_error()` and match the returned reference.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:204-223: Distinguish rejected derivation provenance from unknown provenance
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4326845221)
The new writer correctly validates the stored row and public-key material before lending coordinates, but it still treats an origin-aware load that returns no derivation coordinates exactly like an ordinary unknown-origin refresh. If a legacy external key carries an inferred breadcrumb, the load can reject that derivation while this writer preserves the same-key breadcrumb, so SQLite continues reporting a derivation claim that the load disproved. The signing public-key binding prevents an invalid signature, making this a data-accuracy issue rather than a signing-safety blocker, but the changeset needs an explicit rejected-versus-unknown provenance signal to clear the legacy row. This remaining work is tracked in `dashpay/platform#5542` item 5.
…let-update-at-revision
…oved identity that holds every cached key Drive's InvalidIdentityRevisionError reaches the wallet from one node without a proof, so lowering the cache to the revision it carries let a node roll a confirmed cache back while it kept newer keys, and a later older fetch then passed the lowered gate. The refusal now only prompts a proved fetch of the identity. The cache takes the fetched identity whole, and stores it as a refresh does, when the fetch shows the refusal's revision, below the cache's, the cache is still at the revision the refused transition was built from, and the fetch holds every key the cache holds with the same data and disabled state. An update Drive takes adds or disables a key, so a cache holding an update the proof lacks fails that check. The fetch uses the caller's request settings. The revision-error matcher uses the SDK's Error::consensus_error(), which replaced the wallet-local helper on v5.1-dev. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
|
@llbartekll a follow-up to the Drive-error route: thepastaclaw pointed out that So in 2be1862 the refusal only triggers a proved fetch of the identity. The cache takes the fetched identity whole only when all of these hold:
Drive refuses an empty update, so an accepted one always adds or disables a key. That means a cache holding a newer update fails the check, and an inflated cache passes it. It is still a small change: the stuck-identity case heals on the second attempt, as before. I also merged v5.1-dev, because |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Two blocking wallet-state correctness issues remain. Revision recovery is proof-authenticated but not freshness-authenticated, so a historical snapshot can still roll back a confirmed re-disable; SQLite breadcrumb preservation also prevents an origin-aware load from clearing a legacy false derivation claim. The Rust workspace test check is still queued and Kotlin build/tests are still in progress.
🔴 2 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes identity key-state merging and persistence in identity_ops.rs and revision-controlled signing flows in update.rs, directly affecting key handling rather than merely surrounding wallet code. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:466-477: Do not lower the cached revision from an unauthenticated DAPI error
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4358967206)
The refusal is no longer trusted by itself, but the replacement proof still is not sufficient to authorize lowering a confirmed cache. `holds_every_key` compares key data and the disabled boolean, not the provenance or freshness of the cached revision. After a valid revision-6 update re-disables a key that was already disabled at revision 5, a proof of the revision-5 identity has the same key data and disabled boolean and therefore passes this guard. The code then sets the cache to revision 5 and `store_fetched_identity` persists the historical snapshot, including its older disable timestamp. A correctly pinned revision-7 update can then fail locally, while an unpinned update can reuse the consumed revision and be rejected by Platform. Require an authenticated freshness boundary for recovery, or leave the cache unchanged when revision provenance is ambiguous; add a regression for confirmed re-disable followed by a lower-revision refusal and an older proved fetch.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:215-221: Distinguish rejected derivation provenance from unknown provenance
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4326845221)
The same-public-key and column-consistency checks validate the stored row's shape, but they do not establish that its derivation coordinates reproduce the key. A legacy external-key row created by the previous landed-update path can therefore pass these checks. When an origin-aware load derives the candidate and rejects it, the resulting breadcrumb-less entry reaches this branch, which restores the coordinates that the load failed to verify. The new preservation rule consequently makes a legacy false derivation claim impossible to clear through loading. Although public-key binding prevents signing with an actually mismatched derived key, the persisted wallet state still reports an invalid derivation and can affect signer preflight and recovery behavior. Preserve breadcrumbs for genuinely unknown-origin updates, but carry an explicit Rust-owned rejection state that allows an authoritative failed derivation check to clear the stored claim; add the legacy-external-row to origin-aware-load regression.
|
@thepastaclaw about the re-disable residual on 2be1862 (the proof is authenticated but not fresh). We have two ways to make the heal freshness-authenticated without relying on the local clock. Both use the per-identity watermark Option A: height floor, plus no re-disables from this wallet.
Option B: height floor, plus the landed block as the watermark.
Both leave one residual. A cache whose revision an older build confirmed through its own local apply, and that hasn't been refreshed since, still has a watermark below that update. For that cache a proof within the SDK's tolerance can still pass. Any refresh closes it, and the damage is the bounded one: one revision back, no key or disable lost. On the SQLite breadcrumb finding: the maintainer asked for it to be dismissed for this PR (#5487 (review)). It is tracked in #5542 item 5. |
|
@romchornyi on it — I've queued this as job |
|
@romchornyi reviewsys handles this review finding and the proposed fixes/deferrals. It answers on the finding’s thread and re-reviews on push (for drafts, once marked ready), so I’m leaving the A/B assessment and the SQLite disposition to it. |
…oof above the cache's watermark A proved identity is authenticated but not necessarily fresh. An update that only re-disables an already-disabled key changes no key, so a proof from before it passed `holds_every_key` and could take the cache back a revision. The heal now also requires the proof to come from a block above the cache's verified-height watermark (`last_updated_balance_block_time`), which is at or above every fetch the cache took, so a revision learned from a fetch cannot be undone by an older proof. A revision learned from an update this wallet landed moves no watermark, so the wallet now refuses, before signing, an update that disables a key the cache holds disabled: every update it lands then adds a key or disables an enabled one, which a proof from before it lacks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The revision-pinned update path and most cache-ordering, persistence, and FFI changes are correctly implemented. One blocking issue remains: the proved-refusal recovery can still lower a legitimately confirmed revision because its freshness checks do not cover legacy local re-disables or cache writers that can overwrite newer disabled state. CI checks covering Rust, Kotlin, Swift, JS, and image builds were still pending at the supplied snapshot.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The change is large and intricate and directly modifies key handling in signing_key.rs, managed_identity/identity_ops.rs, and sqlite/schema/identity_keys.rs, including key selection, reconciliation, and persistence across update and recovery flows. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:572-588: Refuse a stale re-disable only when the update carries no other key change
`refuse_disabled_again` rejects an entire identity update when any requested disable ID is already disabled, even if `add_public_keys` is non-empty. Drive accepts an add-plus-stale-disable transition and the added key changes the identity, so rejecting the whole operation is an unnecessary behavior change and prevents a valid key registration from landing. The stale re-disable guard is only needed to support the recovery invariant for disable-only updates. Scope the guard to updates with no added keys, or document and test that mixed updates are intentionally refused.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:475-478: Do not lower the cached revision from an unauthenticated DAPI error
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4358967206)
The direct unauthenticated-error rollback is removed, but the replacement recovery is still not safe for every cache state it can encounter. `take_rejected_revision` accepts a proved snapshot only when its height is strictly above the balance watermark and `holds_every_key` matches key data and the disabled boolean. That does not establish freshness for a revision raised by an older local apply: a legitimate cached revision 6 can contain a re-disable committed at H2 while its watermark remains H0, and revision 5 can have the same key data and disabled boolean from H1. A genuine proof at H1 therefore passes the current checks and lines 489–490 persist revision 5 and the older disable timestamp. The same authorization can be reached in a current-build flow because `replace_key` replaces the cached key unconditionally during a delayed key-limits confirmation, allowing a newer cached disable to be overwritten before a later re-disable and refusal recovery. Do not lower the cache unless Rust has an authoritative revision/key-freshness boundary; otherwise leave ambiguous legacy snapshots unchanged. Add regressions for a legitimately confirmed legacy re-disable and for a delayed key-limits update crossing a landed disable.
… instead of lowering the cache Lowering the cached revision on Drive's refusal, even from a proved fetch, could take a confirmed cache back: a proof is authenticated but not fresh, and some cache states (an older build's local re-disable, a key-limits confirmation over a newer disable) cannot be told from an older snapshot. The cache is now never lowered. When Drive refuses an update for its revision and reports one below the cache's (a cache an older build left one past the chain, which no fetch heals), an update asked for no revision is sent once more at the revision Drive takes, as a new transition with a fresh nonce. It lands at the cache's own revision, so cache and chain agree again; a node that lies can only cost paid refusals. A pinned update is not re-sent at another revision: it fails with IdentityRevisionRefused (FFI code 63), the refusal is noted in memory, and next_update_revision (FFI platform_wallet_identity_next_update_revision, Swift nextIdentityUpdateRevision) gives Drive's revision plus one, which a pin may then ask for while the cache has not moved. A landed update, a fetch taken whole, or Drive at the cache's revision clears the note. The landed update's cache-ahead branch also disables a key the inflated cache holds enabled. This drops the proved-fetch heal, the watermark check and the refusal of re-disables from the previous commits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 4 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The revision-pinned update path still has blocking correctness and security issues. Same-revision identity fetches can roll key state backward because key freshness ignores proof height, refusal metadata is lost across restart, and an unauthenticated revision refusal can trigger a second signed broadcast. The landed-update cache-behind path also still couples durable revision persistence to key-row writes; CI checks covering the changed code remain queued or in progress.
🔴 4 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes identity key handling and signing/update flows in packages/rs-platform-wallet/src/wallet/identity/network/update.rs and signing_key.rs, while also modifying persisted identity-key storage and FFI boundaries. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs:421-438: Order same-revision key snapshots by proof height
`taken` becomes true whenever the fetched identity's revision equals the cached revision, and the fetched key set is then used as the cache's key-state snapshot. `block_time` is passed only to `take_fetched_balance`; it does not gate key replacement, and `last_synced_keys_block_time` is neither updated nor consulted here. Core-managed identities can change key state without changing identity revision, including revision-0 re-enable operations. Consequently, a lagging but proof-verified same-revision response can restore an older disabled state over a newer cache and persist that rollback. Key snapshots need their own proof-height freshness boundary while still allowing a newer Core snapshot to re-enable keys.
In `packages/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:489-491: Persist the refusal revision used by next_update_revision across restarts
`next_update_revision` uses Drive's reported revision only when the in-memory `revision_refused_at` note is present. That note is not part of `IdentityEntry`, a changeset, or any persisted snapshot. A cache at revision 6 can receive a pinned refusal reporting Drive at 5, return retry revision 6, and then lose that recovery information if the wallet restarts before retrying; after restart the reader returns 7 and the previously recorded revision-6 update fails locally with `IdentityRevisionMismatch`. Persist a bounded refusal/retry record or provide an explicit proof-backed recovery operation.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:257-284: Do not retry a signed identity update from an unauthenticated revision error
`broadcast_and_wait` returns `StateTransitionBroadcastError` as soon as the wait response contains an error, before verifying the quorum proof. `revision_refusal` therefore extracts an attacker-controlled `current_revision`, and the unpinned path uses it to sign and broadcast the same logical update again at another revision. If the first disable-only transition is still in the mempool, Drive can accept both disables, charging another fee and consuming another nonce; the pinned path also records the unverified revision for later use. Only proof-verified state should authorize changing the transition revision or issuing a second signed broadcast. Otherwise return the original refusal without mutating retry state.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:410-437: Persist the landed revision independently of disable-key persistence
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4316888335)
In the cache-behind branch, `managed.identity.set_revision(transition_revision)` updates memory, but durable storage is reached only through the subsequent `add_key` and `disable_keys` calls. A valid disable-only confirmation whose key is absent from the cache makes `disable_keys` produce no changeset, so the new revision is never persisted; a disable persistence failure has the same result because that path logs the failure. After restart, the identity returns with the old revision and the next update is built stale. Store the scalar snapshot as its own fallible write, or use one combined atomic write for the revision and key changes and propagate failure.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:3805-3817: Keep the wallet wrapper alive through detached FFI calls
`nextIdentityUpdateRevision` copies only the raw `handle` into a `Task.detached` closure. If the last `ManagedPlatformWallet` reference is released while that task is queued, `deinit` calls `platform_wallet_destroy(handle)` before the task enters Rust, leaving the FFI call with an invalid registry handle. The new method should capture `self` and use `withExtendedLifetime(self)` around the complete synchronous FFI call. The same lifetime protection should be applied to `updateIdentity`, whose closure also currently passes only the raw wallet handle.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Prevent worker panics from escaping the identity-update C ABI — The shared identity-update FFI body still joins the worker with an expect-like panic boundary, so a worker panic can abort a host or leave broadcast status ambiguous. This is a concrete hardening issue but is tracked separately in #5542 and is not included as an additional blocker here.
- Follow-up: Address the panic-to-ambiguous-FFI-result guard for both identity-update exports in #5542.
| match managed.revision_refused_at { | ||
| Some((at, drive)) if at == cached => Ok(drive + 1), | ||
| _ => update_revision(identity_id, cached, None, None), |
There was a problem hiding this comment.
🔴 Blocking: Persist the refusal revision used by next_update_revision across restarts
next_update_revision uses Drive's reported revision only when the in-memory revision_refused_at note is present. That note is not part of IdentityEntry, a changeset, or any persisted snapshot. A cache at revision 6 can receive a pinned refusal reporting Drive at 5, return retry revision 6, and then lose that recovery information if the wallet restarts before retrying; after restart the reader returns 7 and the previously recorded revision-6 update fails locally with IdentityRevisionMismatch. Persist a bounded refusal/retry record or provide an explicit proof-backed recovery operation.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Not persisted, on purpose, for two reasons:
- Storing the note needs a new persisted field through the changeset, SQLite, and the Swift and Kotlin stores. That is the provenance scope the maintainer asked to keep out of this PR (platform-wallet: follow-ups left out of #5487 (identity fetch and update persistence) #5542).
- A proof-backed recovery after a restart isn't safe. Accepting a pin below the cache's revision plus one requires knowing that the earlier refused transition can never execute. That needs the transition's nonce, which a restart loses. A pin accepted on the revision proof alone could let a transition still held back by a node land after it.
So after a restart, as of d67e5a5:
- A recorded pin at the noted revision fails with
IdentityRevisionMismatchbefore anything is signed or broadcast, so it costs no fee. next_update_revisiongives the cache's revision plus one. An update at that revision is refused once more (one processing fee), the proofs are read again, and the note is back. The next pin then carries the proved revision plus one.
The cost is bounded: one paid refusal per restart, only for a cache that an older build left ahead of the chain, and never a double landing. This is documented on next_update_revision, on the FFI reader, on the Swift wrapper and under the PR's Known limitations.
There was a problem hiding this comment.
Deferred (re-reviewed at d67e5a51): The refusal note remains memory-only, and the restart-safe persistence design requires changes across the Rust changeset and mobile stores. The bounded extra refusal is documented and the durable follow-up is tracked in #5542.
…its nonce spent Drive's InvalidIdentityRevisionError comes without a proof, so its current_revision no longer decides anything. It only prompts two proved reads: the identity nonce and the identity revision. The update is sent once more (unpinned), or fails with IdentityRevisionRefused and is noted (pinned), only when the proved nonce word shows the refused transition's nonce spent by Drive's own merge rule (validate_identity_nonce_update), so that transition can never execute, and the proved revision is below the transition's by more than one. Had the refused transition executed after all, the chain is at its revision or above and Drive refuses the re-send, whose revision is not above it, so the update cannot land twice. A lagging proof costs one more paid refusal. The note keeps the highest proved revision and goes once a proof shows Drive at or above the cache's revision. A fetch at the cache's revision takes its key states only from a block at or above the key watermark (last_synced_keys_block_time), which a fetch taken whole now moves. A lagging proof at the same revision can no longer bring back a disable Core lifted on a masternode identity, or undo one it applied. apply_landed_update stores the landed revision and the update's keys as one snapshot write in both branches and returns a failed store, so a disable of a key the cache lacks no longer leaves the stored revision behind. The Swift nextIdentityUpdateRevision and updateIdentity keep the wallet alive through their detached FFI calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The revision-pinned retry path has a real maximum-revision arithmetic defect: an authenticated proof at u64::MAX can overflow before the refusal decision, causing a debug panic or a wrapped retry revision in release builds. The remaining documentation issue is non-blocking, while restart durability for refusal notes, key freshness metadata, and failed persistence writes remains a tracked follow-up. CI has no reported code failures, but Rust workspace, functional, browser, and PR Hygiene checks were still in progress at the supplied snapshot.
🔴 1 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Residual watch-only wording contradicts three-state origin
packages/rs-platform-wallet/src/changeset/changeset.rs:1008-1027
The final source still describes a missing breadcrumb as marking a watch-only key in KeyDerivationBreadcrumb and KeyWithBreadcrumb. In the current persistence contract, None can also mean unknown origin or an update that supplies no new coordinates while the persister must retain an existing breadcrumb. These comments contradict the adjacent IdentityKeyEntry clarification and could lead another persistence adapter to erase valid signing provenance.
source: gpt-6.1-sol (phase2-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change that modifies identity signing/key handling and wallet storage behavior, including identity key persistence and update revision safety in files such as packages/rs-platform-wallet/src/wallet/identity/network/update.rs and packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:625-633: Guard revision retry arithmetic against u64 overflow
`after_refusal` evaluates `proved + 1` before deciding whether the proof is sufficiently below the transition revision, and evaluates `drive + 1` again when constructing `Retry`. If a valid proof reports `u64::MAX`, the first addition panics in debug builds; in release builds it wraps to zero and can select an invalid retry revision instead of failing closed. This conflicts with the checked-overflow handling already used by `update_revision` and can cause an unnecessary paid retry or malformed transition.
In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/changeset.rs:1008-1027: Residual watch-only wording contradicts three-state origin
The final source still describes a missing breadcrumb as marking a watch-only key in `KeyDerivationBreadcrumb` and `KeyWithBreadcrumb`. In the current persistence contract, `None` can also mean unknown origin or an update that supplies no new coordinates while the persister must retain an existing breadcrumb. These comments contradict the adjacent `IdentityKeyEntry` clarification and could lead another persistence adapter to erase valid signing provenance.
Out-of-scope follow-up suggestions (4)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Persist refused-revision recovery metadata across restarts —
revision_refused_atis intentionally memory-only, so a restart can lose the proved lower revision and require one additional paid refusal beforenext_update_revisionrecovers the usable value. The behavior is bounded and documented, and the durable cross-store design is tracked separately rather than being a new cache-corruption path in this revision.- Follow-up: #5542, item 1
- Persist the key-state freshness watermark through mobile stores — The Rust fold records
last_synced_keys_block_time, but the Swift and Kotlin persistence contracts do not restore it. Consequently, the first same-revision fetch after a mobile restart can still be treated as a whole snapshot and regress a Core-managed key state. This is a concrete lifecycle limitation tracked separately from the Rust fold change.- Follow-up: #5542, item 1
- Retain failed fetched-key persistence obligations durably —
store_fetched_identityandstore_fetched_identity_keysmutate the in-memory fold before the combined store and do not retain a durable retry obligation when a generic persister rejects the write. A later restart can therefore lose newly discovered key rows or derivation metadata unless the backend or a later origin-aware load repairs them. The trimmed retry design is tracked separately.- Follow-up: #5542, items 2-4
- Contain worker panics at the new wallet C ABI boundary — The new revision-pinned export routes through
block_on_worker, whose worker join can panic on a panicking task; the panic is not converted into an ambiguous FFI result in this revision. This follows the shared legacy FFI runtime path and is explicitly tracked as boundary hardening rather than a revision-specific correctness defect.- Follow-up: #5542
…erflowing after_refusal computed the proved revision plus one before deciding, which panics in debug builds and wraps in release when a proof reports u64::MAX. It now uses checked_add and fails the update when there is no next revision. The breadcrumb docs on KeyDerivationBreadcrumb and KeyWithBreadcrumb no longer say None means watch-only; they point to IdentityKeyEntry::derivation_indices, where None means no coordinates and a persister keeps the breadcrumb it holds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
On the suggestion that is not inline (the watch-only wording in |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The revision-pinned update implementation has two remaining in-scope correctness and safety issues. The new C ABI exports still allow worker panics to reach the host process, and the revision check is performed on a snapshot that can become stale before signing; Kotlin convenience methods also cannot use the new revision pin. CI checks covering the changed workspace are still in progress.
🔴 1 blocking | 🟡 2 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Thread revision pinning through Kotlin convenience update methods
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.kt:192-207
The new revision argument is available on update, but the public addKey and disableKeys convenience methods always call it without a revision. Android callers using these common entry points therefore cannot use the caller-recorded revision guarantee introduced by this PR. Add an optional revision to both convenience methods and forward it to update; keep ABI conversion validation in the JNI layer.
suspend fun addKey(
walletHandle: Long,
identityId: ByteArray,
key: IdentityPubkey,
signerHandle: Long,
revision: Long? = null,
) = update(
walletHandle,
identityId,
listOf(key),
emptyList(),
signerHandle,
revision,
)
suspend fun disableKeys(
walletHandle: Long,
identityId: ByteArray,
keyIds: List<Int>,
signerHandle: Long,
revision: Long? = null,
) = update(
walletHandle,
identityId,
emptyList(),
keyIds,
signerHandle,
revision,
)
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate cross-cutting change that modifies identity signing/key handling and wallet persistence in files such aspackages/rs-platform-wallet/src/wallet/identity/network/signing_key.rs,update.rs, and the SQLite identity-key storage, meeting the critical-surface bar. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:173-205: Revalidate or reserve the pinned revision before signing
The revision guard checks a cloned identity while the wallet-manager read lock is held, then releases that lock before nonce fetching, transition construction, signer selection, signing, and broadcast. Another refresh or update can advance the managed identity after the guard succeeds, causing this call to sign and submit the stale cloned revision anyway. Drive will reject it and charge the refusal, defeating the guarantee that a pinned update fails before signing when the cache has moved. Serialize the revision-pinned operation with cache advancement, or revalidate immediately before signing under a coordination mechanism that closes the check-to-use race.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.kt:192-207: Thread revision pinning through Kotlin convenience update methods
The new revision argument is available on `update`, but the public `addKey` and `disableKeys` convenience methods always call it without a revision. Android callers using these common entry points therefore cannot use the caller-recorded revision guarantee introduced by this PR. Add an optional revision to both convenience methods and forward it to `update`; keep ABI conversion validation in the JNI layer.
In `packages/rs-platform-wallet-ffi/src/identity_update.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/identity_update.rs:364-414: Prevent worker panics from escaping the new C ABI entry point
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4316888345)
Both identity-update exports and the revision reader ultimately call `block_on_worker`, whose worker join uses `expect("tokio worker panicked")`. A panic in the wallet future or signer callback therefore reaches the `extern "C"` boundary instead of returning a `PlatformWalletFFIResult`, which can abort the Swift or Android host. For the update path the panic may occur after signing or broadcast, so the caller must receive an ambiguous outcome rather than a definitive failure. Wrap the complete identity-update FFI operation, including the revision reader, in the crate's panic-to-result guard and return an ambiguous `ErrorUnknown` result.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Persist refusal recovery metadata across wallet restarts — The refusal note remains process-local, so a caller-recorded recovery revision can be lost across a restart and require another paid refusal to re-establish proof-backed recovery. This is a concrete lifecycle limitation, but safe persistence requires coordinated changes across the Rust changeset and mobile stores and is tracked separately rather than being introduced by the current implementation path.
- Follow-up: #5542
- Reject unreadable JNI key arrays instead of treating them as empty — The JNI disable-key reader still converts array length or region-read failures into an empty list, which can omit requested disables. This behavior predates the revision-pin bridge and is not changed by this PR's revision forwarding; the corrective JNI change is tracked separately.
- Follow-up: #5540
- Keep the wallet handle alive through the key-limits detached FFI call — This is a pre-existing sibling-wrapper lifetime issue outside the identity revision-pinning changes. The PR's changed identity-update and revision-reader wrappers do capture the wallet and use withExtendedLifetime.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| // identity_index, key_index)` derivation metadata for | ||
| // the iOS Keychain re-derivation path. | ||
| let index = manager.identity_index(identity_id); | ||
| (identity, index) | ||
| let refused_at = manager | ||
| .identity(identity_id) | ||
| .and_then(|m| m.revision_refused_at); | ||
| (identity, index, refused_at) | ||
| }; | ||
|
|
||
| // Increment revision for the update transition. | ||
| let original_revision = identity.revision(); | ||
| identity.set_revision(original_revision + 1); | ||
| let built_from = identity.revision(); | ||
| let mut transition_revision = | ||
| update_revision(identity_id, built_from, revision, refused_at)?; |
There was a problem hiding this comment.
🟡 Suggestion: Revalidate or reserve the pinned revision before signing
The revision guard checks a cloned identity while the wallet-manager read lock is held, then releases that lock before nonce fetching, transition construction, signer selection, signing, and broadcast. Another refresh or update can advance the managed identity after the guard succeeds, causing this call to sign and submit the stale cloned revision anyway. Drive will reject it and charge the refusal, defeating the guarantee that a pinned update fails before signing when the cache has moved. Serialize the revision-pinned operation with cache advancement, or revalidate immediately before signing under a coordination mechanism that closes the check-to-use race.
source: gpt-6-astra (phase2-reviewer: security-auditor)
There was a problem hiding this comment.
Not changed here, on purpose. The pin promises that the update can land only at the revision the caller recorded. The race does not break that: the transition is built from the snapshot and carries exactly the pinned revision, whatever the cache does after the check. If a refresh or another update moves the identity in that window, the chain is already at the pinned revision or above. Drive then refuses the transition (one processing fee), the proofs show Drive at or above it, and after_refusal fails instead of re-sending. So the race costs at most one paid refusal and never lands an update twice or at another revision.
A check repeated just before signing would only narrow the window, because the cache can still move between that check and the broadcast. Closing it would mean serializing identity updates against cache refreshes in the wallet manager, which is a wider change than this PR. The doc on update_identity_with_external_signer_at_revision describes the check as made when the update starts.
There was a problem hiding this comment.
Still applies (re-reviewed at d415e70a): The transition remains pinned and cannot land at another revision, but the cache snapshot and revision check still occur before the nonce fetch, signing, and broadcast. The documented before-signing guarantee is therefore still racy under concurrent cache advancement.
The single-key convenience methods now take an optional revision and hand it to update, so Android callers on these entry points can pin the revision they recorded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
On the re-review of a6e23a6:
|
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The revision forwarding, proof-gated refusal handling, and most fetch-fold corrections are coherent, and the CI snapshot shows no completed changed-code failure. Three blocking durability/freshness gaps remain: mobile persistence does not restore the key-state watermark, failed fetched-key writes can disappear from the durable store after an in-process refresh, and refresh suppresses a failed folded-key persistence operation. The pinned revision check and duplicated Kotlin/JNI validation remain non-blocking suggestions.
🔴 3 blocking | 🟡 2 suggestion(s)
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change that modifies identity update signing and key-handling paths, wallet persistence, and cross-language APIs, including packages/rs-platform-wallet/src/wallet/identity/network/update.rs and packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs:463-475: Order same-revision key snapshots by proof height
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4362471075)
The Rust fold correctly uses `last_synced_keys_block_time` to reject same-revision responses from older blocks, but the Swift and Kotlin persistence paths do not persist or restore that watermark. After a mobile-wallet restart, the managed identity starts with `None`, so the first accepted same-revision response is treated as `KeyFold::Whole` regardless of proof height. A lagging proof can therefore replace a newer disabled/enabled key state with an older snapshot and persist that rollback. Persist the key watermark through the mobile snapshot contracts, or keep the first post-restart fetch from authoritatively replacing same-revision key state until a freshness boundary is restored.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs:527-537: Retain failed fetched key-row writes for retry
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4320042412)
`store_fetched_identity_keys` folds the fetched identity and materializes its key decisions before `store_keys_and_balance` persists them. If that store fails, the new or changed keys remain in memory but are absent from durable storage. A later refresh compares the fetched response with the already-mutated in-memory map, sees no key delta, and emits only the scalar snapshot, so it does not retry the missing key rows. Restarting before an origin-aware load or discovery can therefore restore an incomplete or non-signable key set. Retain a durable retry obligation, or roll back the materialized key changes while explicitly preserving them for a later persistence attempt.
In `packages/rs-platform-wallet/src/wallet/identity/network/loading.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/loading.rs:389-397: Propagate persistence failure when folding an older fetch's disable
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4318756290)
`refresh_identity` calls `store_fetched_identity`, logs a persistence failure, and still returns success. The fold has already changed the in-memory key state, while the durable row may still contain the active key. A subsequent refresh can see the disable already present in memory and omit the key-row upsert, so the failed write is neither surfaced nor retried; after restart the key can be restored as active and selected for signing. Propagate the persistence failure or retain an explicit retry obligation for the affected key row.
In `packages/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:177-205: Revalidate or reserve the pinned revision before signing
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4367252946)
The pinned-revision guard snapshots the managed identity under a read lock and releases that lock before nonce fetching, transition construction, signer selection, signing, and broadcast. A concurrent refresh or update can advance the cache after the guard succeeds, so the call can still sign and submit the stale pinned revision and pay a Drive refusal. The transition cannot land at another revision, but this still violates the documented fast-fail-before-signing guarantee. Serialize pinned updates with cache advancement, or add a reservation/revalidation mechanism whose coordination remains valid through signing and submission.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.kt:246-248: Keep JNI range validation in the ABI marshaling layer
(existing thread: https://github.com/dashpay/platform/pull/5487#discussion_r4325269986)
Kotlin still rejects negative `disablePublicKeyIds` values here, while `rs-unified-sdk-jni` repeats the same validation immediately before converting JVM integers to `u32`. The JNI marshaler must retain this check for ABI soundness, but keeping the rule in both wrappers allows their error behavior to drift. Kotlin should marshal the list and map the native result rather than deciding the conversion range independently.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Persist revision-refusal recovery metadata across restarts — The in-memory refusal note is lost on restart, so a caller may need one additional paid refusal before the wallet can recover the proved retry revision. This is a concrete limitation of the new recovery path, but the required persisted field and safe nonce/provenance design are tracked separately.
- Follow-up: #5542
- Harden identity-update FFI panic boundaries — The new revision-aware C export shares the existing worker panic boundary and can still expose an ambiguous worker panic across the foreign-function boundary. This is shared update-export hardening rather than a revision-marshaling defect and is tracked separately.
- Follow-up: #5542
- Reject unreadable JNI key-id arrays instead of treating them as empty — The JNI bridge still converts an unreadable disable-key array into an empty list, which can silently alter an update request. The behavior predates the reviewed revision-parameter change and is tracked in the separate JNI follow-up.
- Follow-up: #5540
Basic explanation
What this does: It adds an optional "this revision" argument to the wallet's identity update. Every identity update carries a revision number: the identity's current revision plus one. With the argument set, the update fails before it is signed or sent if the wallet's copy of the identity would give it a different number. It also stops the wallet's copy of an identity from going backwards or past the chain around an update, and keeps key derivation info from being lost when a store fails or a refresh finds a new key.
Value: A wallet that records an update before sending it knows exactly which revision that update can land at. dashwallet-ios needs this for App Connect sign-in (dashwallet-ios#1202): it records each key registration before sending it, and later tells "can still land" from "can never land" by comparing that revision with the one on chain.
Risks: Low to medium, wallet-side only; no consensus code changes. The update entry points behave as before unless a caller passes a revision, except that an identity at
u64::MAXis now refused instead of wrapping. Fetched identities are now folded into the cache by revision and proof height instead of replacing it.Issue being fixed or feature implemented
Revision pin.
update_identity_with_external_signerreads the cached identity when it starts and builds the transition atrevision + 1. Drive accepts an identity update only at exactly the state revision plus one. A caller cannot see the revision the update was built at: it reads the revision, records it, then calls the update, and anything that refreshes the cache in between changes the number. A later check can then decide that a registration still on its way can never land. The review of dashwallet-ios#1202 raised this as a blocker and asked for the fix at this boundary.The other pieces kept here each have a scenario test that drives the base code through a real flow (mock SDK, mock or real SQLite persister) and reproduces the defect:
loadIdentity(atIndex:), or a pull-to-refresh, within about one block hits a node one block behind, which the SDK's height tolerance accepts. On base the cache goes back to revision 1, the disabled key is enabled again, the added key disappears, and the balance goes back to its pre-fee value. The next update then carries revision 2 and Drive refuses it.apply_landed_update. A load or refresh hits an up-to-date node between the update's snapshot and its post-broadcast revision bump. On base the cache ends at 3 while the chain is at 2, so the next update is refused. The bump also overwrites the on-chaindisabled_atwith the local clock.refresh_identitystays in memory only. On base it is gone after a restart while the stored revision claims it.The SQLite keep-breadcrumb rule and the changeset merge rule are what make B3a safe. A refresh row carries no breadcrumb, and on base a breadcrumb-less upsert replaced the whole SQLite row and wiped the stored breadcrumb. The Swift and Kotlin persisters already kept it.
Removed after the maintainer's review. The scenarios found no field trigger for the rest, so it is out of this PR:
Constrainttracing.The external-key breadcrumb fix (#5501) moves to its own PR (link: #5540). The key-provenance work stays parked under #5525.
What was done?
rs-platform-wallet:IdentityWallet::update_identity_with_external_signer_at_revision(identity_id, add_public_keys, disable_public_keys, revision: Option<u64>, signer, settings). WithSome(revision), it refuses withPlatformWalletError::IdentityRevisionMismatch { identity_id, expected, actual }before the nonce fetch, the signing and the broadcast.update_identity_with_external_signercalls it withNone.update_revisioncomputes the revision withchecked_add.apply_landed_update, the post-broadcast step. If the cache is behind the transition, it takes the transition's revision (not its own plus one) and the update's keys and disables, as before. If the cache is already at the transition's revision or past it, it already holds the update as Platform has it, so the payload is not applied over it. The added keys are stored from the cache's copies with the breadcrumb the update gives them. Both branches store the revision and the update's keys as one snapshot write and return a failed store, so a disable of a key the cache lacks still stores the landed revision.ManagedIdentity::take_fetched_identity, the fetch fold shared by refresh, load by index and discovery. A fetch above the cache's revision is taken, key states included. So is one at the cache's revision from a block at or above the key watermark (last_synced_keys_block_time, now moved by every fetch taken whole): Core disables and re-enables a masternode identity's keys at revision 0, so key states at one revision are ordered by proof height. A same-revision fetch from an older block changes no key state. A fetch at a lower revision leaves the cache's revision, keys and disables. It takes only a raised limit, or a disable it shows on a key the cache holds enabled, and never rewrites a disable the cache holds. Each key limit keeps the higher of the two values. The balance follows the proof's block height (fetch_with_metadataand the existing balance watermark), not the revision.IdentityManager::add_identity_unstoredand store it with its verified keys as one changeset (store_fetched_identity_keys).IdentityKeysChangeSet::merge: a later entry without a breadcrumb keeps the earlier entry's.InvalidIdentityRevisionError(40203). That error comes without a proof, so its revision is not used: it only prompts two proved reads, the identity nonce (IdentityNonceFetcher) and the revision (IdentityBalanceAndRevision). Only when the nonce word shows the refused transition's nonce spent by Drive's own merge rule (validate_identity_nonce_update), so that transition can never execute, and the proved revision is below the transition's by more than one, an update asked for no revision is sent once more at the proved revision plus one, as a new transition with a fresh nonce. It lands at the cache's own revision, so cache and chain agree again. The update cannot land twice: had the refused transition executed after all (a node that lied), the chain is at its revision or above, and Drive refuses the re-send, whose revision is not above it. A pinned update is not re-sent at another revision. It fails with the newPlatformWalletError::IdentityRevisionRefused, and the proved revision is noted in memory only (ManagedIdentity.revision_refused_at).IdentityWallet::next_update_revisionthen returns that revision plus one, andupdate_revisionaccepts it as the pin while the cache has not moved. The note keeps the highest revision proved; it is cleared when an update lands, when a fetch is taken whole, and when a proof shows Drive at or above the cache's revision. Without those proofs the refusal fails the call as it is and changes nothing. The cache is never lowered.apply_landed_update's cache-ahead branch also applies a disable the cache lacks, for an inflated cache that did not hold the update it just landed.platform_wallet_identity_next_update_revision(wallet, identity_id, out_revision), andErrorIdentityRevisionRefused = 63(recorded inERROR_CODE_REGISTRY.md). Swift:ManagedPlatformWallet.nextIdentityUpdateRevision(identityId:)andPlatformWalletError.identityRevisionRefused. Kotlin maps code 63 toDashSdkError.PlatformWallet.IdentityRevisionRefused. The reader is not bridged to Kotlin: the reference Swift app has no caller, perkotlin-sdk/CLAUDE.md.rs-platform-wallet-storage: the SQLiteidentity_keyswriter keeps the breadcrumb (and its wallet id) a row holds when an upsert for the same row carries none. It does so only for a row that passes the reader's column checks (contradicts_columns, shared withload_state) and holds the same public key: the same type, data and hash. Any other stored row is replaced without its coordinates.rs-platform-wallet-ffi: newplatform_wallet_update_identity_with_signer_at_revision(..., revision: u64, signer_handle).IdentityRevisionMismatchmaps toErrorIdentityRevisionMismatch = 62(61 is held by open feat(platform-wallet): spend only final coins and report funds waiting on the network #5395), recorded inERROR_CODE_REGISTRY.md.platform_wallet_update_identity_with_signerkeeps its C ABI.swift-sdk:ManagedPlatformWallet.updateIdentity(identityId:addPublicKeys:disablePublicKeyIds:revision:signer:)withrevision: UInt64? = nil. When it is set, the call goes to the new export. A mismatch throwsPlatformWalletError.identityRevisionMismatch. The signer and the wallet are now kept alive withwithExtendedLifetimefor the whole synchronous FFI call, which borrows the signer's vtable and the wallet's handle; before,_ = signerdid not guarantee that.nextIdentityUpdateRevisionkeeps the wallet alive the same way.kotlin-sdk/rs-unified-sdk-jni:IdentityUpdates.update(..., revision: Long? = null), where a negative revision is refused. The JNIupdateIdentitytakes trailinghasRevision: Boolean, revision: Longand calls the_at_revisionexport whenhasRevisionis set. Code 62 maps toDashSdkError.PlatformWallet.IdentityRevisionMismatch.Known limitations:
last_synced_keys_block_time, and the note is never stored. After a restart, a pin at the noted revision fails before signing (IdentityRevisionMismatch), the next update at the cache's revision plus one is refused once more and noted again, and the first fetch is taken whole, as on base. Storing either needs a new persisted field across the stores, the provenance scope kept out of this PR.How Has This Been Tested?
cargo test -p platform-wallet -p platform-wallet-ffi -p platform-wallet-storage -p rs-unified-sdk-jni --all-features: 3206 pass, 0 fail. That isplatform-walletlib 1627,platform-wallet-ffilib 478, storage lib 431, storage integration 561, JNI 53, and the remaining test targets and doc-tests.cargo clippyfor the same crates with--all-features --tests -- -D warningsis clean, and so iscargo fmt.Tests added here:
wallet/identity/network/lagging_fetch_tests.rs, A1:should_keep_a_landed_update_when_a_lagging_fetch_answers_older(load by index and refresh).should_keep_raised_key_limits_when_a_lagging_fetch_answers_older.should_not_put_the_cache_past_the_chain_when_a_fetch_lands_before_the_apply.should_keep_a_second_disable_when_two_lagging_fetches_answer_older. Two revision-2 answers after an update at 3 disables a key again keep the revision and the disable.rs-platform-wallet-storage/tests/integration/sqlite_identity_key_restore.rs, B1a:should_not_leave_watch_only_key_rows_when_the_disk_fills_during_a_restore.should_store_a_key_a_refresh_finds_so_it_survives_a_restart.should_store_a_key_core_re_enabled_so_it_survives_a_restart, a revision-0 key re-enabled by a refresh, through a restart.should_store_a_key_a_load_at_index_finds_with_its_breadcrumb.loading::tests::should_store_a_newly_loaded_identity_with_its_keys_in_one_changeset. It fails on the base load path, which writes three changesets, the first with breadcrumb-less key rows.update_revision_tests(should_carry_one_above_the_cached_revision,should_refuse_an_update_asked_for_another_revision,should_refuse_an_identity_at_the_last_revision) andshould_refuse_another_revision_before_signing_or_reaching_platform. That last one checks both mismatch directions, that the signer is never called, and that the cache is unchanged.should_take_a_landed_update_only_into_a_cache_behind_it,should_store_an_added_key_with_its_breadcrumb_when_the_cache_is_ahead.should_order_key_states_at_the_cache_s_revision_by_proof_height(a lagging answer neither disables a key Core re-enabled nor undoes a later change; no watermark takes the first answer). Landed revision:should_store_a_landed_revision_with_its_keys_in_one_write(an added key with a disable, a disable of a key the cache lacks, a failed store).should_send_once_more_at_the_proved_revision_only_once(the decision after a failed broadcast: re-send once unpinned, refuse pinned, stop otherwise),should_take_a_refused_identity_s_revision_only_from_a_spent_nonce(tip, merged below it, past the window; above the tip, missing, too far ahead, none),should_prove_a_refusal_only_when_both_reads_answer(mock SDK, a failed read),should_keep_the_highest_proved_revision_until_drive_reaches_the_cache,should_tell_a_revision_refusal_only_of_this_identity(both error shapes, another identity, another error),should_take_drives_revision_asked_for_only_while_the_cache_is_where_it_was_refused,should_let_the_next_update_carry_drives_revision_without_lowering_the_cache(the note,next_update_revision, a pin at Drive's revision getting past the check, and the note cleared),should_take_an_update_landed_at_drives_revision_into_a_cache_ahead_without_it, FFIshould_read_the_next_update_revision_through_the_exportandshould_preserve_identity_revision_refused_code_and_message(63), SwifttestIdentityRevisionRefusedFFIResultMapping, KotlinshouldPreserveIdentityRevisionRefusedAsItsOwnError. The mock SDK cannot answerbroadcast_and_wait, so the send loop itself is covered through its decision function.should_take_a_fetched_identity_whole_only_from_a_newer_revision_or_block,should_keep_the_higher_of_each_key_limit,should_fold_an_older_fetch_s_key_decisions_into_the_cache_s_keys,should_keep_a_raised_limit_in_a_fetch_s_key_decisions,should_store_the_keys_a_refresh_adds_or_changes.should_keep_an_earlier_breadcrumb_when_a_later_key_entry_has_none.sqlite_identity_keys_reader.rs):should_keep_a_stored_breadcrumb_when_an_upsert_carries_none,should_keep_a_stored_wallet_id_with_the_breadcrumb_when_an_upsert_carries_neither,should_replace_a_stored_row_that_does_not_decode,should_replace_an_oversized_stored_row_without_reading_it,should_lend_a_stored_breadcrumb_only_to_the_same_key_from_a_valid_row.should_preserve_identity_revision_mismatch_code_and_message(code 62), andshould_forward_the_revision_to_the_guard_before_signing, which checks that the_at_revisionexport reaches the guard before the signer.should_pin_a_revision_only_with_has_revision_and_refuse_a_negative_one.DashSdkErrorTest.shouldPreserveIdentityRevisionMismatchAsItsOwnError../gradlew :sdk:testDebugUnitTest: 540 pass, 0 fail.ErrorHandlingTests.testIdentityRevisionMismatchFFIResultMapping.build_ios.sh --target simbuilds the SDK and SwiftExampleApp.swift test(all ofSwiftDashSDKTests): 863 run, 1 skipped, 0 fail.Breaking Changes
platform_wallet_update_identity_with_signer_at_revisionandplatform_wallet_identity_next_update_revision, and a new result codeErrorIdentityRevisionRefused = 63. The existingplatform_wallet_update_identity_with_signeris unchanged.PlatformWalletError(not#[non_exhaustive]) gainsIdentityRevisionMismatchandIdentityRevisionRefused, so a downstream crate that matches it exhaustively needs a new arm. Every match inside this workspace has a wildcard arm.ManagedPlatformWallet.updateIdentitygainsrevision: UInt64? = nil, andPlatformWalletErrorgainsidentityRevisionRefused, so an exhaustiveswitchover it needs a new case. Kotlin:IdentityUpdates.updategainsrevision: Long? = null. Existing call sites compile unchanged.TransactionsNative.updateIdentitytakes trailinghasRevision: Boolean, revision: Long.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
d415e70/skip-botsproceeds without the ones not yet reported/self-reviewedkotlin-sdk(packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt,packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/TransactionsNative.kt,packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.ktand 1 more) — HashEngineeringrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md,packages/rs-platform-wallet-ffi/src/error.rs,packages/rs-platform-wallet-ffi/src/identity_update.rs) — HashEngineering or ZocoLini or llbartekllwallet-storage(packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs,packages/rs-platform-wallet-storage/tests/integration/main.rs,packages/rs-platform-wallet-storage/tests/integration/sqlite_identity_key_restore.rsand 1 more) — lklimekrs-platform-wallet(packages/rs-platform-wallet/src/changeset/changeset.rs,packages/rs-platform-wallet/src/error.rs,packages/rs-platform-wallet/src/wallet/identity/network/discovery.rsand 8 more) — HashEngineering or ZocoLini or llbartekllpackages/rs-unified-sdk-jni/src/transactions.rs) — QuantumExplorer or shumkovswift-sdk— you own itWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.