Splice: Interop Final (probably) with Eclair #8021

ddustin · 2025-01-21T15:32:52Z

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 · 2025-01-21T16:18:55Z

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

niftynei

very cool!

niftynei · 2025-02-04T19:59:05Z

lightningd/channel.c

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


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

niftynei · 2025-02-04T20:04:24Z

channeld/channeld.c

@@ -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);


yeah tal_steal and psbt code aren't good friends

No more theft among psbt 🙅

niftynei · 2025-02-04T20:11:45Z

channeld/channeld.c


-	if (!fromwire_splice_locked(msg, &chanid))
+	if (!fromwire_splice_locked(msg, &chanid, &splice_txid))


we're not checking it?

Added a check for it in e117ec4

niftynei

ACK e91b6b7

pending tests pass :)

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

This is an alternative to what we discussed on Friday to fix the interop test where we splice-out funds from cln that were received via an htlc payment. ElementsProject#8021 (comment) It should be safer to use `amount_msat_to_sat_round_down` instead of `amount_msat_to_sat` because we can drop sub-sat amounts for splices. Also if `amount_msat_to_sat` fails it won't set `local_funds` and create UB.

This is needed to remember if a splice was locked and reconnect occurs mid `splice_locked` attempted so it can be resumed in 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.

Upscale user provided PSBTs to v2 and convert them back to user preference when returned.

This is needed to remember if a splice was locked and reconnect occurs mid `splice_locked` attempted so it can be resumed in reestablish.

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`

PSBT changeset routines were using linearize_output which mutated the memory of the objects it was comparing. This commit fixes that and also cleans up the memory usage to be more clear and more guarentee there is no memory corruption. Changelog-None

Update splice flows to use the new `clone_psbt` method instead of stealing back and forth.

`bitcoin_tx_with_psbt` would somewhat opaquely steal the passed `psbt` value. This caused a bug where code made a `bitcoin_tx` using a psbt without realizing the value was stolen. Because the resulting `bitcoin_tx` was placed in tmpctx it was not immediately clear that using `psbt` afterwards was an error until the tmpctx was cleared — creating a valgrind backtrace far from the actual issue. Switching to the routine to using TAKES and adding documentation in the header, makes it explicitly clear which operation the user is doing — helping prevent future regressions of this kind. Changelog-None

A routine that audit’s and asserts PSBT memory to confirm it has a sane memory allocation hierarchy. Changelog-None

Default wally_tal_ctx to NULL, add extra asserts and tal_checks, and documentation explaning the usage of tal_wally_start/end. Changelog-None

An extra check to ensure the user doesn’t try to sign a splice that wasn’t finalized.

Cleaning up the memory hierarchy of PSBT usage in splicing and `psbt_finalize_input`

Error statement was misformatted and caused a crash instead of reporting the error. Changelog-None

The value of `our_funds` inlightningd is the funds added to the channel during creation. Splicing is a quasi-creation event. This change makes our pending funds be considered funding funds at the moment of splice confirmation. Changelog-None

Other implementations are sending commit_sig batches in different orders. We add support for them being in any order by ordering the batch of messages after receiving them. Changelog-Changed: Increase interop compatability by loosening requirement that commitment signed messages be received in a particular order and sorting them internally.

Update to use Eclair’s spec’d version of reestablish. Changelog-None

Eclair requires `next_commitment_number` to be decremented to resend the individual splice commitment_signed message.

Be more conservative about when we request of send commit sig for splice to match the Eclair behavior.

Turn on the receiving of splice RBF attempts.

The result of splice_signed can fail for many reasons that are non-critical (already in mempool for instance). Don’t abort channels in this case as that causes a force close.

Allow user’s to RBF existing splices. For now this is done by simple executing an additional splice command, in the future this will can also be done with dedicated RPCs. Changelog-Added: Enabled the ability to RBF splices

A splice where reestablish happens at the right moment causes an infinite loop of announcement signatures being sent back and forth. Limit the announcement sigs we send in response to announcement sigs to once per channel session. ChangelogNone

ddustin marked this pull request as ready for review January 21, 2025 16:17

ddustin changed the title ~~Splice: Rotating funding pubkey fix~~ Splice: Interop (probably) with Eclair Feb 1, 2025

ddustin changed the title ~~Splice: Interop (probably) with Eclair~~ Splice: Interop Final (probably) with Eclair Feb 1, 2025

ddustin force-pushed the ddustin/splice_interop_final_(probably) branch from e86bfbc to facfeeb Compare February 1, 2025 15:56

ddustin added this to the v25.02 milestone Feb 1, 2025

ddustin force-pushed the ddustin/splice_interop_final_(probably) branch 9 times, most recently from 29d52d1 to 45ae772 Compare February 4, 2025 19:35

niftynei reviewed Feb 4, 2025

View reviewed changes

ddustin force-pushed the ddustin/splice_interop_final_(probably) branch 3 times, most recently from e117ec4 to e91b6b7 Compare February 4, 2025 20:43

niftynei approved these changes Feb 4, 2025

View reviewed changes

ddustin force-pushed the ddustin/splice_interop_final_(probably) branch 10 times, most recently from c31f4cc to d991a45 Compare February 7, 2025 17:19

ddustin force-pushed the ddustin/splice_interop_final_(probably) branch 2 times, most recently from cd71829 to 6c9979e Compare May 2, 2025 17:51

ddustin and others added 25 commits May 2, 2025 14:19

splice: Add locked field to inflight db

0ac377a

This is needed to remember if a splice was locked and reconnect occurs mid `splice_locked` attempted so it can be resumed in reestablish.

splice: Resume splice_locked on reestablish

93712cb

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.

Splice: Update PSBT version handling

e6d8a68

Upscale user provided PSBTs to v2 and convert them back to user preference when returned.

Add debug outputs and fix prevtx issue

a458001

splice: Add locked field to inflight db

922c16e

This is needed to remember if a splice was locked and reconnect occurs mid `splice_locked` attempted so it can be resumed in reestablish.

splice: Add check for correct txid in splice_locked

202f097

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`

