bolt12: add Merkle tree and BIP-340 message signatures - #11061
Conversation
|
/gateway review |
| } | ||
|
|
||
| return nil | ||
| return VerifyInvoiceRequest(ir) |
There was a problem hiding this comment.
These comments say the string encoder rejects unsigned messages, but the generic Bech32 Encode accepts any nonempty TLV bytes, so both typed Encode methods can emit a missing, stale, or wrong key signature.
Should final writer validation require and verify the signature now that the signing primitives exist?
There was a problem hiding this comment.
Thanks, yes, I was planning to check this in another layer basically, in EncodeInvoiceRequestString and EncodeInvoiceString, which comes at a later stage and that one calls the Encode function. This will be part of the next PR. I've updated the comments a bit.
There was a problem hiding this comment.
I'll un-export Encode/Decode, which makes a it a bit clearer hopefully.
There was a problem hiding this comment.
Encode/Decode are still exported. Fine to fold the un-export into the string layer PR, just flagging so it doesn't get lost.
There was a problem hiding this comment.
This will stay open after the 11146 is merged. I would first like to do a check of what needs to be exported looking at the package as a whole. So I decided to not do it here to keep diffs minimal.
7ae7478 to
75b77b6
Compare
|
/gateway review |
🟡 PR Severity: MEDIUM
🟡 Medium (3 files)
🟢 Low (6 files)
AnalysisThis PR adds BOLT12 merkle-tree and signature helper logic ( To override, add a |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 6 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 4 Minor · 🔵 0 Nit
Summary
This PR completes the BOLT 12 signature story: Merkle root construction over TLV records, BIP-340 tagged-hash message signatures, and — the behavioural change that matters — ValidateInvoiceRead and ValidateInvoiceRequestRead now end in a real cryptographic check instead of a presence-only TODO. The primitives are pinned against the vendored spec vectors at leaf, nonce, branch, root, and digest granularity, and the pairwise reduction with odd-node promotion reproduces the pairing order both the 3-leaf n1 vector and the 6-leaf invoice_request vector document. That is a well-tested core.
The concerns are at the edges of that core rather than in the hash construction. The most consequential is what the signature actually commits to: the root is derived from records this package re-encodes, not from the bytes the peer sent, so verification is only correct as long as decode-then-encode is byte-exact for every BOLT 12 field — and the new fixtures are structured so that a divergence there cannot fail a test. Second, the verify path's error classification is incomplete in a way a peer selects: a signature encoding that fails to parse does not return ErrInvalidSignature, and the one test that claims to cover malformed signatures passes without exercising that branch.
One limitation on this review: the working tree was not available, so bolt12/invoice.go, bolt12/invoice_request.go (AllRecords, decodedTLVs), lnwire.EncodeRecords, and the tlv codecs could not be read. F1 and F5 both hinge on those files and are stated conditionally and scored accordingly.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
75b77b6 to
dce48e5
Compare
|
Ready for the next round @GeorgeTsagk and @ViktorT-11 (cc @Abdulkbk @vctt94 if you have time). |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Generally looks great 🔥! Leaving some initial comments.
| return err | ||
| } | ||
| if vec.SerializeSize() != int(l) { | ||
| return ErrNonMinimalFeatures |
There was a problem hiding this comment.
Just a note here that I double checked, and the no other Lightning implementation rejects valid feature vectors with leading zero bytes. So if we go ahead and add this, LND will be the outlier when it comes to this behaviour.
There was a problem hiding this comment.
You are right that this deviates from other's behavior. I think in practice it won't be a problem because the spec mandates minimal encoding of features and all implementations use minimal encoding when writing. If this should become an issue, we can add a bolt12 local type that relaxes the minimal encoding requirement, which would still be compatible with minimally encoded invoices/requests, but the motivation is to reuse lnd-native types as much as possible.
| tag := signatureTagPrefix + messageName + fieldName | ||
| digest := taggedHash(tag, root[:]) | ||
|
|
||
| sig, err := schnorr.Sign(privKey, digest[:]) |
There was a problem hiding this comment.
One recommendation by Codex here:
Hardening Concern
This does not pass auxiliary randomness to schnorr.Sign. BIP-340 recommends fresh auxiliary randomness because it improves protection against fault injection and side channels. The btcec implementation also explicitly warns that its signer has variable-time operations involving the key and nonce. See the btcec.Sign warning and BIP-340 signing guidance.
There was a problem hiding this comment.
Deferred. keychain/btcwallet.go calls schnorr.Sign with no auxiliary data. I looked at other implementations, all three of them don't add aux data.
vctt94
left a comment
vctt94
left a comment
There was a problem hiding this comment.
Great work so far! I can feel BOLT 12 getting closer 🚀
I reviewed the Merkle construction, signature flow, and the invoice/request validation paths. The core implementation looks solid, and the official test vectors give good confidence around the cryptographic pieces.
I left one comment around offer_paths and the selected-path identity binding. Other than that, things look good from my side!
| return nil | ||
| // - MUST reject the invoice if signature is not a valid signature using | ||
| // invoice_node_id as described in Signature Calculation. | ||
| return VerifyInvoice(inv) |
There was a problem hiding this comment.
I think we may still need a way to verify that the invoice was signed by the final node of the offer_path the payer selected.
I tested a multi-hop path Alice → Bob, with Alice as an intermediate blinded node and Bob as the final node. If the returned invoice uses Alice as invoice_node_id and carries a valid Alice signature, both ValidateInvoiceRead and ValidateInvoiceAgainstRequest return nil.
As a control, the equivalent offer_issuer_id = Bob / invoice_node_id = Alice case is correctly rejected with ErrInvoiceNodeIDMismatch.
For offer_paths, BOLT 12 requires invoice_node_id to match the final blinded_node_id of the path the payer actually sent the invoice_request to. Since the selected path is state held by the caller, would it make sense to expose a helper or validation API that accepts that final blinded node and enforces this check?
There was a problem hiding this comment.
Good catch! 11146 will contain a high-level checker that a invoice requester should use to validate against.
Edit: this commit has it ea88ddd
Add strictFeaturesRecord, a TLV record for the top-level features fields whose decoder rejects non-minimally encoded feature vectors with ErrNonMinimalFeatures. RawFeatureVector re-encodes to minimal length, so accepting a padded encoding would change the Merkle leaf bytes and invalidate an otherwise valid signature. The payinfo features guard in decodeBlindedPayInfos is the same check one subtype level down. All three message types use it, so the features fields decode through a single path. An offer holds no signature of its own, so the record buys it nothing directly, but no producer emits a padded vector and a second lenient path would earn its keep only if one did. Dropping this guard would not make lnd accept a padded vector on a signed message: the vector decodes, re-encodes one byte shorter, and the message is then refused at signature verification, which is the same rejection behind a less obvious error. Accepting one requires deriving the root from the bytes the peer sent rather than from re-encoded records.
BOLT 12 signs the Merkle root of a tree built from the message's TLV records. MerkleRoot constructs the tree from a []tlv.Record by hashing each record into an LnLeaf and a uniqueness LnNonce, combining them into LnBranch nodes, and recursing to a single root. signableTLVs selects the records that enter the root, excluding the 240-1000 range the spec reserves for signature fields. The vendored signature-test.json vectors pin the construction byte-for-byte against the spec, down to every intermediate LnLeaf, LnNonce, and LnBranch digest.
dce48e5 to
04ab408
Compare
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Did a second round deep review, and I think this looks solid given that we're ok with the tradeoff mentioned in #11061 (comment).
LGTM 🔥🎉!!
Only leaving a test related comment.
SignMessage and VerifySignature wrap btcec's Schnorr API around the Merkle root with the spec's tagged-hash domain separator. Typed helpers bind the construction to invoice requests and invoices, so callers sign and verify messages rather than raw roots. The signed vectors in signature-test.json verify the construction against the spec, and a tamper matrix pins rejection of modified roots, signatures, keys, and tags.
ValidateInvoiceRequestRead and ValidateInvoiceRead now run VerifyInvoiceRequest and VerifyInvoice as their final step, so a decoded message whose BIP-340 signature does not verify against invreq_payer_id or invoice_node_id is rejected instead of only checked for presence. This satisfies the reader-side MUSTs of the BOLT 12 invoice_request and invoice requirements, and the docstrings now state the contract instead of deferring it to callers. The invoice_request wiring is pinned by the spec's signed vector; no signed invoice vector exists, so the invoice side is pinned by round-trip tests. Reader-validation fixtures now sign with Bob's key, and the happy-path invoice_request test reuses the shared validInvoiceRequest fixture.
Add changes for the Merkle tree and BIP-340 signature work in bolt12.
04ab408 to
9cfd5a6
Compare
6 participants
Part of #10736. Based on #11001.
Adds Merkle tree construction over TLV records (
MerkleRoot) and BIP-340 Schnorr message signatures (signMessage,verifySignature) for BOLT 12 invoice requests and invoices, and verifies the signature on read so a decoded message with an invalid signature is rejected.Additionally, this PR vendors upstream
lightning/boltssignature test vectors (signature-test.json) to pin Merkle construction and signature verification byte-for-byte against the spec.