feat(crowdnode): message signing via the platform SDK; fence the disabled on-chain send paths - #1537
feat(crowdnode): message signing via the platform SDK; fence the disabled on-chain send paths#1537HashEngineering wants to merge 3 commits into
Conversation
|
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Platform pin requirements for building this PRThe SDK artifact ( signMessage code (both already merged into
Minimum platform pin for the signing surface: rust-dashcore: no special pin — signMessage builds against the ordinary committed pin (verified at The catch: this wallet branch (
That combination (without any Maya/asset-lock extras) satisfies every code path this PR exercises. Once #4310 (or the integration line generally) reaches |
bfoss765
left a comment
There was a problem hiding this comment.
The signing seam, the typed no-empty-string error contract, and the fencing are all well-executed — the crowdnode module going dashj-free is exactly the Phase-1 goal, and the fencing provably throws before any side effect (the verifyNoInteractions tests, including the resendConfirmationTx case that previously locked outputs as its first statement, are exactly the right proof). Requesting changes on the pin; everything else is minor.
Blocking
-
build.gradle:dashSdkVersion = '0.1.0-v41int18-maya8-SNAPSHOT'resolves only from your local Maven repo — merged as-is it breaks every checkout and CI (your own limitations section says so). It's also already a merge conflict: the base has moved past int18, and the integration line is now onv41int21-SNAPSHOT, somaya8would additionally be a regression of the pin. Suggest dropping the pin change from this PR entirely and listing "signMessage present in the published AAR" as a merge precondition — the right vehicle is folding the platform-side signMessage surface into the next official intN cut, then a one-line pin bump on the branch. -
Mechanical: the branch is currently CONFLICTING against
feat/kotlin-sdk-phase1(build.gradle+PlatformSdkModule.kt— the module was touched by a later base commit). Needs a rebase regardless of item 1.
Non-blocking
-
Security posture —
SecurityFunctions.signMessage/DashSdkMessageSigner: the dashj path had an implicit auth barrier (it couldn't sign withoutSecurityGuard.retrievePassword()+ key derivation). The SDK path signs with no user authentication, and a CrowdNode signature is what authorizes a server-side withdrawal. Please confirm the withdrawal UX still forcesAuthenticationManager.authenticate(PIN/biometric) beforeCrowdNodeApi.withdrawreaches signing, and consider a KDoc note onSdkMessageSignerstating that callers own the auth gate now — the barrier's disappearance is invisible at the call site. -
Known-limitation 4 (restore of an old wallet →
SIGNING_KEY_UNAVAILABLEbecause discovery hasn't derived the CrowdNode account address yet): reasonable to ship as a documented limitation; a widen-and-retry heal for restored-wallet address windows is under consideration on the integration line, so a TODO pointing at the limitation rather than hard-coding around it is the right move. -
Nit:
strings-crowdnode.xmldeclarescrowdnode_signup_deposits_disabled_messagebut the diff only uses the title string inEntryPointFragment— either wire it (it reads like a dialog body) or drop it. -
Nit:
ResultFragment'ssignUpRetryRetiredkeys offviewModel.crowdNodeError !is MessageStatusException— a one-line comment on why aMessageStatusExceptionstill deserves a retry (API-path error, not a fenced-sender error) would keep the next editor from "simplifying" it.
66f0062 to
dd65b73
Compare
|
I will check item 4 by restoring a wallet with a CrowdNode account. |
|
Thanks for the review — everything is addressed as of Blocking 1 (pin): Adopted your suggestion exactly — the PR no longer touches Blocking 2 (rebase): Done — rebased onto the current 3 (security posture): Honest answer to your "please confirm": the withdrawal UX never forced authentication — and neither did the dashj path, whose 4 (restore window): The TODO you suggested is in, at the 5 (unused string): Wired — and doing so surfaced a real bug in the fencing commit: the disabled-notice title was being written into a TextView that is a child of the hidden signup button's CardView, so the explanation was invisible. Both strings now land in the screen's own headline/body views (new ids), visible with the button gone. 6 (retry comment): Added — Verification unchanged and green: 32/32 module tests, ktlint, |
Found during device testing: restored wallets do not detect existing CrowdNode accounts on this branchRestoring a wallet with an existing CrowdNode account (mainnet, Pixel 9 emulator) never detects the account — the CrowdNode UI stays in the not-signed-up state and the fee call goes out with Root cause (pre-existing on the integration lineage, not introduced by this PR): account detection reads the dashj transaction store — Upgrading installs are unaffected (their dashj store retains pre-cutover history), which is why this only surfaced on a restore test. This corrects the earlier discussion of non-blocking item 4 / limitation 4: the "any wallet that found the signup transactions has derived the address" argument assumed something still finds them on the dashj side. During the transition era, restored wallets never do. (The end-state argument stands — with SDK-only sync the finder and signer are the same wallet and this failure mode cannot exist.) Fix direction (contained): the SDK's Room store keeps full raw transaction bytes ( |
|
We cannot test the changes because the CrowdNode account detection requires DashJ. We will try Tools / DashJ to see if the DashJ sync will find the account. |
bfoss765
left a comment
There was a problem hiding this comment.
Re-reviewed at head 3a7302766 against my earlier CHANGES_REQUESTED. All six items are addressed — approving.
| # | Original concern | Now |
|---|---|---|
| 1 | dashSdkVersion pin override; signMessage must be in the published AAR first |
Resolved — no build.gradle change in the PR, and the base pin 0.1.0-v42int4 genuinely carries it (verified in the AAR: ManagedPlatformWallet.signMessage, getMnemonicResolverHandle, SigningKeyUnavailable, and core_wallet_sign_message in libdash_sdk_jni.so) |
| 2 | Branch conflicting | Mergeable; merged tree compiles (:wallet:compile_testNet3DebugKotlin green) with 42 tests passing (SecurityFunctionsSignMessageTest 10/10, crowdnode 32/32) |
| 3 | Withdrawal must force authentication before signing | Verified: TransferFragment.kt:237 hoists authenticate(...) ?: return out of the !isWithdraw branch, so it gates both directions before handleWithdraw() → CrowdNodeApi.withdraw → requestWithdrawal (the signing site). Call-site closure checked: signMessage has exactly two production callers and no worker/retry path reaches it |
| 4 | TODO for the SIGNING_KEY_UNAVAILABLE restore-window limitation | Present at SecurityFunctions.kt:291-300 |
| 5 | Unused crowdnode_signup_deposits_disabled_message |
Now rendered via get_started_title/get_started_hint; also fixes the notice previously being nested in the hidden button's CardView |
| 6 | Comment on signUpRetryRetired |
ResultFragment.kt:99-109 |
Two things to fix up outside the code, neither blocking:
- The PR description is stale. Limitation 1 ("should not be merged before the AAR publishes") no longer applies — the base pin has advanced past it. Limitation 4 still states the "downgraded to a note / cannot strand a restored wallet" claim you retracted in comments. Worth correcting so the description matches reality.
- The new withdrawal PIN prompt is code-verified but not field-verified — noted from your comment that CrowdNode account detection needs dashj and can't be exercised on this lineage. Flagging it so it gets an explicit manual check when a CrowdNode-capable build is available. Low risk: the change only adds an auth gate.
One minor observation for a follow-up, not this PR: CrowdNodeAPIConfirmationHandler.handle (CrowdNodeConfirmationTxHandler.kt:85-90) swallows the retirement throw and leaves online-account status unchanged indefinitely. Inert while the online account is unreachable, but it would be a silent permanent stall if that flag is ever flipped.
|
Thanks — description corrected on both counts:
On your follow-up observation about |
Replace the dashj implementation of SecurityFunctions.signMessage with the Dash Platform Kotlin SDK's ManagedPlatformWallet.signMessage. The AuthenticationManager interface and CrowdNode's two call sites (CrowdNodeWebApi registerEmail + requestWithdrawal) are unchanged. The SDK's mnemonic resolver reads the seed Rust-side from its own Keystore-backed storage, so signing no longer retrieves the PIN-derived password or derives the wallet encryption key. Access follows the existing seam convention (SdkL1SendSource/DashSdkL1SendSource): a new SdkMessageSigner interface with a DashSdkMessageSigner production impl bound in PlatformSdkModule, using the manager's mnemonicResolverHandle and the single bound wallet. No dashj fallback, per the fail-closed cutover philosophy (cf. cutoverSendRoute). Error contract. The dashj code returned "" when the wallet did not own the address, which made CrowdNode POST an unsigned request that failed server-side with an opaque message. Failures now throw. Because :integrations:crowdnode depends on :common only and cannot see DashSdkError, the contract is expressed in :common as MessageSigningException with SIGNING_KEY_UNAVAILABLE (FFI code 31), INVALID_ADDRESS (platform-wallet code 2, which has no dedicated Kotlin type and surfaces as Generic(2)) and UNAVAILABLE, always preserving the SDK error as the cause. Cancellation propagates unwrapped. Withdrawal crash fix. CrowdNodeApi.withdraw caught only HttpException and UnknownHostException, TransferFragment.handleWithdraw only WithdrawalLimitsException, AdaptiveDialog.withProgress is try/finally, and the enclosing lifecycleScope.launch has no handler - so a thrown signing error would have propagated out uncaught and crashed the app. Add a MessageSigningException arm to withdraw that routes to the existing handleError path. The email flow already handled it via registerEmailForAccount's catch. Testability. SecurityFunctions cannot be constructed in a unit test because PinRetryController's static initializer builds an Android-dependent singleton, so the signing logic is lifted to a top-level internal signMessageViaSdk() and the class method delegates to it - the same "pure logic as a top-level internal fun" convention as classifyCoreSendFailure. 10 host-JVM tests cover the mapping, the no-empty-string-on-failure guard and cancellation. Also bumps dashSdkVersion to 0.1.0-v41int18-maya8-SNAPSHOT, which the signMessage API requires. No other wallet code needed changes for int18. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CrowdNode has disabled account creation and deposits service-side, and the API path covers every remaining user, so the dashj senders in CrowdNodeBlockchainApi are fenced off behind a flag rather than ported to the platform SDK. Each of the six senders - topUpAddress, makeSignUpRequest, acceptTerms, deposit, requestWithdrawal and resendConfirmationTx - gains a single guard on the new CrowdNodeConstants.SIGNUP_AND_DEPOSITS_ENABLED (false) that throws the new CrowdNodeServiceUnavailableException. The guard is the FIRST statement in every one, so nothing is attempted before the refusal: no send, no output locking, no top-up self-send, no partially executed flow. That is what retires the partial-deposit stranding hazard. resendConfirmationTx is the sharpest case - it used to call lockOutputsPayingTo as its first statement, and the guard now precedes that lock. Throwing rather than returning quietly is deliberate: an operation that reports success while moving no funds is the failure mode being refused. Everything after each guard is the ORIGINAL dashj implementation, preserved byte-for-byte (the diff to this file is 62 additions and zero deletions). It is the working template for a future SDK port, which is blocked on an add_inputs_from_outpoints JNI binding - the SDK has no equivalent of the SpendSelection.ByAddress / ExactOutput input pinning these flows need. The class KDoc records that, and records that flipping the flag back to true will NOT by itself make these sends work: the fail-closed cutover in SendCoinsTaskRunner (cutoverSendRoute) rejects a custom coin selection outright once the SDK cutover is committed. CrowdNodeServiceUnavailableException extends CrowdNodeException so the existing handlers turn it into an error state rather than an uncaught crash: CrowdNodeApi.signUp and .deposit catch Exception, and CrowdNodeConfirmationTxHandler catches CrowdNodeException. That handler gets a dedicated arm - reporting a fenced-off resend as a wrong-address error would send users chasing a problem with their address that does not exist. Passive observers are untouched and still serve balances, history, withdrawals and restore: the waitFor* family, getDeposits, getDepositConfirmations, getApiAddressConfirmationTx, getFullSignUpTxSet and getWithdrawalsForTheLast. The same flag gates the user-facing entry points, replacing the ad-hoc `isVisible = false` lines that hid these affordances one at a time: EntryPointFragment's new-account button (the sole route into signup) now hides and explains itself, PortalFragment's deposit button reads the flag, and ResultFragment stops offering a signup retry that could only reproduce the same failure. The throws are the backstop for any path the gate misses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…il signing; review follow-ups Signing through the platform SDK performs NO user authentication - the mnemonic resolver reads the seed Rust-side from Keystore-backed storage, so a signature is produced silently for whoever asks. The dashj implementation this replaced could not do that: it had to retrieve the PIN-derived password and derive the wallet encryption key on every call. Both CrowdNode signatures authorize real server-side actions, so the authentication that was implicit before is now made explicit at the call sites. - TransferFragment.continueTransfer authenticates for BOTH directions. The gate moves above the isWithdraw branch, so a withdrawal - which instructs CrowdNode to move real funds - now needs the same proof-of-presence a deposit did. The first-deposit banner check stays in the deposit branch. A null return (cancelled or failed) aborts before any transfer. - OnlineAccountEmailFragment.continueCreating authenticates before signAndSendEmail. CrowdNode treats that signature as authorization to bind the account address to an email, which is what account recovery hangs off. The fragment gains an AuthenticationManager injection, matching the sibling fragments; `?.let` means a cancelled check sends nothing. SdkMessageSigner's KDoc now states the no-authentication property and that callers whose signatures authorize sensitive actions own the gate, so a future caller decides deliberately rather than inheriting a prompt that no longer exists. Also from review: - A TODO at the SigningKeyUnavailable site records that this may be transient after a wallet restore, while the SDK wallet's address discovery catches up with dashj's, and that a widen-and-retry heal is under consideration on the integration line. Deliberately documented rather than worked around: a retry or fallback here would mask the difference between "not ours yet" and "not ours", which is what the fail-closed contract rests on. - crowdnode_signup_deposits_disabled_message was declared but never shown. It is now the body of the disabled entry-point screen. This also fixes a real bug: the title was being written to requiredDashTxt, which is a child of the new-account card and therefore already hidden. The screen's own title/hint TextViews - which sit outside that card - get ids and carry the explanation instead. - A comment in ResultFragment explains why signUpRetryRetired excludes MessageStatusException: that is an API-path failure and still worth retrying, whereas any other signup error would re-enter the fenced-off senders and re-throw at once. No test added for the two gates: this module has no fragment/UI test rig (no Robolectric, and src/androidTest has no sources), and both gates live in Fragments. Building a rig for them was out of scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3a73027 to
50ded86
Compare
What this does
Completes the CrowdNode item of #1520: the integration's one functional dashj
dependency — classic Dash message signing for API withdrawals and email
registration — now routes through the platform Kotlin SDK, and the dormant
on-chain send paths are typed-fenced rather than ported, since CrowdNode has
disabled account creation and deposits service-side and every remaining user
is served by the API path.
Two commits:
1.
feat(crowdnode): route message signing through the platform SDKSecurityFunctions.signMessagedelegates toManagedPlatformWallet.signMessage(address, message, coreSignerHandle)via a new
SdkMessageSignerseam (same DI pattern asSdkL1SendSource).The dashj
ECKeypath, keystore password retrieval, and Kotlin-side keyderivation are gone from signing; signing authority is the SDK's mnemonic
resolver, same as SDK sends.
""-on-missing-key return is replaced by a typedMessageSigningException(in:common, because:integrations:crowdnodecannot see SDK types) withSIGNING_KEY_UNAVAILABLE/INVALID_ADDRESS/UNAVAILABLEreasons andthe SDK error preserved as cause. A regression test asserts no failure
mode can ever return an empty string again.
withdrawal propagated uncaught (
CrowdNodeApi.withdrawhandled onlyHttpException/UnknownHostException). One catch arm added, mirroringthe existing error handling.
2.
feat(crowdnode): fence the on-chain senders behind a service-status flagCrowdNodeBlockchainApi(topUpAddress,makeSignUpRequest,acceptTerms,deposit,requestWithdrawal,resendConfirmationTx) throwCrowdNodeServiceUnavailableExceptionbefore any side effect when
CrowdNodeConstants.SIGNUP_AND_DEPOSITS_ENABLEDis false (it is). Original bodies are preserved byte-for-byte below the
guards — the file's diff is +62/−0 — as the template for a future SDK port.
CrowdNodeExceptiondeliberately:CrowdNodeConfirmationTxHandlercatches only that type inside a barehandlerScope.launch, and its existing catch would otherwise misreport a"wrong address" error. It gets a dedicated arm.
accounts or deposits" notice (new strings in
strings-crowdnode.xml),consolidating the ad-hoc
isVisible = falsehides that already existed.waitFor*, deposit/withdrawal history, signup-txrestore) is untouched — balance, history, and account restore keep
working for existing users.
The CrowdNode module now has zero bitcoinj/dashj imports (it was already
insulated behind the
:commonabstractions; this PR removes its lastfunctional reliance on dashj-backed services).
Testing
no-empty-string regression guard; 8 new fencing tests incl.
verifyNoInteractions(paymentService)proving the fenced bodies areunreached); ktlint clean;
prodand_testNet3flavors compile.the SDK, and CrowdNode's live server accepted the signed message — it
replied
Receivedand gave the message an id. The signature is 65 bytesin the same format the old dashj code produced, so CrowdNode cannot tell
the two apart.
wallet address it already owns, standing in for a CrowdNode account,
because restored wallets do not detect their CrowdNode account on this
branch (see the comment below). No money moved and no real CrowdNode
account took part.
so we cannot create an account to test against, and email registration
cannot be reached without a detected account. That leaves the withdrawal
signature above as the end-to-end evidence.
field-verified, for the same reason — it needs an explicit manual check
on a build where a CrowdNode account is detectable.
(RFC6979) and landed upstream in dashpay/platform as 92f94526ef + #4321.
Known limitations
The SDK artifact this PR needs is not published yet.RESOLVED.The PR does not change
dashSdkVersion; the base branch has since movedto
0.1.0-v42int4, which carries the signMessage surface (confirmed inthe AAR:
ManagedPlatformWallet.signMessageandcore_wallet_sign_messagein
libdash_sdk_jni.so). No publishing step is outstanding, and theearlier pin-requirements comment on this PR is historical.
ported. CrowdNode disabled these service-side. Flipping
SIGNUP_AND_DEPOSITS_ENABLEDalone will NOT restore them: the sendcutover fail-closes custom coin selections, and the SDK has no
ByAddress/ExactOutputinput-pinning surface yet (needs theadd_inputs_from_outpointsJNI binding in dashpay/platform, for which adesign sketch exists). The preserved bodies are the port template.
requestWithdrawal(on-chain signal) had no callers even before thisPR — it is fenced like the rest but was already dead code.
so the signing paths cannot be reached from the UI. Detection reads the
dashj transaction store, which no longer receives blocks after the sync
cutover, so a restored wallet's store stays empty (details in the comment
below). This is pre-existing and not introduced here, and we are not
fixing it in this PR. It goes away in the SDK-only end state. A
SIGNING_KEY_UNAVAILABLETODO inSecurityFunctionsrecords the relatedkey-derivation window without working around it.
(An earlier revision of this entry argued the restore case could not be
stranded; device testing disproved that, and this text replaces it.)
gate is closed):
TransferFragment.handleDepositshows the successscreen regardless of the deposit result, and
BlockchainServiceImplinjects
CrowdNodeBlockchainApiwithout using it. Also,SecurityFunctionslogs underSendCoinsTaskRunner's logger category.our signature was checked by hand on mainnet (see Testing). The automated
tests stop at the edge of the module, with the network calls mocked, and
for the reasons above they cannot go further.
🤖 Generated with Claude Code