Fixed splice_locked messgae to include splice_txid field and other interop fixes

[remyers]

After Eclair successfully sends splice_locked I saw this message in the eclair log:

2025-01-22 14:00:12,833 INFO  f.a.eclair.Diagnostics n:034f3bafd420714d9a2c0e6986a4109af5e934604f199eafef4076844c9faf0a4c c:daf879e0b02d7c4c23c15768a3c975d605165c08e38fefe1946daf0b92fb78b7 - OUT msg=SpliceLocked(daf879e0b02d7c4c23c15768a3c975d605165c08e38fefe1946daf0b92fb78b7,c8f7611495e85cab7cb244ffe87537ba1cf4104c527b9a0e08cb9dba45b4aa54,TlvStream(Set(),Set()))
2025-01-22 14:00:38,600 ERROR f.a.e.c.TransportHandler CON n:034f3bafd420714d9a2c0e6986a4109af5e934604f199eafef4076844c9faf0a4c - cannot deserialize 004ddaf879e0b02d7c4c23c15768a3c975d605165c08e38fefe1946daf0b92fb78b7: fundingTxHash: expected exactly 256 bits but got 0 bits

Apparently splice_locked in clightning is missing the splice_txid field as described in the latest splice PR.

This PR includes the minimal changes I needed to continue my testing, but there are probably other things that should be done to check the splice_txid is correct, update tests, etc.

[remyers]

I also noticed the clightning is not resending splice_locked on reconnection - this could be because it received the splice_locked from Eclair so clightning has updated it's commit_number and now will never send it's splice_locked.

On reconnection:
  - MUST retransmit its last `splice_locked` if the `commitment_number`
    is the same as before sending `splice_locked`.

Should the spec be changed to always respond to a splice_locked with your own splice_locked if the fields match?

[remyers]

I've attached the logs for the test:

