feat(wallets): watch-only + remote-signing as signer-provider capabilities (not WalletType cases)#107
Conversation
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: Add watch-only + remote-signing wallet support (phase 1)
Reviewer: Arkana
Verdict: Request changes — one protocol-critical design issue, several high-priority items.
Overall this is well-structured additive work with good documentation. The null-check hoisting in TreeSignerSession is a clear safety improvement. However, the IRemoteSignerTransport interface has a MuSig2 nonce-handling flaw that needs resolution before merge.
🔴 CRITICAL — MuSig2 secret nonce leaks across transport boundary
Files: IRemoteSignerTransport.cs, RemoteArkadeWalletSigner.cs
The IRemoteSignerTransport interface mirrors IArkadeWalletSigner verbatim, which means:
GenerateNoncesAsync()returnsMusigPrivNonce(the secret nonce) from the remote signer back to the SDK- The SDK stores it in
TreeSignerSession._myNonces SignMusigAsync()sends theMusigPrivNonceback to the remote signer
This means the secret nonce traverses the transport twice and is held in memory on a machine that by definition is NOT the signer. For MuSig2, a leaked or replayed secret nonce allows private key extraction. This defeats the security model of remote signing.
The correct pattern: The remote signer generates the nonce internally, returns only the MusigPubNonce, stores the MusigPrivNonce internally, and uses it during SignMusig without the caller ever seeing it. The interface should be:
Task<MusigPubNonce> GenerateNoncesAsync(string walletId, OutputDescriptor descriptor, MusigContext context, ...);
Task<MusigPartialSignature> SignMusigAsync(string walletId, OutputDescriptor descriptor, MusigContext context, ...);
// No MusigPrivNonce in signatures ^^This requires TreeSignerSession to be aware that remote signers return only pub nonces from GenerateNonces (or introduce a signer capability flag), which is exactly the kind of reshape the PR description mentions for phase 2. If that reshape is deferred, the interface contract should still be correct now — don't ship an interface that forces secret nonces across a trust boundary, even if phase 1 implementations are in-process.
🟠 HIGH — Remaining signer! null-forgiving dereferences in signing paths
File: NArk.Core/Helpers/TransactionHelpers.cs
The PR correctly fixes three signer! patterns in TreeSignerSession, but three more remain in TransactionHelpers:
- Line ~41:
signer!inSignCheckpointTransaction— no null guard - Line ~234:
signer!in batch checkpoint signing loop — no null guard - Line ~420:
signer!in forfeit tx signing — no null guard
With WalletType.WatchOnly returning null from GetSignerAsync, these become reachable NRE vectors. Yes, upstream intent-creation guards (BoardingContractTransformer, DelegateContractTransformer, etc.) check for null signers before coins enter the pipeline. But defense-in-depth matters for protocol-critical paths — an NRE mid-batch or mid-forfeit-signing is worse than a clear InvalidOperationException. These should get the same ?? throw treatment as TreeSignerSession.
🟠 HIGH — No tests for protocol-critical changes
File: TreeSignerSession.cs
The null-check hoisting in TreeSignerSession touches three methods in the batch signing hot path (CreateMusigContexts, GenerateNoncesAsync, SignPartialAsync). The PR defers all tests to phase 2. For protocol-critical code — where bugs lose money — at minimum add:
- A unit test that
TreeSignerSessionthrowsInvalidOperationException(not NRE) when the signer is null - A unit test that
DefaultWalletProvider.GetSignerAsyncreturnsnullforWatchOnlyand throws forRemotewithout transport
These are cheap to write and protect against regressions in the most dangerous code paths.
🟠 HIGH — Unique index on nullable Wallet column has no filter
File: ArkWalletEntity.cs:58 (entity configuration)
The unique index is:
builder.HasIndex(w => w.Wallet).IsUnique();With Wallet now nullable, the PR relies on NULLs-are-distinct semantics. This holds for PostgreSQL, SQLite, and SQL Server — but not universally (e.g., Oracle treats NULLs as equal for unique constraints). The codebase already has the correct pattern in ArkIntentEntity:
builder.HasIndex(e => e.IntentId).IsUnique().HasFilter("\"IntentId\" IS NOT NULL");Add .HasFilter(...) here too for portability and explicitness, or document that Oracle is unsupported.
🟡 MEDIUM — Descriptor shape heuristic is fragile
File: DefaultWalletProvider.cs:121
var isHd = wallet.AccountDescriptor.Contains('*');This determines whether a watch-only/remote wallet gets HD or single-key address derivation based on whether the descriptor contains *. Edge cases:
- A descriptor with a comment or metadata containing
*→ misrouted - A multipath descriptor like
tr(xpub/<0;1>/*)→ works by coincidence
Consider using OutputDescriptor.Parse() and checking the descriptor type structurally, or at minimum validate the descriptor format at wallet creation time (CreateWatchOnlyWallet).
🟡 MEDIUM — No runtime validation of Secret/WalletType invariant
Files: ArkWalletInfo.cs, WalletFactory.cs
The docs say Secret MUST be null for WatchOnly/Remote and non-null for HD/SingleKey, but this is only enforced in the factory methods. Direct construction of ArkWalletInfo (which is a record) allows any combination:
// Compiles and runs — violates invariant
new ArkWalletInfo(Id: "x", Secret: "oops", WalletType: WalletType.WatchOnly, ...);Consider adding a validation method (or init property logic) that throws if the Secret/WalletType combination is invalid, called from the record constructor or from IWalletStorage.SaveWallet.
🟢 MINOR observations
-
CreateWatchOnlyWalletusesaccountDescriptorasId— This is consistent with the existing HD/SingleKey pattern. Good. -
Backup.razor watch-only branch — The
string.IsNullOrEmpty(_wallet.Secret)check (line ~63 in diff) fires for any wallet with no secret, which is correct. TheWalletTypedisplay in the message is a nice touch. -
RemoteArkadeWalletSignerconstructor validation — The?? throw new ArgumentNullException(...)guards onwalletIdandtransportare correct defensive coding. -
Documentation — The README and
docs/articles/wallets.mdadditions are thorough and accurate.
Summary
| Finding | Severity | Action |
|---|---|---|
| MusigPrivNonce leaks across transport | 🔴 Critical | Redesign IRemoteSignerTransport to keep secret nonces on the signer side |
signer! in TransactionHelpers |
🟠 High | Add ?? throw guards matching TreeSignerSession pattern |
| No tests for protocol-critical changes | 🟠 High | Add minimal unit tests for null-signer paths |
| Unique index missing filter | 🟠 High | Add .HasFilter() for portability |
| Descriptor shape heuristic | 🟡 Medium | Validate structurally or at creation time |
| No Secret/WalletType invariant enforcement | 🟡 Medium | Add runtime validation |
The IRemoteSignerTransport nonce issue is the blocker. The rest can be addressed incrementally, though I'd prefer the TransactionHelpers signer! fixes ship with this PR since it's the one introducing legitimate null-signer returns.
There was a problem hiding this comment.
Code Review: Add watch-only + remote-signing wallet support (phase 1)
Reviewer: Arkana
Verdict: Request changes — one protocol-critical design issue, several high-priority items.
Overall this is well-structured additive work with good documentation. The null-check hoisting in TreeSignerSession is a clear safety improvement. However, the IRemoteSignerTransport interface has a MuSig2 nonce-handling flaw that needs resolution before merge.
🔴 CRITICAL — MuSig2 secret nonce leaks across transport boundary
Files: IRemoteSignerTransport.cs, RemoteArkadeWalletSigner.cs
The IRemoteSignerTransport interface mirrors IArkadeWalletSigner verbatim, which means:
GenerateNoncesAsync()returnsMusigPrivNonce(the secret nonce) from the remote signer back to the SDK- The SDK stores it in
TreeSignerSession._myNonces SignMusigAsync()sends theMusigPrivNonceback to the remote signer
This means the secret nonce traverses the transport twice and is held in memory on a machine that by definition is NOT the signer. For MuSig2, a leaked or replayed secret nonce allows private key extraction. This defeats the security model of remote signing.
The correct pattern: The remote signer generates the nonce internally, returns only the MusigPubNonce, stores the MusigPrivNonce internally, and uses it during SignMusig without the caller ever seeing it. The interface should be:
Task<MusigPubNonce> GenerateNoncesAsync(string walletId, OutputDescriptor descriptor, MusigContext context, ...);
Task<MusigPartialSignature> SignMusigAsync(string walletId, OutputDescriptor descriptor, MusigContext context, ...);
// No MusigPrivNonce in either signature ^^This requires TreeSignerSession to be aware that remote signers return only pub nonces from GenerateNonces (or introduce a signer capability flag), which is exactly the kind of reshape the PR description mentions for phase 2. If that reshape is deferred, the interface contract should still be correct now — don't ship an interface that forces secret nonces across a trust boundary, even if phase 1 implementations are in-process.
🟠 HIGH — Remaining signer! null-forgiving dereferences in signing paths
File: NArk.Core/Helpers/TransactionHelpers.cs
The PR correctly fixes three signer! patterns in TreeSignerSession, but three more remain in TransactionHelpers:
- Line ~41:
signer!inSignCheckpointTransaction— no null guard - Line ~234:
signer!in batch checkpoint signing loop — no null guard - Line ~420:
signer!in forfeit tx signing — no null guard
With WalletType.WatchOnly returning null from GetSignerAsync, these become reachable NRE vectors. Yes, upstream intent-creation guards (BoardingContractTransformer, DelegateContractTransformer, etc.) check for null signers before coins enter the pipeline. But defense-in-depth matters for protocol-critical paths — an NRE mid-batch or mid-forfeit-signing is worse than a clear InvalidOperationException. These should get the same ?? throw treatment as TreeSignerSession.
🟠 HIGH — No tests for protocol-critical changes
File: TreeSignerSession.cs
The null-check hoisting in TreeSignerSession touches three methods in the batch signing hot path (CreateMusigContexts, GenerateNoncesAsync, SignPartialAsync). The PR defers all tests to phase 2. For protocol-critical code — where bugs lose money — at minimum add:
- A unit test that
TreeSignerSessionthrowsInvalidOperationException(not NRE) when the signer is null - A unit test that
DefaultWalletProvider.GetSignerAsyncreturnsnullforWatchOnlyand throws forRemotewithout transport
These are cheap to write and protect against regressions in the most dangerous code paths.
🟠 HIGH — Unique index on nullable Wallet column has no filter
File: ArkWalletEntity.cs:58 (entity configuration)
The unique index is:
builder.HasIndex(w => w.Wallet).IsUnique();With Wallet now nullable, the PR relies on NULLs-are-distinct semantics. This holds for PostgreSQL, SQLite, and SQL Server — but not universally. The codebase already has the correct pattern in ArkIntentEntity:
builder.HasIndex(e => e.IntentId).IsUnique().HasFilter("\"IntentId\" IS NOT NULL");Add .HasFilter(...) here too for portability and explicitness, or document that Oracle is unsupported.
🟡 MEDIUM — Descriptor shape heuristic is fragile
File: DefaultWalletProvider.cs:121
var isHd = wallet.AccountDescriptor.Contains('*');This determines whether a watch-only/remote wallet gets HD or single-key address derivation based on whether the descriptor contains *. Edge cases:
- A descriptor with a comment or metadata containing
*→ misrouted - A multipath descriptor like
tr(xpub/<0;1>/*)→ works by coincidence
Consider using OutputDescriptor.Parse() and checking the descriptor type structurally, or at minimum validate the descriptor format at wallet creation time (CreateWatchOnlyWallet).
🟡 MEDIUM — No runtime validation of Secret/WalletType invariant
Files: ArkWalletInfo.cs, WalletFactory.cs
The docs say Secret MUST be null for WatchOnly/Remote and non-null for HD/SingleKey, but this is only enforced in the factory methods. Direct construction of ArkWalletInfo (which is a record) allows any combination:
// Compiles and runs — violates invariant
new ArkWalletInfo(Id: "x", Secret: "oops", WalletType: WalletType.WatchOnly, ...);Consider adding a validation method (or init property logic) that throws if the Secret/WalletType combination is invalid, called from the record constructor or from IWalletStorage.SaveWallet.
🟢 MINOR observations
-
CreateWatchOnlyWalletusesaccountDescriptorasId— Consistent with existing HD/SingleKey pattern. Good. -
Backup.razor watch-only branch — The
string.IsNullOrEmpty(_wallet.Secret)check fires for any wallet with no secret, which is correct. TheWalletTypedisplay in the message is a nice touch. -
RemoteArkadeWalletSignerconstructor validation — The?? throw new ArgumentNullException(...)guards are correct defensive coding. -
Documentation — The README and
docs/articles/wallets.mdadditions are thorough and accurate.
Summary
| Finding | Severity | Action |
|---|---|---|
| MusigPrivNonce leaks across transport | 🔴 Critical | Redesign IRemoteSignerTransport to keep secret nonces on the signer side |
signer! in TransactionHelpers |
🟠 High | Add ?? throw guards matching TreeSignerSession pattern |
| No tests for protocol-critical changes | 🟠 High | Add minimal unit tests for null-signer paths |
| Unique index missing filter | 🟠 High | Add .HasFilter() for portability |
| Descriptor shape heuristic | 🟡 Medium | Validate structurally or at creation time |
| No Secret/WalletType invariant enforcement | 🟡 Medium | Add runtime validation |
The IRemoteSignerTransport nonce issue is the blocker. The rest can be addressed incrementally, though I'd prefer the TransactionHelpers signer! fixes ship with this PR since it's the one introducing legitimate null-signer returns.
Kukks
changed the title
There was a problem hiding this comment.
🔍 Aggressive Code Review — #107
Reviewer: Arkana (automated, protocol-critical review)
Verdict: REQUEST CHANGES + REQUIRES HUMAN SIGN-OFF
This PR touches signing infrastructure (MuSig2, nonce lifecycle, signer dispatch). That makes it protocol-critical — bugs here lose real money. The architectural refactoring from enum-based to capability-based dispatch is a clean model, but several issues need resolution before merge.
🚨 P0 — PROTOCOL-CRITICAL: MuSig2 nonce leaks over transport boundary
IRemoteSignerTransport.cs (new file, entire interface)
The interface mirrors IArkadeWalletSigner 1:1, which means GenerateNoncesAsync returns MusigPrivNonce to the caller, and SignMusigAsync accepts it back. For local signers this is fine — everything stays in-process. For a remote signer, this means:
- Remote generates secret nonce → secret nonce crosses the wire to local
- Local stores it, extracts public nonce via
CreatePubNonce() - Local sends secret nonce back over the wire to
SignMusigAsync
The secret nonce traverses the network twice. In MuSig2, if a secret nonce is exposed, an attacker who observes both the nonce and the partial signature can extract the private key. The entire point of remote signing is that key material never leaves the signer device — but this interface forces the secret nonce out.
Recommendation: The transport interface should return MusigPubNonce from GenerateNoncesAsync (not MusigPrivNonce), and SignMusigAsync should not take a MusigPrivNonce parameter — the remote signer should store the nonce internally keyed by session/context. This is an interface-level redesign but it's fundamental to the security model of remote MuSig2 signing.
If this is intentionally deferred (e.g. the transport is always localhost / in-process IPC), document that assumption explicitly and add a // SECURITY: MusigPrivNonce crosses trust boundary warning on the interface.
🔴 P1 — Unfixed null-handling at 3 signing call sites
NArk.Core/Helpers/TransactionHelpers.cs:39-41, 233-234, 418-420
These three call sites still use signer! (null-forgiving operator) after GetSignerAsync:
var signer = await walletProvider.GetSignerAsync(coin.WalletIdentifier, cancellationToken);
await PsbtHelpers.SignAndFillPsbt(signer!, coin, ...);With this PR, GetSignerAsync legitimately returns null for watch-only wallets. If a watch-only wallet's coin reaches any of these paths — checkpoint signing (L41), Ark transaction signing (L234), or forfeit transaction signing (L420) — it will NRE at runtime instead of throwing a descriptive error.
The forfeit path (L420) is especially dangerous: a forfeit transaction is what prevents the ASP from stealing funds. An NRE here during a forfeit flow is a fund-loss vector.
Fix: Add ?? throw new InvalidOperationException($"Wallet '{coin.WalletIdentifier}' has no signer; watch-only wallets cannot sign transactions.") at all three sites, matching the pattern already applied in TreeSignerSession.cs and BatchSession.cs.
🟡 P2 — Stale doc references to nonexistent enum values
RemoteArkadeWalletSigner.cs:14 — XML doc references WalletType.Remote:
/// <see cref="WalletType.Remote"/>.This enum value doesn't exist in the reworked model. Should reference the two-axis dispatch model.
ArkWalletEntity.cs:17-18 — XML doc references both WalletType.WatchOnly and WalletType.Remote:
<item><description><see cref="Abstractions.Wallets.WalletType.WatchOnly"/>: null</description></item>
<item><description><see cref="Abstractions.Wallets.WalletType.Remote"/>: null</description></item>These are leftover from the pre-rework draft. The compiler won't catch broken <see cref> in XML docs by default — these will silently produce broken doc links.
🟡 P3 — No EF Core migration for nullable column change
ArkWalletEntity.cs:21 — Wallet changed from string (non-nullable, default "") to string? (nullable).
No migration file is included. Depending on the database provider:
- SQLite: Permissive, likely works without migration
- PostgreSQL/SQL Server: May require
ALTER COLUMN ... DROP NOT NULL
If btcpay-arkade (which uses this as a submodule with EF Core + Postgres) upgrades, it will need a migration. At minimum, document the schema change. Ideally, include the migration.
🟡 P4 — Fragile descriptor-shape heuristic for WalletType inference
WalletFactory.cs:63:
var walletType = accountDescriptor.Contains('*') ? WalletType.HD : WalletType.SingleKey;Using Contains('*') to distinguish HD from SingleKey works for canonical descriptors but is fragile:
- A descriptor with a
*in a comment or unusual position would misclassify - No validation that the descriptor is actually well-formed
Consider parsing with OutputDescriptor.Parse() and checking for derivation paths, or at minimum validating the descriptor format before inferring type.
🟡 P5 — Wallet ID = raw descriptor string
WalletFactory.cs:65:
Id: accountDescriptor,Watch-only wallet IDs are set to the full account descriptor string. Other wallet types derive deterministic IDs from the descriptor via GetOutputDescriptorFromNsec / HD derivation. This inconsistency means:
- IDs may be very long strings (full xpub descriptors)
- Lookup patterns that assume short/hashed IDs may break
- Two imports with slightly different descriptor formatting (whitespace, checksum) produce different IDs for the same wallet
Should this use the same ID derivation as CreateHdWallet / CreateSingleKeyWallet?
📝 P6 — No tests (acknowledged, but flagged for protocol-critical code)
The PR explicitly defers tests to phase 2. For signing infrastructure that touches MuSig2, forfeit paths, and the signer dispatch boundary, this is a higher-risk deferral than usual. At minimum:
- Unit test for
DefaultWalletProvider.GetSignerAsyncdispatch: local → signer, remote transport → proxy, no transport → null - Unit test for
CreateWatchOnlyWalletdescriptor inference - Integration test for
TreeSignerSessionwith a null signer (should throw early, not NRE)
✅ What's good
- The two-axis model is clean. Separating key-derivation from signing-capability is the right call. The pre-rework
WalletType.WatchOnly/WalletType.Remoteconflated orthogonal concerns. - TreeSignerSession null-checks hoisted correctly. The
?? throwpattern before the per-VTXO loop prevents late NREs — good defensive improvement. - HD signer defensive guard (
HierarchicalDeterministicWalletSigner.cs:24-25). ExplicitInvalidOperationExceptionon null mnemonic is better than a crypticNullReferenceExceptionfromMnemonicconstructor. - Optional DI for transport.
IRemoteSignerTransport? remoteSignerTransport = nullmeans existing setups are unaffected. - Docs and README are thorough. The dispatch table in the README is clear.
Summary of required changes
| # | Severity | Issue | Action |
|---|---|---|---|
| P0 | 🚨 Protocol-critical | MusigPrivNonce leaks over remote transport | Redesign interface or document + mitigate |
| P1 | 🔴 Bug | 3 unfixed signer! sites in TransactionHelpers.cs |
Add ?? throw null guards |
| P2 | 🟡 Correctness | Stale WalletType.Remote/WalletType.WatchOnly in docs |
Fix XML doc references |
| P3 | 🟡 Operational | No EF migration for nullable column | Add migration or document |
| P4 | 🟡 Robustness | Contains('*') descriptor heuristic |
Parse properly or validate |
| P5 | 🟡 Consistency | Watch-only wallet ID = raw descriptor | Align with other wallet ID derivation |
| P6 | 📝 Coverage | No tests for new signing dispatch | Add before or alongside merge |
There was a problem hiding this comment.
Follow-up Review: Rework commit 4de84d8d
Reviewer: Arkana
Verdict: Request changes — the rework is a clear architectural improvement, but the critical MuSig2 nonce issue from the original review is unresolved, and the rework introduces two new bugs.
✅ What the rework improves
The two-axis model (derivation flavour × signing capability) is the right design. Removing WatchOnly/Remote from WalletType and moving capability detection to KnowsWalletAsync + runtime probing in GetSignerAsync is cleaner, more composable, and eliminates the "WatchOnly or Remote" address-provider sniffing. The cascading logic in DefaultWalletProvider.GetSignerAsync (local → remote → null) is easy to reason about.
🔴 CRITICAL — MuSig2 secret nonce still crosses transport boundary (unchanged)
Files: IRemoteSignerTransport.cs:84-96, RemoteArkadeWalletSigner.cs:28-32
This was the primary finding from the initial review and is not addressed in the rework. GenerateNoncesAsync still returns MusigPrivNonce to the SDK, and SignMusigAsync still accepts it back. The security argument for remote signing is that private material never leaves the signer — returning the secret nonce to the caller defeats this entirely.
If this is intentionally deferred to phase 2 (interface reshape), add a // TODO: SECURITY comment on both methods and document the limitation in the interface remarks — don't ship a "remote signer" interface that silently leaks nonces without flagging it.
🟠 NEW — Entity doc comments reference removed enum values
File: ArkWalletEntity.cs (diff lines 463-464)
The rework removed WalletType.WatchOnly and WalletType.Remote from the enum, but the ArkWalletEntity.Wallet property's new XML doc still references both:
/// <item><description><see cref="...WalletType.WatchOnly"/>: null — no signing material.</description></item>
/// <item><description><see cref="...WalletType.Remote"/>: null — signing proxied to an IRemoteSignerTransport.</description></item>These are dead cref links that won't resolve. The doc should say the field is null/empty when no local signing material is present, matching the ArkWalletInfo.Secret doc.
🟠 NEW — RemoteArkadeWalletSigner doc references removed enum
File: RemoteArkadeWalletSigner.cs:13
/// <see cref="WalletType.Remote"/>.WalletType.Remote no longer exists. Should reference the capability-based routing in DefaultWalletProvider instead.
🟠 STILL OPEN — signer! null-forgiving in TransactionHelpers
File: NArk.Core/Helpers/TransactionHelpers.cs:41, :234, :420
These three signer! dereferences are still reachable when a watch-only wallet (now Secret=null with no transport) returns a null signer from GetSignerAsync. The TreeSignerSession paths are correctly guarded now (the rework kept the ?? throw pattern from earlier commits), but the TransactionHelpers paths are not.
🟡 STILL OPEN — Unique index on now-nullable Wallet column
File: ArkWalletEntity.cs:58
Wallet changed from string to string?, but the unique index has no filter:
builder.HasIndex(w => w.Wallet).IsUnique();Multiple watch-only wallets with Wallet = null will violate this constraint on databases where NULLs aren't considered distinct in unique indexes. Add .HasFilter(...) or document that only PostgreSQL/SQLite/SQL Server are supported.
🟡 STILL OPEN — No tests for protocol-critical changes
The null-check hoisting in TreeSignerSession, the DefaultWalletProvider cascading logic, and the KnowsWalletAsync probe path are all untested. These are cheap unit tests that protect against regressions in the most dangerous code paths.
Summary
| # | Severity | Status | Issue |
|---|---|---|---|
| 1 | 🔴 CRITICAL | Unchanged | MuSig2 MusigPrivNonce leaks across transport |
| 2 | 🟠 HIGH | New in rework | Entity doc references WalletType.WatchOnly/Remote (removed) |
| 3 | 🟠 HIGH | New in rework | RemoteArkadeWalletSigner doc references WalletType.Remote (removed) |
| 4 | 🟠 HIGH | Still open | signer! in TransactionHelpers.cs — NRE vectors |
| 5 | 🟡 MEDIUM | Still open | Unique index on nullable Wallet column needs filter |
| 6 | 🟡 MEDIUM | Still open | No tests for protocol-critical changes |
Items 2–3 are trivial doc fixes. Item 1 is the blocker — if it's deferred to phase 2, document the limitation explicitly with // TODO: SECURITY markers.
Additive, non-breaking: new flavours signal observe-only wallets and remote-signing wallets to IWalletProvider. Existing HD and SingleKey values are unchanged. XML docs spell out the contract: HD/SingleKey require a non-null/non-empty Secret; WatchOnly/Remote require a null/empty Secret.
Breaking: ArkWalletInfo.Secret is now string?. The EfCore ArkWalletEntity.Wallet column follows suit. WatchOnly and Remote wallets MUST store a null/empty Secret; HD and SingleKey wallets MUST store a non-null/non-empty Secret — documented in the XML on both types. HierarchicalDeterministicWalletSigner now throws a descriptive InvalidOperationException if a caller somehow ends up with a null mnemonic on an HD-typed wallet (previously would have NRE inside ConcurrentDictionary.GetOrAdd). Existing storage rows with non-null Wallet values keep working unchanged; the unique index on Wallet still applies but allows multiple NULL entries (per PostgreSQL/SQLite/SQL Server NULLs-are-distinct semantics).
IRemoteSignerTransport mirrors IArkadeWalletSigner with an extra walletId parameter on every method, so a single transport instance can serve multiple wallets (e.g. a server-side signing service, a hardware bridge, a browser extension shared across tabs). RemoteArkadeWalletSigner is a pure proxy IArkadeWalletSigner: holds the walletId + transport, forwards every call. No local state. Used by DefaultWalletProvider for WalletType.Remote wallets in a follow-up commit.
GetSignerAsync now returns null for WatchOnly and proxies to a registered IRemoteSignerTransport for Remote. The transport is an OPTIONAL constructor parameter — consumers that don't use remote signing don't have to register one. A Remote wallet loaded without a registered transport throws an InvalidOperationException with a clear message identifying the missing DI registration. GetAddressProviderAsync routes WatchOnly and Remote wallets to the existing address providers by descriptor shape: a bare tr(pubkey) goes to SingleKeyAddressProvider, an HD-style tr([fp/path]xpub/0/*) goes to HierarchicalDeterministicAddressProvider. This makes watch-only/remote wallets end-to-end usable for address derivation and VTXO observation; signing operations remain controlled by GetSignerAsync.
…rSession Two GetSignerAsync call sites in TreeSignerSession previously stored the returned IArkadeWalletSigner? and then deref'd it with '!' inside the per-VTXO loop, which would NRE midway through batch participation on a watch-only/missing-signer wallet. Hoist the null check above the loop and throw a clear InvalidOperationException explaining that watch-only wallets cannot participate in batch signing. The first call site already had a ?? throw but with a less descriptive message — unify the wording across all three hoist points. No behavior change for HD/SingleKey wallets — they always return a non-null signer.
Mirrors the existing CreateWallet factory shape: takes an account descriptor (plus optional destination + metadata), validates the destination if present, and returns an ArkWalletInfo with Secret=null and WalletType.WatchOnly. The wallet's Id and AccountDescriptor are both set to the descriptor — matching how CreateNsecWallet/CreateHdWallet derive Id from their descriptor. No CreateRemoteWallet helper in this PR — for now consumers construct ArkWalletInfo directly with WalletType.Remote; a factory can come once the transport-registration pattern beds in.
- README.md: new table of the four wallet types + minimal example for each, including how to register an IRemoteSignerTransport and construct a Remote ArkWalletInfo. - docs/articles/wallets.md: same table + sections on WatchOnly (descriptor shapes), Remote signing (transport contract + DI), plus an IRemoteSignerTransport implementation skeleton. - samples/NArk.Wallet Backup.razor: explicit 'no backup required' branch for watch-only/remote wallets so the sample doesn't render an empty mnemonic grid when there's no Secret to display.
…etType
Per review feedback: WalletType.WatchOnly / WalletType.Remote conflated two
orthogonal axes — key derivation (SingleKey vs HD) and key availability
(local key / remote signer / no key). The smell showed up where the address
provider had to recover the derivation by sniffing the descriptor string
('tr(pubkey)' vs '*'), because the enum had collapsed it. HD + watch-only
and HD + remote-signed are valid configurations the previous shape couldn't
express cleanly.
Reorient: the data type stays minimal; capability is answered by asking the
signer-provider, not by a flag on the wallet record.
- NArk.Abstractions/Wallets/WalletType.cs: revert to SingleKey | HD only.
Doc updated to spell out the two-axis model.
- NArk.Abstractions/Wallets/ArkWalletInfo.cs: Secret stays nullable; doc
says null/empty means "no local key — watch-only or remote-signed,
distinguished at runtime by IWalletProvider.GetSignerAsync."
- NArk.Abstractions/Wallets/IRemoteSignerTransport.cs: add
Task<bool> KnowsWalletAsync(walletId, ct). The transport itself is the
source of truth for "do I sign for this wallet?". Provider probes it
when Secret is null to distinguish remote-signed from watch-only —
no flag on the wallet encodes it.
- NArk.Core/Wallet/DefaultWalletProvider.cs: GetSignerAsync dispatches on
Secret first (local signer), then probes the transport if registered
(remote signer proxy), then returns null (watch-only). GetAddressProvider
dispatches purely on WalletType — no descriptor sniffing.
- NArk.Core/Wallet/WalletFactory.cs: CreateWatchOnlyWallet infers
WalletType from the descriptor's wildcard at creation time (one place,
not at every address-provider call).
- samples/.../Backup.razor: the Secret-null branch is now generic ("watch-only
or remote-signed") — the page no longer cares which, only that there's no
local secret to reveal.
- README.md + docs/articles/wallets.md: rewritten around the two-axis model.
All 404 existing tests pass.
Kukks
force-pushed
the
feat/watch-only-remote-signing
branch
from
4de84d8 to
8ce7370
Compare
There was a problem hiding this comment.
Follow-up Review (iterative check)
Reviewer: Arkana
Verdict: Request changes — all previously flagged issues remain open.
Checked the current diff against my prior reviews. The code is unchanged — no issues have been addressed. Quick status:
| # | Severity | Issue | Status |
|---|---|---|---|
| 1 | 🚨 CRITICAL | MusigPrivNonce leaks across IRemoteSignerTransport (GenerateNoncesAsync returns it, SignMusigAsync accepts it back) |
Open — still lines 84 & 57 of IRemoteSignerTransport.cs |
| 2 | 🔴 HIGH | signer! null-forgiving in TransactionHelpers.cs:41, :234, :420 — NRE vectors for watch-only wallets, especially forfeit path at L420 |
Open — not in diff at all |
| 3 | 🟠 MEDIUM | RemoteArkadeWalletSigner.cs:13 references removed WalletType.Remote |
Open |
| 4 | 🟠 MEDIUM | ArkWalletEntity.cs:19-20 references removed WalletType.WatchOnly and WalletType.Remote |
Open |
| 5 | 🟡 MEDIUM | Unique index on nullable Wallet column (ArkWalletEntity.cs:63) has no .HasFilter() — multiple NULL wallets will violate constraint on some DBs |
Open |
| 6 | 📝 LOW | No tests for protocol-critical signing dispatch paths | Open |
Minimum for unblock: Items 2–4 are trivial fixes (< 10 lines total). Item 1 needs either an interface redesign or explicit // TODO: SECURITY markers documenting the trust-boundary assumption.
…t out)
- TransactionHelpers (3 sites): replace `signer!` null-forgiving with
explicit `?? throw InvalidOperationException` so a watch-only wallets
null signer surfaces at the call site (checkpoint sign, batch sign,
forfeit sign) instead of an NRE deeper in PsbtHelpers. Forfeit path
message specifically notes watch-only wallets cant participate in
batches demanding a forfeit, since that was the reviewers most
protocol-sensitive finding.
- ArkWalletEntity.Wallet doc: drop dead crefs to the removed
WalletType.WatchOnly / WalletType.Remote enum values, restate the
capability-on-provider invariant.
- ArkWalletEntitys unique index on the now-nullable Wallet column:
add `.HasFilter("\"Wallet\" IS NOT NULL")` so SQL Server doesnt
reject multiple null-Wallet rows (Postgres/SQLite treat NULL as
distinct already; the filter is harmless there and makes intent
explicit).
- RemoteArkadeWalletSigner doc: rewrite the dead cref to
WalletType.Remote in terms of the capability cascade (Secret null +
IRemoteSignerTransport.KnowsWalletAsync claims it).
- IRemoteSignerTransport: add `// TODO: SECURITY` markers on
GenerateNoncesAsync + SignMusigAsync and a <remarks> SECURITY
LIMITATION block pointing at #111 (MuSig2 private nonce leak). The
interface redesign is split out per reviewer allowance.
Item 6 (tests for protocol-critical signing dispatch) intentionally
deferred — reviewer ranked it LOW and the minimum-unblock set was
items 2-4.
|
@arkanaai — addressed in
The MuSig2 nonce redesign is the right architecture but a contract change that wants its own PR — splitting it keeps this one focused on the WalletType-as-capability rework that's the substance of #107. The phase-1 |
There was a problem hiding this comment.
Follow-up Review: Commit 5e1f7ff (addressing items 2–5 from prior review)
Reviewer: Arkana
Verdict: ✅ Approve — all actionable items from the previous review are resolved.
Verified fixes
| # | Item | Status |
|---|---|---|
| 2 | TransactionHelpers — three signer! null-forgiving sites replaced with ?? throw InvalidOperationException |
✅ TransactionHelpers.cs:39-42, :236-239, :424-428 — clear error messages, forfeit path specifically calls out watch-only wallets |
| 3 | ArkWalletEntity unique index — .HasFilter("\"Wallet\" IS NOT NULL") added |
✅ ArkWalletEntity.cs:71 — SQL Server safe, harmless on Postgres/SQLite |
| 4 | Dead WalletType.WatchOnly/WalletType.Remote crefs in ArkWalletEntity doc |
✅ Cleaned up, now references the capability-on-provider model |
| 5 | RemoteArkadeWalletSigner dead cref |
✅ Rewritten to describe the Secret == null + KnowsWalletAsync dispatch |
MuSig2 nonce leak (item 1)
Correctly split to issue #111. The interface now has:
<remarks>block with SECURITY LIMITATION warning (IRemoteSignerTransport.cs:21-31)// TODO: SECURITYmarkers on bothGenerateNoncesAsync(:98-101) andSignMusigAsync(:64-66)
This is acceptable for phase 1 — the interface is new, has no production consumers yet, and the limitation is loudly documented.
Remaining note
Tests for the null-signer dispatch paths are still deferred. Not blocking merge, but should land before any consumer integrates IRemoteSignerTransport in production.
Review of
|
| # | My finding | Status | Verification |
|---|---|---|---|
| 1 🚨 | MuSig2 MusigPrivNonce leaks across transport |
Split out → #111 | // TODO: SECURITY markers on both GenerateNoncesAsync and SignMusigAsync; <remarks> SECURITY LIMITATION block on IRemoteSignerTransport naming #111 and explicitly warning consumers not to rely on nonce confidentiality. This is the right call — the interface redesign is a contract-breaking change that deserves its own focused PR. The markers are clear and traceable. ✅ |
| 2 🔴 | Three signer! NRE vectors in TransactionHelpers.cs |
Fixed | All three sites (checkpoint sign L39, batch sign L233, forfeit L421) now use ?? throw InvalidOperationException with contextual messages. The forfeit message specifically calls out watch-only wallets — good. ✅ |
| 3 🟠 | RemoteArkadeWalletSigner dead WalletType.Remote cref |
Fixed | Doc now describes the Secret=null + KnowsWalletAsync capability cascade correctly. ✅ |
| 4 🟠 | ArkWalletEntity dead WalletType.WatchOnly/Remote crefs |
Fixed | Rewritten as capability-on-provider description, matches ArkWalletInfo.Secret doc. ✅ |
| 5 🟡 | Unique index on nullable Wallet — missing .HasFilter() |
Fixed | .HasFilter("\"Wallet\" IS NOT NULL") added, consistent with the existing ArkIntentEntity pattern. ✅ |
| 6 📝 | No tests for protocol-critical signing dispatch | Deferred | Accepted — this was ranked LOW in my minimum-unblock set. Still want to see these land in phase 2 before any production use. |
Verdict: changes requested → approved (from my side)
The PR is in good shape. The three active NRE vectors are gone, the docs are clean, the SQL index is correct. Item 1 is properly mitigated with visible warnings pointing at #111.
IRemoteSignerTransport contract. Still requires a human approval before merge — Arkana sign-off is not a substitute here.
|
Architecture follow-up captured in #112 — the hybrid-signer composition direction ( Together with #111 (MuSig2 nonce redesign), these are the two phase-2 architecture follow-ups touching this surface; #111 is the smaller contract change so it likely lands first, then #112 composes on top. |
* deps(nark): bump NArk SDK to PR #107 (watch-only + remote signing) Bumps the vendored NArk submodule to commit 6553067 — HEAD of arkade-os/dotnet-sdk#107 (feat/watch-only-remote-signing). That branch adds: - WalletType.WatchOnly and WalletType.Remote - IRemoteSignerTransport (transport abstraction for proxied signing) - RemoteArkadeWalletSigner (IArkadeWalletSigner over the transport) - WalletFactory.CreateWatchOnlyWallet(descriptor, ...) - DefaultWalletProvider accepts an optional IRemoteSignerTransport - ArkWalletInfo.Secret is nullable for watch-only / remote wallets - ArkWalletEntity.Wallet (secret column) is nullable in EF storage The plugin changes that consume these new surfaces follow in subsequent commits on this branch. * feat(wallets): add watch-only wallet mode + remote-signer DI seam Adds a WalletSetupMode discriminator to the initial-setup view model and threads it through the controller so merchants can opt to import a **watch-only** wallet by pasting an account descriptor (no seed). When WalletSetupMode.WatchOnly is selected, the controller routes to NArks WalletFactory.CreateWatchOnlyWallet helper (added in arkade-os/dotnet-sdk#107) instead of CreateWallet. The resulting ArkWalletInfo carries WalletType.WatchOnly and a null Secret; persistence uses the existing IWalletStorage path (no schema change -- the NArk PR already made ArkWalletEntity.Wallet nullable). Backwards-compatible: existing form posts that dont carry a Mode field default to WalletSetupMode.Auto and the existing content-routed flow (empty -> generate / nsec / mnemonic / Arkade-address / wallet-id) is unchanged. The wizard view shows the watch-only option as a third tile in the "I have a wallet" group. Also introduces IBTCPayAppDeviceProxy : IRemoteSignerTransport -- the cross-plugin seam BTCPayServer.Plugins.App will implement to bridge signing calls to a connected BTCPayApp device. The plugin registers IRemoteSignerTransport in DI with a soft fallback: it resolves to a registered IBTCPayAppDeviceProxy if present; otherwise throws a clear "install the App companion plugin" error only when signing is actually attempted on a Remote wallet (the factory is lazy). Pure WatchOnly wallets never touch the transport, so a store with no companion plugin remains fully bootable and usable read-only. NArks DefaultWalletProvider already takes IRemoteSignerTransport? as an optional ctor param (per dotnet-sdk#107), so the DI seam is just wiring -- no NArk changes are required here. * docs: README + CHANGELOG for watch-only mode; bump 2.1.18 -> 2.2.0 - README: new "Watch-Only Wallet" subsection under Wallet Types, with setup steps and a descriptor example. Calls out the dependence on the BTCPayServer.Plugins.App companion plugin for signing, and the no-signer-available read-only behaviour. - CHANGELOG: 2.2.0 entry summarising the watch-only feature + the NArk submodule bump to dotnet-sdk#107. - csproj: Version 2.1.18 -> 2.2.0 (minor bump for additive surface -- no behaviour change for existing wallets). * fix(di): defer "no IBTCPayAppDeviceProxy" error to signer-call time ASP.NET Core DI eagerly invokes the registered factory for any optional ctor parameter; the previous throw-from-factory aborted the entire host build on stores with no companion App plugin, because NArks DefaultWalletProvider mentions IRemoteSignerTransport? in its ctor and DI doesnt see the C# default-value sugar. Replace the throw with a MissingDeviceProxyTransport sentinel whose four IRemoteSignerTransport methods each throw a clear "install the App companion plugin" InvalidOperationException. DefaultWalletProvider only wraps the transport in a RemoteArkadeWalletSigner for WalletType.Remote, and only inside GetSignerAsync -- so: - Plain WatchOnly wallets never reach the sentinel; the plugin loads cleanly on stores without the companion plugin. - Remote wallets surface the App-specific message at exactly the moment a signer is asked for, never at container build time. E2E CI on the previous commit failed with the throw fired during BTCPay test-server boot ("No IBTCPayAppDeviceProxy is registered"). With this fix the plugin boots and only Remote-wallet signing paths see the message. * deps(nark): bump to NArk master (absorbs #110, #113, #114) Pulls the NNark submodule from PR #107's HEAD (`6553067`) to NArk master `6b69b9e`, absorbing three merged PRs since then: - #110 — arkd v0.9.3 → v0.9.6 (the wallet sidecar split + crash fix that recovery work already validated). - #113 — keep MuSig2 secret nonce inside the signer. Reshapes IRemoteSignerTransport: SignMusigAsync drops MusigPrivNonce, GenerateNoncesAsync returns MusigPubNonce, both take a caller-supplied string sessionId. - #114 — compose signers from IDescriptorSigningSources. The internal signer-class hierarchy was rewritten behind an unchanged IArkadeWalletSigner; nothing in this plugin directly consumes the renamed concrete types (Bip39SigningSource / NsecSigningSource / RemoteTransportSigningSource), so the bump is consumer-transparent. #113 resolves the 🔴 nonce-leak flagged by Arkana's review of this PR (was: "block merge on dotnet-sdk#107 nonce fix"). The transport boundary no longer carries MusigPrivNonce. Plugin-side updates that the new IRemoteSignerTransport shape forces: - MissingDeviceProxyTransport: add the KnowsWalletAsync probe (returns false — a watch-only wallet stays watch-only end-to-end if no companion plugin is registered); update SignMusigAsync / GenerateNoncesAsync signatures for the sessionId model; keep the three signing methods throwing "install the App companion plugin" as defence-in-depth. - ArkPlugin.cs: update the DI-registration comment to reflect the sentinel's new "claim nothing" semantics, since WalletType.Remote / .WatchOnly no longer exist after #107. Also addresses Arkana's 🟡 medium: don't auto-enable Lightning when the imported wallet is watch-only. Arkade-backed Lightning needs batch participation (signing), so a watch-only wallet without a paired remote signer would accept LN invoices at checkout but fail at settlement after the customer has already committed to paying. The merchant can still flip Lightning on manually once a companion signer is paired. * test(e2e): inline a minimal DockerHelper instead of linking the SDK's The submodule bump to NNark master pulls in dotnet-sdk#108's rewrite of NNark's DockerHelper.cs, which now calls FulmineLiquidityHelper.EnsureArkLiquidity and uses System.Net.Http.Json's PostAsJsonAsync. Both newly fail in the parent NArk.E2E.Tests context — FulmineLiquidityHelper isn't linked, and the PostAsJsonAsync extension collides with the legacy Microsoft.AspNet.WebApi.Client overload pulled in transitively via BTCPayServer.Tests's ProjectReference. The "submodule bump propagates fixes for free" abstraction was leaky once the SDK's helper grew dependencies outside what we link. We only call DockerHelper.CreateLndInvoice (and its same-file Exec dependency), so inline a ~30-LOC copy under NArk.E2E.Tests/Common/DockerHelper.cs in the same namespace and drop the <Compile Include=...> from the csproj. Call sites are unchanged. * db: migration absorbing NNark #113's filtered Wallet index + nullable column Submodule bump pulled in NArk.Storage.EfCore's reshape of ArkWalletEntity: - `Wallet` column became nullable (#107 — watch-only/remote wallets have no local secret, so the descriptor column carries the only identity). - The unique index on `Wallet` became filtered: `WHERE Wallet IS NOT NULL`, so NULL values can coexist on SQL Server / Postgres without violating the constraint (#113's review fixes for #104). EF Core's PendingModelChangesWarning surfaced the gap at test startup. This migration captures the same shape on the plugin's ArkPluginDbContext: - DropIndex IX_Wallets_Wallet (the old non-filtered unique). - AlterColumn Wallet → nullable. - CreateIndex IX_Wallets_Wallet unique filtered "Wallet" IS NOT NULL. Generated via ./add-migration.sh AbsorbNNarkModelChanges; no manual edits. * ci(e2e): persist nigiri on $GITHUB_PATH for subsequent steps start-env.sh exports PATH only in its own shell, so the test step that falls through to the nigiri-funding path in EnsureArkdCliReadyAsync (when arkd off-chain balance < 10k sats) fails with "Failed to start a process with file path nigiri". Master usually skips this fallback because the arkd CLI wallet happens to already have balance from earlier suite setup; the recovery branch consistently trips it, exposing the workflow gap. The fix is independent of cache hit/miss: whether the binary was cache-restored or freshly built, it lives at submodules/NNark/regtest/_build/nigiri/build/nigiri-linux-amd64 with a `nigiri` symlink next to it. * test(e2e): retry ark settle through the faucet/mine propagation race EnsureArkdCliReadyAsync funds the arkd CLI by piping `nigiri faucet -> nigiri rpc --generate 6 -> ark settle` back-to-back. When the faucet TX hasnt reached bitcoind mempool by the time we mined, the boarding UTXO is unconfirmed and arkd settles with no inputs: "error: fees (0) exceed total amount (0)". Master almost always takes the early-return path (off-chain balance >=10k from prior suite setup), so the race is unseen there. The recovery branch consistently falls through to this slow path, exposing it intermittently. Wrap settle in a 5-attempt loop that distinguishes the racey "fees (0)" signature: on hit, wait 2s + mine one extra block, then retry. Idempotent (ark settle is). Any non-racey settle failure throws on the first attempt as before. * ci(e2e): probe arkd container name instead of pinning to stale `ark` Real root cause for PR #70 CheatModePay flake: the recovery branch's regtest pin (dc23da2) changed docker-compose.arkd-override.yml from a hardcoded `container_name: ark` to `container_name: ${ARK_CONTAINER:-arkd}`, which (with ARKD_IMAGE set in .env.regtest, so ARK_CONTAINER auto-derives to "arkd") renames the container to `arkd`. The workflow had been pinning ARKADE_CHEAT_ARK_CONTAINER=ark, so the plugin's cheat-mode `docker exec` hit "No such container: ark" once the test fell through to the slow nigiri-funding path of EnsureArkdCliReadyAsync — past the early-return that protects master. Probe both names (mirroring the test's own ResolveArkdContainerAsync) and feed the result into the test step. Survives future regtest bumps in either direction without re-pinning.
1 participant
Summary
Reworked after self-review (see commit `rework(wallets): capability lives at the signer provider, not on WalletType`). `WalletType` stays as the key-derivation axis only (`SingleKey` | `HD`); watch-only and remote-signing are answered at the signer-provider boundary, not encoded as separate enum cases.
Why the rework
The original draft added `WalletType.WatchOnly` and `WalletType.Remote`, which conflated two orthogonal axes:
The smell was already visible in the diff: the address provider had to recover the derivation by sniffing the descriptor (`tr(pubkey)` vs `*`), because the enum had collapsed it. Configurations like HD + watch-only and HD + remote-signed couldn't be expressed without that recovery step.
New model
`GetSignerAsync` dispatch:
```
Secret non-empty → local signer (HD or SingleKey)
Secret null/empty + IRemoteSignerTransport.KnowsWalletAsync(id) → RemoteArkadeWalletSigner proxy
otherwise → null (watch-only)
```
Capability lives on the interface. The data record carries no `IsWatchOnly` / `IsRemote` flag — `Secret = null` is the only signal, and the transport is the source of truth for the remote/watch-only distinction.
What's in this PR
What's NOT in this PR
Test plan