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.

Sign up for GitHub

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

Jump to bottom

Last(?) Updates to dual-funding Spec #5956

Merged
merged 4 commits into from Feb 8, 2023
Merged

Last(?) Updates to dual-funding Spec #5956

merged 4 commits into from Feb 8, 2023

Conversation

niftynei
Copy link
Collaborator

@niftynei niftynei commented Feb 3, 2023 •
edited

Adds requested spec proposal changes, including addition of est_witness_weight field and second_commitment_point to the open/accept_channel2 messages.

(Note that accept_channel2 also includes second_commitment_point, per commit lightning/bolts@ee5bef7).

Built on top of #5900.

@t-bast this is a good one to test for interop!

@endothermicdev endothermicdev added this to the v23.02 milestone Feb 5, 2023
rustyrussell
rustyrussell requested changes Feb 5, 2023
View reviewed changes
Copy link
Contributor

@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.

Ack ef98b10

Please put something for CHANGELOG.md?

Changelog-EXPERIMENTAL: Protocol: dual-funding spec changed in incompatible ways, won't work with old versions (but maybe soon with Eclair!)

@t-bast
Copy link

t-bast commented Feb 6, 2023

including addition of est_witness_weight field

But on the contrary, we decide that we should drop that field, not add it?

The changes that we discussed during the last spec meeting that are still missing from the spec are:

  • adding second_per_commitment_point to accept_channel2
  • removing feature dependency on static_remotekey
  • removing est_witness_weight
  • adding a rationale paragraph on the fee griefing attack and the need to actively double-spent inputs in concurrent interactive-tx attempts

At first glance, this PR implements the first point but not the other two?

@niftynei
Copy link
Collaborator Author

niftynei commented Feb 7, 2023

Thanks for the review @rustyrussell + @t-bast.

Ah ok, I'm on board with removing est_witness_weight. Updated here and in the spec. We still verify that the provided bytes at least meet/match the provided feerate.

Also removed the assumption that static remotekey is flagged on.

I think that's everything?? Will keep an eye on the CI.

@rustyrussell
Copy link
Contributor

Ack 91b04af

@niftynei @rustyrussell
rfc-dual-fund: update to latest spec for dual-funding
2dba51f 
- Renamed zerod_channel_ids to temporary_channel_id
- Renamed witness_stack->witnesses
- Renamed witness_element->witness_elements
- open_channel2 now includes second commitment point
- accept_channel2 now includes second commitment point

Current commit on rfc branch 64f7f360b9f3c2664d078e2129cfe83098fc4617

Changelog-EXPERIMENTAL: Protocol: dual-funding spec changed in incompatible ways, won't work with old versions (but maybe soon with Eclair!!)
@niftynei @rustyrussell
df: fetch both the first+second commitment point
3833aa5 
Ignore the sent back second commitment point that we get; we'll get it
again at `channel_ready`.
@niftynei @rustyrussell
df: remove minimum witness weight for input calculations
6bb55bc 
We can't know how much taproot etc inputs weight will be, so we just
make sure that a peer covers the known bytes, at least.
@niftynei @rustyrussell
df: remove static remote key dependency
c7d8c29 
Must be negotiated independently.

Requested-By: @t-bast
@rustyrussell
Copy link
Contributor

Trivial rebase on master, now #5900 is merged, as GH complained about conflicts for some reason.

Ack c7d8c29

@rustyrussell rustyrussell merged commit 35d02a7 into ElementsProject:master Feb 8, 2023
@t-bast
Copy link

t-bast commented Feb 8, 2023

Awesome, thanks @niftynei I'll start working on cross-compatibility tests!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rustyrussell rustyrussell rustyrussell approved these changes

Assignees
No one assigned
Labels
dualfunding spec
Projects
None yet
Milestone
v23.02
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@niftynei @t-bast @rustyrussell @endothermicdev