Skip to content

Remove spurious interactive-tx commit_sig retransmission #2966

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
Merged
t-bast merged 1 commit into from
Mar 11, 2025
Merged

Remove spurious interactive-tx commit_sig retransmission #2966

t-bast merged 1 commit into from
Mar 11, 2025

Conversation

@t-bast
Copy link
Member

@t-bast t-bast commented Dec 13, 2024 •
edited
Loading

We fully implement lightning/bolts#1214 to stop retransmitting commit_sig when our peer has already received it. We also correctly set next_commitment_number to let our peer know whether we have received their commit_sig or not.

Note that if our peer hasn't upgraded and always sends next_commitment_number = current_commitment_number + 1 even when they haven't received our commit_sig, this won't lead to a force-close: we will simply wait for them to send an error, which will never happen. Their channel will be stuck until they upgrade and send next_commitment_number = current_commitment_number.

This change must only be merged once:

@t-bast t-bast marked this pull request as draft December 13, 2024 09:18
@t-bast t-bast mentioned this pull request Dec 13, 2024
Merged
@t-bast t-bast force-pushed the avoid-spurious-commit-sig-retransmit-reconnect branch from e3cf443 to c713beb Compare January 30, 2025 13:59
@t-bast t-bast mentioned this pull request Jan 30, 2025
Open
@t-bast t-bast force-pushed the avoid-spurious-commit-sig-retransmit-reconnect branch from c713beb to 8f586d5 Compare February 19, 2025 16:05
@t-bast t-bast force-pushed the avoid-spurious-commit-sig-retransmit-reconnect branch from 8f586d5 to 9625a6e Compare March 3, 2025 09:10
@t-bast t-bast marked this pull request as ready for review March 5, 2025 13:49
@t-bast t-bast requested review from pm47 and remyers March 5, 2025 13:49
pm47
pm47 previously approved these changes Mar 5, 2025
View reviewed changes
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

That's subtle, hard to be sure all combinations are correctly handled, but LGTM.

@remyers
Copy link
Contributor

remyers commented Mar 6, 2025

How do we test the behavior "...if our peer hasn't upgraded and always sends next_commitment_number = current_commitment_number + 1 even when they haven't received our commit_sig, this won't lead to a force-close: we will simply wait for them to send an error, which will never happen." ? If both Bob and Alice send commit_sig and Bob does not receive it but sends bobCommitIndex + 1 in their channel_reestablish (legacy behavior), then after a reconnect would we expect Alice to wait for Bob to send tx_signatures and Bob to wait for Alice to send commit_sig again? Perhaps a test that shows their hung state and how a reconnect with the correct commit index from bob get's them to be unstuck.

@t-bast t-bast force-pushed the avoid-spurious-commit-sig-retransmit-reconnect branch from 426f55a to be61b2e Compare March 6, 2025 17:57
@t-bast
Copy link
Member Author

t-bast commented Mar 6, 2025 •
edited
Loading

Perhaps a test that shows their hung state and how a reconnect with the correct commit index from bob get's them to be unstuck.

I have rebased to fix conflicts and added de331c8 where I showcase the behavior with non-upgraded nodes.

@t-bast t-bast force-pushed the avoid-spurious-commit-sig-retransmit-reconnect branch from be61b2e to de331c8 Compare March 6, 2025 18:01
pm47
pm47 previously approved these changes Mar 6, 2025
View reviewed changes
@t-bast
Copy link
Member Author

t-bast commented Mar 10, 2025

I'll ask feedback during tonight's spec meeting before merging this PR. If other implementations are fine with this, we'll merge this PR and make an eclair release tomorrow.

@t-bast
Remove spurious interactive-tx commit_sig retransmission
5e35a21 
We fully implement lightning/bolts#1214 to stop
retransmitting `commit_sig` when our peer has already received it. We
also correctly set `next_commitment_number` to let our peer know whether
we have received their `commit_sig` or not.

We also retransmit `tx_signatures` (and, if requested, `commit_sig`)
after sending `channel_ready` in the 0-conf case. This was missing and
was a bug.
@t-bast t-bast force-pushed the avoid-spurious-commit-sig-retransmit-reconnect branch from a4bed35 to 5e35a21 Compare March 11, 2025 08:41
@t-bast
Copy link
Member Author

t-bast commented Mar 11, 2025

We've discussed this during yesterday's spec meeting: we have an ACK to release, so I've done a last rebase and we can now merge!

remyers
remyers approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@remyers remyers left a comment

Choose a reason for hiding this comment

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

LGTM!

@t-bast t-bast merged commit 6a8df49 into master Mar 11, 2025
1 check passed
@t-bast t-bast deleted the avoid-spurious-commit-sig-retransmit-reconnect branch March 11, 2025 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pm47 pm47 pm47 left review comments

+1 more reviewer

@remyers remyers remyers approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@t-bast @remyers @pm47