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

Add funding outpoint sigs to tx_signatures message #1009

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

ddustin
Copy link
Contributor

@ddustin ddustin commented Jun 30, 2022

Dependent on PR #851

niftynei and others added 30 commits June 8, 2021 12:59
This commit adds the interactive transaction construction protcol, as
well as the first practical example of using it, v2 of channel
establishment.

Note that for v2 we also update the channel_id, which now uses the hash
of the revocation_basepoints. We move away from using the funding
transaction id, as the introduction of RBF* makes it such that a single
channel may have many funding transaction id's over the course of
its lifetime.

*Later, also splicing
So that it knows what open_channel2 is responding to.

Spotted-By: @rustyrussell
Remove the fee_step in favor of using an explicit bump feerate, with
the constraint that it must be at least 1/64th greater than the last
sent rate.

Suggested-By: @rustyrussell
As @t-bast points out, we define 'point' in 01, which covers the
encoding.
"Failing the channel" may not be the right action in all cases where the
interactive construction protocol is used (e.g. closes), so we move
reference to it as a mitigation technique specifically to the usage of
the interactive protocol in the v2 open sequence.

Suggested-By: @t-bast
For open_channel2 + accept_channel2 we use the opener's basepoint and a
zeroed out point for the accepter.  This is because we don't know what
the accepter's basepoint is yet. (It's transmitted in accept_channel2)

Pointed-Out-By: @t-bast
P2SH-wrapped inputs are segwit inputs and should be eligible for
spending, however the spec as written didn't permit them

Reported-by: @SomberNight
P2SH outputs have larger dust requirements than native SegWit (and
Taproot) scripts; we disallow the creation of them on funding txs as a
they're problematic for relay when allowing dust limits below 546 sats
(see lightning#894)

Suggested-by: @t-bast
input_count of 1 times min witness weight of 110, is 110. not 107

the sum of 612 is correct, however.

Reported-By: @SimonVrouwe
* Move RBF inside the interactive-tx protocol
* Use same tlvs as open_channel / accept_channel
* Require native segwit
* Fix a few typos and whitespaces issues (thanks markdown linter)
t-bast and others added 6 commits May 10, 2022 15:13
And add `channel_type` tlv.
We use 1/24 instead of 1/64 to produce larger steps in fee-bumping.
Add signature data, remove wrapped segwit inputs, update calculations
to be more explicit about how to figure fee
If both peers contribute an equal amount to a interactive tx,
peer with "lower" pubkey sends first
The weight of a witness stack for a p2wpkh low-r signature is 107 bytes.
Don't show up in the CLN generator w/o this
@t-bast
Copy link
Collaborator

t-bast commented Aug 1, 2022

I'm not sure it makes sense to have this PR outside of the splicing one...at the spec level it's usually better to have one big long-lived PR that contains everything a feature needs, instead of small atomic PRs like you'd do for code.

@ddustin
Copy link
Contributor Author

ddustin commented Aug 3, 2022

Ah good point. The change is needed for both dual funding and splicing cause they share the interactive tx protocol.

Since dual funding is still in draft we are in a weird PR dependency situation =/.

Talking it over with @niftynei IIRC putting it here seemed like the ideal compromise but there may be a better way to refactor things.

@t-bast
Copy link
Collaborator

t-bast commented Aug 3, 2022

The change is needed for both dual funding and splicing cause they share the interactive tx protocol.

Can you explain why dual funding would need that tlv? I haven't felt the need for it while implementing dual funding in eclair.

@ddustin
Copy link
Contributor Author

ddustin commented Aug 3, 2022

The change is needed for both dual funding and splicing cause they share the interactive tx protocol.

Can you explain why dual funding would need that tlv? I haven't felt the need for it while implementing dual funding in eclair.

Ya totally -- dual funding doesn't need the tlv. There could be a cleaner way to do this 🤷.

The problem is that dual funding introduces tx_signatures and we need to modify tx_signatures for splicing.

The ways to do it I see are:
A) Put the change into the dual funding PR -> #851
B) Rebase the splice PR off the dual funding PR and add it there -> #863
C) Make a separate PR that's rebased off dual funding PR and add it there -> #1009

None seem ideal to me but C seemed like the simplest. B is probably the most correct but in my experience large interdependent PRs lead to chaotic problems. So far the splicing PR has been able to avoid being merge dependent on dual funding.

Once the dual funding PR gets merged in the dependency problem goes away on its own.

@t-bast
Copy link
Collaborator

t-bast commented Aug 3, 2022

Got it, this is just a dependency issue for the spec PRs.

I've got a better proposal to resolve this: we verify dual funding interop between eclair and cln, merge the dual funding PR 😎, and then those tlvs are on master and it's much easier to work on new proposals that leverage the interactive-tx protocol.

We should be ready for interop testing soon, so we'll have acks from eclair and cln. It would be really great it someone from ldk and lnd could grant a concept ACK on dual funding, if that happens I think we should be able to merge it.

@ddustin
Copy link
Contributor Author

ddustin commented Aug 3, 2022

Got it, this is just a dependency issue for the spec PRs.

I've got a better proposal to resolve this: we verify dual funding interop between eclair and cln, merge the dual funding PR 😎, and then those tlvs are on master and it's much easier to work on new proposals that leverage the interactive-tx protocol.

We should be ready for interop testing soon, so we'll have acks from eclair and cln. It would be really great it someone from ldk and lnd could grant a concept ACK on dual funding, if that happens I think we should be able to merge it.

Yessssss love this plan!

@optout21
Copy link

This is based on #851, which has just been merged, so this could be rebased and reviewed. Alternatively, I suggest to included it in splicing PR #863, as this change is needed by splicing.

@t-bast
Copy link
Collaborator

t-bast commented Feb 14, 2024

I think this should be directly included in the splice PR, so that everything splice-related is in the same place. The quiescence PR shouldn't take too long to land, and at that point the splice PR won't have any dependency anymore.

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.

None yet

4 participants