Support BOLT12 offers for submarine swaps - #224
Conversation
WalkthroughThis PR adds BOLT12 invoice parsing and Boltz offer swap flows, introduces a unified ChangesBOLT12 Invoice and Swap Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ghost
mentioned this pull request
There was a problem hiding this comment.
Concept ACK — BOLT12 submarine flow looks solid, nothing hard-blocking. CI green (25/25).
Correct:
- VHTLC address check before funding — and applied to the BOLT11 paths too, good call.
LnInvoicestays a bare string on the wire, so oldSubmarineSwapDatarows still deserialize. No migration needed.- Trust model documented honestly.
Issues:
- Medium — fetched BOLT12 invoice is never verified against the offer (
bolt12.rs:201). Boltz picks the invoice, so it could return one payable to its own node. The one new trust surface here — details in my follow-up comments. - Cleanup — the swap HTTP + VHTLC-check block is triplicated across
boltz.rsandbolt12.rs. One shared helper; the address check shouldn't live in 3 places. - Cleanup —
CreateStringInvoiceSubmarineSwapRequest(bolt12.rs:366) duplicatesCreateSubmarineSwapRequest. Just make the existing one takeinvoice: String. - Cleanup —
LnInvoice::payment_hash()(boltz.rs:118) is unused and itsResultnever fails. Drop it, or returnsha256::Hash. - Suggestion — full LDK
lightningcrate for justBolt12Invoiceis heavy for a wasm target. Probably unavoidable — fine if the team's OK with it.
nit: SubmarineSwapData.preimage_hash doc still says "BOLT11 invoice" (boltz.rs:3924).
Only #1 needs a real decision before mainnet funds — the rest are cleanups.
There was a problem hiding this comment.
Follow-up on point 1.
The use case: a user pays a BOLT12 offer, but Boltz is the one resolving that offer into the invoice — so the user pays whatever invoice Boltz returns.
The inline comment below anchors the gap to the code. Verdict is unchanged — point 1 is the call to make before real funds.
Co-authored-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Parse the offer locally with LDK's Offer::from_str and run three checks before using the invoice Boltz returns: 1. If the offer has an explicit issuer_signing_pubkey, the invoice's signing_pubkey must match it. 2. Otherwise, if the offer has blinded message paths, the invoice's signing_pubkey must match the blinded_node_id of the final hop in one of those paths. 3. If the invoice carries an offer_id, it must match the offer's own id. This closes the central trust gap in the Boltz-resolves-offer flow: Boltz can no longer return an invoice payable to a different node without detection.
Otherwise HTTPS connections fail!?
Extract the common Boltz submarine swap creation path into a shared helper used by BOLT11 prepare, BOLT11 pay, and BOLT12 offer flows. The helper now owns the HTTP request, response parsing, VHTLC script construction, VHTLC address verification, swap data construction, and persistence. Callers only provide invoice-specific data and perform their own post-creation actions. Also consolidates the submarine swap request to use a string invoice, which is wire-compatible with the previous BOLT11 request because Bolt11Invoice serializes as a string.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Code Review — #224
Verdict: Request changes. This PR touches submarine swap creation and VHTLC construction — protocol-critical paths. The VHTLC address validation is an excellent security addition. However, I found issues in the BOLT12 invoice verification logic that need attention before merge.
🔴 P0 — Blinded-path verification is broken
ark-client/src/boltz/bolt12.rs — verify_bolt12_invoice_against_offer, lines ~340–348 (new file)
When the offer has no issuer_signing_pubkey but has blinded message paths, the code checks:
let matches = paths.iter()
.filter_map(|path| path.blinded_hops().last())
.any(|last_hop| invoice_signing_pk == last_hop.blinded_node_id);This comparison is incorrect. In BOLT12, blinded_node_id of the last hop is the blinded version of the recipient's pubkey (computed as b_n * G + real_node_id). The invoice's signing_pubkey() returns the recipient's real node key. These will never match for blinded-path offers.
Impact: All BOLT12 payments to offers that use blinded paths (which is the privacy-preserving default in many LN implementations) will be rejected with a false-positive verification error. The feature is non-functional for this class of offers.
Fix options:
- Use LDK's
Bolt12Invoice::verify_using_metadataorverify_using_payer_datafor proper cryptographic verification (preferred). - If LDK doesn't expose what's needed, log a warning for blinded-path offers instead of rejecting, and document the trust assumption.
- At minimum, skip the blinded-path check (it provides a false sense of security since the comparison is wrong) and rely on the
offer_idcheck (Check 3) which IS correct.
🟡 P1 — No invoice amount validation after Boltz fetch
ark-client/src/boltz/bolt12.rs — fetch_bolt12_invoice / create_bolt12_submarine_swap
After fetching a BOLT12 invoice from Boltz via bolt12_fetch, the code verifies the signing key and offer_id — but never checks that the invoice amount matches what the user requested.
If a user calls pay_bolt12_offer(offer, Some(Amount::from_sat(2_000))) and Boltz returns an invoice for 200,000 sats, the code will proceed to create a submarine swap for the inflated amount. The VHTLC expected_amount comes from Boltz's response, not from the user's intent.
Suggested fix: After parsing the invoice, verify invoice.invoice().amount_msats() matches the requested amount (or the offer's fixed amount if no user amount was provided). Reject if they diverge beyond a reasonable fee tolerance.
🟡 P1 — Silent accept-all when offer has no signing key and no paths
ark-client/src/boltz/bolt12.rs — verify_bolt12_invoice_against_offer, lines ~350–353
// If neither issuer_signing_pubkey nor paths are set, we cannot verify the
// invoice's signing key — the offer is unusually permissive. Accept it.This means a malformed or stripped offer (no node_id, no paths) bypasses ALL verification. Boltz could return an invoice for any recipient. Consider at minimum logging a warning, or requiring the offer_id check to pass in this case.
🟢 Positive findings
-
VHTLC address validation (
boltz.rs,create_submarine_swap): Locally rebuilding the VHTLC script and verifying the address matches Boltz's response is a significant security improvement. Previously the code trusted Boltz's address blindly. Well done. -
Refactoring
create_submarine_swapas shared code between BOLT11 and BOLT12 paths eliminates duplication and ensures both paths get the VHTLC validation. Clean. -
LnInvoiceserde: Custom deserializer that tries BOLT11 first then falls back to BOLT12 is backward-compatible with existing SQLite-stored swap data. No migration needed. -
Timeout on HTTP client: Adding
self.inner.timeouttoreqwest::Clientis a good hardening — previously it was unbounded. -
ark-grpcerror display fix (error.rs:103): Adding the:separator between description and source was a real bug fix.
📝 Minor / Nits
-
No unit tests for
verify_bolt12_invoice_against_offer: This is the most security-critical function in the PR. It deserves test coverage for each branch: matchingissuer_signing_pubkey, matching blinded path, mismatching keys,offer_idmismatch, and the no-key-no-paths fallthrough. -
e2e-tests/tests/boltz_bolt12_submarine.rs:43:wait_until_balance!(&alice, confirmed: Amount::ZERO, pre_confirmed: alice_fund_amount - res.amount)— this doesn't account for Ark round fees. May be flaky. Consider using a tolerance or>=check.
⚠️ Protocol-critical flag
This PR modifies submarine swap creation, VHTLC script construction, and adds a new payment flow. Requires human review and sign-off before merge, per protocol-critical code policy.
🤖 Reviewed by Arkana (#224)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ark-client/src/boltz/bolt12.rs`:
- Around line 323-341: The code currently accepts invoices when neither
issuer_signing_pubkey nor offer.paths() are present, allowing unauthenticated
invoice signers; change this to fail closed by returning an Err instead of
permissive success: in the branch where issuer_signing_pubkey is None and
paths.is_empty(), return Error::ad_hoc with a descriptive message (mentioning
invoice_signing_pk and that neither issuer_signing_pubkey nor blinded paths are
present) so verification cannot proceed without an authenticated signer; update
the logic around offer.paths(), invoice_signing_pk, and Error::ad_hoc to enforce
this rejection.
- Around line 240-291: The fetched/parsed BOLT12 invoice must be checked to
ensure its amount equals the caller-supplied amount or, when amount was None,
equals the fixed amount encoded by the offer; update fetch_bolt12_invoice to
read the resolved invoice amount from ParsedBolt12Invoice (e.g., invoice.amount
or equivalent) and compare it to the requested Amount (amount parameter) or to
the offer's fixed amount (derive from the offer if amount.is_none()), and if
they differ return an error (use Error::ad_hoc with a clear message) instead of
proceeding to verify_bolt12_invoice_against_offer and creating the swap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9408be03-173e-4b39-82e4-673cfaf41cd2
📒 Files selected for processing (9)
ark-client/Cargo.tomlark-client/src/boltz.rsark-client/src/boltz/bolt12.rsark-client/src/lib.rsark-client/src/swap_storage/sqlite.rsark-grpc/src/client.rsark-grpc/src/error.rse2e-tests/tests/boltz_bolt12_submarine.rse2e-tests/tests/common.rs
✅ Files skipped from review due to trivial changes (2)
- ark-client/Cargo.toml
- ark-grpc/src/error.rs
| async fn fetch_bolt12_invoice( | ||
| &self, | ||
| offer: &str, | ||
| amount: Option<Amount>, | ||
| ) -> Result<ParsedBolt12Invoice, Error> { | ||
| let request = Bolt12FetchInvoiceRequest { | ||
| offer: offer.to_string(), | ||
| amount: amount.map(|amount| amount.to_sat()), | ||
| }; | ||
|
|
||
| let url = format!( | ||
| "{}/v2/lightning/BTC/bolt12/fetch", | ||
| self.inner.boltz_bolt12_url().trim_end_matches('/') | ||
| ); | ||
| let client = reqwest::Client::builder() | ||
| .timeout(self.inner.timeout) | ||
| .build() | ||
| .map_err(|e| Error::ad_hoc(e.to_string()))?; | ||
| let response = client | ||
| .post(&url) | ||
| .json(&request) | ||
| .send() | ||
| .await | ||
| .map_err(|e| Error::ad_hoc(e.to_string())) | ||
| .context("failed to fetch bolt12 invoice from offer")?; | ||
|
|
||
| if !response.status().is_success() { | ||
| let error_text = response | ||
| .text() | ||
| .await | ||
| .map_err(|e| Error::ad_hoc(e.to_string())) | ||
| .context("failed to read bolt12_fetch error text")?; | ||
|
|
||
| return Err(Error::ad_hoc(format!( | ||
| "failed to fetch bolt12 invoice from offer: {error_text}" | ||
| ))); | ||
| } | ||
|
|
||
| let response: Bolt12FetchInvoiceResponse = response | ||
| .json() | ||
| .await | ||
| .map_err(|e| Error::ad_hoc(e.to_string())) | ||
| .context("failed to deserialize bolt12_fetch response")?; | ||
| let invoice = ParsedBolt12Invoice::parse(response.invoice) | ||
| .context("bolt12_fetch returned invalid BOLT12 invoice")?; | ||
|
|
||
| // Verify the invoice matches the offer before using it. | ||
| verify_bolt12_invoice_against_offer(offer, &invoice)?; | ||
|
|
||
| tracing::info!("Fetched and verified BOLT12 invoice from offer"); | ||
|
|
||
| Ok(invoice) |
There was a problem hiding this comment.
Bind the resolved invoice amount to the requested amount.
This only authenticates who signed the invoice and which offer it references. A buggy or malicious bolt12_fetch endpoint can still return a valid invoice for a higher amount from the same offer, and this flow will create/fund the swap for that value. Please reject any fetched invoice whose amount does not exactly match the caller-supplied amount, or the fixed amount encoded by the offer when amount is None.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ark-client/src/boltz/bolt12.rs` around lines 240 - 291, The fetched/parsed
BOLT12 invoice must be checked to ensure its amount equals the caller-supplied
amount or, when amount was None, equals the fixed amount encoded by the offer;
update fetch_bolt12_invoice to read the resolved invoice amount from
ParsedBolt12Invoice (e.g., invoice.amount or equivalent) and compare it to the
requested Amount (amount parameter) or to the offer's fixed amount (derive from
the offer if amount.is_none()), and if they differ return an error (use
Error::ad_hoc with a clear message) instead of proceeding to
verify_bolt12_invoice_against_offer and creating the swap.
| } else { | ||
| // Check 2: offer has blinded paths — verify invoice signing key is the | ||
| // final hop in one of them. | ||
| let paths = offer.paths(); | ||
| if !paths.is_empty() { | ||
| let matches = paths | ||
| .iter() | ||
| .filter_map(|path| path.blinded_hops().last()) | ||
| .any(|last_hop| invoice_signing_pk == last_hop.blinded_node_id); | ||
| if !matches { | ||
| return Err(Error::ad_hoc(format!( | ||
| "BOLT12 invoice signing pubkey ({invoice_signing_pk}) does not match \ | ||
| the final hop of any blinded message path in the offer", | ||
| ))); | ||
| } | ||
| } | ||
| // If neither issuer_signing_pubkey nor paths are set, we cannot verify the | ||
| // invoice's signing key — the offer is unusually permissive. Accept it. | ||
| } |
There was a problem hiding this comment.
Fail closed when the offer cannot authenticate the invoice signer.
If the offer has neither issuer_signing_pubkey nor blinded paths, this branch accepts the invoice without any authenticated signer binding. If the returned invoice also omits offer_id, Boltz can substitute an arbitrary invoice and still pass verification. That should be rejected rather than treated as a permissive success case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ark-client/src/boltz/bolt12.rs` around lines 323 - 341, The code currently
accepts invoices when neither issuer_signing_pubkey nor offer.paths() are
present, allowing unauthenticated invoice signers; change this to fail closed by
returning an Err instead of permissive success: in the branch where
issuer_signing_pubkey is None and paths.is_empty(), return Error::ad_hoc with a
descriptive message (mentioning invoice_signing_pk and that neither
issuer_signing_pubkey nor blinded paths are present) so verification cannot
proceed without an authenticated signer; update the logic around offer.paths(),
invoice_signing_pk, and Error::ad_hoc to enforce this rejection.
AI is right here but probably not the case of boltz
This sounds wrong |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Reviewed at head 1d33d77. The BOLT12 flow closes one important trust gap (invoice signing key ↔ offer) but leaves another wide open: the amount is entirely Boltz-controlled and never cross-checked, and pay_bolt12_offer funds the VHTLC without user confirmation. Details below.
Blocking — fund-loss vector: no amount validation on the fetched invoice
ark-client/src/boltz/bolt12.rs:200-219 fetches an invoice from Boltz, then hands it straight to create_submarine_swap. create_submarine_swap (ark-client/src/boltz.rs:305-340) trusts swap_response.expected_amount verbatim and writes it into data.amount. pay_bolt12_offer at bolt12.rs:170-192 then calls self.send(vec![SendReceiver::bitcoin(vhtlc_address, amount)]) with zero user interaction.
Consequence: a compromised or malicious Boltz bolt12/fetch endpoint can return an invoice for any amount it likes for the user's offer. When user calls pay_bolt12_offer(offer, Some(2_000)), Boltz can reply with an invoice for, say, 500_000 sats; expected_amount will reflect that (plus fees); the user's wallet drains up to whatever it can spend. Even a partially-compromised Boltz can inflate the delta between invoice.amount_msats() and expected_amount and pocket the difference on top of a legitimate invoice. verify_bolt12_invoice_against_offer at bolt12.rs:307-355 only covers signing-key/offer_id — an invoice for the right node with the wrong amount still passes.
Minimum required checks in fetch_bolt12_invoice (or right after it):
- If
amountwas supplied: assertinvoice.invoice().amount_msats() == amount * 1000(or explicit tolerance). - If offer fixes an amount: assert
invoice_amount_msats == offer.amount(). - After
create_submarine_swap: boundswap_response.expected_amount - invoice_amount_satsagainst an explicit fee ceiling (either configurable or a hard % cap), reject otherwise.
The BOLT11 flow gets away with skipping the second/third of these because the user supplied the invoice, so they know its amount before calling. BOLT12 has no such anchor.
High — verify function accepts offers with neither issuer key nor paths
bolt12.rs:339-340:
// If neither issuer_signing_pubkey nor paths are set, we cannot verify the
// invoice's signing key — the offer is unusually permissive. Accept it.Silently accepting means the entire binding between offer and invoice collapses; Boltz can return any invoice. BOLT12 requires at least one of the two, so rejecting is spec-conformant. Please turn this branch into an Err.
High — no invoice expiry check
Nothing consults invoice.invoice().relative_expiry() / created_at. Boltz can return a long-expired invoice; the VHTLC gets funded and then just waits until the refund timeout because Boltz cannot route the payment. The refund path exists so funds aren't lost, but this is easy to catch at fetch time and should be.
Medium — verify_bolt12_invoice_against_offer has zero adversarial test coverage
The three tests in bolt12.rs:369-400 only cover happy-path parse, wrong-HRP parse, and the LnInvoice untagged roundtrip. The function that stops a substituted invoice has no tests for:
- mismatched
signing_pubkeyvsissuer_signing_pubkey - signing_pubkey not matching any blinded-path last hop
offer_idmismatch- the permissive no-issuer/no-paths branch (which should be a failure once the item above is addressed)
This is the security check on a fund-flow path — please add unit tests before merge.
Medium — confirm the units on Bolt12FetchInvoiceRequest.amount
bolt12.rs:361: amount: Option<u64> populated with amount.to_sat(). BOLT12 uses msats everywhere natively; the units on Boltz's endpoint aren't obvious from the field name. If it turns out Boltz expects msat here, the request will 1000×-underpay and the invoice will come back for a rounding-error amount. Please confirm against Boltz's OpenAPI and add a comment locking in the units.
Low / nits
SubmarineSwapData.invoiceat boltz.rs:3923 changes fromBolt11InvoicetoLnInvoice. This is a semver-major break for any external Rust consumer that matches/destructures the field. SQLite backward-compat is fine (untagged serde tries Bolt11 first; prefixes are disjoint). Just call the break out in the changelog.boltz_bolt12_urlis only consulted forbolt12/fetch(bolt12.rs:252). The subsequentPOST /v2/swap/submarinestill usesboltz_urlat boltz.rs:268. In the e2e-tests setup you point at:9005for BOLT12 and:9001for the rest, which implies two Boltz deployments in play. If that split is intentional, please document it onwith_boltz_bolt12_url; if not, route the swap creation through the same URL when BOLT12 is in use.prepare_bolt12_offer_paymentdocstring at bolt12.rs:108-132 tells users to sendamounttovhtlc_addresswithout warning thatdata.amountwas chosen by Boltz on the basis of a Boltz-fetched invoice. Callers building UIs on top of this deserve an explicit "verify before funding" note (or, better, the amount-validation checks above so they don't have to).ark-grpc/src/client.rs:120swapsEndpoint::from_shared(self.url.clone())forEndpoint::new(self.url.clone()). Unrelated to the BOLT12 work, uncommented, and touches connect-time transport setup.from_shared(String)is stable;newgoes throughTryInto<Endpoint>(URI parsing). Behaviorally likely identical for valid URIs but the commit message doesn't say why. A one-line rationale (or reverting and doing it separately) would help.ark-grpc/src/error.rs:103— the": "separator fix is correct; before,Displaywas jamming description and source together as one word.
|
No longer relevant. |
4 participants
Based on #197.
Now provides an e2e test. I left out the proposed BOLT12 helper for reverse submarine swaps because they were unused.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores