feat(swaps): BOLT 12 submarine swap support (Arkade → Lightning offer)#105
feat(swaps): BOLT 12 submarine swap support (Arkade → Lightning offer)#105d4rp4t wants to merge 9 commits into
Conversation
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — feat(swaps): BOLT 12 submarine swap support
Solid work overall. The BOLT 12 TLV parser is well-documented, the Merkle hashing passes all four official signature-test.json vectors, and the test suite is thorough (723 lines of unit tests). The high-level swap plumbing mirrors the BOLT 11 path faithfully.
However, there are protocol-critical findings that need fixing before merge. This PR touches VHTLC construction and invoice verification — bugs here lose real money.
🔴 CRITICAL — Verification bypass in VerifyInvoiceMatchesOffer
Bolt12InvoiceParser.cs:213-265
All three verification checks can be silently skipped, allowing any invoice to pass verification against a crafted offer:
- Check 1 (issuer key match): skipped if offer has no
offer_issuer_id(TLV 22) - Check 2 (blinded path match): skipped if
pathsValueis null, empty, orParseBlindedPathLastHopsreturns zero results - Check 3 (Merkle root): skipped if
ComputeOfferIdMerkleRootreturns null (no TLVs in range 2–22)
While a valid BOLT 12 offer must carry either offer_issuer_id or offer_paths (both in range 2–22), the parser does not enforce well-formedness. A malformed or adversarially crafted offer string could bypass all verification.
Fix: Track whether at least one check was actually performed. Throw if none were:
bool anyCheckPerformed = false;
// ... set to true inside each check that actually executes ...
if (!anyCheckPerformed)
throw new InvalidOperationException(
"BOLT 12 offer has no verifiable fields (no issuer_id, no blinded paths, " +
"no offer-range TLVs) — cannot confirm invoice authenticity.");🔴 CRITICAL — Missing invoice_node_id length validation in blinded-path branch
Bolt12InvoiceParser.cs:241-248 (Check 2)
Check 1 validates invoiceNodeId.Length != 33, but the blinded-path branch (Check 2) does not:
var invoiceNodeIdBlinded = FindTlvRecord(invoiceTlv, InvoiceNodeIdType);
if (invoiceNodeIdBlinded is null)
throw ...;
// ← Missing: if (invoiceNodeIdBlinded.Length != 33) throw ...
if (!lastHops.Any(hop => invoiceNodeIdBlinded.AsSpan().SequenceEqual(hop)))A malformed invoice with an invoice_node_id of length ≠ 33 would never match any 33-byte hop key, but the error message ("does not match the final hop") would be misleading. More importantly, a 33-byte-prefix attack against a shorter field could theoretically pass SequenceEqual if the hop array were also truncated. Add the length check for consistency and defense-in-depth.
🟡 HIGH — ReadBigSize lacks bounds checking
Bolt12InvoiceParser.cs:483-496
ReadBigSize does not validate that enough bytes remain before reading multi-byte values. For the 0xFD/0xFE/0xFF cases, it blindly indexes data[pos++] up to 8 times. On truncated or malicious input, this throws a raw IndexOutOfRangeException instead of a controlled FormatException.
Since this parser processes untrusted data from Boltz (the invoice string), this should be hardened:
0xFD => pos + 2 <= data.Length
? (ulong)data[pos++] << 8 | data[pos++]
: throw new FormatException("Truncated BigSize (expected 2 more bytes)"),🟡 HIGH — Double parsing of BOLT 12 invoice
BoltzSwapsService.cs:100 + SwapsManagementService.cs:298-299
ExtractPaymentHash is called in CreateSubmarineSwapBolt12, then InitiateSubmarineSwapBolt12 calls it again on the same invoice string to get paymentHashHex for storage. This decodes the bech32 and walks the TLV stream twice. Consider returning the payment hash hex from CreateSubmarineSwapBolt12 (e.g., as part of SubmarineSwapResult) to avoid re-parsing. In the BOLT 11 path, invoice.Hash is already parsed.
🟡 MEDIUM — Significant code duplication
SwapsManagementService.cs:269-359
InitiateSubmarineSwapBolt12 is a near-copy of InitiateSubmarineSwap (lines 166–235). The only differences are:
- How the swap is created (BOLT 11 vs BOLT 12)
- How the invoice string / payment hash are obtained
The ArkSwap construction, ImportContract call, SaveSwap call, auto-pay logic, and error-handling catch block are duplicated verbatim. If the swap storage schema changes, both methods must be updated in lockstep. Extract a shared SaveAndFundSwap helper.
🟢 LOW — Minor issues
-
SubmarineSwapResult.cs: Missing trailing newline at EOF. -
Bolt12InvoiceParser.cs:106-107: Thecatch (FormatException) { throw; }inExtractPaymentHashis a no-op — it catches and immediately re-throws. Remove it. -
BoltzSwapsService.cs:89: No validation thatamountSats > 0inCreateSubmarineSwapBolt12. Boltz will reject it, but failing fast with a clear message is better than a cryptic HTTP 400. -
BoltzSwapsService.cs:69: The existing BOLT 11CreateSubmarineSwapwas changed to passinvoice.ToString()as the 4th arg. This is correct and backward-compatible sinceInvoicedefaults tonullin the record — but the BOLT 11InitiateSubmarineSwapcaller doesn't useswap.Invoice, so it's currently dead data on that path. Not a bug, just noting for future reference.
✅ What looks good
- Payment hash handling is consistent: BOLT 12 path uses
RIPEMD160(paymentHash)same as BOLT 11 path. Both treat the extracted value as the SHA-256 preimage hash. - VHTLC construction is identical: Same contract parameters, same address verification against Boltz response.
- Three-check verification design is sound (key match, blinded path match, Merkle root) — just needs the bypass guard.
- Merkle hashing passes all four official test vectors from
signature-test.json. - Blinded path parser correctly handles both compressed-pubkey and sciddir introduction nodes, and guards against infinite loops.
- Test coverage is excellent: 723 lines covering round-trips, edge cases, all 20 valid offers from
offers-test.json, invalid encodings, and Merkle vectors. - Cross-repo impact is minimal:
SubmarineSwapResultchange is backward-compatible (optional param with default null). No other SDK (ts/go/rust) is affected. - E2E test is properly gated with
[Explicit]— won't break CI.
There was a problem hiding this comment.
Follow-up Review — Previous Findings Still Open
The force push appears to be a rebase; the code is unchanged. All findings from my previous review remain unaddressed. Summarizing the blockers:
🔴 CRITICAL — Verification bypass in VerifyInvoiceMatchesOffer (unchanged)
Bolt12InvoiceParser.cs:213-265
All three verification checks can still be silently skipped by a malformed/adversarial offer. No anyCheckPerformed guard was added. A crafted offer with no offer_issuer_id, no parseable offer_paths, and no offer-range TLVs passes all three checks vacuously.
Required fix: Track that at least one check executed; throw if none did.
🔴 CRITICAL — Missing invoice_node_id length validation in blinded-path branch (unchanged)
Bolt12InvoiceParser.cs:241-245
Check 2 does not validate invoiceNodeIdBlinded.Length != 33 before the SequenceEqual comparison. Check 1 (line 223) does this correctly — the blinded-path branch must match.
🟡 HIGH — ReadBigSize lacks bounds checking (unchanged)
Bolt12InvoiceParser.cs:483-496
The 0xFD/0xFE/0xFF branches still blindly index forward without verifying remaining bytes. Truncated input from Boltz throws an uncontrolled IndexOutOfRangeException instead of a FormatException. This parser processes untrusted data.
🟡 MEDIUM — Double parsing of BOLT 12 invoice (unchanged)
BoltzSwapsService.cs:681 → SwapsManagementService.cs:291
ExtractPaymentHash is called in CreateSubmarineSwapBolt12 to build the VHTLC, then again in InitiateSubmarineSwapBolt12 to get paymentHashHex for storage. The SubmarineSwapResult now carries the invoice string but not the extracted payment hash — add it to avoid re-parsing.
⚠️ Protocol-critical — requires human review
This PR touches VHTLC construction and invoice verification. Even after the above fixes, a human must sign off before merge.
🤖 Arkana
There was a problem hiding this comment.
✅ Follow-up Review — All Findings Addressed
Commit ae80254 ("fix(swaps): bolt12 validation") resolves every finding from my two previous reviews. Verified each fix:
🔴 CRITICAL — Verification bypass → ✅ Fixed
Bolt12InvoiceParser.cs:210,215,236,259,267-270 — anyCheckPerformed guard added to all three checks, with a clear throw if none executed. Correct.
🔴 CRITICAL — Missing invoice_node_id length validation → ✅ Fixed
Bolt12InvoiceParser.cs:241-244 — Length != 33 check now matches the Check 1 branch. Correct.
🟡 HIGH — ReadBigSize bounds checking → ✅ Fixed
Bolt12InvoiceParser.cs:490-508 — All three multi-byte branches (0xFD/0xFE/0xFF) now validate remaining bytes before reading. Initial pos >= data.Length guard also added. Correct.
🟡 HIGH — Double parsing of invoice → ✅ Fixed
BoltzSwapsService.cs:152 — PaymentHashHex now computed once in CreateSubmarineSwapBolt12 and carried through SubmarineSwapResult. SwapsManagementService.cs:242-243 reads it from the result. No re-parsing. Correct.
🟡 MEDIUM — Code duplication → ✅ Fixed
SwapsManagementService.cs:253-298 — Extracted SaveAndFundSubmarineSwap helper. Both BOLT 11 (line 178) and BOLT 12 (line 245) paths use it. BuildSwap local function eliminates the pending/failed ArkSwap duplication. Clean refactor.
🟢 LOW — Minor issues → ✅ All fixed
catch (FormatException) { throw; }no-op replaced withwhen (ex is not FormatException)filter (line 100)amountSats <= 0guard added (BoltzSwapsService.cs:104-105)- Trailing newline added to
SubmarineSwapResult.cs
🤖 Arkana
There was a problem hiding this comment.
✅ Follow-up Review — All Findings Addressed
Commit ae80254 ("fix(swaps): bolt12 validation") resolves every finding from my two previous reviews. Verified each fix:
🔴 CRITICAL — Verification bypass → ✅ Fixed
Bolt12InvoiceParser.cs:210,215,236,259,267-270 — anyCheckPerformed guard added to all three checks, with a clear throw if none executed. Correct.
🔴 CRITICAL — Missing invoice_node_id length validation → ✅ Fixed
Bolt12InvoiceParser.cs:241-244 — Length != 33 check now matches the Check 1 branch. Correct.
🟡 HIGH — ReadBigSize bounds checking → ✅ Fixed
Bolt12InvoiceParser.cs:490-508 — All three multi-byte branches (0xFD/0xFE/0xFF) now validate remaining bytes before reading. Initial pos >= data.Length guard also added. Correct.
🟡 HIGH — Double parsing of invoice → ✅ Fixed
BoltzSwapsService.cs:152 — PaymentHashHex now computed once in CreateSubmarineSwapBolt12 and carried through SubmarineSwapResult. SwapsManagementService.cs:242-243 reads it from the result. No re-parsing. Correct.
🟡 MEDIUM — Code duplication → ✅ Fixed
SwapsManagementService.cs:253-298 — Extracted SaveAndFundSubmarineSwap helper. Both BOLT 11 (line 178) and BOLT 12 (line 245) paths use it. BuildSwap local function eliminates the pending/failed ArkSwap duplication. Clean refactor.
🟢 LOW — Minor issues → ✅ All fixed
catch (FormatException) { throw; }no-op replaced withwhen (ex is not FormatException)filter (line 100)amountSats <= 0guard added (BoltzSwapsService.cs:104-105)- Trailing newline added to
SubmarineSwapResult.cs
🤖 Arkana
There was a problem hiding this comment.
✅ Incremental Review — New Commits (4a1f1e1..a7eb484)
Four new commits since my last approval (ae80254). All test infrastructure — no production code changed. Previous findings remain fixed.
New files reviewed:
TestWaiter.cs— polling helpers replacing ad-hoc deadline loopsMockBoltzServer.cs— in-process HTTP + WebSocket Boltz mock (743 lines)DockerHelper.cs—GetCurrentBlockHeight/MineRegtestBlocksToHeightadditionsChainSwapTests.cs— refactored to useTestWaiter.WaitForWithMining
🟡 MEDIUM — MockBoltzServer.BuildVhtlc skips ParseSequence logic
MockBoltzServer.cs:785-786
The mock constructs Sequence values via new Sequence(t.UnilateralRefundWithoutReceiver) (block-count constructor). Production code (BoltzSwapsService.cs:24-27) uses ParseSequence which switches to new Sequence(TimeSpan.FromSeconds(val)) for values ≥ 512.
DefaultTimeouts() sets UnilateralRefundWithoutReceiver = 576, which is ≥ 512. The mock will create a block-count Sequence, but the SDK will create a time-based Sequence. These encode differently — the VHTLC address won't match when ServerInfo is set.
Fix: Replicate the ParseSequence threshold in BuildVhtlc:
static Sequence Seq(int val) => val >= 512
? new Sequence(TimeSpan.FromSeconds(val))
: new Sequence(val);🟢 LOW — Minor observations
-
MockBoltzServer.cs:871-877—GetFreePorthas the classic TOCTOU race (port freed beforeapp.StartAsyncbinds it). Acceptable in tests, butUseUrls("http://127.0.0.1:0")+ reading the bound port fromapp.UrlsafterStartAsyncwould be race-free. -
MockBoltzServer.cs:879—DisposeAsynconly disposes_app, doesn't dispose_sessionsLock(SemaphoreSlim). Minor leak in tests, not a real problem. -
TestWaiter.cs— Clean implementation. Timeout + cancellation token handling is correct.await taskat the end ofWaitForWithMiningcorrectly propagates faulted task exceptions. -
ChainSwapTests.cs— The refactor toTestWaiter.WaitForWithMiningis a net improvement: eliminates duplicated deadline-loop boilerplate and the behavior is identical.
✅ What looks good
- MockBoltzServer VHTLC role assignment is correct: submarine (user=sender, boltz=receiver), reverse (boltz=sender, user=receiver), BTC→ARK chain (boltz=sender, user=receiver) — matches production
BoltzSwapsService.cs - WebSocket session management correctly snapshots under lock, iterates outside lock, handles disconnected clients
- ConcurrentDictionary + Interlocked for swap state tracking is thread-safe
- Mock doesn't leak into production — entirely within
NArk.Tests.End2End - No protocol-critical production code changes in this batch
Approving. The ParseSequence mismatch in the mock will surface as a test failure when someone actually uses MockBoltzServer with ServerInfo — not a blocker for merge.
🤖 Arkana
There was a problem hiding this comment.
✅ Incremental Review — Force Push (rebase only)
PR head is ae80254 — identical to the commit I approved on 2026-05-28T17:14:40Z. The force push removed the test infrastructure commits (TestWaiter, MockBoltzServer, ChainSwapTests) that were reviewed in my a7eb484d pass. No production code changed.
Previous approval stands. All critical findings from earlier reviews remain fixed.
🤖 Arkana
1 participant
Summary
Adds a minimal BOLT 12 TLV parser (
Bolt12InvoiceParser) that decodeslni1…invoices andlno1…offers without an external BOLT 12 library.Three-check verification confirms an invoice was generated from the given
offer: (1)
invoice_node_id/offer_issuer_idkey match, (2) blindedpath final-hop key match, (3) Merkle root of offer-range TLV fields
(types 2–22) must be identical — all four official
signature-test.jsontest vectors pass.
Extends Boltz client with the BOLT 12 offer-bridge endpoints
(
GET /bolt12/{receiving},POST /bolt12,PATCH /bolt12,POST /bolt12/delete) and the fetch endpoint(
POST /bolt12/fetch) used by the submarine flow.Adds
BoltzSwapService.CreateSubmarineSwapBolt12(internal): fetches aBOLT 12 invoice from the offer via Boltz, verifies it matches the offer,
then constructs the VHTLC contract identical to the BOLT 11 path.
Adds
SwapsManagementService.InitiateSubmarineSwapBolt12(public): thetop-level entry point — resolves a wallet descriptor, calls the service
above, stores the swap with
ArkSwapType.Submarineand the fetchedlni1…invoice, and optionally funds the VHTLC immediately viaSpendingService.Spend.Extends
SubmarineSwapResultwith an optionalInvoicestring so boththe BOLT 11 and BOLT 12 paths can carry the invoice through to storage.
Adds
DockerHelper.CreateClnBolt12Offerusingdocker exec cln lightning-cli offer(Nigiri ships CLN).Adds E2E test
CanPayBolt12OfferWithArkadeUsingBoltz([Explicit],[Category("Bolt12")]): reads the offer fromBOLT12_OFFERenv var orgenerates one from the Nigiri CLN container. Skipped in CI until Boltz is
configured with a BOLT 12-capable Lightning backend (CLN/LDK).
Test plan
dotnet test NArk.Tests— 493 unit tests pass (Merkle vectors,parser, blinded-path checks)
docker exec cln lightning-cli --network=regtest offer any "nark bolt12 e2e",then run
BOLT12_OFFER=lno1... dotnet test --filter CanPayBolt12OfferWithArkadeUsingBoltzagainst a regtest stack with Boltz configured for CLN