channeld: fix channel_ready retransmission after splice via funding_tx_index

[nGoline]

Problem

On reconnect, channeld decides whether to retransmit channel_ready based on whether the peer's channel_reestablish references a splice (is_splice_active). It computed that by comparing the peer's my_current_funding_locked txid against peer->channel->funding.txid.

That comparison is wrong once a splice locks: channel_update_funding() overwrites funding.txid with the splice txid, so afterwards both sides agree on the same txid, the check falls through, and channel_ready is incorrectly retransmitted on reconnect, which can disrupt channels.

This is the problem @ddustin raised reviewing #9079 (comparing against the txid is incorrect, and we should track metadata on the inflight instead), suggesting a funding_tx_index, matching what eclair does.

Fix

Give every funding tx a stable funding_tx_index: 0 for the original funding (including RBF attempts), incrementing by 1 per splice. It is:

• carried on the channeld inflight and on the channel,
• threaded through the channeld_init and channeld_add_inflight wire,
• assigned on splice creation (parent + 1) and copied onto the channel funding when a splice locks,
• persisted in channel_funding_inflights and channels (two migrations, DEFAULT 0, so existing channels are unaffected).

The reestablish check now resolves the my_current_funding_locked txid to its funding_tx_index (current funding or a pending inflight) and treats > 0 as a splice.

Commits (bisectable)

1. splice: add tests for funding_tx_index tracking, regression tests (xfail).
2. splice: track funding_tx_index on inflights and channel funding, the plumbing.
3. channeld: detect splice on reestablish via funding_tx_index, not txid, the fix.

Tests

• test_splice_reconnect_after_lock_no_channel_ready: splice, restart, reestablish; asserts the index persists and channel_ready is not retransmitted. It fails on commit 2 (old txid compare) and passes on commit 3, so it genuinely guards the fix.
• test_splice_funding_tx_index_increments: two sequential splices reach index 2.
• test_splice_inflight_funding_tx_index: a pending splice inflight carries index 1 and survives a restart.
• Existing test_reconnect_no_update confirms non-splice channels still retransmit channel_ready.

Risk and plan

The behavioural change is a single line in the reestablish check. The plan is to get the funding_tx_index semantics reviewed properly, and if we don't converge before the v26.09 freeze, revert that one line with a FIXME and keep the plumbing for the follow-up. So this carries low risk.

@ddustin, this implements the funding_tx_index approach you suggested; would appreciate your review of the assignment semantics and whether it matches what you and tbast had in mind.

[ddustin]

This is great! 👏 Mostly style notes with only one major concern:

test_splice_reconnect_after_lock_no_channel_ready may not actually be testing for the lack of channel_ready because of the unneeded billboard: Channel ready for use. check.

[nGoline]

@ddustin comments addressed. Just waiting for your review.

[ddustin]

Reviewing this it appears to be deviating from the spec. Doing so inside of reestablish is particularly dangerous and such behavior changes realistically trigger a new cycle of interop testing with LDK and ACINQ.

[ddustin]

These extra conditions appear to be leaving the spec, perhaps by quite a lot. The code should match what the spec says, referenced in the comment above.

*Anode:
*-if `next_commitment_number` iszero:
*-MUSTimmediatelyfailthechannelandbroadcastanyrelevantlatestcommitment*transaction.
*-if `next_commitment_number` is1inboththe `channel_reestablish` it*sentandreceived, andnoneofthose `channel_reestablish` messages*contain `my_current_funding_locked` or `next_funding` forasplicetransaction:
*-MUSTretransmit `channel_ready`.
*-otherwise:
*-MUSTNOTretransmit `channel_ready`, butMAYsend `channel_ready` with*adifferent `short_channel_id` `alias` field.

If the spec is wrong in some way, which is always possible, we need to get the spec updated and then conform to the new spec.

Adding conditions to these things in disagreement with the spec is precisely how we get interop issues that are extraordinarily difficult to track down.

[nGoline]

You were right, and my earlier reply missed your point. The shape you NACKed on #9079 was still in the condition, and I had only reimplemented one disjunct inside it. That's fixed now.

The check is back to the original three conditions plus the single conjunct the spec adds:

