feat(wallets): compose signers from IDescriptorSigningSources

[Kukks]

Closes #112. Draft for review.

Summary

Pushes signer composition down one layer. IArkadeWalletSigner is now always a CompositeArkadeWalletSigner built from one or more IDescriptorSigningSources. Each source answers CanProvideAsync(descriptor) and exposes the signing operations rooted in the key it owns; the composite dispatches each call to the first source that claims the descriptor.

The three previous concrete signers collapse into signing sources:

Signing source	Origin	CanProvideAsync
Bip39SigningSource	BIP-39 mnemonic	descriptor origin's master fingerprint matches
NsecSigningSource	Nostr nsec (single key)	descriptor's x-only pubkey matches
RemoteTransportSigningSource	IRemoteSignerTransport	transport.KnowsWalletAsync(walletId)

DefaultWalletProvider builds the composition automatically — adds the matching local source if Wallet.Secret is set, then a remote source if an IRemoteSignerTransport claims the wallet. Order is significant: local sources get first refusal; remote is fallback.

Why an operation-level source rather than Task<ECPrivKey> GetPrivKeyAsync as the issue sketched

The original sketch in #112 had the abstraction returning an ECPrivKey. That works fine for the local sources, but a remote source would have to round-trip the secret over the wire — defeating the cryptographic claim of remote signing. So IDescriptorSigningSource instead exposes Sign/SignMusig/GenerateNonces directly, the same shape as IArkadeWalletSigner plus a CanProvideAsync probe. (The name in the issue body was IPrivateKeyProvider; renamed mid-review to IDescriptorSigningSource since the interface never exposes a key — it describes a signing capability scoped to descriptors.)

Why IArkadeWalletSigner is unchanged

Consumers of IArkadeWalletSigner (only the TreeSignerSession path internally) don't change. The new abstraction is added strictly below the existing one. The three previous concrete signer classes are deleted because they're now equivalent to a CompositeArkadeWalletSigner wrapping the matching single source — NsecSigningSource.FromNsec is the new entry point that mirrors the old NSecWalletSigner.FromNsec static.

MuSig2 nonce store

Each local signing source keeps its own per-session ConcurrentDictionary<string, MusigPrivNonce> with the same sessionId-keyed semantics established in #111. The RemoteTransportSigningSource is stateless — the transport on the far side retains nonces. DefaultWalletProvider caches the composite per wallet.Id so the stores survive between GenerateNonces and SignMusig.

Granularity (the hybrid question)

RemoteTransportSigningSource.CanProvideAsync delegates to KnowsWalletAsync, which is wallet-grained, not descriptor-grained. So "I have a local mnemonic for view paths and a remote HWI for the spend path on the same wallet" is not expressible in this PR. The hybrid case will require extending IRemoteSignerTransport with a per-descriptor probe (KnowsDescriptorAsync-style) — flagging as a separate follow-up; this PR is the structural refactor only.

Cross-repo impact

• btcpay-arkade (this BTCPay plugin) — no source changes needed; it only consumes IArkadeWalletSigner. Submodule bump only.
• No impact on ts-sdk, go-sdk, rust-sdk — independent implementations.

Breaking change

For direct users of the SDK who construct signers manually (rare):

• HierarchicalDeterministicWalletSigner → new CompositeArkadeWalletSigner(new Bip39SigningSource(mnemonic))
• NSecWalletSigner / NSecWalletSigner.FromNsec(nsec) → new CompositeArkadeWalletSigner(NsecSigningSource.FromNsec(nsec))
• RemoteArkadeWalletSigner → new CompositeArkadeWalletSigner(new RemoteTransportSigningSource(transport, walletId))

DefaultWalletProvider callers don't need to change.

Test plan

• dotnet build NArk.sln — clean
• dotnet test NArk.Tests — 418 passed, 0 failed (incl. renamed NsecSigningSourceParityTests)
• E2E CI — green on the previous commit; rerunning post-rename

Builds on the work in #113.

[arkanaai]

Arkana Code Review — feat(wallets): compose signers from IPrivateKeyProviders

Reviewer: Arkana (aggressive review, protocol-level scrutiny)
Verdict: Request changes — one caching bug that silently breaks the hybrid feature, one performance regression on the hot path. Signing logic itself is a clean mechanical port.

---

🔴 BUG: Signer cache key doesn't capture KnowsWalletAsync result — hybrid composition is silently stale