splice: Use clone instead of steal for PSBT

2f43fc2

Update splice flows to use the new `clone_psbt` method instead of stealing back and forth.

PSBT: Add audi_psbt routine

48fbdc8

A routine that audit’s and asserts PSBT memory to confirm it has a sane memory allocation hierarchy. Changelog-None

PSBT: tal_wally updates and docs

1328c30

Default wally_tal_ctx to NULL, add extra asserts and tal_checks, and documentation explaning the usage of tal_wally_start/end. Changelog-None

splice: Prevent user from signing an unfinal splice

fcd700e

An extra check to ensure the user doesn’t try to sign a splice that wasn’t finalized.

PSBT: Clean up PSBT chunk allocations

7ff2026

Cleaning up the memory hierarchy of PSBT usage in splicing and `psbt_finalize_input`

splice: Clean error statement

1d78e8d

Error statement was misformatted and caused a crash instead of reporting the error. Changelog-None

splice: Update our_funds during splice_lock

7679698

The value of `our_funds` inlightningd is the funds added to the channel during creation. Splicing is a quasi-creation event. This change makes our pending funds be considered funding funds at the moment of splice confirmation. Changelog-None

splice: Update to Eclair style of reestablish

ae875db

Update to use Eclair’s spec’d version of reestablish. Changelog-None

splice: Decrement next_commitment_number for Eclair

283005d

Eclair requires `next_commitment_number` to be decremented to resend the individual splice commitment_signed message.

splice: Only send or recv commit sig if needed

7bcf8dc

Be more conservative about when we request of send commit sig for splice to match the Eclair behavior.

splice: Enable the receiving of splice RBF

24246da

Turn on the receiving of splice RBF attempts.

splice: Splice script should not abort on sign

c39fd30

The result of splice_signed can fail for many reasons that are non-critical (already in mempool for instance). Don’t abort channels in this case as that causes a force close.

splice: Enable user splice RBF

7240d05

Allow user’s to RBF existing splices. For now this is done by simple executing an additional splice command, in the future this will can also be done with dedicated RPCs. Changelog-Added: Enabled the ability to RBF splices

splice: Add verbose log messages for new reestablish TLV

229aaa7

Update autogen schema

fdd0731

gossip: Limit announcement sigs reply

52f74f6

A splice where reestablish happens at the right moment causes an infinite loop of announcement signatures being sent back and forth. Limit the announcement sigs we send in response to announcement sigs to once per channel session. ChangelogNone

ddustin force-pushed the ddustin/splice_interop_final_(probably) branch from 6c9979e to 52f74f6 Compare May 2, 2025 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splice: Interop Final (probably) with Eclair #8021

Splice: Interop Final (probably) with Eclair #8021

ddustin commented Jan 21, 2025 •

edited

Loading

ddustin commented Jan 21, 2025

niftynei left a comment

niftynei Feb 4, 2025

ddustin Feb 4, 2025

niftynei Feb 4, 2025

ddustin Feb 4, 2025

niftynei Feb 4, 2025

ddustin Feb 4, 2025 •

edited

Loading

niftynei left a comment


		if (!fromwire_splice_locked(msg, &chanid))
		if (!fromwire_splice_locked(msg, &chanid, &splice_txid))

Splice: Interop Final (probably) with Eclair #8021

Are you sure you want to change the base?

Splice: Interop Final (probably) with Eclair #8021

Conversation

ddustin commented Jan 21, 2025 • edited Loading

ddustin commented Jan 21, 2025

niftynei left a comment

Choose a reason for hiding this comment

niftynei Feb 4, 2025

Choose a reason for hiding this comment

ddustin Feb 4, 2025

Choose a reason for hiding this comment

niftynei Feb 4, 2025

Choose a reason for hiding this comment

ddustin Feb 4, 2025

Choose a reason for hiding this comment

niftynei Feb 4, 2025

Choose a reason for hiding this comment

ddustin Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

niftynei left a comment

Choose a reason for hiding this comment

ddustin commented Jan 21, 2025 •

edited

Loading

ddustin Feb 4, 2025 •

edited

Loading