fix(wallet): stop a bounded scan from erasing a live CrowdNode link - #1058
Open
romchornyi wants to merge 1 commit into
Open
fix(wallet): stop a bounded scan from erasing a live CrowdNode link#1058romchornyi wants to merge 1 commit into
romchornyi wants to merge 1 commit into
Conversation
`ownsAddress` answers "is this stored CrowdNode address mine?" by re-deriving
`m/44'/<coin>'/0'/{0,1}/{0..299}` and matching hash160 — the SDK exposes no
address→index lookup. That bound came from the message signer (2e44a26),
where running out is safe: no key, no signature. Reusing it for the ownership
question (3766008) turned the bound into an ownership horizon: a wallet whose
CrowdNode address sits past index 300 was told the address was foreign, and
`validatePrefs` answered by resetting the stored account.
The reset is not cosmetic. Without `signUpState == .finished`/`.linkedOnline`
the CrowdNode shortcut leaves `customizableActions`, and that shortcut is the
only way into CrowdNode — there is no menu entry — so the customer's account
becomes unreachable in the app (ticket 32026: "CrowdNode disappeared", logs
show "Found alien address in CrowdNode prefs" on every launch).
Exhaustion now answers `nil` (unknown), which `validatePrefs` already handles
by keeping the stored account and re-checking on the next `restoreState`.
`false` is left to the one case that really proves foreignness: an address that
is not a P2PKH address of the running network. The signer is untouched and
still fails closed.
Trade-off taken deliberately: an address belonging to a genuinely different
wallet now reads as unknown rather than foreign, so a stale link survives where
it used to be cleared. Keeping a stale pointer is recoverable; erasing a live
account is not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCrowdNode ownership checks now return ChangesCrowdNode ownership validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change preserves a stored CrowdNode link when a bounded address scan cannot prove ownership, preventing valid links from being erased; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Support ticket 32026: a customer reports CrowdNode has disappeared from the app after the update. Their logs show it happening on every launch:
The stored CrowdNode account address is theirs, and their funds are untouched on CrowdNode's side — the app just decided the address was not its own and erased the link.
What was done?
CrowdNodeMessageSigner.ownsAddressanswers "is this stored address mine?" by re-derivingm/44'/<coin>'/0'/{0,1}/{0..299}and matching hash160, because the SDK exposes no address→index lookup. That bound arrived with the SDK-native message signer (2e44a26), where exhausting the scan is the safe answer: no key found, no signature produced. A day later (3766008) the same helper started answering the ownership question, and there exhaustion means something entirely different — yet it was still reported asfalse, andCrowdNode.validatePrefsresponds tofalseby resetting the stored account.The reset is not cosmetic. Once
signUpStateis no longer.finished/.linkedOnline,ShortcutActionType.customizableActionsstops offering the CrowdNode shortcut — and that shortcut is the only entry point, since CrowdNode has no menu entry. A wallet whose CrowdNode address sits beyond index 300 loses access to its account entirely.ownsAddressnow returnsnil(unknown) when the scan runs out.validatePrefsalready treatsnilas "cannot validate — keep the stored account and re-check on the nextrestoreState", so no control flow changed; the destructive path simply no longer fires for a case that never proved anything.falseis reserved for the one answer that does prove foreignness: an address that is not a P2PKH address of the running network. The signer is untouched and still fails closed.Raising the bound was deliberately not the fix — it would postpone the same failure to the next customer with a longer history. The doc comments were corrected too: they claimed an out-of-bound address had "never been observed in practice", which this ticket disproves.
Breaking Changes
None.
Behaviour trade-off worth stating: an address belonging to a genuinely different wallet now reads as unknown rather than foreign, so a stale link survives where it used to be cleared. Keeping a stale pointer is recoverable and visible; erasing a live account is neither. The proper end state is to ask the SDK directly (
ManagedCoreWallet.signMessage(address:)knows the wallet's own addresses authoritatively) and drop the manual scan — out of scope here.How Has This Been Tested?
Clean
dashpayDebug build, no new warnings in the touched files.The condition cannot be reproduced on a normal wallet, whose CrowdNode address sits well within the first 300 indices — that is precisely why it took a customer report to surface. Reviewers can force it by temporarily setting
scanLimitto1: before this change the CrowdNode shortcut disappears on the next launch and the stored account is wiped; after it, the account survives and the log reads "validation inconclusive … keeping stored account".The unit-test target does not build repo-wide, so no test was added; a meaningful one would need a live SDK
Walletand a seam aroundSwiftDashSDKHost.shared.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit