bolt-simple-taproot: fix the output key derivations, signature rules and test vectors - #1352
Open
erickcestari wants to merge 15 commits into
Open
bolt-simple-taproot: fix the output key derivations, signature rules and test vectors#1352erickcestari wants to merge 15 commits into
erickcestari wants to merge 15 commits into
Conversation
`taproot_nums_point` was used throughout the commitment transaction sections but never defined anywhere; the point it refers to is defined as `simple_taproot_nums`. Use that name everywhere. The `to_remote` section also defined its own value for the NUMS internal key, `0245b181...`, which disagrees with the value given under "Nothing Up My Sleeves Points", with the `to_remote.internal_key` in the test vectors, and with both lnd and eclair. Drop the local redefinition so the single definition earlier in the document applies. The `to_remote_control_block` named `combined_funding_key` as the internal key for the same output. A control block commits to the output's internal key, which is the NUMS point here, so the block as written would not validate. The design overview named `combined_funding_key` for that output as well. Point it at `simple_taproot_nums` too: an unspendable internal key is what stops the remote party from bypassing the 1 block CSV delay. Also state explicitly that a control block carries the 32-byte x-only serialization, since the value is quoted in 33-byte compressed form where it is defined.
The `to_local` tapscript tree has two leaves, so a control block spending either path must carry an `inclusion_proof`: the `tap_leaf` hash of the path not taken, as the HTLC sections already phrase it. Both blocks instead concatenated the revealed script itself, and the delay path was described as `33` bytes when a two-leaf tree makes it `65`. Verified against the test vectors: `TapBranch(sort(8fcd64..., dbf0400e...))` is the published `to_local.tapscript_root`. Also drop the claim that a breach sweeps "the top-level key spend path". The internal key is a NUMS point, so no key path spend exists; the breach sweep is the script path spend described immediately below it.
The fence opened for `to_delay_script` was never closed, so the `revoke_script` bullet that follows was rendered as part of the code block.
Only the anyone-can-spend script path was described, but the anchor commits to a real internal key rather than a NUMS point, so its owner can sweep it immediately without waiting 16 blocks. Both lnd (`TaprootAnchorSpend`) and eclair (`Taproot.anchor`) rely on that path.
`next_closee_nonce` is required by the `closing_sig` requirements and implemented by lnd, but was missing from the list of new TLV types. It also reuses type `22` with a different payload from `next_local_nonces`, which contradicts the note that these type IDs are always the same. Since type `22` is now ambiguous on its own, annotate both entries with the messages they appear in. `next_local_nonces` is defined for `revoke_and_ack` as well as for `channel_reestablish`, and lnd carries it in both under the same type (`LocalNoncesRecordTypeDef`), so name both. The JIT nonce summary also said `closing_sig` carries no nonce at all, contradicting both the message definition and its requirements.
The first argument was called `msg` but is used as the HMAC key: lnd keys the HMAC with the `taproot-rev-root` tag and passes the revocation root hash as the message. Name both arguments so the order is unambiguous.
The funding vector's `combined_key` held the BIP 86 tweaked output key, not the `KeyAgg` result the specification calls `combined_funding_key`. Publish both under the names the specification uses.
Both leaves of the `accepted_htlc_*` script vectors carried the local and remote HTLC keys in the wrong positions, disagreeing with the script templates in this document, with lnd (`ReceiverHtlcTapLeafSuccess`, `ReceiverHtlcTapLeafTimeout`) and with eclair (`receivedHtlcSuccess`, `receivedHtlcTimeout`). The transaction vectors in this document already disagreed with them. Recovering the sibling leaf from the resolution transactions' control blocks yields the timeout leaf keyed by `<remote_htlcpubkey>`, and the corrected entry produces pkScript `51209ce82cd1...`, which is exactly output 2 of the five-HTLC commitment (1000 sat, incoming, expiry 500). The previous entry matched no output in any of the transactions. The `*_remote_commit` entries were byte-identical copies of their `*_local_commit` counterparts, so they exercised nothing: the published parameters carried only `local_per_commit_point` and `remote_revocation_basepoint`, which are the local commitment's inputs. Add `local_revocation_basepoint` and `remote_per_commit_point` under the existing `SHA256(seed || label)` convention, along with the three keys they derive, and regenerate the two entries from them. Two things change on the remote commitment, both per lnd `lnwallet/commitment.go`: `DeriveCommitmentKeys` takes the revocation basepoint from the local channel config when the commitment is the remote party's, and `GenTaprootHtlcScript` swaps the sender and receiver arguments, so an HTLC we offer is an offered HTLC on our commitment and an accepted one on theirs. Leaf hashes, tapscript roots, output keys and pkScripts recomputed.
The link to the musig2 BIP was missing its closing parenthesis, so it rendered as literal text, and the sentence opened with a typo.
A BIP 327 secnonce is 97 bytes: NonceGen appends the signer's public key to the two scalars, and Sign refuses a secnonce whose trailing key is not the one it is signing with. The test vectors already publish 97-byte secret nonces, so only this section was short.
The sighash type of the second level signatures was only stated much later, in the HTLC Second Level Transactions section, while the packing rule here says the signature is 64 bytes. BIP 342 requires 65 bytes for any sighash type other than SIGHASH_DEFAULT, so a receiver has to know that it must append the 0x83 byte when it builds the witness.
Second level transactions are meant to be aggregated with each other and a fee bumping input, but BIP 341 commits SIGHASH_SINGLE to the output at the same index as the signed input. Aggregating them without preserving that pairing silently invalidates the pre-signed signatures.
Both second level outputs pay to the very same script the to_local output uses, but they called it `htlc_success` and `htlc_timeout`, which reads as if there were three distinct scripts. The test vectors show they are one: both entries have leaf hash dbf0400e..., as does `to_local`'s settle leaf.
Five output key formulas added the tweak hash straight to the internal key, which adds a scalar to a point. BIP 341 computes the output key as Q = P + int(hashTapTweak(...))G, which is what the other formulas in this document, and the test vectors, already do.
BIP 341 consumes an internal key in its 32-byte x-only form, both inside the TapTweak hash and in the control block, but only the NUMS point said so. Every other internal key here is written in its 33-byte compressed form, including in the test vectors, so name the conversion and state the rule once for all of them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Corrections to the Simple Taproot Channels extension, found by re-deriving every
published value from the seed and checking the document against the BIP 340,
341, 342 and 327 reference code and against lnd and eclair.
No protocol change is intended: every fix moves the document toward what the two
implementations already do.
Output keys and control blocks
level outputs) added the tweak hash straight to the internal key. BIP 341
computes
Q = P + int(hashTapTweak(...))G.point said so.
x_only()is now defined once and used for every internal key,including in the control block serialization.
taproot_nums_pointwas used throughout the commitment sections but neverdefined, and the
to_remotesection defined its own value for it, whichmatched neither the definition earlier in the document nor the test vectors.
Its control block also named
combined_funding_keyas the internal key.to_localcontrol blocks carried the wrong data and the delay path claimedto be 33 bytes when it needs the sibling leaf hash.
to_local, but it wascalled
htlc_successin one place andhtlc_timeoutin the other, reading asthree distinct scripts. An unclosed code fence in the
to_localsection hadalso swallowed the revoke script into the delay script's block.
Signatures and nonces
commitment_signeddid not say which sighash type the HTLC signatures use.They are
SIGHASH_SINGLE|SIGHASH_ANYONECANPAY, so the receiver has to appendthe
0x83byte to the 64-byte field when building the witness.SIGHASH_SINGLEcommits to the output at the same index as the signed input,which constrains how second level transactions may be aggregated.
next_closee_nonce, and fixes the BIP 327 link.Test vectors
accepted_htlc_*entries carried the local and remote HTLC keys in thewrong positions, disagreeing with the script templates here, with the
transaction vectors in this same document, and with both implementations.
*_remote_commitentries were byte-identical copies of the local ones, sothey exercised nothing. They are now derived from
remote_per_commit_pointand
local_revocation_basepoint.combined_keyactually held the BIP 86 tweaked output key, so it is split intocombined_funding_keyandfunding_key.Verification
Every key, script, leaf hash, tapscript root and output key re-derived from the
seed; every signature checked against the BIP 341 sighash using the BIP 340 and
BIP 327 reference implementations, including a full MuSig2 replay of each
commitment signature; every witness executed under BIP 342 rules.
Both implementations ship a copy of these vectors, byte-identical to each other
and, in their
transactionssection, to this document. Running each project'sown suite against the JSON from this branch:
SimpleTaprootCommitmentsTestVectorSpecpasses with no sourcechanges. It re-derives the keys, rebuilds all three commitment transactions
and every HTLC transaction, reproduces
remote_partial_sig, and runscorrectlySpendson each. It reads onlyparams,scripts.fundingandtransactions, so the script entries are covered indirectly, through the leafscripts embedded in the resolution witnesses.
TestTaprootVectorspasses onparams, the three transactions, thesignature verification, the MuSig2 replay and 5 of the 11 script entries. The
6 failures are the changes above: three are lnd's vector generator, and three
are the
combined_keyand second level leaf renames its test structs wouldfollow.
The 15 commits are split for review only, one defect each, so they can be read
against the BIP text one at a time and any single one can be dropped without
disturbing the rest. They might be squashed into fewer commits before landing.
I've also asked claude to write a script to verify if the new test vectors are correct.
https://gist.github.com/erickcestari/8cd853821529c57aaf6109ee77409dd8