DefaultWalletProvider.cs:70-71 (new code)

var cacheKey = $"{wallet.Id}:{wallet.Secret?.GetHashCode():x}:{remoteSignerTransport?.GetHashCode():x}";
return _signerCache.GetOrAdd(cacheKey, _ => new CompositeArkadeWalletSigner(providers));

The cache key includes remoteSignerTransport?.GetHashCode() (the object identity of the transport), but not the result of KnowsWalletAsync. Scenario:

1. Wallet has local secret, remote transport is registered in DI.
2. First GetSignerAsync call: KnowsWalletAsync returns false → providers = [Bip39KeyProvider] → composite cached with only local.
3. Wallet is later registered on the remote signer → KnowsWalletAsync now returns true.
4. Second GetSignerAsync call: cache key is identical (same wallet, same secret hash, same transport object) → cache hit → returns stale composite with no remote provider.

The hybrid feature — the stated reason for this PR — is silently broken for any wallet where the transport's claim arrives after the first GetSignerAsync call.

Fix options:

• (a) Include a boolean hasRemote in the cache key: ...:remote={knowsWallet}. This forces a cache miss when the transport flips.
• (b) Check cache before building providers. Only build providers on cache miss. Use a Lazy<Task<>> or double-check pattern.
• (c) Separate the provider-list construction into the GetOrAdd factory (tricky with async, but ConcurrentDictionary + Lazy<Task<>> is the .NET pattern).

I'd lean toward (a) for simplicity — it's one extra bool in the key and it captures the actual composition shape.

---

🟡 PERFORMANCE: Provider construction + KnowsWalletAsync on every call, even cache hits

DefaultWalletProvider.cs:51-69 (new code)

On master, a wallet with a local secret triggered an early return — the remote transport was never consulted. The new code always falls through to the remote check:

// This runs on EVERY GetSignerAsync call, even when cache will hit:
providers.Add(wallet.WalletType switch {
    WalletType.HD => new Bip39KeyProvider(wallet.Secret),  // constructs object, computes fingerprint
    ...
});

// Potentially a network round-trip, runs every time:
if (remoteSignerTransport is not null
    && await remoteSignerTransport.KnowsWalletAsync(wallet.Id, cancellationToken))

For every GetSignerAsync call on a wallet with a local secret and a registered transport:

• A new Bip39KeyProvider / NsecKeyProvider is constructed (the Bip39KeyProvider constructor calls GetExtKey() → hits the static ExtKey cache, plus derives the fingerprint).
• KnowsWalletAsync is awaited — could be a network call for remote transports.
• Both are thrown away when the cache hits.

This is a latency regression on the hot path during batch participation, where GetSignerAsync is called per VTXO.

Suggestion: Check the cache first, build providers only on miss. Something like:

var cacheKey = $"{wallet.Id}:{wallet.Secret?.GetHashCode():x}:{remoteSignerTransport?.GetHashCode():x}";
if (_signerCache.TryGetValue(cacheKey, out var cached))
    return cached;

// Build providers only on miss...

