feat(swaps): deterministic Boltz preimages via BIP-340 sign-and-hash#116
Conversation
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — feat(swaps): deterministic Boltz preimages via BIP-340 sign-and-hash
Verdict: Request changes. This is protocol-critical code (swap preimage derivation, VHTLC claiming). The design is sound — sign-and-hash with BIP-340 is the right shape, and reusing the existing Sign(descriptor, hash) surface avoids bloating consumer contracts. But there are gaps that need closing before merge.
🔴 P0 — Must fix
1. Chain swap recovery is missing
DerivePreimageAsync is wired at all three Initiate*Swap call sites (reverse, BtcToArk, ArkToBtc), but the recovery path in RestoreSwaps only re-derives for reverse swaps:
SwapsManagementService.cs:670— gated onrestored.IsReverseSwapMapRestoredSwap(line ~720) returnsnullfor anything other than"reverse"or"submarine"— chain swaps fall through
This means chain swaps created with deterministic preimages cannot recover those preimages from seed via RestoreSwaps. The docs (swaps.md) claim the scheme covers "reverse and chain swaps" — that's true at creation time but not at recovery time.
Either:
- (a) Extend
MapRestoredSwap+ the recovery branch to handle chain swap types, or - (b) Scope the docs/PR description to "reverse swaps only for recovery" and leave chain swap recovery as a follow-up
2. Zero test coverage on DerivePreimageAsync
This is the linchpin of the entire feature. A wrong derivation = silently unclaimable VHTLCs = lost funds. There are no unit tests for:
- Idempotency: same
(walletId, descriptor, index)→ same preimage across calls - Descriptor isolation: different descriptors → different preimages
- Index variation: different indices → different preimages
- Watch-only fallback:
GetSignerAsync == null→ random (non-deterministic) preimage
The method is private, but it's testable via internal + InternalsVisibleTo, or by extracting it to a static helper that takes a signer. Protocol-critical derivation schemes need determinism tests before shipping.
🟡 P1 — Should fix
3. SHOULD vs MUST on IRemoteSignerTransport.SignAsync
IRemoteSignerTransport.cs:81 says:
Implementations SHOULD sign with
aux_randset to null
But docs/articles/swaps.md says:
IRemoteSignerTransport.SignAsyncMUST useaux_rand=null
This is a money-losing failure mode (recovery silently fails). The interface contract should say MUST. Integrators read the XML doc, not the markdown. If a hardware signer randomises aux_rand, every swap preimage is irrecoverable and the user finds out only when they try to restore — too late.
4. descriptor.ToString() stability as a derivation input
SwapsManagementService.cs:136 — the preimage is bound to descriptor.ToString(). If OutputDescriptor.ToString() changes its formatting across SDK versions (whitespace, key encoding, normalization), a wallet restored on a newer SDK version will derive a different message hash → different signature → different preimage → silent claim failure.
Consider either:
- Documenting
OutputDescriptor.ToString()as a stability invariant that must not change across versions, or - Using a canonical byte serialization (e.g. the raw pubkey bytes + derivation path) instead of the string representation
5. Recovery doesn't enrich the VHTLC contract entity
The recovery path at SwapsManagementService.cs:680-690 attaches the preimage to swap.Metadata[Preimage] only. The existing manual path EnrichReverseSwapPreimage (line ~790) also recreates the VHTLCContract with the preimage bytes and persists it via _contractStorage.SaveContract. The sweeper reads from metadata (BoltzSwapProvider.cs:1532), so claiming works, but the contract entity remains un-enriched — inconsistent with the manual path. If any code path ever reads the preimage from the contract rather than swap metadata, it'll silently fail.
🟢 P2 — Nits / observations
6. Concatenation without length prefix
SwapsManagementService.cs:133-138 — tag || descriptor || index concatenation is technically ambiguous (a descriptor ending in bytes that look like a u32 LE index could collide with a shorter descriptor + different index). Practically safe since the tag is constant-length and descriptors are structured strings, but a length-prefixed scheme would be more robust for a v1 spec you're committing to forever.
7. PR body is stale
The first commit's scheme was SHA-256(signer.Sign(descriptor, SHA-256("NArk-Boltz-Preimage-v1"))). The second commit changed it to include descriptor || index in the signed message. The PR body still shows the old scheme. Future readers will be confused.
8. First-mover across SDKs
The dotnet-sdk is the first to implement deterministic swap preimages. ts-sdk still uses randomBytes(32) at arkade-swaps.ts:457,2321 and skips unresolvable swaps during restore (swap-manager.ts:1033). Worth tracking as a cross-SDK initiative — the derivation spec should be portable so wallets restored across SDK implementations produce the same preimages.
Summary
The cryptographic design is clean: BIP-340 sign-and-hash with domain separation, no new abstraction surface, backward-compatible optional parameters. The main gaps are (1) chain swap recovery not wired, (2) no tests on the derivation function, and (3) the SHOULD/MUST inconsistency on the remote signer contract.
There was a problem hiding this comment.
Arkana Follow-up Review — commit a05f1fb (tag rename NArk → Arkade)
New commit assessment
The tag rename from NArk-Boltz-Preimage-v1 to Arkade-Boltz-Preimage-v1 is correct and well-motivated — SDK-agnostic naming is the right call for cross-SDK preimage recovery interop. Code change is trivial (const + docs), no issues.
Previous findings — status
None of the P0s or P1s from my first review have been addressed. Summarizing what's still open:
🔴 P0 — Still blocking
1. Chain swap recovery still missing
MapRestoredSwap (SwapsManagementService.cs:710-714) still returns null for anything other than "reverse" or "submarine". Chain swaps created with deterministic preimages via InitiateBtcToArkChainSwap / InitiateArkToBtcChainSwap cannot be recovered from seed. The preimage is derived at create time but lost on restore.
2. Zero test coverage on DerivePreimageAsync
No test files in any of the three commits. This is the derivation function that determines whether users can recover funds. Needs idempotency, descriptor-isolation, and watch-only-fallback tests at minimum.
🟡 P1 — Still open
3. SHOULD vs MUST — IRemoteSignerTransport.cs:82
XML doc still says SHOULD. Internal comment at SwapsManagementService.cs:120 says MUST. Docs at swaps.md say MUST. The XML doc is what integrators read — this should be MUST.
4. descriptor.ToString() stability — no response yet.
5. Recovery doesn't enrich VHTLC contract entity — no response yet.
Maintaining request-changes. The tag rename is good, but P0-1 (chain swap recovery) and P0-2 (tests) are still blockers.
Kukks
force-pushed
the
feat/deterministic-swap-preimages
branch
from
da268c2 to
b4506f8
Compare
There was a problem hiding this comment.
Arkana Follow-up Review — commit b4506f8 (ci: nigiri → denigiri migration)
New commit assessment
The CI migration from nigiri/Go to denigiri/Node is straightforward. Container rename ark → arkd, chopsticks → mempool /api/ base path, submodule bump — all make sense. A few things were missed:
🟡 P1 — Incomplete migration (stale references)
1. SharedDelegationInfrastructure.cs:37 — still references ./regtest/start-env.sh
The same error-message fix applied to SharedArkInfrastructure.cs:29 and SharedSwapInfrastructure.cs:37 was not applied here. Should be node regtest/regtest.mjs start --profile boltz,delegate.
2. docs/articles/development.md — entire file still references old infra
Lines 28, 32, 39–41, 47 all reference start-env.sh, clean-env.sh, stop-env.sh, and nigiri. This is the developer onboarding doc — if someone follows it they'll get a "command not found". Should be updated to the new node regtest/regtest.mjs commands.
3. CLAUDE.md:54,58 — references nigiri
Lines 54 and 58 mention nigiri as a dependency. If the repo's own AI coding assistant instructions are stale, agents will give wrong setup advice.
🟢 P2 — Nits
4. SharedArkInfrastructure.cs:8 — ChopsticksEndpoint naming
The variable is still named ChopsticksEndpoint but chopsticks no longer exists (replaced by mempool). The inline comment correctly explains the change, but renaming to e.g. EsploraEndpoint or MempoolEndpoint would prevent future confusion.
5. DockerHelper.cs:71,129 — comments reference "nigiri lnd"
XML doc on CreateLndInvoice (line 71) and PayLndInvoice (line 129) still say "nigiri lnd container". Cosmetic, but worth cleaning up in the same pass.
6. .env.regtest:3,9 — references start-env.sh and nigiri
Comments in this file still reference the old infra. Minor since it's a config file, but contributes to the overall stale-reference problem.
Previous P0/P1 findings — still open
None of the protocol-critical findings from my first two reviews have been addressed. This commit is infra-only, so that's expected, but they remain blockers:
- P0-1: Chain swap recovery not wired in
RestoreSwaps - P0-2: Zero test coverage on
DerivePreimageAsync - P1-3:
SHOULDvsMUSTonIRemoteSignerTransport.SignAsync - P1-4:
descriptor.ToString()stability as derivation input - P1-5: Recovery doesn't enrich VHTLC contract entity
Maintaining request-changes. The CI migration commit needs the stale references cleaned up (P1-1 and P1-2 at minimum), and the protocol-critical items from prior reviews are still outstanding.
There was a problem hiding this comment.
Arkana Follow-up Review — commits b21dca2 + a5f17d5
New commits assessment
Commit 5 (test(e2e): drive bitcoin-cli with denigiri regtest flags) — Good infra fix. Centralises the regtest connection flags into DockerHelper.BitcoinCli with proper error checking. One minor inconsistency noted below.
Commit 6 (fix(swaps): derive preimage from x-only pubkey, not descriptor.ToString()) — Excellent fix. This directly addresses my prior P1-4 finding and is the right design: anchoring on the canonical 32-byte x-only pubkey instead of the non-canonical descriptor.ToString(). The message format tag(24B) || xonly(32B) || u32le(4B) is fixed-layout with no ambiguity — also resolves the P2-6 length-prefix concern from my first review. The BuildPreimageMessage extraction as internal static is clean and testable.
The new PreimageDerivationTests are well-designed:
Message_IsTagPlusXOnlyPubkeyPlusIndexLittleEndian— pins the wire formatMessage_IndexEncodedAsUInt32LittleEndian— validates endiannessMessage_IsIndependentOfDescriptorStringForm— the critical test: proves HD signing descriptor and bare receiver descriptor produce the same message, which is the exact failure mode this commit fixes
Prior findings status
| # | Finding | Status |
|---|---|---|
| P0-1 | Chain swap recovery missing | ❌ Still open |
| P0-2 | No test coverage on derivation | ✅ Addressed (wire format + descriptor independence tests) |
| P1-3 | SHOULD vs MUST on aux_rand |
❌ Still open — IRemoteSignerTransport.cs:82 says SHOULD, swaps.md:128 says MUST |
| P1-4 | descriptor.ToString() stability |
✅ Fixed — switched to x-only pubkey |
| P1-5 | Recovery doesn't enrich VHTLC contract entity | ❌ Still open |
| P2-6 | Concatenation without length prefix | ✅ Resolved — fixed-layout format now |
| P2-7 | Stale PR body | ❌ Still open |
New findings in these commits
1. MineBlocks inconsistency (minor)
DockerHelper.cs — MineBlocks still routes through Exec() (with the spread BitcoinCliArgs) instead of using the new BitcoinCli() helper:
public static async Task MineBlocks(int count = 20, CancellationToken ct = default)
=> await Exec("bitcoin", [.. BitcoinCliArgs, "-generate", count.ToString()], ct);Exec doesn't check exit codes or trim output. BitcoinCli does both. A silent MineBlocks failure would make tests hang waiting for confirmations rather than fail fast. Should use BitcoinCli(["-generate", count.ToString()], ct) for consistency.
Remaining blockers
The P0-1 (chain swap recovery) is still the primary blocker. The MapRestoredSwap method only handles "reverse" and "submarine" types — chain swaps that were created with deterministic preimages cannot recover those preimages via RestoreSwaps. The commit message for a5f17d5 specifically mentions the restore scenario, and the new x-only-pubkey scheme makes it possible to recover correctly, but the RestoreSwaps code path doesn't exercise it for chain swaps.
The P1-3 (SHOULD → MUST on aux_rand) is a one-word fix with outsized impact — integrators read the XML doc, not the markdown. A hardware signer that randomises aux_rand silently breaks every swap recovery. This should be MUST in the interface contract.
Maintaining request-changes on P0-1 and P1-3. The preimage derivation scheme itself is now solid — the x-only-pubkey anchor is the right call.
There was a problem hiding this comment.
Arkana Follow-up Review — commits b21dca20, a5f17d54, 3bbcd18e
Three new commits since my last review. The big one is a5f17d54 which fixes the descriptor.ToString() instability I flagged as P1-4.
New commits assessment
a5f17d54 — fix(swaps): derive preimage from x-only pubkey ✅ Good fix
This directly addresses my prior P1-4 finding. The change is correct:
BuildPreimageMessagenow anchors onExtract(descriptor).XOnlyPubKey.ToBytes()(canonical 32-byte x-only key) instead ofdescriptor.ToString()(non-canonical, differs between HD signing descriptor and bare receiver descriptor at restore time).- Extracted as
internal staticpure function — testable, reviewable, and reusable. PreimageDerivationTests.cspins the wire format with 3 tests:Message_IsTagPlusXOnlyPubkeyPlusIndexLittleEndian— byte-level format correctnessMessage_IndexEncodedAsUInt32LittleEndian— index encodingMessage_IsIndependentOfDescriptorStringForm— the critical invariant: HD signing descriptor vs bare receiver descriptor → same message
One subtlety worth noting: the test's HD descriptor uses Extract() which resolves the wildcard at position 0 via hd.GetPubKey(0, _ => null, out _). This works because both create-time (GetNextSigningDescriptor) and restore-time (contract.Receiver) ultimately resolve to the same x-only key for the swap. The test correctly demonstrates this invariant.
b21dca20 — test(e2e): bitcoin-cli denigiri connection flags ✅ Clean
Centralizes bitcoin-cli invocations behind DockerHelper.BitcoinCli() with proper -regtest -rpcuser=admin1 -rpcpassword=123 flags. Good dedup. Fixes 8 test failures from the denigiri migration.
Minor nit: MineBlocks at DockerHelper.cs:59-60 still uses Exec() directly with BitcoinCliArgs instead of routing through BitcoinCli(). Exec() doesn't throw on non-zero exit, so mine failures are silently swallowed. Should either use BitcoinCli(["-generate", count.ToString()], ct) for consistency, or document that mine is intentionally best-effort.
3bbcd18e — docs(swaps): update scheme description ✅ Correct
Docs now accurately describe xonly_pubkey(32) instead of descriptor.ToString(). Cross-SDK interop rationale is clear.
Previously flagged items — status
| # | Finding | Status |
|---|---|---|
| P0-1 | Chain swap recovery missing (MapRestoredSwap returns null for chain types, RestoreSwaps:691 gated on IsReverseSwap) |
Still open |
| P0-2 | Zero test coverage on derivation | Addressed — PreimageDerivationTests covers BuildPreimageMessage. End-to-end DerivePreimageAsync tests (idempotency through actual signing, watch-only fallback) are still missing but the critical format invariant is now pinned. |
| P1-3 | SHOULD vs MUST on IRemoteSignerTransport.SignAsync:82 aux_rand |
Still open — still says SHOULD, docs say MUST. This is a money-losing mismatch. |
| P1-4 | descriptor.ToString() stability |
Fixed ✅ |
| P1-5 | Recovery doesn't enrich VHTLC contract entity | Still open |
Verdict
The x-only pubkey fix is solid and well-tested — that was the biggest technical risk. The CI/test infrastructure changes are clean.
Maintaining request-changes because P0-1 (chain swap recovery) and P1-3 (SHOULD→MUST on aux_rand) are still outstanding. P0-1 is the more important one: chain swaps created with deterministic preimages can't recover them from seed.
🤖 Arkana iterative review — focused on commits since b4506f8
Closes the TODO at BoltzSwapsService.cs:83 ("deterministic hash somehow
instead?"). Until now reverse and chain swaps generated their preimages
via RandomUtils.GetBytes(32). That works for the happy path but loses the
preimage on crash before SwapMetadata.Preimage is persisted, and a wallet
re-imported from seed has no way to claim outstanding VHTLCs it
rediscovers via Boltz /v2/swap/restore.
The new scheme derives the preimage deterministically from the wallet's
existing signing surface — no new abstraction:
preimage = SHA-256( signer.Sign( receiver_descriptor, SHA-256("NArk-Boltz-Preimage-v1") ) )
BIP-340 Schnorr with aux_rand=null is deterministic per (key, message)
so same descriptor + same wallet → same preimage. The signature reveals
nothing about the key beyond the preimage itself, and the domain-separated
tag prevents collision with any other use of the signing key. Pinning a
"-v1" suffix in the tag leaves room to ship a "-v2" later that ALSO
tries the v1 derivation on recovery, keeping pre-existing on-chain VHTLCs
claimable.
Plumbing:
- BoltzSwapsService: the three swap-create methods (CreateReverseSwap,
CreateBtcToArkSwapAsync, CreateArkToBtcSwapAsync) now take an optional
byte[]? preimage parameter. null falls back to RandomUtils.GetBytes(32)
so existing callers (and watch-only / no-signer scenarios) keep
working unchanged.
- SwapsManagementService: a single private DerivePreimageAsync helper
fetches a signer for the wallet, signs the tag-hash with the
descriptor key, and SHA-256s the signature. If GetSignerAsync returns
null (watch-only) it falls back to a random preimage. The helper is
invoked at all three Initiate*Swap call sites.
- Restore path (RestoreSwaps): after ReconstructContract resolves the
receiver descriptor for a reverse swap, re-derive the candidate
preimage, verify SHA-256(candidate) == restored.PreimageHash, and
attach it to swap.Metadata[Preimage] on match. Mismatch (legacy
random preimage, or wrong descriptor matched) leaves the metadata
out; EnrichReverseSwapPreimage remains the manual path.
Submarine swaps are unaffected — Boltz controls that preimage and reveals
it on LN payment, so there's nothing for us to derive.
Watch-only wallets can still create swaps (with random preimages); they
just don't get the deterministic-recovery guarantee until a signer is
paired. Remote-signed wallets work the same way they sign any other
message: the transport's SignAsync produces a deterministic Schnorr sig
that hashes to the same preimage, so recovery works for remote wallets
too without any IRemoteSignerTransport changes.
The IArkadeWalletSigner / IDescriptorSigningSource surfaces stay
unchanged — the derivation lives in the swap layer where it's used, and
relies only on the existing Sign(descriptor, hash) API.
Two small but meaningful changes to the v1 derivation message:
- Descriptor folded into the signed message in addition to the tag.
Domain separation now defends against accidental signature reuse
even within this codebase (a future feature that signs a
descriptor-derived hash for some unrelated proof can't repurpose
the same signature). The descriptor is also what would scope the
preimage if the tag ever got reused outside our control.
- u32 little-endian index baked into the message (always 0 today).
Forward-compatibility for multi-preimage-per-descriptor flows
without a scheme bump: when a caller wants two preimages from one
descriptor, they pass index=1 and recovery iterates 0..N.
So the v1 scheme is now:
message = SHA-256( "NArk-Boltz-Preimage-v1"
|| descriptor.ToString()
|| u32_le(index) )
sig = BIP-340-Schnorr(descriptor_key, message, aux_rand=null)
preimage = SHA-256(sig)
DerivePreimageAsync gains an explicit `uint index` parameter; all four
call sites (three Initiate*Swap + the RestoreSwaps recovery branch)
pass index=0. The restore path leaves a comment noting that a future
multi-preimage variant should iterate index=0..MAX here.
Also documents the determinism requirement on IRemoteSignerTransport.SignAsync:
implementations MUST sign with aux_rand=null for remote-signed wallets
to recover outstanding preimages from seed. The contract was implicit
before — implementations that randomise (hardware wallet side-channel
hardening) would silently break recovery; now it's explicit in XML doc.
docs/articles/swaps.md updated to describe the bundled message
construction and the remote-signer caveat.
NArk is the .NET SDK's internal shorthand; using it in the on-the-wire derivation tag locks the scheme to one specific SDK implementation. The preimage is rooted in BIP-340 signing of a fixed message — there's nothing about it that's .NET-specific. Renaming the tag to "Arkade- Boltz-Preimage-v1" lets any Arkade SDK (ts-sdk, go-sdk, rust-sdk, NArk) implement the same scheme byte-for-byte and recover swaps that another SDK created, which is the entire point of having a documented spec for the message construction. Arkade is the protocol brand per CLAUDE.md; Boltz is the provider (future Loop/Submarine/etc. would have their own tag); -v1 stays as the version hook. Pre-merge: nothing has shipped under the old tag yet, so this is a free rename — no v1/v2 fallback needed. docs/articles/swaps.md updated to make the cross-SDK interop intent explicit so any future implementer knows the tag is canonical and not an SDK-specific opaque value.
…String() The deterministic-preimage message bundled UTF8(descriptor.ToString()), which is non-canonical: the HD signing descriptor used at swap creation and the bare receiver descriptor a wallet reconstructs at restore time stringify differently for the same key (origin/path/checksum/key form all vary). The re-derived preimage then differed from the one Boltz holds, so reverse-swap restore silently failed to recover the preimage. Anchor the message on the canonical x-only public key extracted from the descriptor instead: PreimageTag || xonly(32) || u32le(index). Create-time and restore-time now agree, and the scheme is reproducible by any Arkade SDK (a descriptor string is NBitcoin-specific; a pubkey is universal). The descriptor is still the signing-key selector passed to signer.Sign — only the hashed message changed. Scheme stays v1 (unreleased). Extracts BuildPreimageMessage as an internal pure function + tests pinning the wire format and descriptor-form independence.
Kukks
force-pushed
the
feat/deterministic-swap-preimages
branch
from
3bbcd18 to
f7577ba
Compare
There was a problem hiding this comment.
Arkana Follow-up Review — rebase with major new code (503ed10..f7577ba)
The branch was rebased and pulled in ~3,600 new lines beyond the preimage commits I previously reviewed: BoltzSwapProvider partial-class refactor, BoltzOperationClassifier, WalletRecoveryService, expanded E2E chain-swap tests, IndexerVtxoDiscoveryProvider rewrite, and a BatchManagementService race fix. Reviewed all of it.
Previously flagged items — status
| # | Finding | Status |
|---|---|---|
| P0-1 | Chain swap recovery missing in MapRestoredSwap |
❌ Still open — line 763-769: only "reverse" and "submarine" are mapped; chain types fall through to null. Preimage re-derivation at line 733 still gated on restored.IsReverseSwap. |
| P0-2 | No test coverage on derivation | ✅ Fixed — PreimageDerivationTests pins wire format + descriptor independence |
| P1-3 | SHOULD vs MUST on aux_rand |
❌ Still open — IRemoteSignerTransport.cs:82 says SHOULD; swaps.md says MUST |
| P1-4 | descriptor.ToString() stability |
✅ Fixed — switched to x-only pubkey |
| P1-5 | Recovery doesn't enrich VHTLC contract entity | ❌ Still open |
New code — protocol-critical findings
🟢 APPROVE: ARK→BTC VHTLC address verification (BoltzSwapsService.cs:~281)
The CreateArkToBtcSwapAsync path now independently reconstructs the VHTLC contract from Boltz's response and validates computedAddress == lockupDetails.LockupAddress. This prevents a malicious Boltz from providing a lockup address that doesn't match the expected VHTLC — a real fund-theft vector the old code was vulnerable to. Excellent hardening.
🟢 APPROVE: BatchManagementService race fix (BatchManagementService.cs:~170)
The reconciliation on StreamStartedEvent — re-subscribing all active intent topics — fixes a real race where an intent reaching WaitingForBatch during the gRPC stream-connect window had its topic subscription silently dropped, causing a permanent settle stall. The UpdateStreamTopics add is idempotent, so re-adding is safe. Correct fix, well-commented.
🟢 APPROVE: BoltzOperationClassifier (BoltzOperationClassifier.cs)
Clean separation of the action-routing decision from inline if/else chains. Test coverage in BoltzOperationClassifierTests is exemplary — every Boltz status × swap type combination is pinned. The narrowing of chain swap refund triggers to swap.expired only (with transaction.lockupFailed routed through renegotiation first) is correct per Boltz's protocol.
🟢 APPROVE: IndexerVtxoDiscoveryProvider rewrite (IndexerVtxoDiscoveryProvider.cs)
The Lazy<Task> → SemaphoreSlim + nullable fix eliminates a real bug where a faulted first GetServerInfoAsync call permanently poisoned all subsequent probes. The cross-product candidate generation ({current + deprecated signers} × {current + legacy delay}) with mainnet-only legacy-delay probing matches ts-sdk behavior. Correct.
🟢 APPROVE: BoltzSwapProvider partial-class split
Verified all methods are preserved. The _spendingService field removal is correct (dead code). The submarine refund destination caching via GetOrDeriveRefundDestinationAsync fixes the orphan contract leak on retries. Status mapping change (operational statuses → null instead of Failed at restore time) is correct — lets the poll loop take corrective action.
New code — issues to address
🟡 P1-NEW-1: No delegate contract test coverage (IndexerVtxoDiscoveryProviderTests.cs)
BuildCandidates has a delegate-contract branch that generates ArkDelegateContract variants for each configured delegate descriptor. None of the 4 tests pass a RecoveryDelegateConfig, so that code path is completely untested. If the delegate contract construction is wrong, recovery will silently miss delegated VTXOs — funds stuck until manual intervention.
🟢 P2-NEW-1: SQL interpolation in DockerHelper (DockerHelper.cs)
SetBtcToArkChainSwapExpiredWithLockup and SetArkToBtcChainSwapExpiredWithLockup use direct string interpolation into SQL ($"... WHERE id = '{swapId}'") while TrySetBoltzSwapStatus correctly uses psql -v variable binding. Test-only, zero practical risk (swap IDs are alphanumeric), but worth making consistent.
Remaining blockers
P0-1 is still the primary blocker. Chain swaps are created with deterministic preimages via InitiateBtcToArkChainSwap / InitiateArkToBtcChainSwap, but MapRestoredSwap returns null for chain swap types, and the preimage re-derivation is gated on IsReverseSwap. A wallet restored from seed that had outstanding chain swaps will silently skip them — the user discovers the problem when their VHTLC expires unclaimed.
P1-3 remains a one-word fix (SHOULD → MUST at IRemoteSignerTransport.cs:82) with outsized impact — integrators read the XML doc, not the markdown.
Maintaining request-changes on P0-1 and P1-3.
🤖 Arkana iterative review — focused on new code since rebase
1 participant
Closes the long-standing TODO at
BoltzSwapsService.cs:83("deterministic hash somehow instead?"). Until now reverse and chain swaps generated their preimages viaRandomUtils.GetBytes(32); this loses the preimage on crash before persistence and leaves a wallet re-imported from seed unable to claim outstanding VHTLCs it rediscovers via/v2/swap/restore.Scheme (v1)
The signed input bundles three things:
Arkade+Boltz), not SDK-scoped, so any Arkade SDK (ts-sdk, go-sdk, rust-sdk, NArk) implementing the same scheme can derive the same preimage and recover swaps the .NET SDK created. Versioned (-v1) so a future scheme bump can ship as-v2while recovery still tries v1 for older swaps.descriptor.ToString()is non-canonical: the HD signing descriptor used at create time and the bare receiver descriptor a restore reconstructs stringify differently for the same key (origin / path / checksum / key form all vary), which would make the re-derived preimage diverge and silently break recovery. The pubkey is canonical and universal, so create-time and restore-time agree and any Arkade SDK can reproduce it. (The descriptor is still passed tosigner.Signto select the key — only the hashed message changed.)0today; baked into v1 so recovery iteration is forward-compatible without a scheme bump.BIP-340 with
aux_rand=nullis deterministic per(key, message), so same(wallet, pubkey, index)always yields the same preimage. The signature reveals nothing about the key beyond the preimage itself.Why this shape
Considered (and rejected, with reviewer help):
SHA-256(pubkey)alone — unsafe; the public key isn't secret (it's on-chain), so a leak would compromise every past and future swap on that key. Signing first roots the preimage in secret key material.IArkadeWalletSigner.DerivePreimagemethod — bloats a consumer-facing contract for one subsystem's needs; remote signers would need aNotSupportedExceptionpath that breaks recovery for them.IDeterministicPreimageSourcecapability sibling + probe on signer — added one method toIArkadeWalletSignerfor one feature; pattern not used elsewhere in this codebase.Sign-and-hash uses the existing
Sign(descriptor, hash)API. No new abstraction. Works forBip39SigningSource,NsecSigningSource, andRemoteTransportSigningSourceuniformly — remote-signed wallets get deterministic preimages and recovery for free if the transport signs withaux_rand=null(see below).Plumbing
BoltzSwapsService— three swap-create methods (CreateReverseSwap,CreateBtcToArkSwapAsync,CreateArkToBtcSwapAsync) gain an optionalbyte[]? preimageparameter.nullfalls back toRandomUtils.GetBytes(32), so existing callers + watch-only/no-signer scenarios keep working.SwapsManagementService— single privateDerivePreimageAsync(walletId, descriptor, uint index, ct)helper fetches a signer for the wallet, builds the bundled message, signs the SHA-256 of it with the descriptor key, and SHA-256s the signature.GetSignerAsync == null(watch-only) → random preimage fallback. Wired at all threeInitiate*Swapcall sites withindex: 0.RestoreSwaps) — afterReconstructContractresolves the receiver descriptor for a reverse swap, re-derive the candidate preimage (index: 0), verifySHA-256(candidate) == restored.PreimageHash, and attach toswap.Metadata[Preimage]on match. Mismatch leaves it out;EnrichReverseSwapPreimageremains the manual path. A future multi-preimage variant should iterateindex=0..MAXhere.Remote-signer determinism
Local sources (
Bip39SigningSource,NsecSigningSource) already sign BIP-340 withaux_rand=nullso signatures are byte-identical per(key, hash). Remote-signer transports must honour the same convention — hardware wallets that randomiseaux_rand(for side-channel resistance) will silently break recovery for their wallets. The requirement is now explicit inIRemoteSignerTransport.SignAsync's XML doc; recovery on such wallets is a known phase-1 gap.Scope
IArkadeWalletSigner/IDescriptorSigningSourcesurfaces stay unchanged. OnlyIRemoteSignerTransport.SignAsyncgets a determinism note in XML (no breaking change).Test plan
dotnet build NArk.sln— cleandotnet test NArk.Tests— 418 passed, 0 failedDocs updated:
docs/articles/swaps.mdgot a "Deterministic preimages" section under Restore After Data Loss describing the bundled message construction, the cross-SDK interop intent, and the remote-signer caveat.Infra: regtest migration to denigiri (added in this PR)
Bumps the
regtestsubmodule to the arkade-regtest #27 (denigiri) head, replacing the nigiri binary + shell scripts with the zero-dependency Node orchestrator, and migrates the E2E harness accordingly:node regtest/regtest.mjs start --profile boltz,delegate/clean; failure-log containerark→arkd./api/;DockerHelper.BitcoinClicentral helper runningbitcoin-cli -regtest -rpcuser=admin1 -rpcpassword=123(denigiri's btcpayserver image), routed through every faucet/mine call site;arkdcontainer name.E2E on denigiri: 38 passed / 1 skipped / 0 failed in ~6 min (vs ~14 on nigiri).