Skip to content
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

[WIP] bolt-taproot-gossip #2

Closed
wants to merge 1 commit into from
Closed

[WIP] bolt-taproot-gossip #2

wants to merge 1 commit into from

Conversation

ellemouton
Copy link
Owner

@ellemouton ellemouton commented Mar 10, 2023

NOTE: PR & discussions have been moved to: lightning#1059

@ellemouton ellemouton force-pushed the gossip1.5 branch 3 times, most recently from 76450ea to 8507d94 Compare March 13, 2023 16:36
@ellemouton ellemouton changed the title bolt-taproot-gossip [WIP] bolt-taproot-gossip Mar 13, 2023
Copy link

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I owe you a beer (or several) for doing this! Amazing.

Mainly minor nitpicks; this seems really solid to me.

This document aims to update the gossip protocol defined in [BOLT 7][bolt-7] to
allow for advertisement and verification of taproot channels. An entirely new
set of gossip messages are defined that use Schnorr signatures and that use a
purely TLV based schema.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with TLV is that you now need everyone to check that compulsory fields are present. But some fields don't actually make sense as optional. We end up needing another version anyway.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responded here

Comment on lines +49 to +50
fields. Timestamp fields are also updated to be block heights instead of Unix
timestamps.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

single one. Verifiers will then be able to aggregate the four keys
(`bitcoin_key_1`, `bitcoin_key_2`, `node_ID_1` and `node_ID_2`) using MuSig2 key
aggregation and then they can do a single signature verification check instead
of four individual checks.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we weaken this (and future proof it) a little, by publishing only the taproot_output_key? Sure, that means any taproot output can pretend to be a channel, but this is also a feature.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that means any taproot output can pretend to be a channel

Can you elaborate a bit more on this? What do you mean by "output can pretend"?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he means that you can "announce" a channel that is not actually a channel.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without checking node_ID_1 or node_ID_2 somehow?

If we do only publish taproot_output_key does that mean we may accept a phony channel announcement?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, both node ids have to sign it. But we don't care what the UTXO looks like internally, just that they prove they could spend it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might want to care a little bit - like at least that they cant spend it via the script path? ie, create some LN context binding

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responded here


## Timestamp fields

_TODO: write about the benefits of switching to block-height based timestamps_

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatically gains ratelimiting, and solves the "we don't have a timestamp for channel_announcements" which plagues gossip_timestamp_filter (though, there's a proposal to vastly simplify this request).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the cases where the reduced granularity of block height is a disadvantage? And are there consequences in case of reorgs?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice, implementations will backdate a little (obviously, less than 2000 blocks!) so you can spam a bit, but it's still a limit.

Reorgs in practice never reduce block height, so that's not a problem. And if it did, you would only discard updates on the now-missing block, which (see above) nobody is likely to produce.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what about two updates that follow in (relative) quick succession that have no ordering anymore? Maybe it is all good to discourage using the p2p network for high frequency updates, but also I think it isn't ruled out that there will be other ways of transportation where the coarse grained timestamping is going to cause a problem.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completed the section on the benefits of using blockheight here

let me know if I missed anything :)

There also does not seem to be a big benefit in spreading the
`node_announcement_2` message quickly since the speed of propagation of the
`channel_announcement_2` and `channel_update_2` messages will still depend on a
network wide upgrade.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, simplicity wins.

bolt-taproot-gossip.md Show resolved Hide resolved
Comment on lines +569 to +572
15. type: 7 (`htlc_minimum_msat`)
16. type: 8 (`fee_base_msat`)
17. type: 9 (`fee_prop_millionths`)
18. type: 10 (`htlc_max_msat`)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's renumber to make min and max adjacent, and decide on max vs maximum and min vs minimum to keep pedants like me happy!

They're all tu64, ofc.

Comment on lines +314 to +322
1. type: 0 (`channel_id`)
2. data:
* [`channel_id`:`channel_id`]
3. type: 1 (`short_channel_id`)
4. data:
* [`short_channel_id`:`short_channel_id`]
5. type: 2 (`partial_signature`)
6. data:
* [`partial_signature`:`partial_signature`]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely not use a TLV for channel_id. Every other message starts with channel_id, we should keep it fixed. The other two fields, well, they're OK, but it seems weird that they're optional.

bolt-taproot-gossip.md Show resolved Hide resolved

This section fleshes out the algorithms that the peers will need in order to
construct and verify the partial signature that will be included in the
`announcement_signatures_2` messages.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I didn't review this, but I believe your math from here on down :)

Nodes will do this by checking that they are able to derive the output key
found on-chain by using the advertised `bitcoin_key_1` and `bitcoin_key_2`
along with a [BIP86 tweak][bip86-tweak]. This provides a slightly weaker
binding to the LN context than legacy channels do but at least somewhat

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe useful to the reader to explain how it is weaker?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joostjager You mean in terms of what is visible to the blockchain?

