feat(platform-wallet): classic Dash message signing (signMessage) over FFI, JNI, Kotlin, and Swift - #4259
Conversation
…ssage) over FFI CoreWallet::sign_message signs a string with the key behind one of the wallet's own P2PKH addresses (signable funds accounts only) and returns the 65-byte BIP-137-style recoverable signature, base64 — same semantics as dashj ECKey.signMessage and Dash Core's signmessage RPC, verified byte-for-byte against dashj (RFC6979 golden + dashj test vector). The digest and serialization come from dashcore::sign_message; the recovery id is found by trial against the signer's pubkey, so every Signer backend gets signed-message support without a recoverable-signing method. Key material never crosses the FFI: core_wallet_sign_message takes the caller's MnemonicResolverHandle like the send paths do. Unknown/watch-only addresses map to ErrorSigningKeyUnavailable (31). Serves the Android/iOS wallets' CrowdNode integrations (API withdrawal and email registration are signed-message proofs of address ownership). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Kotlin: ManagedPlatformWallet.signMessage(address, message, coreSignerHandle) suspend over the coreWalletSignMessage JNI binding. Swift: ManagedCoreWallet.signMessage(address:message:) constructing the per-call MnemonicResolver like every other seed-backed Swift call site. The Swift marshalling is what surfaced the FFI's empty-message contract bug (empty Swift strings marshal as a nil base address) — the read_utf8 len == 0 short-circuit shipped with the primitive commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdded classic Dash message signing for wallet-owned P2PKH addresses. The implementation spans CoreWallet, Rust FFI, Kotlin, JNI, and Swift APIs. It returns Base64 recoverable signatures and maps unavailable signing keys to native error code 31. ChangesClassic Dash message signing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedPlatformWallet
participant WalletManagerNative
participant core_wallet_sign_message
participant CoreWallet
participant Signer
ManagedPlatformWallet->>WalletManagerNative: Request signature
WalletManagerNative->>core_wallet_sign_message: Pass address, message, and signer handle
core_wallet_sign_message->>CoreWallet: Invoke sign_message
CoreWallet->>Signer: Sign Dash message digest
Signer-->>CoreWallet: Recoverable signature
CoreWallet-->>core_wallet_sign_message: Base64 signature
core_wallet_sign_message-->>WalletManagerNative: Allocated signature
WalletManagerNative-->>ManagedPlatformWallet: Return signature
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
⛔ Blockers found — Sonnet deferred (commit 64146a2) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The signed-message implementation is cryptographically aligned with Dash Core and dashj, and its address-ownership and recovery-ID checks are sound. One blocking issue remains: the public generic Rust API ignores the signer's advertised capabilities and can invoke blind digest signing on a backend that explicitly disallows it. Non-blocking follow-ups cover binding documentation, FFI lock lifetime and ABI regression coverage, and malformed JVM strings.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 4 suggestion(s)
🤖 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/core/sign_message.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/sign_message.rs:189-196: Honor the signer's digest capability before signing
`key_wallet::signer::Signer` explicitly assigns capability dispatch to callers and documents `sign_ecdsa` as valid only when the signer advertises `SignerMethod::Digest`. This public generic method calls it unconditionally. The production mnemonic resolver supports digest signing, but a legitimate transaction-only hardware signer may reject the call, panic because the method is unreachable, or blind-sign a host-computed digest despite advertising a policy that forbids that operation. Fail before entering the backend, and add a regression signer that advertises only `Transaction(...)` and panics if `sign_ecdsa` is invoked.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:182-184: Document the signed message's UTF-8 byte length
`signed_msg_hash` prefixes `msg.len()`, which is the serialized UTF-8 byte count. Kotlin's `message.length` counts UTF-16 code units, so the documented formula is wrong for non-ASCII messages; for example, `"é"` has length 1 but contributes 2 UTF-8 bytes. State that the varint contains `message.toByteArray(Charsets.UTF_8).size`. The analogous Swift documentation at `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:145` must use `message.utf8.count` instead of `message.count`, which counts extended grapheme clusters.
In `packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs:109-138: Release the global Core-wallet storage lock before signing
`HandleStorage::with_item` retains the global storage map's read guard throughout the closure. This closure performs `runtime().block_on(...)` and invokes the host-owned mnemonic resolver callback, so a potentially slow signing operation prevents insertion or removal of every Core-wallet handle. A callback that re-enters an operation requiring the write lock can deadlock. `CoreWallet` is an inexpensive Arc-backed clone, and the broadcast entry points already clone it out of storage before blocking; use the same pattern here.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/sign_message.rs:98-101: Cover the null-pointer empty-message ABI contract
The new FFI contract intentionally accepts `message_ptr == NULL` when `message_len == 0`, which is required by the Swift empty-array marshalling path. No endpoint test exercises this behavior, and the Rust wallet tests only sign `"hello"`. Add a direct `core_wallet_sign_message` regression test that passes a null message pointer with zero length, asserts success, and verifies the returned signature against `signed_msg_hash("")`; this prevents a future unconditional `check_ptr!(message_ptr)` from silently breaking empty messages.
In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:1063-1075: Do not silently alter malformed Java UTF-16 messages
`env.get_string(...).into()` decodes JNI modified UTF-8 through `jni::JavaStr`. For a Java/Kotlin `String` containing an unpaired UTF-16 surrogate, the JNI crate's decoder fails and falls back to `String::from_utf8_lossy`, silently replacing the input before its bytes are signed. A verifier encoding the same Java string through the platform UTF-8 encoder can therefore hash different replacement bytes, contradicting the API's claim that the message is signed verbatim. Marshal a Kotlin-produced UTF-8 `ByteArray`, or read the UTF-16 code units strictly and reject unpaired surrogates rather than signing transformed text.
| let hash = signed_msg_hash(message); | ||
| let (signature, public_key) = signer | ||
| .sign_ecdsa(&path, hash.to_byte_array()) | ||
| .await | ||
| .map_err(|e| PlatformWalletError::MessageSigningFailed { | ||
| address: target.to_string(), | ||
| reason: format!("signer rejected the digest at {path}: {e}"), | ||
| })?; |
There was a problem hiding this comment.
🔴 Blocking: Honor the signer's digest capability before signing
key_wallet::signer::Signer explicitly assigns capability dispatch to callers and documents sign_ecdsa as valid only when the signer advertises SignerMethod::Digest. This public generic method calls it unconditionally. The production mnemonic resolver supports digest signing, but a legitimate transaction-only hardware signer may reject the call, panic because the method is unreachable, or blind-sign a host-computed digest despite advertising a policy that forbids that operation. Fail before entering the backend, and add a regression signer that advertises only Transaction(...) and panics if sign_ecdsa is invoked.
| let hash = signed_msg_hash(message); | |
| let (signature, public_key) = signer | |
| .sign_ecdsa(&path, hash.to_byte_array()) | |
| .await | |
| .map_err(|e| PlatformWalletError::MessageSigningFailed { | |
| address: target.to_string(), | |
| reason: format!("signer rejected the digest at {path}: {e}"), | |
| })?; | |
| if !signer.supports(key_wallet::signer::SignerMethod::Digest) { | |
| return Err(PlatformWalletError::MessageSigningFailed { | |
| address: target.to_string(), | |
| reason: "signer does not support digest signing".to_string(), | |
| }); | |
| } | |
| let hash = signed_msg_hash(message); | |
| let (signature, public_key) = signer | |
| .sign_ecdsa(&path, hash.to_byte_array()) | |
| .await | |
| .map_err(|e| PlatformWalletError::MessageSigningFailed { | |
| address: target.to_string(), | |
| reason: format!("signer rejected the digest at {path}: {e}"), | |
| })?; |
source: ['codex']
| * **The format.** The signed digest is | ||
| * `SHA256d(prefix ‖ varint(message.length) ‖ message)`, where the prefix is | ||
| * the historical `"\x19DarkCoin Signed Message:\n"` — *not* `"Dash"`. Dash |
There was a problem hiding this comment.
🟡 Suggestion: Document the signed message's UTF-8 byte length
signed_msg_hash prefixes msg.len(), which is the serialized UTF-8 byte count. Kotlin's message.length counts UTF-16 code units, so the documented formula is wrong for non-ASCII messages; for example, "é" has length 1 but contributes 2 UTF-8 bytes. State that the varint contains message.toByteArray(Charsets.UTF_8).size. The analogous Swift documentation at packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:145 must use message.utf8.count instead of message.count, which counts extended grapheme clusters.
source: ['codex']
| let option = CORE_WALLET_STORAGE.with_item(handle, |wallet| { | ||
| let address = read_utf8(address_ptr, address_len, |e| { | ||
| PlatformWalletError::MessageSigningAddressInvalid { | ||
| address: "<non-UTF-8>".to_string(), | ||
| reason: format!("address is not valid UTF-8: {e}"), | ||
| } | ||
| })?; | ||
| // A non-UTF-8 message is reported against the (now known) address, so the | ||
| // error names the signing target the caller asked about. | ||
| let message = read_utf8(message_ptr, message_len, |e| { | ||
| PlatformWalletError::MessageSigningFailed { | ||
| address: address.clone(), | ||
| reason: format!("message is not valid UTF-8: {e}"), | ||
| } | ||
| })?; | ||
| let network = wallet.network(); | ||
| let wallet_id = wallet.wallet_id(); | ||
| // SAFETY: `signer_addr` came from `core_signer_handle`, which the caller | ||
| // pinned alive for this call; the `MnemonicResolverCoreSigner` lives | ||
| // only on this stack frame and is dropped before returning. | ||
| let signer = MnemonicResolverCoreSigner::new( | ||
| signer_addr as *mut MnemonicResolverHandle, | ||
| wallet_id, | ||
| network, | ||
| ); | ||
| runtime().block_on(wallet.sign_message(&address, &message, &signer)) | ||
| }); | ||
|
|
||
| let result = unwrap_option_or_return!(option); | ||
| let signature = unwrap_result_or_return!(result); |
There was a problem hiding this comment.
🟡 Suggestion: Release the global Core-wallet storage lock before signing
HandleStorage::with_item retains the global storage map's read guard throughout the closure. This closure performs runtime().block_on(...) and invokes the host-owned mnemonic resolver callback, so a potentially slow signing operation prevents insertion or removal of every Core-wallet handle. A callback that re-enters an operation requiring the write lock can deadlock. CoreWallet is an inexpensive Arc-backed clone, and the broadcast entry points already clone it out of storage before blocking; use the same pattern here.
| let option = CORE_WALLET_STORAGE.with_item(handle, |wallet| { | |
| let address = read_utf8(address_ptr, address_len, |e| { | |
| PlatformWalletError::MessageSigningAddressInvalid { | |
| address: "<non-UTF-8>".to_string(), | |
| reason: format!("address is not valid UTF-8: {e}"), | |
| } | |
| })?; | |
| // A non-UTF-8 message is reported against the (now known) address, so the | |
| // error names the signing target the caller asked about. | |
| let message = read_utf8(message_ptr, message_len, |e| { | |
| PlatformWalletError::MessageSigningFailed { | |
| address: address.clone(), | |
| reason: format!("message is not valid UTF-8: {e}"), | |
| } | |
| })?; | |
| let network = wallet.network(); | |
| let wallet_id = wallet.wallet_id(); | |
| // SAFETY: `signer_addr` came from `core_signer_handle`, which the caller | |
| // pinned alive for this call; the `MnemonicResolverCoreSigner` lives | |
| // only on this stack frame and is dropped before returning. | |
| let signer = MnemonicResolverCoreSigner::new( | |
| signer_addr as *mut MnemonicResolverHandle, | |
| wallet_id, | |
| network, | |
| ); | |
| runtime().block_on(wallet.sign_message(&address, &message, &signer)) | |
| }); | |
| let result = unwrap_option_or_return!(option); | |
| let signature = unwrap_result_or_return!(result); | |
| let wallet = unwrap_option_or_return!(CORE_WALLET_STORAGE.with_item(handle, Clone::clone)); | |
| let address = unwrap_result_or_return!(read_utf8(address_ptr, address_len, |e| { | |
| PlatformWalletError::MessageSigningAddressInvalid { | |
| address: "<non-UTF-8>".to_string(), | |
| reason: format!("address is not valid UTF-8: {e}"), | |
| } | |
| })); | |
| // A non-UTF-8 message is reported against the known address, so the | |
| // error identifies the signing target requested by the caller. | |
| let message = unwrap_result_or_return!(read_utf8(message_ptr, message_len, |e| { | |
| PlatformWalletError::MessageSigningFailed { | |
| address: address.clone(), | |
| reason: format!("message is not valid UTF-8: {e}"), | |
| } | |
| })); | |
| let network = wallet.network(); | |
| let wallet_id = wallet.wallet_id(); | |
| // SAFETY: `signer_addr` came from `core_signer_handle`, which the caller | |
| // keeps alive for this call; the signer is dropped before returning. | |
| let signer = MnemonicResolverCoreSigner::new( | |
| signer_addr as *mut MnemonicResolverHandle, | |
| wallet_id, | |
| network, | |
| ); | |
| let signature = unwrap_result_or_return!( | |
| runtime().block_on(wallet.sign_message(&address, &message, &signer)) | |
| ); |
source: ['codex']
| // An address is never legitimately empty, so its pointer must be present. | ||
| // `message_ptr` is deliberately NOT checked: a null pointer with | ||
| // `message_len == 0` is the empty message, which is signable (see | ||
| // [`read_utf8`]). It is only dereferenced when the length is non-zero. |
There was a problem hiding this comment.
🟡 Suggestion: Cover the null-pointer empty-message ABI contract
The new FFI contract intentionally accepts message_ptr == NULL when message_len == 0, which is required by the Swift empty-array marshalling path. No endpoint test exercises this behavior, and the Rust wallet tests only sign "hello". Add a direct core_wallet_sign_message regression test that passes a null message pointer with zero length, asserts success, and verifies the returned signature against signed_msg_hash(""); this prevents a future unconditional check_ptr!(message_ptr) from silently breaking empty messages.
source: ['codex']
| let message: String = match env.get_string(&message) { | ||
| Ok(v) => v.into(), | ||
| Err(_) => { | ||
| let _ = env.exception_clear(); | ||
| throw_sdk_exception(env, 1, "message string was invalid"); | ||
| return ptr::null_mut(); | ||
| } | ||
| }; | ||
|
|
||
| // Both cross as UTF-8 bytes + length (no trailing NUL), so an embedded | ||
| // NUL cannot truncate what actually gets signed. | ||
| let address_bytes = address.as_bytes(); | ||
| let message_bytes = message.as_bytes(); |
There was a problem hiding this comment.
🟡 Suggestion: Do not silently alter malformed Java UTF-16 messages
env.get_string(...).into() decodes JNI modified UTF-8 through jni::JavaStr. For a Java/Kotlin String containing an unpaired UTF-16 surrogate, the JNI crate's decoder fails and falls back to String::from_utf8_lossy, silently replacing the input before its bytes are signed. A verifier encoding the same Java string through the platform UTF-8 encoder can therefore hash different replacement bytes, contradicting the API's claim that the message is signed verbatim. Marshal a Kotlin-produced UTF-8 ByteArray, or read the UTF-16 code units strictly and reject unpaired surrogates rather than signing transformed text.
source: ['codex']
2 participants
What this adds
CoreWallet::sign_message— classic Dash signed messages ("proof you own thisaddress"), exposed through the full binding chain. Given a P2PKH address the
wallet holds keys for and an arbitrary UTF-8 message, it returns the 65-byte
recoverable signature, base64-encoded — the same wire format as dashj's
ECKey.signMessageand Dash Core'ssignmessageRPC, so existing verifiers(
verifymessage, dashjsignedMessageToKey) accept it as-is.Primary consumer: the Android/iOS wallets' CrowdNode integrations, where API
withdrawal and email registration are signed-message proofs of address
ownership (currently done in dashj on Android; this makes the kotlin-sdk /
swift-sdk route possible).
API surface
CoreWallet::sign_message(&self, address, message, signer) -> Result<String>core_wallet_sign_message(handle, address_ptr/len, message_ptr/len, core_signer_handle, out_signature)ManagedPlatformWallet.signMessage(address: String, message: String, coreSignerHandle: Long): String(suspend)ManagedCoreWallet.signMessage(address: String, message: String) throws -> String(Swift takes no signer parameter deliberately — the swift-sdk convention is a
per-call internal
MnemonicResolver, as at every other seed-backed call site.)Design decisions
serialization come from
dashcore::sign_message; the recovery id is foundby trying the four candidates against the signer's public key — the same
approach dashj itself uses (
ECKey.findRecoveryId). This means everySignerbackend (soft seed, future hardware/HSM) gets signed-messagesupport without needing a recoverable-signing method. Cost: at most four
recovery attempts on one 32-byte digest, off the hot path.
core_wallet_sign_messagetakes thecaller's existing
MnemonicResolverHandle, exactly like the send paths.refused with a typed error; BIP44/BIP32/CoinJoin and DashPay receiving
accounts sign. The predicate is a local copy of the QA integration branch's
funding_privacy::is_signable_funding_account(identical body), so whenthat module lands the port converges by pure deletion.
(
ErrorSigningKeyUnavailable); malformed address →ErrorInvalidParameter;both mirrored in Kotlin (
DashSdkError.PlatformWallet.SigningKeyUnavailable)and Swift (
signingKeyUnavailable).Why error code 31, and why the 27–30 gap
Code 31 is what the keystore rework (#4183) assigns to
ErrorSigningKeyUnavailableon the QA integration line. This PR carries thevariant at the same number so the enums converge instead of colliding; the
27–30 gap is intentional and reserved. Until #4183's guarded catch-all lands,
MessageSigningFailed(signer-internal failure) falls through toErrorUnknown— noted in a comment at the mapping site.One contract fix included
Swift's empty-string marshalling (
Array("".utf8).withUnsafeBufferPointer)yields a nil base address, which the FFI's null-check rejected — despite an
empty message being legitimately signable (matches dashj and Core).
read_utf8now short-circuits at
len == 0without dereferencing;# Safetydocs statethe contract precisely.
address_ptrremains required non-null.Verification
ECKey.signMessageand via this Rust produce the identical base64(
HzW1qnS7pYhopcbz1ZbiQY+axMMDQ2cMXBjey37IQS15OOluF6l/rfEre7Bl8AzUOwYdANkLYRelpKJmIH4kfZM=).platform-walletlib suite 498/498;platform-wallet-ffi207/207(including the new mapping tests); goldens unchanged across the v4.2-dev port.
:sdk:compileDebugKotlinclean;DashSdkErrorTestpasses.build_ios.sh --target simsucceeds — cbindgen header carries..._ERROR_SIGNING_KEY_UNAVAILABLE = 31,ManagedCoreWallet.swiftcompiles,and SwiftExampleApp installs and runs on the iOS 26.4 simulator.
has one today); end-to-end CrowdNode use happens in the wallet integrations.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes