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: Interop Final (probably) with Eclair #8021

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

Conversation

ddustin
Copy link
Collaborator

@ddustin ddustin commented Jan 21, 2025

Interop testing with Eclair revealed an issue with remote funding key rotation.

This searches for the funding output using the rotated remote funding pubkey instead of the current funding pubkey.

Update the variable name to be more clear which this represents.

Implement new pending spec changes for splice_locked resuming in reestablish.

Fixes #8030

@ddustin ddustin marked this pull request as ready for review January 21, 2025 16:17
@ddustin
Copy link
Collaborator Author

ddustin commented Jan 21, 2025

@remyers reported this fix made his splice interop test pass! 🎉🚀

@ddustin ddustin changed the title Splice: Rotating funding pubkey fix Splice: Interop (probably) with Eclair Feb 1, 2025
@ddustin ddustin changed the title Splice: Interop (probably) with Eclair Splice: Interop Final (probably) with Eclair Feb 1, 2025
@ddustin ddustin force-pushed the ddustin/splice_interop_final_(probably) branch from e86bfbc to facfeeb Compare February 1, 2025 15:56
@ddustin ddustin added this to the v25.02 milestone Feb 1, 2025
@ddustin ddustin force-pushed the ddustin/splice_interop_final_(probably) branch 9 times, most recently from 29d52d1 to 45ae772 Compare February 4, 2025 19:35
Copy link
Contributor

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

very cool!

@@ -200,6 +200,7 @@ new_inflight(struct channel *channel,

inflight->i_am_initiator = i_am_initiator;
inflight->force_sign_first = force_sign_first;
inflight->is_locked = false;
inflight->splice_locked_memonly = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be great to add this to wallet/tests/run-wallet.c for the inflights

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

@@ -3829,7 +3829,7 @@ static void splice_accepter(struct peer *peer, const u8 *inmsg)
new_inflight->remote_funding = peer->splicing->remote_funding_pubkey;
new_inflight->outpoint = outpoint;
new_inflight->amnt = both_amount;
new_inflight->psbt = tal_steal(new_inflight, ictx->current_psbt);
new_inflight->psbt = clone_psbt(new_inflight, ictx->current_psbt);
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah tal_steal and psbt code aren't good friends

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No more theft among psbt 🙅


if (!fromwire_splice_locked(msg, &chanid))
if (!fromwire_splice_locked(msg, &chanid, &splice_txid))
Copy link
Contributor

Choose a reason for hiding this comment

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

we're not checking it?

Copy link
Collaborator Author

@ddustin ddustin Feb 4, 2025

Choose a reason for hiding this comment

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

Added a check for it in e117ec4

Interop testing with Eclair revealed an issue with remote funding key rotation.

This searches for the funding output using the rotated remote funding pubkey instead of the furrent funding pubkey.

Also update the variable name to be more clear which this represents.

Changelog-Changed: Interop fixes for compatability with Eclair
@ddustin ddustin force-pushed the ddustin/splice_interop_final_(probably) branch 2 times, most recently from e1d7602 to e117ec4 Compare February 4, 2025 20:26
ddustin and others added 10 commits February 4, 2025 15:37
This is needed to remember if a splice was locked and reconnect occurs mid `splice_locked` attempted so it can be resumed in reestablish.
This field should be serialized for communication between channeld and lightningd.
To support resuming `splice_locked` across channel reconnect, we need to pass inflight `is_locked` between lightningd and channeld.

This implements that interface between channeld and lightningd so each inflight should have up to date `is_locked` values between restarts.
Use the inflight information to set a correct `locked_txid` value on restart. This is critical for handling interrupted `splice_locked` events that need to be resumed on reconnect/reestablish.
A new case where `splice_locked` must be sent again on reestablish.

This handles the case where `splice_locked` did not complete locally or remotely and must be resumed.
The interaction betwen libwally and CLN’s memory management is tricky. Let’s dodge that problem and just clone the PSBTs.

Clean up some unused PSBT / ictx code while we’re at it
Upscale user provided PSBTs to v2 and convert them back to user preference when returned.
Check that the peer sent the correct txid in their `splice_locked` message.

We have to check this later on in `check_mutal_splice_locked` so we store the value in `splice_state`
@ddustin ddustin force-pushed the ddustin/splice_interop_final_(probably) branch from e117ec4 to e91b6b7 Compare February 4, 2025 20:43
Copy link
Contributor

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

ACK e91b6b7

pending tests pass :)

@ddustin ddustin force-pushed the ddustin/splice_interop_final_(probably) branch from 2e7907a to f5ce59c Compare February 4, 2025 21:55
@ddustin ddustin force-pushed the ddustin/splice_interop_final_(probably) branch from 7de1dbe to 463fd96 Compare February 4, 2025 23:23
@ddustin ddustin force-pushed the ddustin/splice_interop_final_(probably) branch from 03db84e to 8907321 Compare February 5, 2025 07:35
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.

[Splice] Should abort if received tx_add_input for the shared input and prevtx is not empty
3 participants