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

Splice with htlcs #568

Merged
merged 2 commits into from Feb 6, 2024
Merged

Splice with htlcs #568

merged 2 commits into from Feb 6, 2024

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Dec 11, 2023

This PR builds on splices #430 to account for pending htlcs that may exist if the channel has first negotiated quiescence #558. This PR is modeled after Eclair PR # 2720.

The helper function makeCommitTxsWithoutHtlcs is renamed to makeCommitTxs and uses the set of committed htlcs that exist when InteractiveTxSigningSession is instantiated for a splice.

A new attribute htlcsAmount is added to InteractiveTxOutput.Shared to account for the value committed to htlcs from the shared funding input but not assigned to localAmount or remoteAmount.

This PR assumes the quiescence feature is mandatory rather than handle splices without exchanging stfu.

Updated tests for #421 with a pending splice with 5 instead of 6 htlcs maximum on each side.

@remyers
Copy link
Contributor Author

remyers commented Dec 19, 2023

@t-bast sorry for the delay in getting the rebased version. I'm getting an error with the liquidity ads lease backwards compatibility test where it compares the current state with the checkpoints on lines L119 and L138. When I review the difference with the checkpoint I notice what must be the additional discriminator and zero value added in 9aa3be5.

I don't think it makes sense here that the checkpoint should exactly match all future states. I believe we should remove these assert lines otherwise it will always fail when we change the state data structures, even if backwards compatibility is preserved.

@t-bast
Copy link
Member

t-bast commented Dec 19, 2023

I don't think it makes sense here that the checkpoint should exactly match all future states. I believe we should remove these assert lines otherwise it will always fail when we change the state data structures, even if backwards compatibility is preserved.

Sure, modify what you need to modify to get the rebase working, and then we'll see during the review.

@remyers remyers marked this pull request as ready for review December 19, 2023 18:44
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Looking good, I improved the tests and found a small bug in b7b4364, which you can probably apply to your branch and then squash with the existing commit?

@remyers
Copy link
Contributor Author

remyers commented Dec 20, 2023

Looking good, I improved the tests and found a small bug in b7b4364, which you can probably apply to your branch and then squash with the existing commit?

Thanks for the review/fixes. I've squashed them into 699d52f.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK 699d52f, let's wait to make sure liquidity ads is stable before merging this to master, but it's looking good to me 👍

Copy link
Member

@t-bast t-bast 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 1737fa9 into ACINQ:master Feb 6, 2024
2 checks passed
@t-bast t-bast mentioned this pull request Feb 6, 2024
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

2 participants