if (peer->channel_ready[LOCAL]
&&peer->next_index[LOCAL] ==1&&next_commitment_number==1&& !reestablish_mentions_splice(peer, send_tlvs, recv_tlvs)) {

locked_ready[LOCAL] and the two bare next_funding checks are gone. The helper applies funding_tx_index > 0 symmetrically to both the channel_reestablish we sent and the one we received, which is a direct reading of "contain my_current_funding_locked or next_funding for a splice transaction".

On why the previous version was wrong: check_mutual_splice_locked() clears locked_ready[LOCAL] and calls channel_update_funding() to rewrite channel->funding.txid to the splice txid, in the same call. Both proxies for "is this a splice transaction" went stale at that exact moment, so both halves of the check were wrong once a splice locked, not just the received side. And per the sending rules, a node that has never spliced still includes my_current_funding_locked with the funding txid, which is why the "for a splice transaction" qualifier can't be answered by a presence check or a txid comparison.

On interop: an unrecognized txid is now treated as a splice, so behaviour is unchanged from master for non-splice channels, pending inflights, and txids we can't account for. The splice-locked case is the only difference.

To check the test actually guards this rather than passing incidentally, I swapped the helper body back to the old !bitcoin_txid_eq(txid, &funding.txid) and rebuilt: test_splice_reconnect_after_lock_no_channel_ready fails, and passes again with funding_tx_index. So next_index[LOCAL] == 1 does hold after a splice and that branch is live.

Pushed as three fixup commits rather than a rewrite so you can see just the delta. I'll autosquash before merge.

[daywalker90]

Needs rebase

[nGoline]

Problem

On reconnect, channeld decides whether to retransmit channel_ready based on whether the peer's channel_reestablish references a splice (is_splice_active). It computed that by comparing the peer's my_current_funding_locked txid against peer->channel->funding.txid.

That comparison is wrong once a splice locks: channel_update_funding() overwrites funding.txid with the splice txid, so afterwards both sides agree on the same txid, the check falls through, and channel_ready is incorrectly retransmitted on reconnect, which can disrupt channels.

This is the problem @ddustin raised reviewing #9079 (comparing against the txid is incorrect, and we should track metadata on the inflight instead), suggesting a funding_tx_index, matching what eclair does.

Fix

Give every funding tx a stable funding_tx_index: 0 for the original funding (including RBF attempts), incrementing by 1 per splice. It is:

• carried on the channeld inflight and on the channel,
• threaded through the channeld_init and channeld_add_inflight wire,
• assigned on splice creation (parent + 1) and copied onto the channel funding when a splice locks,
• persisted in channel_funding_inflights and channels (two migrations, DEFAULT 0, so existing channels are unaffected).

The reestablish check now resolves the my_current_funding_locked txid to its funding_tx_index (current funding or a pending inflight) and treats > 0 as a splice.

Commits (bisectable)

1. splice: add tests for funding_tx_index tracking, regression tests (xfail).
2. splice: track funding_tx_index on inflights and channel funding, the plumbing.
3. channeld: detect splice on reestablish via funding_tx_index, not txid, the fix.

Tests

• test_splice_reconnect_after_lock_no_channel_ready: splice, restart, reestablish; asserts the index persists and channel_ready is not retransmitted. It fails on commit 2 (old txid compare) and passes on commit 3, so it genuinely guards the fix.
• test_splice_funding_tx_index_increments: two sequential splices reach index 2.
• test_splice_inflight_funding_tx_index: a pending splice inflight carries index 1 and survives a restart.
• Existing test_reconnect_no_update confirms non-splice channels still retransmit channel_ready.

Risk and plan

The behavioural change is a single line in the reestablish check. The plan is to get the funding_tx_index semantics reviewed properly, and if we don't converge before the v26.09 freeze, revert that one line with a FIXME and keep the plumbing for the follow-up. So this carries low risk.

@ddustin, this implements the funding_tx_index approach you suggested; would appreciate your review of the assignment semantics and whether it matches what you and tbast had in mind.

[ddustin]

This is great! 👏 Mostly style notes with only one major concern:

test_splice_reconnect_after_lock_no_channel_ready may not actually be testing for the lack of channel_ready because of the unneeded billboard: Channel ready for use. check.

[nGoline]

@ddustin comments addressed. Just waiting for your review.

[ddustin]

Reviewing this it appears to be deviating from the spec. Doing so inside of reestablish is particularly dangerous and such behavior changes realistically trigger a new cycle of interop testing with LDK and ACINQ.

[ddustin]

These extra conditions appear to be leaving the spec, perhaps by quite a lot. The code should match what the spec says, referenced in the comment above.

*Anode:
*-if `next_commitment_number` iszero:
*-MUSTimmediatelyfailthechannelandbroadcastanyrelevantlatestcommitment*transaction.
*-if `next_commitment_number` is1inboththe `channel_reestablish` it*sentandreceived, andnoneofthose `channel_reestablish` messages*contain `my_current_funding_locked` or `next_funding` forasplicetransaction:
*-MUSTretransmit `channel_ready`.
*-otherwise:
*-MUSTNOTretransmit `channel_ready`, butMAYsend `channel_ready` with*adifferent `short_channel_id` `alias` field.

If the spec is wrong in some way, which is always possible, we need to get the spec updated and then conform to the new spec.

Adding conditions to these things in disagreement with the spec is precisely how we get interop issues that are extraordinarily difficult to track down.

[nGoline]

You were right, and my earlier reply missed your point. The shape you NACKed on #9079 was still in the condition, and I had only reimplemented one disjunct inside it. That's fixed now.

The check is back to the original three conditions plus the single conjunct the spec adds:

if (peer->channel_ready[LOCAL]
&&peer->next_index[LOCAL] ==1&&next_commitment_number==1&& !reestablish_mentions_splice(peer, send_tlvs, recv_tlvs)) {

locked_ready[LOCAL] and the two bare next_funding checks are gone. The helper applies funding_tx_index > 0 symmetrically to both the channel_reestablish we sent and the one we received, which is a direct reading of "contain my_current_funding_locked or next_funding for a splice transaction".

On why the previous version was wrong: check_mutual_splice_locked() clears locked_ready[LOCAL] and calls channel_update_funding() to rewrite channel->funding.txid to the splice txid, in the same call. Both proxies for "is this a splice transaction" went stale at that exact moment, so both halves of the check were wrong once a splice locked, not just the received side. And per the sending rules, a node that has never spliced still includes my_current_funding_locked with the funding txid, which is why the "for a splice transaction" qualifier can't be answered by a presence check or a txid comparison.

On interop: an unrecognized txid is now treated as a splice, so behaviour is unchanged from master for non-splice channels, pending inflights, and txids we can't account for. The splice-locked case is the only difference.

To check the test actually guards this rather than passing incidentally, I swapped the helper body back to the old !bitcoin_txid_eq(txid, &funding.txid) and rebuilt: test_splice_reconnect_after_lock_no_channel_ready fails, and passes again with funding_tx_index. So next_index[LOCAL] == 1 does hold after a splice and that branch is live.

Pushed as three fixup commits rather than a rewrite so you can see just the delta. I'll autosquash before merge.

[daywalker90]

Needs rebase

[nGoline]

Problem

On reconnect, channeld decides whether to retransmit channel_ready based on whether the peer's channel_reestablish references a splice (is_splice_active). It computed that by comparing the peer's my_current_funding_locked txid against peer->channel->funding.txid.

That comparison is wrong once a splice locks: channel_update_funding() overwrites funding.txid with the splice txid, so afterwards both sides agree on the same txid, the check falls through, and channel_ready is incorrectly retransmitted on reconnect, which can disrupt channels.

This is the problem @ddustin raised reviewing #9079 (comparing against the txid is incorrect, and we should track metadata on the inflight instead), suggesting a funding_tx_index, matching what eclair does.

Fix

Give every funding tx a stable funding_tx_index: 0 for the original funding (including RBF attempts), incrementing by 1 per splice. It is:

• carried on the channeld inflight and on the channel,
• threaded through the channeld_init and channeld_add_inflight wire,
• assigned on splice creation (parent + 1) and copied onto the channel funding when a splice locks,
• persisted in channel_funding_inflights and channels (two migrations, DEFAULT 0, so existing channels are unaffected).

The reestablish check now resolves the my_current_funding_locked txid to its funding_tx_index (current funding or a pending inflight) and treats > 0 as a splice.

Commits (bisectable)

1. splice: add tests for funding_tx_index tracking, regression tests (xfail).
2. splice: track funding_tx_index on inflights and channel funding, the plumbing.
3. channeld: detect splice on reestablish via funding_tx_index, not txid, the fix.

Tests

• test_splice_reconnect_after_lock_no_channel_ready: splice, restart, reestablish; asserts the index persists and channel_ready is not retransmitted. It fails on commit 2 (old txid compare) and passes on commit 3, so it genuinely guards the fix.
• test_splice_funding_tx_index_increments: two sequential splices reach index 2.
• test_splice_inflight_funding_tx_index: a pending splice inflight carries index 1 and survives a restart.
• Existing test_reconnect_no_update confirms non-splice channels still retransmit channel_ready.

Risk and plan

The behavioural change is a single line in the reestablish check. The plan is to get the funding_tx_index semantics reviewed properly, and if we don't converge before the v26.09 freeze, revert that one line with a FIXME and keep the plumbing for the follow-up. So this carries low risk.

@ddustin, this implements the funding_tx_index approach you suggested; would appreciate your review of the assignment semantics and whether it matches what you and tbast had in mind.

[ddustin]

This is great! 👏 Mostly style notes with only one major concern:

test_splice_reconnect_after_lock_no_channel_ready may not actually be testing for the lack of channel_ready because of the unneeded billboard: Channel ready for use. check.

[nGoline]

@ddustin comments addressed. Just waiting for your review.

[ddustin]

Reviewing this it appears to be deviating from the spec. Doing so inside of reestablish is particularly dangerous and such behavior changes realistically trigger a new cycle of interop testing with LDK and ACINQ.

[ddustin]

These extra conditions appear to be leaving the spec, perhaps by quite a lot. The code should match what the spec says, referenced in the comment above.

*Anode:
*-if `next_commitment_number` iszero:
*-MUSTimmediatelyfailthechannelandbroadcastanyrelevantlatestcommitment*transaction.
*-if `next_commitment_number` is1inboththe `channel_reestablish` it*sentandreceived, andnoneofthose `channel_reestablish` messages*contain `my_current_funding_locked` or `next_funding` forasplicetransaction:
*-MUSTretransmit `channel_ready`.
*-otherwise:
*-MUSTNOTretransmit `channel_ready`, butMAYsend `channel_ready` with*adifferent `short_channel_id` `alias` field.

If the spec is wrong in some way, which is always possible, we need to get the spec updated and then conform to the new spec.

Adding conditions to these things in disagreement with the spec is precisely how we get interop issues that are extraordinarily difficult to track down.

[nGoline]

You were right, and my earlier reply missed your point. The shape you NACKed on #9079 was still in the condition, and I had only reimplemented one disjunct inside it. That's fixed now.

The check is back to the original three conditions plus the single conjunct the spec adds:

if (peer->channel_ready[LOCAL]
&&peer->next_index[LOCAL] ==1&&next_commitment_number==1&& !reestablish_mentions_splice(peer, send_tlvs, recv_tlvs)) {

locked_ready[LOCAL] and the two bare next_funding checks are gone. The helper applies funding_tx_index > 0 symmetrically to both the channel_reestablish we sent and the one we received, which is a direct reading of "contain my_current_funding_locked or next_funding for a splice transaction".

On why the previous version was wrong: check_mutual_splice_locked() clears locked_ready[LOCAL] and calls channel_update_funding() to rewrite channel->funding.txid to the splice txid, in the same call. Both proxies for "is this a splice transaction" went stale at that exact moment, so both halves of the check were wrong once a splice locked, not just the received side. And per the sending rules, a node that has never spliced still includes my_current_funding_locked with the funding txid, which is why the "for a splice transaction" qualifier can't be answered by a presence check or a txid comparison.

On interop: an unrecognized txid is now treated as a splice, so behaviour is unchanged from master for non-splice channels, pending inflights, and txids we can't account for. The splice-locked case is the only difference.

To check the test actually guards this rather than passing incidentally, I swapped the helper body back to the old !bitcoin_txid_eq(txid, &funding.txid) and rebuilt: test_splice_reconnect_after_lock_no_channel_ready fails, and passes again with funding_tx_index. So next_index[LOCAL] == 1 does hold after a splice and that branch is live.

Pushed as three fixup commits rather than a rewrite so you can see just the delta. I'll autosquash before merge.

[daywalker90]

Needs rebase

[nGoline]

Problem

On reconnect, channeld decides whether to retransmit channel_ready based on whether the peer's channel_reestablish references a splice (is_splice_active). It computed that by comparing the peer's my_current_funding_locked txid against peer->channel->funding.txid.

That comparison is wrong once a splice locks: channel_update_funding() overwrites funding.txid with the splice txid, so afterwards both sides agree on the same txid, the check falls through, and channel_ready is incorrectly retransmitted on reconnect, which can disrupt channels.

This is the problem @ddustin raised reviewing #9079 (comparing against the txid is incorrect, and we should track metadata on the inflight instead), suggesting a funding_tx_index, matching what eclair does.

Fix

Give every funding tx a stable funding_tx_index: 0 for the original funding (including RBF attempts), incrementing by 1 per splice. It is:

• carried on the channeld inflight and on the channel,
• threaded through the channeld_init and channeld_add_inflight wire,
• assigned on splice creation (parent + 1) and copied onto the channel funding when a splice locks,
• persisted in channel_funding_inflights and channels (two migrations, DEFAULT 0, so existing channels are unaffected).

The reestablish check now resolves the my_current_funding_locked txid to its funding_tx_index (current funding or a pending inflight) and treats > 0 as a splice.

Commits (bisectable)

1. splice: add tests for funding_tx_index tracking, regression tests (xfail).
2. splice: track funding_tx_index on inflights and channel funding, the plumbing.
3. channeld: detect splice on reestablish via funding_tx_index, not txid, the fix.

Tests

• test_splice_reconnect_after_lock_no_channel_ready: splice, restart, reestablish; asserts the index persists and channel_ready is not retransmitted. It fails on commit 2 (old txid compare) and passes on commit 3, so it genuinely guards the fix.
• test_splice_funding_tx_index_increments: two sequential splices reach index 2.
• test_splice_inflight_funding_tx_index: a pending splice inflight carries index 1 and survives a restart.
• Existing test_reconnect_no_update confirms non-splice channels still retransmit channel_ready.

Risk and plan

The behavioural change is a single line in the reestablish check. The plan is to get the funding_tx_index semantics reviewed properly, and if we don't converge before the v26.09 freeze, revert that one line with a FIXME and keep the plumbing for the follow-up. So this carries low risk.

@ddustin, this implements the funding_tx_index approach you suggested; would appreciate your review of the assignment semantics and whether it matches what you and tbast had in mind.

[ddustin]

This is great! 👏 Mostly style notes with only one major concern:

test_splice_reconnect_after_lock_no_channel_ready may not actually be testing for the lack of channel_ready because of the unneeded billboard: Channel ready for use. check.

[nGoline]

@ddustin comments addressed. Just waiting for your review.

[ddustin]

Reviewing this it appears to be deviating from the spec. Doing so inside of reestablish is particularly dangerous and such behavior changes realistically trigger a new cycle of interop testing with LDK and ACINQ.

[ddustin]

These extra conditions appear to be leaving the spec, perhaps by quite a lot. The code should match what the spec says, referenced in the comment above.

*Anode:
*-if `next_commitment_number` iszero:
*-MUSTimmediatelyfailthechannelandbroadcastanyrelevantlatestcommitment*transaction.
*-if `next_commitment_number` is1inboththe `channel_reestablish` it*sentandreceived, andnoneofthose `channel_reestablish` messages*contain `my_current_funding_locked` or `next_funding` forasplicetransaction:
*-MUSTretransmit `channel_ready`.
*-otherwise:
*-MUSTNOTretransmit `channel_ready`, butMAYsend `channel_ready` with*adifferent `short_channel_id` `alias` field.

If the spec is wrong in some way, which is always possible, we need to get the spec updated and then conform to the new spec.

Adding conditions to these things in disagreement with the spec is precisely how we get interop issues that are extraordinarily difficult to track down.

[nGoline]

You were right, and my earlier reply missed your point. The shape you NACKed on #9079 was still in the condition, and I had only reimplemented one disjunct inside it. That's fixed now.

The check is back to the original three conditions plus the single conjunct the spec adds:

if (peer->channel_ready[LOCAL]
&&peer->next_index[LOCAL] ==1&&next_commitment_number==1&& !reestablish_mentions_splice(peer, send_tlvs, recv_tlvs)) {

locked_ready[LOCAL] and the two bare next_funding checks are gone. The helper applies funding_tx_index > 0 symmetrically to both the channel_reestablish we sent and the one we received, which is a direct reading of "contain my_current_funding_locked or next_funding for a splice transaction".

On why the previous version was wrong: check_mutual_splice_locked() clears locked_ready[LOCAL] and calls channel_update_funding() to rewrite channel->funding.txid to the splice txid, in the same call. Both proxies for "is this a splice transaction" went stale at that exact moment, so both halves of the check were wrong once a splice locked, not just the received side. And per the sending rules, a node that has never spliced still includes my_current_funding_locked with the funding txid, which is why the "for a splice transaction" qualifier can't be answered by a presence check or a txid comparison.

On interop: an unrecognized txid is now treated as a splice, so behaviour is unchanged from master for non-splice channels, pending inflights, and txids we can't account for. The splice-locked case is the only difference.

To check the test actually guards this rather than passing incidentally, I swapped the helper body back to the old !bitcoin_txid_eq(txid, &funding.txid) and rebuilt: test_splice_reconnect_after_lock_no_channel_ready fails, and passes again with funding_tx_index. So next_index[LOCAL] == 1 does hold after a splice and that branch is live.

Pushed as three fixup commits rather than a rewrite so you can see just the delta. I'll autosquash before merge.

[daywalker90]

Needs rebase

[nGoline]

Problem

On reconnect, channeld decides whether to retransmit channel_ready based on whether the peer's channel_reestablish references a splice (is_splice_active). It computed that by comparing the peer's my_current_funding_locked txid against peer->channel->funding.txid.

That comparison is wrong once a splice locks: channel_update_funding() overwrites funding.txid with the splice txid, so afterwards both sides agree on the same txid, the check falls through, and channel_ready is incorrectly retransmitted on reconnect, which can disrupt channels.

This is the problem @ddustin raised reviewing #9079 (comparing against the txid is incorrect, and we should track metadata on the inflight instead), suggesting a funding_tx_index, matching what eclair does.

Fix

Give every funding tx a stable funding_tx_index: 0 for the original funding (including RBF attempts), incrementing by 1 per splice. It is:

• carried on the channeld inflight and on the channel,
• threaded through the channeld_init and channeld_add_inflight wire,
• assigned on splice creation (parent + 1) and copied onto the channel funding when a splice locks,
• persisted in channel_funding_inflights and channels (two migrations, DEFAULT 0, so existing channels are unaffected).

The reestablish check now resolves the my_current_funding_locked txid to its funding_tx_index (current funding or a pending inflight) and treats > 0 as a splice.

Commits (bisectable)

1. splice: add tests for funding_tx_index tracking, regression tests (xfail).
2. splice: track funding_tx_index on inflights and channel funding, the plumbing.
3. channeld: detect splice on reestablish via funding_tx_index, not txid, the fix.

Tests

• test_splice_reconnect_after_lock_no_channel_ready: splice, restart, reestablish; asserts the index persists and channel_ready is not retransmitted. It fails on commit 2 (old txid compare) and passes on commit 3, so it genuinely guards the fix.
• test_splice_funding_tx_index_increments: two sequential splices reach index 2.
• test_splice_inflight_funding_tx_index: a pending splice inflight carries index 1 and survives a restart.
• Existing test_reconnect_no_update confirms non-splice channels still retransmit channel_ready.

Risk and plan

The behavioural change is a single line in the reestablish check. The plan is to get the funding_tx_index semantics reviewed properly, and if we don't converge before the v26.09 freeze, revert that one line with a FIXME and keep the plumbing for the follow-up. So this carries low risk.

@ddustin, this implements the funding_tx_index approach you suggested; would appreciate your review of the assignment semantics and whether it matches what you and tbast had in mind.

[ddustin]

This is great! 👏 Mostly style notes with only one major concern:

test_splice_reconnect_after_lock_no_channel_ready may not actually be testing for the lack of channel_ready because of the unneeded billboard: Channel ready for use. check.

[nGoline]

@ddustin comments addressed. Just waiting for your review.

[ddustin]

Reviewing this it appears to be deviating from the spec. Doing so inside of reestablish is particularly dangerous and such behavior changes realistically trigger a new cycle of interop testing with LDK and ACINQ.

[ddustin]

These extra conditions appear to be leaving the spec, perhaps by quite a lot. The code should match what the spec says, referenced in the comment above.

*Anode:
*-if `next_commitment_number` iszero:
*-MUSTimmediatelyfailthechannelandbroadcastanyrelevantlatestcommitment*transaction.
*-if `next_commitment_number` is1inboththe `channel_reestablish` it*sentandreceived, andnoneofthose `channel_reestablish` messages*contain `my_current_funding_locked` or `next_funding` forasplicetransaction:
*-MUSTretransmit `channel_ready`.
*-otherwise:
*-MUSTNOTretransmit `channel_ready`, butMAYsend `channel_ready` with*adifferent `short_channel_id` `alias` field.

If the spec is wrong in some way, which is always possible, we need to get the spec updated and then conform to the new spec.

Adding conditions to these things in disagreement with the spec is precisely how we get interop issues that are extraordinarily difficult to track down.

[nGoline]

You were right, and my earlier reply missed your point. The shape you NACKed on #9079 was still in the condition, and I had only reimplemented one disjunct inside it. That's fixed now.

The check is back to the original three conditions plus the single conjunct the spec adds:

if (peer->channel_ready[LOCAL]
&&peer->next_index[LOCAL] ==1&&next_commitment_number==1&& !reestablish_mentions_splice(peer, send_tlvs, recv_tlvs)) {

locked_ready[LOCAL] and the two bare next_funding checks are gone. The helper applies funding_tx_index > 0 symmetrically to both the channel_reestablish we sent and the one we received, which is a direct reading of "contain my_current_funding_locked or next_funding for a splice transaction".

On why the previous version was wrong: check_mutual_splice_locked() clears locked_ready[LOCAL] and calls channel_update_funding() to rewrite channel->funding.txid to the splice txid, in the same call. Both proxies for "is this a splice transaction" went stale at that exact moment, so both halves of the check were wrong once a splice locked, not just the received side. And per the sending rules, a node that has never spliced still includes my_current_funding_locked with the funding txid, which is why the "for a splice transaction" qualifier can't be answered by a presence check or a txid comparison.

On interop: an unrecognized txid is now treated as a splice, so behaviour is unchanged from master for non-splice channels, pending inflights, and txids we can't account for. The splice-locked case is the only difference.

To check the test actually guards this rather than passing incidentally, I swapped the helper body back to the old !bitcoin_txid_eq(txid, &funding.txid) and rebuilt: test_splice_reconnect_after_lock_no_channel_ready fails, and passes again with funding_tx_index. So next_index[LOCAL] == 1 does hold after a splice and that branch is live.

Pushed as three fixup commits rather than a rewrite so you can see just the delta. I'll autosquash before merge.

[daywalker90]

Needs rebase

[nGoline]

Problem

On reconnect, channeld decides whether to retransmit channel_ready based on whether the peer's channel_reestablish references a splice (is_splice_active). It computed that by comparing the peer's my_current_funding_locked txid against peer->channel->funding.txid.

That comparison is wrong once a splice locks: channel_update_funding() overwrites funding.txid with the splice txid, so afterwards both sides agree on the same txid, the check falls through, and channel_ready is incorrectly retransmitted on reconnect, which can disrupt channels.

This is the problem @ddustin raised reviewing #9079 (comparing against the txid is incorrect, and we should track metadata on the inflight instead), suggesting a funding_tx_index, matching what eclair does.

Fix

Give every funding tx a stable funding_tx_index: 0 for the original funding (including RBF attempts), incrementing by 1 per splice. It is:

• carried on the channeld inflight and on the channel,
• threaded through the channeld_init and channeld_add_inflight wire,
• assigned on splice creation (parent + 1) and copied onto the channel funding when a splice locks,
• persisted in channel_funding_inflights and channels (two migrations, DEFAULT 0, so existing channels are unaffected).

The reestablish check now resolves the my_current_funding_locked txid to its funding_tx_index (current funding or a pending inflight) and treats > 0 as a splice.

Commits (bisectable)

1. splice: add tests for funding_tx_index tracking, regression tests (xfail).
2. splice: track funding_tx_index on inflights and channel funding, the plumbing.
3. channeld: detect splice on reestablish via funding_tx_index, not txid, the fix.

Tests

• test_splice_reconnect_after_lock_no_channel_ready: splice, restart, reestablish; asserts the index persists and channel_ready is not retransmitted. It fails on commit 2 (old txid compare) and passes on commit 3, so it genuinely guards the fix.
• test_splice_funding_tx_index_increments: two sequential splices reach index 2.
• test_splice_inflight_funding_tx_index: a pending splice inflight carries index 1 and survives a restart.
• Existing test_reconnect_no_update confirms non-splice channels still retransmit channel_ready.

Risk and plan

The behavioural change is a single line in the reestablish check. The plan is to get the funding_tx_index semantics reviewed properly, and if we don't converge before the v26.09 freeze, revert that one line with a FIXME and keep the plumbing for the follow-up. So this carries low risk.

@ddustin, this implements the funding_tx_index approach you suggested; would appreciate your review of the assignment semantics and whether it matches what you and tbast had in mind.

[ddustin]

This is great! 👏 Mostly style notes with only one major concern:

test_splice_reconnect_after_lock_no_channel_ready may not actually be testing for the lack of channel_ready because of the unneeded billboard: Channel ready for use. check.

[nGoline]

@ddustin comments addressed. Just waiting for your review.

[ddustin]

Reviewing this it appears to be deviating from the spec. Doing so inside of reestablish is particularly dangerous and such behavior changes realistically trigger a new cycle of interop testing with LDK and ACINQ.

[ddustin]

These extra conditions appear to be leaving the spec, perhaps by quite a lot. The code should match what the spec says, referenced in the comment above.

*Anode:
*-if `next_commitment_number` iszero:
*-MUSTimmediatelyfailthechannelandbroadcastanyrelevantlatestcommitment*transaction.
*-if `next_commitment_number` is1inboththe `channel_reestablish` it*sentandreceived, andnoneofthose `channel_reestablish` messages*contain `my_current_funding_locked` or `next_funding` forasplicetransaction:
*-MUSTretransmit `channel_ready`.
*-otherwise:
*-MUSTNOTretransmit `channel_ready`, butMAYsend `channel_ready` with*adifferent `short_channel_id` `alias` field.

If the spec is wrong in some way, which is always possible, we need to get the spec updated and then conform to the new spec.

Adding conditions to these things in disagreement with the spec is precisely how we get interop issues that are extraordinarily difficult to track down.

[nGoline]

You were right, and my earlier reply missed your point. The shape you NACKed on #9079 was still in the condition, and I had only reimplemented one disjunct inside it. That's fixed now.

The check is back to the original three conditions plus the single conjunct the spec adds:

if (peer->channel_ready[LOCAL]
&&peer->next_index[LOCAL] ==1&&next_commitment_number==1&& !reestablish_mentions_splice(peer, send_tlvs, recv_tlvs)) {

locked_ready[LOCAL] and the two bare next_funding checks are gone. The helper applies funding_tx_index > 0 symmetrically to both the channel_reestablish we sent and the one we received, which is a direct reading of "contain my_current_funding_locked or next_funding for a splice transaction".

On why the previous version was wrong: check_mutual_splice_locked() clears locked_ready[LOCAL] and calls channel_update_funding() to rewrite channel->funding.txid to the splice txid, in the same call. Both proxies for "is this a splice transaction" went stale at that exact moment, so both halves of the check were wrong once a splice locked, not just the received side. And per the sending rules, a node that has never spliced still includes my_current_funding_locked with the funding txid, which is why the "for a splice transaction" qualifier can't be answered by a presence check or a txid comparison.

On interop: an unrecognized txid is now treated as a splice, so behaviour is unchanged from master for non-splice channels, pending inflights, and txids we can't account for. The splice-locked case is the only difference.

To check the test actually guards this rather than passing incidentally, I swapped the helper body back to the old !bitcoin_txid_eq(txid, &funding.txid) and rebuilt: test_splice_reconnect_after_lock_no_channel_ready fails, and passes again with funding_tx_index. So next_index[LOCAL] == 1 does hold after a splice and that branch is live.

Pushed as three fixup commits rather than a rewrite so you can see just the delta. I'll autosquash before merge.

[daywalker90]

Needs rebase

[nGoline]

Problem

On reconnect, channeld decides whether to retransmit channel_ready based on whether the peer's channel_reestablish references a splice (is_splice_active). It computed that by comparing the peer's my_current_funding_locked txid against peer->channel->funding.txid.

That comparison is wrong once a splice locks: channel_update_funding() overwrites funding.txid with the splice txid, so afterwards both sides agree on the same txid, the check falls through, and channel_ready is incorrectly retransmitted on reconnect, which can disrupt channels.

This is the problem @ddustin raised reviewing #9079 (comparing against the txid is incorrect, and we should track metadata on the inflight instead), suggesting a funding_tx_index, matching what eclair does.

Fix

Give every funding tx a stable funding_tx_index: 0 for the original funding (including RBF attempts), incrementing by 1 per splice. It is:

• carried on the channeld inflight and on the channel,
• threaded through the channeld_init and channeld_add_inflight wire,
• assigned on splice creation (parent + 1) and copied onto the channel funding when a splice locks,
• persisted in channel_funding_inflights and channels (two migrations, DEFAULT 0, so existing channels are unaffected).

The reestablish check now resolves the my_current_funding_locked txid to its funding_tx_index (current funding or a pending inflight) and treats > 0 as a splice.

Commits (bisectable)

1. splice: add tests for funding_tx_index tracking, regression tests (xfail).
2. splice: track funding_tx_index on inflights and channel funding, the plumbing.
3. channeld: detect splice on reestablish via funding_tx_index, not txid, the fix.

Tests

• test_splice_reconnect_after_lock_no_channel_ready: splice, restart, reestablish; asserts the index persists and channel_ready is not retransmitted. It fails on commit 2 (old txid compare) and passes on commit 3, so it genuinely guards the fix.
• test_splice_funding_tx_index_increments: two sequential splices reach index 2.
• test_splice_inflight_funding_tx_index: a pending splice inflight carries index 1 and survives a restart.
• Existing test_reconnect_no_update confirms non-splice channels still retransmit channel_ready.

Risk and plan

The behavioural change is a single line in the reestablish check. The plan is to get the funding_tx_index semantics reviewed properly, and if we don't converge before the v26.09 freeze, revert that one line with a FIXME and keep the plumbing for the follow-up. So this carries low risk.

@ddustin, this implements the funding_tx_index approach you suggested; would appreciate your review of the assignment semantics and whether it matches what you and tbast had in mind.

[ddustin]

This is great! 👏 Mostly style notes with only one major concern:

test_splice_reconnect_after_lock_no_channel_ready may not actually be testing for the lack of channel_ready because of the unneeded billboard: Channel ready for use. check.

[nGoline]

@ddustin comments addressed. Just waiting for your review.

[ddustin]

Reviewing this it appears to be deviating from the spec. Doing so inside of reestablish is particularly dangerous and such behavior changes realistically trigger a new cycle of interop testing with LDK and ACINQ.

[ddustin]

These extra conditions appear to be leaving the spec, perhaps by quite a lot. The code should match what the spec says, referenced in the comment above.

*Anode:
*-if `next_commitment_number` iszero:
*-MUSTimmediatelyfailthechannelandbroadcastanyrelevantlatestcommitment*transaction.
*-if `next_commitment_number` is1inboththe `channel_reestablish` it*sentandreceived, andnoneofthose `channel_reestablish` messages*contain `my_current_funding_locked` or `next_funding` forasplicetransaction:
*-MUSTretransmit `channel_ready`.
*-otherwise:
*-MUSTNOTretransmit `channel_ready`, butMAYsend `channel_ready` with*adifferent `short_channel_id` `alias` field.

If the spec is wrong in some way, which is always possible, we need to get the spec updated and then conform to the new spec.

Adding conditions to these things in disagreement with the spec is precisely how we get interop issues that are extraordinarily difficult to track down.

[nGoline]

You were right, and my earlier reply missed your point. The shape you NACKed on #9079 was still in the condition, and I had only reimplemented one disjunct inside it. That's fixed now.

The check is back to the original three conditions plus the single conjunct the spec adds:

if (peer->channel_ready[LOCAL]
&&peer->next_index[LOCAL] ==1&&next_commitment_number==1&& !reestablish_mentions_splice(peer, send_tlvs, recv_tlvs)) {

locked_ready[LOCAL] and the two bare next_funding checks are gone. The helper applies funding_tx_index > 0 symmetrically to both the channel_reestablish we sent and the one we received, which is a direct reading of "contain my_current_funding_locked or next_funding for a splice transaction".

On why the previous version was wrong: check_mutual_splice_locked() clears locked_ready[LOCAL] and calls channel_update_funding() to rewrite channel->funding.txid to the splice txid, in the same call. Both proxies for "is this a splice transaction" went stale at that exact moment, so both halves of the check were wrong once a splice locked, not just the received side. And per the sending rules, a node that has never spliced still includes my_current_funding_locked with the funding txid, which is why the "for a splice transaction" qualifier can't be answered by a presence check or a txid comparison.

On interop: an unrecognized txid is now treated as a splice, so behaviour is unchanged from master for non-splice channels, pending inflights, and txids we can't account for. The splice-locked case is the only difference.

To check the test actually guards this rather than passing incidentally, I swapped the helper body back to the old !bitcoin_txid_eq(txid, &funding.txid) and rebuilt: test_splice_reconnect_after_lock_no_channel_ready fails, and passes again with funding_tx_index. So next_index[LOCAL] == 1 does hold after a splice and that branch is live.

Pushed as three fixup commits rather than a rewrite so you can see just the delta. I'll autosquash before merge.

[daywalker90]

Needs rebase

[nGoline]

Problem

On reconnect, channeld decides whether to retransmit channel_ready based on whether the peer's channel_reestablish references a splice (is_splice_active). It computed that by comparing the peer's my_current_funding_locked txid against peer->channel->funding.txid.

That comparison is wrong once a splice locks: channel_update_funding() overwrites funding.txid with the splice txid, so afterwards both sides agree on the same txid, the check falls through, and channel_ready is incorrectly retransmitted on reconnect, which can disrupt channels.

This is the problem @ddustin raised reviewing #9079 (comparing against the txid is incorrect, and we should track metadata on the inflight instead), suggesting a funding_tx_index, matching what eclair does.

Fix

Give every funding tx a stable funding_tx_index: 0 for the original funding (including RBF attempts), incrementing by 1 per splice. It is:

• carried on the channeld inflight and on the channel,
• threaded through the channeld_init and channeld_add_inflight wire,
• assigned on splice creation (parent + 1) and copied onto the channel funding when a splice locks,
• persisted in channel_funding_inflights and channels (two migrations, DEFAULT 0, so existing channels are unaffected).

The reestablish check now resolves the my_current_funding_locked txid to its funding_tx_index (current funding or a pending inflight) and treats > 0 as a splice.

Commits (bisectable)

1. splice: add tests for funding_tx_index tracking, regression tests (xfail).
2. splice: track funding_tx_index on inflights and channel funding, the plumbing.
3. channeld: detect splice on reestablish via funding_tx_index, not txid, the fix.

Tests

• test_splice_reconnect_after_lock_no_channel_ready: splice, restart, reestablish; asserts the index persists and channel_ready is not retransmitted. It fails on commit 2 (old txid compare) and passes on commit 3, so it genuinely guards the fix.
• test_splice_funding_tx_index_increments: two sequential splices reach index 2.
• test_splice_inflight_funding_tx_index: a pending splice inflight carries index 1 and survives a restart.
• Existing test_reconnect_no_update confirms non-splice channels still retransmit channel_ready.

Risk and plan

The behavioural change is a single line in the reestablish check. The plan is to get the funding_tx_index semantics reviewed properly, and if we don't converge before the v26.09 freeze, revert that one line with a FIXME and keep the plumbing for the follow-up. So this carries low risk.

@ddustin, this implements the funding_tx_index approach you suggested; would appreciate your review of the assignment semantics and whether it matches what you and tbast had in mind.

[ddustin]

This is great! 👏 Mostly style notes with only one major concern:

test_splice_reconnect_after_lock_no_channel_ready may not actually be testing for the lack of channel_ready because of the unneeded billboard: Channel ready for use. check.

[nGoline]

@ddustin comments addressed. Just waiting for your review.

[ddustin]

Reviewing this it appears to be deviating from the spec. Doing so inside of reestablish is particularly dangerous and such behavior changes realistically trigger a new cycle of interop testing with LDK and ACINQ.

[ddustin]

These extra conditions appear to be leaving the spec, perhaps by quite a lot. The code should match what the spec says, referenced in the comment above.

*Anode:
*-if `next_commitment_number` iszero:
*-MUSTimmediatelyfailthechannelandbroadcastanyrelevantlatestcommitment*transaction.
*-if `next_commitment_number` is1inboththe `channel_reestablish` it*sentandreceived, andnoneofthose `channel_reestablish` messages*contain `my_current_funding_locked` or `next_funding` forasplicetransaction:
*-MUSTretransmit `channel_ready`.
*-otherwise:
*-MUSTNOTretransmit `channel_ready`, butMAYsend `channel_ready` with*adifferent `short_channel_id` `alias` field.

If the spec is wrong in some way, which is always possible, we need to get the spec updated and then conform to the new spec.

Adding conditions to these things in disagreement with the spec is precisely how we get interop issues that are extraordinarily difficult to track down.

[nGoline]

You were right, and my earlier reply missed your point. The shape you NACKed on #9079 was still in the condition, and I had only reimplemented one disjunct inside it. That's fixed now.

The check is back to the original three conditions plus the single conjunct the spec adds:

if (peer->channel_ready[LOCAL]
&&peer->next_index[LOCAL] ==1&&next_commitment_number==1&& !reestablish_mentions_splice(peer, send_tlvs, recv_tlvs)) {

locked_ready[LOCAL] and the two bare next_funding checks are gone. The helper applies funding_tx_index > 0 symmetrically to both the channel_reestablish we sent and the one we received, which is a direct reading of "contain my_current_funding_locked or next_funding for a splice transaction".

On why the previous version was wrong: check_mutual_splice_locked() clears locked_ready[LOCAL] and calls channel_update_funding() to rewrite channel->funding.txid to the splice txid, in the same call. Both proxies for "is this a splice transaction" went stale at that exact moment, so both halves of the check were wrong once a splice locked, not just the received side. And per the sending rules, a node that has never spliced still includes my_current_funding_locked with the funding txid, which is why the "for a splice transaction" qualifier can't be answered by a presence check or a txid comparison.

On interop: an unrecognized txid is now treated as a splice, so behaviour is unchanged from master for non-splice channels, pending inflights, and txids we can't account for. The splice-locked case is the only difference.

To check the test actually guards this rather than passing incidentally, I swapped the helper body back to the old !bitcoin_txid_eq(txid, &funding.txid) and rebuilt: test_splice_reconnect_after_lock_no_channel_ready fails, and passes again with funding_tx_index. So next_index[LOCAL] == 1 does hold after a splice and that branch is live.

Pushed as three fixup commits rather than a rewrite so you can see just the delta. I'll autosquash before merge.

[daywalker90]

Needs rebase