(This also reduces the urgency of the cache-key bug above — the first-hit path would still need fixing, but repeated calls wouldn't pay the construction cost.)

---

🟢 CORRECT: Signing logic is a faithful mechanical port

Verified line-by-line:

• Bip39KeyProvider: Identical key derivation (Mnemonic → ExtKey → Derive(fullPath) → ECPrivKey), same PBKDF2 caching, same nonce lifecycle (ContainsKey guard → GenerateNonce → TryAdd, TryRemove → Sign). Matches deleted HierarchicalDeterministicWalletSigner exactly.
• NsecKeyProvider: Identical nsec decode (Bech32Encoder.ExtractEncoderFromString → DecodeDataRaw → ECPrivKey.Create), same x-only match for CanProvideAsync, same EnsureMatches guard on Sign/SignMusig/GetPubKey, same parity-preserving pubkey return. Matches deleted NSecWalletSigner exactly.
• RemoteTransportKeyProvider: Pure proxy, identical to deleted RemoteArkadeWalletSigner but implementing IPrivateKeyProvider instead of IArkadeWalletSigner. Stateless as expected.
• CompositeArkadeWalletSigner: Clean dispatcher. ResolveAsync iterates providers sequentially (order-preserving, local-first as documented). Error message on no-match is descriptive.
• MuSig2 nonce lifecycle: ContainsKey-before-generate and TryRemove-on-sign guards preserved in both Bip39KeyProvider and NsecKeyProvider. Nonce reuse detection intact. Secret nonce never exposed outside provider. ✅
• IPrivateKeyProvider interface: Shape mirrors IArkadeWalletSigner plus CanProvideAsync. CancellationToken on all methods. No raw key exposure — the "PrivateKeyProvider" name is slightly misleading (as the PR notes) but the abstraction is sound.

🟢 CORRECT: No cross-repo breakage

• IArkadeWalletSigner interface is unchanged — all downstream consumers (TreeSignerSession, IntentProofHelper, PendingArkTransactionRecoveryService, PsbtHelpers, SimpleSeedWallet E2E test) are unaffected.
• IRemoteSignerTransport interface is unchanged (already updated by #113).
• No consumers of the deleted types outside dotnet-sdk. btcpay-arkade only consumes IArkadeWalletSigner via DI — submodule bump only.

🟢 CORRECT: Test coverage

Parity tests correctly renamed and updated to exercise NsecKeyProvider directly. All 418 unit tests pass. E2E pending.

ℹ️ NOTE: Protocol-critical — requires human sign-off

This PR touches transaction signing paths (MuSig2 nonces, Schnorr signatures, key derivation). Even though the signing logic is mechanically identical to the old code, per policy this requires explicit human review before merge.

---

TL;DR: Fix the cache-key bug (the hybrid feature doesn't survive a stale KnowsWalletAsync flip) and the provider-construction-on-cache-hit performance issue. Signing logic is clean.

🤖 Reviewed by Arkana

[arkanaai]

Arkana Follow-up Review — commit 8159945 (rename + cache simplification)

Focused on delta since my first review at 52ff277.

---

✅ RESOLVED: Cache key staleness / naming

Cache key simplified to wallet.Id with an explicit "caller must invalidate" contract in the comment. Combined with dropping the hybrid row from docs — this is an honest tradeoff. The old Secret.GetHashCode() component was unreliable anyway (String.GetHashCode() is randomised per-process in .NET Core), so removing it doesn't lose real safety. Acceptable.

Rename from IPrivateKeyProvider → IDescriptorSigningSource is clean. No stale references remain. Docs, tests, and all concrete types updated consistently.

---

🟡 STILL OPEN: Source construction + KnowsWalletAsync on every call, even cache hits

DefaultWalletProvider.cs:50-76

This is unchanged from the first commit. On every GetSignerAsync call:

// These run BEFORE the cache check at line 76:
var sources = new List<IDescriptorSigningSource>();          // alloc
sources.Add(new Bip39SigningSource(wallet.Secret));          // fingerprint derivation
await remoteSignerTransport.KnowsWalletAsync(...)           // potential network call
// ...
return _signerCache.GetOrAdd(wallet.Id, _ => ...);          // cache hit → all above wasted

During batch participation this method is called per-VTXO. Every call constructs a Bip39SigningSource (which derives the master fingerprint), allocates a List, and potentially round-trips KnowsWalletAsync — then GetOrAdd returns the cached signer and everything is discarded.

Fix is two lines:

if (_signerCache.TryGetValue(wallet.Id, out var cached))
    return cached;

// … build sources only on miss …

Not a blocker for merge since the Bip39SigningSource constructor hits the static ExtKey cache (PBKDF2 is amortised) and KnowsWalletAsync in current transports is likely cheap — but it's free perf left on the table and it'll bite when someone plugs in a transport with a real network round-trip.

---

🟢 Everything else

• Signing logic: unchanged from first commit, still a faithful mechanical port. ✅
• Rename coverage: 100% — no stale IPrivateKeyProvider / *KeyProvider / PrivateKeyProviders namespace references anywhere in .cs, .csproj, .md. ✅
• Cross-repo impact: none — IArkadeWalletSigner unchanged, IRemoteSignerTransport unchanged. ✅
• Test coverage: parity tests properly renamed to NsecSigningSourceParityTests. ✅

---

Verdict: Previous 🔴 is resolved. One 🟡 remains (easy perf fix). Signing correctness is clean. Still protocol-critical — requires human sign-off before merge.

🤖 Reviewed by Arkana

[arkanaai]

Arkana Follow-up Review — commit 8159945 (rename + cache simplification)

Focused on delta since my first review at 52ff277.

---

✅ RESOLVED: Cache key staleness / naming

Cache key simplified to wallet.Id with an explicit "caller must invalidate" contract in the comment. Combined with dropping the hybrid row from docs — this is an honest tradeoff. The old Secret.GetHashCode() component was unreliable anyway (String.GetHashCode() is randomised per-process in .NET Core), so removing it doesn't lose real safety. Acceptable.

Rename from IPrivateKeyProvider → IDescriptorSigningSource is clean. No stale references remain. Docs, tests, and all concrete types updated consistently.

---

🟡 STILL OPEN: Source construction + KnowsWalletAsync on every call, even cache hits

DefaultWalletProvider.cs:50-76

This is unchanged from the first commit. On every GetSignerAsync call:

// These run BEFORE the cache check at line 76:
var sources = new List<IDescriptorSigningSource>();          // alloc
sources.Add(new Bip39SigningSource(wallet.Secret));          // fingerprint derivation
await remoteSignerTransport.KnowsWalletAsync(...)           // potential network call
// ...
return _signerCache.GetOrAdd(wallet.Id, _ => ...);          // cache hit → all above wasted

During batch participation this method is called per-VTXO. Every call constructs a Bip39SigningSource (which derives the master fingerprint), allocates a List, and potentially round-trips KnowsWalletAsync — then GetOrAdd returns the cached signer and everything is discarded.

Fix is two lines:

if (_signerCache.TryGetValue(wallet.Id, out var cached))
    return cached;

// … build sources only on miss …

Not a blocker for merge since the Bip39SigningSource constructor hits the static ExtKey cache (PBKDF2 is amortised) and KnowsWalletAsync in current transports is likely cheap — but it's free perf left on the table and it'll bite when someone plugs in a transport with a real network round-trip.

---

🟢 Everything else

• Signing logic: unchanged from first commit, still a faithful mechanical port. ✅
• Rename coverage: 100% — no stale IPrivateKeyProvider / *KeyProvider / PrivateKeyProviders namespace references anywhere in .cs, .csproj, .md. ✅
• Cross-repo impact: none — IArkadeWalletSigner unchanged, IRemoteSignerTransport unchanged. ✅
• Test coverage: parity tests properly renamed to NsecSigningSourceParityTests. ✅

---

Verdict: Previous 🔴 is resolved. One 🟡 remains (easy perf fix). Signing correctness is clean. Still protocol-critical — requires human sign-off before merge.

🤖 Reviewed by Arkana

[Kukks]

Thanks @arkanaai — the remaining 🟡 is fixed in 14943b5. Two-line TryGetValue short-circuit right after LoadWallet, exactly as suggested; cache hit returns the cached composite without ever allocating sources or awaiting KnowsWalletAsync. Cache miss still runs the full composition path.

Build clean, 418 unit tests pass.

[arkanaai]

Arkana Follow-up — commit 14943b5 ✅

Verified the fix. The two-line short-circuit lands in exactly the right place:

// Right after LoadWallet, before any source construction:
if (_signerCache.TryGetValue(wallet.Id, out var cached))
    return cached;

// Cache miss → full composition path below (unchanged)
var sources = new List<IDescriptorSigningSource>();
...

This is the correct shape. Cache hit returns immediately — no List allocation, no Bip39SigningSource construction (and the fingerprint derivation that comes with it), no KnowsWalletAsync await. Cache miss still runs the full composition path unchanged. The commit message also correctly calls out the per-VTXO hot path as the motivation, which is appreciated context for anyone bisecting later.

All items resolved

🔴 Cache key staleness (hybrid composition)	✅ Resolved in 8159945
🟡 Source construction + KnowsWalletAsync on every call	✅ Resolved in 14943b5
🟢 Signing logic correctness	✅ Unchanged — still a faithful mechanical port
🟢 Rename coverage	✅ No stale references
🟢 Cross-repo impact	✅ None

Remaining gate

This is protocol-critical (MuSig2 nonces, Schnorr signing paths, key derivation) — per policy it needs a human sign-off before merge. The code is correct; this is a process gate, not a technical one. Ready to mark approved from my side once you're ready to take it out of draft.

🤖 Reviewed by Arkana

[arkanaai]

Arkana Follow-up Review — commit 14943b5 (cache-hit short-circuit)

Scope: 1 file, 7 lines — DefaultWalletProvider.cs only.

✅ LGTM — performance fix is correct

The TryGetValue short-circuit at DefaultWalletProvider.cs:54-55 cleanly addresses the 🟡 I flagged previously:

• Placement: after LoadWallet (needed for wallet.Id key), before Bip39SigningSource construction and KnowsWalletAsync round-trip. Hot-path cost is now LoadWallet + one dictionary lookup.
• Key consistency: both TryGetValue and the existing GetOrAdd use wallet.Id — no mismatch.
• Concurrency: ConcurrentDictionary semantics are preserved. TryGetValue reduces the race window vs the GetOrAdd factory path.
• MuSig2 nonce lifecycle: unchanged — cached signer instances are still reused across GenerateNonces → SignMusig calls, which is the whole point of the cache.

📝 Minor observation (not blocking)

LoadWallet (storage I/O) still runs on every GetSignerAsync call, even cache hits, because we need wallet.Id from the identifier. If per-VTXO latency becomes tight, a secondary identifier → wallet.Id lookup cache could eliminate this. The TEMP latency probes suggest you're already watching this — just noting for the record.

⚠️ Standing note

This PR as a whole modifies signing composition infrastructure (MuSig2 nonce lifecycle, signing source dispatch). Human review is still required before merge per protocol-critical code policy.

🤖 Reviewed by Arkana

[arkanaai]

Arkana Follow-up Review — commit 14943b5 (cache-hit short-circuit)

Scope: 1 file, 7 lines — DefaultWalletProvider.cs only.

✅ LGTM — performance fix is correct

The TryGetValue short-circuit at DefaultWalletProvider.cs:54-55 cleanly addresses the 🟡 I flagged previously:

• Placement: after LoadWallet (needed for wallet.Id key), before Bip39SigningSource construction and KnowsWalletAsync round-trip. Hot-path cost is now LoadWallet + one dictionary lookup.
• Key consistency: both TryGetValue and the existing GetOrAdd use wallet.Id — no mismatch.
• Concurrency: ConcurrentDictionary semantics are preserved. TryGetValue reduces the race window vs the GetOrAdd factory path.
• MuSig2 nonce lifecycle: unchanged — cached signer instances are still reused across GenerateNonces → SignMusig calls, which is the whole point of the cache.

📝 Minor observation (not blocking)

LoadWallet (storage I/O) still runs on every GetSignerAsync call, even cache hits, because we need wallet.Id from the identifier. If per-VTXO latency becomes tight, a secondary identifier → wallet.Id lookup cache could eliminate this. The TEMP latency probes suggest you're already watching this — just noting for the record.

⚠️ Standing note

This PR as a whole modifies signing composition infrastructure (MuSig2 nonce lifecycle, signing source dispatch). Human review is still required before merge per protocol-critical code policy.

🤖 Reviewed by Arkana

[arkanaai]

Arkana Code Review — PR #114

Clean structural refactor. The composition model is sound and the IDescriptorSigningSource abstraction is well-designed (operations, not raw keys — correct for remote signing). Three findings, one of which is a performance regression on the signing hot path. Flagging for human review because this is protocol-critical (VTXO signing, MuSig2 nonce lifecycle).

---

🔴 P1 — RemoteTransportSigningSource.CanProvideAsync calls KnowsWalletAsync on every dispatch

File: NArk.Core/Wallet/SigningSources/RemoteTransportSigningSource.cs:29

public Task<bool> CanProvideAsync(OutputDescriptor descriptor, CancellationToken cancellationToken = default)
    => _transport.KnowsWalletAsync(_walletId, cancellationToken);

CompositeArkadeWalletSigner.ResolveAsync calls CanProvideAsync on every Sign/SignMusig/GenerateNonces/GetPubKey invocation. For a remote-only wallet, this means every signing operation round-trips KnowsWalletAsync — which is potentially a network call (the transport interface is explicitly designed for "server-side signing service, HWI bridge, browser-extension wallet"). The old RemoteArkadeWalletSigner forwarded directly without re-probing.

During batch participation this fires per-VTXO. On a busy round with hundreds of VTXOs, you're adding hundreds of unnecessary network round-trips.

Fix: Cache the KnowsWalletAsync result at construction time (the answer won't change mid-session):

public RemoteTransportSigningSource(IRemoteSignerTransport transport, string walletId)
{
    _transport = transport ?? throw new ArgumentNullException(nameof(transport));
    _walletId = walletId ?? throw new ArgumentNullException(nameof(walletId));
}

public Task<bool> CanProvideAsync(OutputDescriptor descriptor, CancellationToken cancellationToken = default)
    => Task.FromResult(true); // Only constructed after KnowsWalletAsync already returned true in DefaultWalletProvider

Or if you want the source to be usable outside DefaultWalletProvider's pre-check:

private bool? _knowsWallet;

public async Task<bool> CanProvideAsync(OutputDescriptor descriptor, CancellationToken cancellationToken = default)
{
    _knowsWallet ??= await _transport.KnowsWalletAsync(_walletId, cancellationToken);
    return _knowsWallet.Value;
}

---

🟡 P2 — Bip39SigningSource lacks descriptor-match guard before signing (defense-in-depth)

File: NArk.Core/Wallet/SigningSources/Bip39SigningSource.cs:65-99

NsecSigningSource calls EnsureMatches(descriptor) at the top of every operation — if the composite somehow routes a mismatched descriptor, you get a clear InvalidOperationException. Bip39SigningSource does not; DerivePrivateKey will silently derive from whatever FullPath the descriptor carries, even if the fingerprint doesn't match this mnemonic. The result is a valid-looking but wrong signature — the worst kind of bug in signing code, because it fails silently downstream instead of at the source.

Yes, CompositeArkadeWalletSigner calls CanProvideAsync first. But defense-in-depth matters here — this is the path that signs forfeit transactions and VTXOs. A routing bug should throw, not silently produce garbage.

Fix: Add a fingerprint check in DerivePrivateKey (or add an EnsureMatches like NsecSigningSource):

private ECPrivKey DerivePrivateKey(OutputDescriptor descriptor)
{
    var metadata = descriptor.Extract();
    if (metadata.AccountPath is not { } accountPath || accountPath.MasterFingerprint != _masterFingerprint)
        throw new InvalidOperationException(
            $"Descriptor fingerprint does not match this Bip39SigningSource (expected {_masterFingerprint}).");
    var fullPath = metadata.FullPath
        ?? throw new InvalidOperationException("Descriptor has no full derivation path.");
    return ECPrivKey.Create(GetExtKey().Derive(fullPath).PrivateKey.ToBytes());
}

---

🟡 P3 — DefaultWalletProvider cache key change drops secret-hash → stale signer on re-import

File: NArk.Core/Wallet/DefaultWalletProvider.cs:33-34 (comment) and :87

Old cache key: $"local:{wallet.Id}:{wallet.Secret.GetHashCode():x}"
New cache key: wallet.Id

The PR description acknowledges this: "if a wallet is re-imported with different signing material the caller must explicitly invalidate." That's a weaker contract than before, and the only mitigation is "restart the host." For a BTCPay plugin that runs as a long-lived service, a user re-importing a wallet with a corrected mnemonic would silently keep signing with the old (wrong) mnemonic until the service restarts. The MuSig2 signatures would fail verification on the server side, but the error would be opaque.

Suggestion: Either keep the secret hash in the key, or add a TryInvalidate(walletId) method, or at minimum document this in a code comment that's louder than the current one (the current comment is easy to miss).

---

📝 Minor: No unit tests for new composition logic

The test rename (NSecWalletSignerParityTests → NsecSigningSourceParityTests) is good. But there are no tests for:

• CompositeArkadeWalletSigner dispatch (multiple sources, first-match semantics, no-match error)
• Bip39SigningSource.CanProvideAsync fingerprint matching
• RemoteTransportSigningSource delegation
• DefaultWalletProvider composition (local + remote sources, cache behavior)

The PR description says E2E tests pass, which covers the happy path. But the composition dispatch logic — the core of this PR — has no direct unit coverage.

---

⚠️ PROTOCOL-CRITICAL — Human review required

This PR rewires the signing path for VTXOs, forfeit transactions, and MuSig2 batch participation. The MuSig2 nonce lifecycle (generate → store → consume-on-sign) is preserved correctly from the deleted classes. The nonce-reuse guards are intact. The signing logic itself is unchanged.

But any bug in the composite dispatch (wrong source, stale cache, missed descriptor) would produce invalid signatures or — in the nonce case — leak private keys. Requires human sign-off per protocol rules.

---

tl;dr: P1 is a real hot-path regression for remote signers. P2 is defense-in-depth on signing code. P3 is a UX footgun. Fix P1 and P2; P3 is your call. Requesting changes for P1.

🤖 Reviewed by Arkana