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

Fee requirement for closing_signed is unclear #499

Closed
TheBlueMatt opened this issue Oct 31, 2018 · 4 comments
Closed

Fee requirement for closing_signed is unclear #499

TheBlueMatt opened this issue Oct 31, 2018 · 4 comments

Comments

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Oct 31, 2018 •
edited
Loading

closing_signed only requires that no HTLCs remain in either commitment transaction, but then goes on to say that fee_satoshis must be less than or equal to the base fee of the "final commitment transaction". It is, however, not clear which commitment transaction it references especially if you have in-flight update_fee updates waiting on the full commitment dance. eg in case of async message delivery:

update_fee        -.
commitment_signed -.
                  <-    shutdown
update_fee         >
commitment_signed  >

SHOULD A now have to send its closing_signed? It has no HTLCs in either commitment transaction, but has different commitment transactions here. And if it should, which fee does it have to meet?

I guess there are two obvious solutions, change the closing_signed SHOULD timing to say that the two commitment transactions have to be the same tx number (which seems way simpler), or clarify which fee needs to be met.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Oct 31, 2018
@TheBlueMatt
Delay closing_signed until update_fee exchanges complete
b96e8c5 
See lightning/bolts#499
for a bit more about the ambiguity we're addressing here.

Also drop holding cell update_fee the same way we drop holding
cell update_add_htlcs when sending shutdown, resolving a bug.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Oct 31, 2018
@TheBlueMatt
Delay closing_signed until update_fee exchanges complete
8adf0cd 
See lightning/bolts#499
for a bit more about the ambiguity we're addressing here.

Also drop holding cell update_fee the same way we drop holding
cell update_add_htlcs when sending shutdown, resolving a bug.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Nov 2, 2018
@TheBlueMatt
Delay closing_signed until update_fee exchanges complete
a2f9d35 
See lightning/bolts#499
for a bit more about the ambiguity we're addressing here.

Also drop holding cell update_fee the same way we drop holding
cell update_add_htlcs when sending shutdown, resolving a bug.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Nov 3, 2018
@TheBlueMatt
Delay closing_signed until update_fee exchanges complete
527d036 
See lightning/bolts#499
for a bit more about the ambiguity we're addressing here.

Also drop holding cell update_fee the same way we drop holding
cell update_add_htlcs when sending shutdown, resolving a bug.
@nayuta-gondo
Copy link
Contributor

nayuta-gondo commented Nov 14, 2018 •
edited
Loading

change the closing_signed SHOULD timing to say that the two commitment transactions have to be the same tx number (which seems way simpler),

I agree to try to strict the conditions.
However, the condition that both tx numbers (commitment numbers?) are the same is meaningless.
For example, when the sequence in the figure below is completed, the status of the commitment transaction matches. But both commitment numbers do not match.
https://github.com/lightningnetwork/lightning-rfc/blob/master/02-peer-protocol.md#normal-operation

I think that closing_signed should be sent in the following state.

  • There are no HTLCs on both sides.
  • There is no pending update_fee on both sides.
  • There is only one valid (non-revoked) commitment transaction on both sides.

@nayuta-gondo
Copy link
Contributor

Same but concise expression

  • the channel is empty of HTLCs and all updates are irrevocably committed

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Dec 3, 2018
@TheBlueMatt
Remove check which makes us sometimes never send closing_signed
a2df43d 
This is the case pointed out by nayuta-gondo at
lightning/bolts#499 (comment)
though this doesn't actually solve the issue of ensuring we have a
consistent fee view when we start shutdown processing. There isn't
a clear solution to that however without adding additional state
tracking in Channel.

This also removes an associated test that tests for the correct
behavior (but didn't consider the bug) as we no longer behave
correctly. This should be fine as we'll be removing all the
update_fee garbage with option_simplified_commitment anyway.
@TheBlueMatt TheBlueMatt mentioned this issue Dec 3, 2018
Merged
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Dec 10, 2018
@TheBlueMatt
Remove check which makes us sometimes never send closing_signed
57170cc 
This is the case pointed out by nayuta-gondo at
lightning/bolts#499 (comment)
though this doesn't actually solve the issue of ensuring we have a
consistent fee view when we start shutdown processing. There isn't
a clear solution to that however without adding additional state
tracking in Channel.

This also removes an associated test that tests for the correct
behavior (but didn't consider the bug) as we no longer behave
correctly. This should be fine as we'll be removing all the
update_fee garbage with option_simplified_commitment anyway.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Dec 11, 2018
@TheBlueMatt
Remove check which makes us sometimes never send closing_signed
4b77ef4 
This is the case pointed out by nayuta-gondo at
lightning/bolts#499 (comment)
though this doesn't actually solve the issue of ensuring we have a
consistent fee view when we start shutdown processing. There isn't
a clear solution to that however without adding additional state
tracking in Channel.

This also removes an associated test that tests for the correct
behavior (but didn't consider the bug) as we no longer behave
correctly. This should be fine as we'll be removing all the
update_fee garbage with option_simplified_commitment anyway.
@nayuta-gondo nayuta-gondo mentioned this issue Apr 14, 2019
Closed
@ysangkok
Copy link
Contributor

I am a bit curious if anybody knows if there is any programmatic/tool-based way to write a protocol spec to avoid issues like this?

@t-bast
Copy link
Collaborator

t-bast commented Sep 18, 2024

Hopefully this becomes much better with #1096.

@t-bast t-bast closed this as completed Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ysangkok @TheBlueMatt @t-bast @nayuta-gondo