• connect
• open single funded channel
• Eclair does a splice-in
• (error, because Eclair can't parse splice_locked from clightning)
• force closed the problem channel
• (update clightning with this patch)
• open a new single funded channel
• Eclair does a splice-in on the new channel
• everything seems ok

eclair.log
cln.log

[ddustin]

Looking into this 👀.

Should the spec be changed to always respond to a splice_locked with your own splice_locked if the fields match?

Core Lightning's current behavior is to send splice_locked on reconnect if we already finished mutual splice lock and the peer sends next_funding (txid) in channel_reestablish tvls.

I believe you are correct that there is a missing second case where splice_locked is sent but never received and reconnect happens. Assuming the splice signatures were exchanged successfully -- than next_funding will not appear in reestablish and no new splice_locked is sent.

[ddustin]

This should be &peer->splice_state->locked_txid -- however this value is not preserved after channel restarts, so that needs to addressed as well.

[ddustin]

This is a bit of a heavy lift. I think this is the best way to make splice_state->locked_txid work over restarts.

1. Adding {SQL("ALTER TABLE channel_funding_inflights ADD locked_onchain BOOL DEFAULT 0"), NULL}, to db.c at the end of the db table be able to mark an inflight locked onchain
2. inflight.c needs to add serializiation of is_locked with inflight->is_locked = fromwire_bool(cursor, max); in fromwire_inflight and towire_bool(pptr, inflight->is_locked); in towire_inflight.
3. Modifying channeld_updating_inflight to include an is_locked parameter
4. Update handle_update_inflight to take is_locked parameter and update that value in its inflight
5. Modify wallet_inflight_save SQL query to save is_locked to the database
6. Modify wallet_channel_load_inflights to load `is_locked from the database
7. Update the inflight copy code in the loop above towire_channeld_init to add is_locked to the infcopy
8. Below fromwire_channeld_init, loop through each inflight in peer->splice_state->inflights and get the txid from the psbt of the one that is_locked, placing it in peer->splice_state->locked_txid. Additionally it would be useful to assert only 1 is locked as two would be an error.

I thiiiiink that would do it.

[remyers]

This is a bit of a heavy lift. I think this is the best way to make splice_state->locked_txid work over restarts.

I can't really comment on if that's the right approach. Do you need to use splice_state->locked_txid during reconnect? It should already be your latest funding tx once you have sent and received splice_locked. If it has reached acceptable depth then your node can resend splice_locked with the txid of the latest funding tx. You must have already have saved that information after you exchanged splice_locked with your peer.

However, when you do a reconnect before exchanging splice_locked I can see that you'll need to save the txid of the funding tx negotiated for the splice. We save this information with the splice commitment along with the signatures exchanged.

[ddustin]

If it has reached acceptable depth then your node can resend splice_locked with the txid of the latest funding tx.

Yes you're right, peer->channel->funding.txid is correct in this case.

There needs to be a new separate case that uses peer->splice_state->locked_txid that I'm working on at the moment.

[ddustin]

I also noticed the clightning is not resending splice_locked on reconnection - this could be because it received the splice_locked from Eclair so clightning has updated it's commit_number and now will never send it's splice_locked.

On reconnection:
  - MUST retransmit its last `splice_locked` if the `commitment_number`
    is the same as before sending `splice_locked`.

Should the spec be changed to always respond to a splice_locked with your own splice_locked if the fields match?

It sounds like we need to rethink the sending of splice_locked here in general. Perhaps we just send it every time? 🤔

[remyers]

I also noticed the clightning is not resending splice_locked on reconnection - this could be because it received the splice_locked from Eclair so clightning has updated it's commit_number and now will never send it's splice_locked.

On reconnection:
  - MUST retransmit its last `splice_locked` if the `commitment_number`
    is the same as before sending `splice_locked`.

Should the spec be changed to always respond to a splice_locked with your own splice_locked if the fields match?

It sounds like we need to rethink the sending of splice_locked here in general. Perhaps we just send it every time? 🤔

I looked a bit more into the spec and our own tests. My understanding is that in the case of a reconnect the node that has sent, but not received, splice_locked will send channel_reestablish with the tlv for next_funding (0) set to the txid of the next funding tx. The node that has both sent and received splice_locked will not set the tlv for next_funding in their channel_reestablish message because it is now in a state where the current funding tx is set to the splice funding tx.

In the test I ran, during channel reestablishment Eclair will have set the next_funding tlv and clightning should see that it matches their current funding tx and resend splice_locked.

From the Message Retransmission:

A receiving node:
  - if `next_funding_txid` is set:
.
.
.
    - if `next_funding_txid` matches the latest funding transaction:
      - if that transaction has reached acceptable depth:
        - MUST send `splice_locked`.  

If you don't have a test for this behavior, then I think that's what you need to add along with a fix to the handling of channel_reestablish to send splice_locked based on the spec.

I'd be happy to retest this situation once you have a fix in place.

[ddustin]

From the Message Retransmission:

A receiving node:
  - if `next_funding_txid` is set:
.
.
.
    - if `next_funding_txid` matches the latest funding transaction:
      - if that transaction has reached acceptable depth:
        - MUST send `splice_locked`.  

If you don't have a test for this behavior, then I think that's what you need to add along with a fix to the handling of channel_reestablish to send splice_locked based on the spec.

I believe the current behavior matches the spec as written

lightning/channeld/channeld.c

Lines 5211 to 5218 in 305c377

	} else if (remote_next_funding) { /* No current inflight */
	if (bitcoin_txid_eq(remote_next_funding,
	&peer->channel->funding.txid)) {
	status_info("We have no pending splice but peer"
	" expects one; resending splice_lock");
	peer_write(peer->pps,
	take(towire_splice_locked(NULL, &peer->channel_id)));
	}

"No current inflight" means the inflight candidate has become the latest funding transaction in CLN.

I believe the issue your test is hitting is if splice_locked got interrupted halfway and did not become the funding transaction.

The spec probably needs to be updated to something like

     - if `next_funding_txid` matches the latest funding transaction or locked splice candidate:

[ddustin]

// - if `next_funding_txid` is set:
 } else if (remote_next_funding) { /* No current inflight */

 	// - if `next_funding_txid` matches the latest funding transaction:
 	if (bitcoin_txid_eq(remote_next_funding, 
 			    &peer->channel->funding.txid)) {
 		status_info("We have no pending splice but peer" 
 			    " expects one; resending splice_lock"); 

 		// - MUST send `splice_locked`.
 		peer_write(peer->pps, 
 			   take(towire_splice_locked(NULL, &peer->channel_id))); 
 	} 

The line

- if that transaction has reached acceptable depth:

Is actually redundant because there is no way to become the funding transaction without reaching acceptable depth.

[ddustin]

In the test I ran, during channel reestablishment Eclair will have set the next_funding tlv and clightning should see that it matches their current funding tx and resend splice_locked.

Do you have a copy of these logs? 👀

[remyers]

Commit dc3ae19 fixes the problem with CLN sending the shared outpoint with prevtx set. With these changes the interop works when CLN intiates the splice with the dev-splice command. Interop where Eclair is the splice initiator works with the previous commit.

[ddustin]

Awesome work! Few little things I would fix up but otherwise a great fix. Looks like the main issue was not setting the shared_outpoint in splice_initiator_user_update which makes sense as when the funding input was sent recently moved around.

Great find!

[ddustin]

This should use fmt_bitcoin_txid instead of bitcoin_txid_to_hex

For example:

	if (!bitcoin_txid_eq(&signed_psbt_txid, &current_psbt_txid))
		status_failed(STATUS_FAIL_INTERNAL_ERROR,
			      "Signed PSBT txid %s does not match"
			      " current_psbt_txid %s",
			      fmt_bitcoin_txid(tmpctx, &signed_psbt_txid),
			      fmt_bitcoin_txid(tmpctx, &current_psbt_txid));

[ddustin]

fmt_bitcoin_txid here as well

[ddustin]

I would cut this log statement

[ddustin]

If it has reached acceptable depth then your node can resend splice_locked with the txid of the latest funding tx.

Yes you're right, peer->channel->funding.txid is correct in this case.

There needs to be a new separate case that uses peer->splice_state->locked_txid that I'm working on at the moment.

[ddustin]

I fixed the changes but you have editing turned off for the PR.

I'll manually pull your commits over