Example

Suggested change
binding to the LN context than legacy channels do but at least somewhat
binding to the LN context than legacy channels do, as taproot addresses do not reveal any details related to the underlying script, but at least somewhat

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean in terms of what is visible to the blockchain?

the proof to other nodes that this output will be used for a channel (and is thus not a fake channel)

Copy link

@joostjager joostjager Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An outsider verifies that the taproot address pays to the combination of the node's bitcoin keys, but what extra is verified for legacy channels then that can't be done anymore? I am comparing to https://github.com/lightning/bolts/blob/33098ad37a442a7677c246ea9e195b7260abe8d2/07-routing-gossip.md?plain=1#L132

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no extra verification but rather that for legacy channels, there is a greater expense for an attacker to advertise a channel that is not actually a channel cause they will need to provide 2 signatures and pay for those bytes. With p2tr, there the on-chain remains the cost of one signature. I will add this detail to the explanation 👍


## Timestamp fields

_TODO: write about the benefits of switching to block-height based timestamps_

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the cases where the reduced granularity of block height is a disadvantage? And are there consequences in case of reorgs?

`announcement_signatures` and `channel_announcement` messages.

The opportunity is also taken to rework the `node_announcement` and
`channel_update` messages to take advantage of Schnorr signatures and TLV

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also not take this opportunity and keep using ecdsa signatures for those? That seems to reduce the delta of this proposal significantly, and maybe without loss of functionality for the user?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responded here

@ellemouton
Copy link
Owner Author

Thanks so much for the initial set of comments @rustyrussell & @joostjager!!

Im planning on finishing up the TODOs today & tomorrow and then I will close this & re-open against the main Bolt repo. Then we can continue discussions there? (want to avoid spreading the discussions across 2 places - rookie error on my side for opening this here publicly 😅 )

`node_1` and `node_2` use to identify their nodes in the network.
- `bitcoin_key_1` and `bitcoin_key_2` respectively refer to the public keys
that `node_1` and `node_2` will use for the specific channel being opened.
The funding transactions output will be derived from these two keys.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The funding transactions output will be derived from these two keys.
The funding transaction's output will be derived from these two keys.

Nodes will do this by checking that they are able to derive the output key
found on-chain by using the advertised `bitcoin_key_1` and `bitcoin_key_2`
along with a [BIP86 tweak][bip86-tweak]. This provides a slightly weaker
binding to the LN context than legacy channels do but at least somewhat

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joostjager You mean in terms of what is visible to the blockchain?

Example

Suggested change
binding to the LN context than legacy channels do but at least somewhat
binding to the LN context than legacy channels do, as taproot addresses do not reveal any details related to the underlying script, but at least somewhat

binding to the LN context than legacy channels do but at least somewhat
limits how the output can be spent due to the script-path being disabled.
3. The owners of `bitcoin_key_1` and `bitcoin_key_2` agree to be associated with
a channel owned by `node_1` and `node_2`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the public announcements of channels pretty much reveal what the output is being used for, would it be fair to mention that this mainly benefits private channels?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gossip only applies to public channels :)

Copy link

@GeorgeTsagk GeorgeTsagk Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be fair to mention that this mainly benefits private channels

Oh sorry by "this" I meant taproot channels, not gossip 🙈

Different phrasing: Taproot outputs all do look the same regardless of being used for a channel or not, but any participant in LN will eventually receive a (public) channel announcement that marks some taproot UTXO as a channel, so effectively it's only the private channels that can remain "incognito"

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your statement is correct but not sure how this applies to the linked comment? sorry if im missing something 🙈

single one. Verifiers will then be able to aggregate the four keys
(`bitcoin_key_1`, `bitcoin_key_2`, `node_ID_1` and `node_ID_2`) using MuSig2 key
aggregation and then they can do a single signature verification check instead
of four individual checks.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that means any taproot output can pretend to be a channel

Can you elaborate a bit more on this? What do you mean by "output can pretend"?

@ellemouton
Copy link
Owner Author

Thanks for the comments everyone! I have now opened a PR to the main Bolt repo. I have done my best so carry over the main comments over to that PR so that we can continue discussing there. Feel free to re-post a comment there if I have missed something!

@ellemouton ellemouton closed this Mar 16, 2023
ellemouton pushed a commit that referenced this pull request Jun 26, 2024
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>




Header from folded patch 'bolt_2__set_an_initiator_in_quiescence.patch':

BOLT #2: Set an initiator in quiescence.

This is especially useful for protocols such as splicing; for
simplified commitment transactions, there is already an implied
initiator at each point, so having the negotiation at splicing
time would be redundant.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>



Header from folded patch 'option_quiesce__feature_to_support_stfu_method.patch':

option_quiesce: feature to support stfu method.

In practice, sftu is useless unless you have something (e.g. channel_upgrade)
which uses it, but adding a feature is best practice IMHO.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants