-
Notifications
You must be signed in to change notification settings - Fork 2
bolt-simple-taproot: update interactive session protocol #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: simple-taproot-chans
Are you sure you want to change the base?
bolt-simple-taproot: update interactive session protocol #8
Conversation
Add extensions to the interactive transaction protocol, which is used for dual-funding, splicing and funding transaction fee bumping.
| or a splice transaction. | ||
| - MUST include a `next_commit_nonce` that will be used to sign the next | ||
| commitment transaction, once the interactive transaction session completes. | ||
| - IF the session is spending a previous funding transaction, MUST include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: that's only the case when spending a previous funding transaction that is using musig2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but my impression was that in this document we assume that we are using simple taproot channels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth clarifying, we want to be future-proof and potentially cover the case where a splice upgrades to taproot (in which case we don't need to include a funding_nonce), don't we?
bolt-simple-taproot.md
Outdated
| 2. types: | ||
| 1. type: 4 (`session_nonces`) | ||
| 2. data: | ||
| * [`66 or 99 * byte`:`commit_nonce` || `next_commit_nonce` || (optional) `funding_nonce`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now wondering whether we should include the optional funding_nonce in this TLV: it is completely orthogonal to the commit nonces (it's only useful when splicing a taproot channel), so it would probably be cleaner to use a separate TLV for it?
In that case this TLV should be renamed commitment_nonces and the new one funding_nonce.
Also, the length looks incorrect, it should be either 2*66*byte or 3*66*byte ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried splitting into two TLVs in ACINQ/eclair#3145 and it is trivial and cleaner, I think we should do this. We should have session_commit_nonces and session_funding_nonce as two separate TLVs fields here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it cleaner with 2 distinct TLV fields, done in 705e8cc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for two diff fields
bolt-simple-taproot.md
Outdated
|
|
||
| - MUST include a `commit_nonce` for the commitment transaction that is being | ||
| built, which can be the first commitment transaction for channel opening, | ||
| or a splice transaction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite unclear, I don't understand what the "or a splice transaction" means here...maybe it would be easier to understand if you say that this is for the current_commitment_number, which is always 0 for the channel opening case but could be anything for splices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 705e8cc.
bolt-simple-taproot.md
Outdated
| 2. types: * [`66*byte`: `public_nonce`] | ||
| 1. type: 22 (`next_local_nonces`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copy-paste?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 705e8cc.
| - MUST use the nonce associated with the empty hash key (32 zero bytes) as the | ||
| primary commitment nonce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this? It is very important to specify that this is used for single-funded channels, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because for each commitment the corresponding commit nonce is indexed by the actual funding txid (I've removed the special case the the current commit tx that was indexed by 32 zero bytes). But I will specify that for single-funded channels, since the funding tx id is not known yet when open/accept are exchanged, the funding txid used to generate the corresponding commit nonce should be 32 zero bytes.
bolt-simple-taproot.md
Outdated
| 3. type: 22 (`local_nonces`) | ||
| 4. data: | ||
| * [`local_nonces`: `nonces_map`] | ||
| * [`next_local_nonces`: `nonces_map`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonces_map doesn't seem to be defined anywhere. I think you're referring to the local_nonces type defined earlier, but it must be made more precise. Also, there is an issue in the current definition of local_nonces, it explicitly encodes the length of the map but it's unnecessary since the length is already encoded in the TLV.
It is currently defined as:
local_nonces
- type: 22
- data:
- [u16: num_entries]
- [num_entries * nonce_entry: entries]
where nonce_entry is:
- [32*byte: txid]
- [66*byte: public_nonce]
But it should be:
local_nonces
- type: 22
- data:
- [... * nonce_entry: entries]
where nonce_entry is:
- [32*byte: txid]
- [66*byte: public_nonce]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 705e8cc, it matches how eclair encodes this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was defined in lightning@4c1314a
This PR looks to be against an older commit,
bolt-simple-taproot.md
Outdated
| 2. types: | ||
| 1. type: 4 (`commit_nonces`) | ||
| 2. data: | ||
| * [`2*66*byte`:`commit_nonce` || `next_commit_nonce`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we need to follow a stricter structure because cln uses a parser to automatically generate the right data structs in C. I believe it should be changed to the following to work correctly:
1. `tlv_stream`: `tx_complete_tlvs`
2. types:
1. type: 4 (`commit_nonces`)
2. data:
* [`66*byte`:`commit_nonce`]
* [`66*byte`:`next_commit_nonce`]
Co-authored-by: Bastien Teinturier <[email protected]>
Roasbeef
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
I did an initial pass over the set of changes. I think somehow the PR was made against an older revision, or maybe it's just the diff rendering oddly.
Not super familiar with the interactive tx protocol, so was mainly looking for consistency related issues.
bolt-simple-taproot.md
Outdated
| 2. types: | ||
| 1. type: 4 (`session_nonces`) | ||
| 2. data: | ||
| * [`66 or 99 * byte`:`commit_nonce` || `next_commit_nonce` || (optional) `funding_nonce`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for two diff fields
| 1. `tlv_stream`: `revoke_and_ack_tlvs` | ||
| 2. types: | ||
| 1. type: 4 (`next_local_nonce`) | ||
| 1. type: 22 (`next_local_nonces`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this diff is against an older version of the set of changes?
See this commit in the BOLTs repo: lightning@4c1314a
| * [`66*byte`: `public_nonce`] | ||
|
|
||
| ### Channel Funding | ||
| ### Channel Establishment v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the prior heading valid?
The new heading serves to delineate the new set of messages.
bolt-simple-taproot.md
Outdated
| 3. type: 22 (`local_nonces`) | ||
| 4. data: | ||
| * [`local_nonces`: `nonces_map`] | ||
| * [`next_local_nonces`: `nonces_map`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was defined in lightning@4c1314a
This PR looks to be against an older commit,
| The recipient: | ||
|
|
||
| - MUST fail the channel if `next_local_nonces` is absent or cannot be parsed. | ||
| - MUST fail the channel if an active commitment funding txid is missing in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't nonces for all the current commitments need to be included?
|
|
||
| The recipient: | ||
|
|
||
| - MUST fail the channel if `next_local_nonce` is absent, or cannot be parsed as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was more precise.
| #### local_nonces | ||
| - type: 22 | ||
| - data: | ||
| * [`u16`: `num_entries`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rationale for dropping the prefix byte of the number of elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is already encoded in the length part of the TLV (type-length-value), so it's completely redundant to re-encode it?
| `musig2.NonceGen` algorithm with the required values, and the `rand'` | ||
| value set to `k_i`. | ||
|
|
||
| 4. Use the id of the commitment's funding transaction as extra input, as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
id -> TXID
|
|
||
| We add `option_simple_taproot` to the set of defined channel types. | ||
|
|
||
| Since the id of the funding transaction is not known yet when nodes exchange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id -> TXID
|
|
||
| Since the id of the funding transaction is not known yet when nodes exchange | ||
| `open_channel` and `accept_channel`, a dummy funding txid of 32 zero-byte | ||
| must be used instead as extra input to the nonce generation algorithm to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must -> SHOULD
The nonce generation protocol defined above is optional.
Add extensions to the interactive transaction protocol, which is used for dual-funding, splicing and funding transaction fee bumping.
Includes a few changes to the current simple taproot channels proposal: