lightningd: don't disconnect when sending error for unknown channel_reestablish

[vincenzopalazzo]

Summary

• When receiving WIRE_CHANNEL_REESTABLISH for an unknown channel, send the error without disconnecting
• Prevents a race where the disconnect arrives before the error, leaving the peer with a stale saved channel in DUALOPEND_OPEN_COMMIT_READY that retries indefinitely

The bug: during dual-funding, if one side saves the channel (reaches DUALOPEND_OPEN_COMMIT_READY) but the other deletes its unsaved copy on disconnect, reconnects create an infinite loop. The saved side sends CHANNEL_REESTABLISH, the other side sends error + disconnect. The disconnect races with the error delivery -- if the peer's dualopend doesn't receive the error before the socket closes, dualopen_errmsg is called with disconnect=false, which calls channel_fail_transient instead of deleting the channel, and the cycle repeats.

By not disconnecting, the error reliably reaches the peer's dualopend, which processes it via peer_failed_received_errmsg(disconnect=true) and properly deletes the stale channel.

Test plan

• Verify make check-units passes (done locally)
• CI integration tests, particularly test_disconnect_opener

Fixes #8822

[madelinevibes]

@vincenzopalazzo can you rebase please

[nGoline]

I agree with the direction: not disconnecting after error is spec-legal, and I think a few people will challenge that, so it's worth putting the citation in the PR.

BOLT #1 ties error to failing the channel, never to the connection:

A sending node:
  - when sending `error`:
    - MUST fail the channel(s) referred to by the error message.

There's no "MUST close the connection" in that section, and BOLT #1 says it explicitly elsewhere when it means it (the init requirements). BOLT #2 makes the dichotomy explicit in ~25 places: "MUST either send a warning and close the connection, or send an error and fail the channel." "Fail the channel" is defined in BOLT #5 Failing a Channel purely as forget/mutual-close/unilateral-close, nothing about TCP. And the BOLT #1 rationale frames dropping the connection as the behaviour error exists to replace: "There are unrecoverable errors that require an abort of conversations; if the connection is simply dropped, then the peer may retry the connection."

Here the requirement is vacuous anyway, since we don't know the channel there's nothing for us to fail. And error rather than warning is right: only error obliges the peer to fail the channel, a warning would leave the stale channel exactly where it is.

Two substantive concerns, then some smaller ones inline.

1. I don't think the race is where the commit message says it is.

The commit says the disconnect races the error delivery on our side. But disconnect_peer() -> drain_peer() in connectd/multiplex.c sets draining_state = WRITING_TO_PEER and gives 5 seconds to flush peer_outq before closing, so the error should reach the wire ahead of the FIN. The race looks like it's on the receiving node instead: its connectd hands the error to dualopend while simultaneously tearing down subds on EOF, so lightningd ends up in dualopen_errmsg() with peer_fd == NULL and disconnect == false. Could you confirm the mechanism from the logs in #8822? It matters, because it changes where the fix belongs.

2. This fixes the peer, not us.

As written, the loop only goes away when the erroring side is running this patch. Our node still loops forever against anything that errors and hangs up: older CLN, LND, eclair, or a peer that simply drops the connection.

The unilateral fix is in dualopen_errmsg() (lightningd/dual_open_control.c). The if (!peer_fd) branch calls channel_fail_transient(channel, disconnect, ...), which leaves a DUALOPEND_OPEN_COMMIT_READY channel alive indefinitely. Every other path in that same function deletes a channel_state_open_uncommitted channel: the (warning || disconnect) check at the top, the !disconnect branch further down, and channel_fail_permanent() itself. That asymmetry looks like the actual bug in #8822, and fixing it there makes us self-healing regardless of what the peer does.

Ideally both land. If only one does, I'd argue for that one.

We're also missing a regression test to #8822. There's no test for the reconnect loop this is meant to fix, which makes the change hard to defend later. Repo convention would be a first commit adding the loop reproduction with @pytest.mark.xfail(strict=True), then the fix commit removing the marker. If the loop is hard to reproduce deterministically, dev-disconnect around the DUALOPEND_OPEN_COMMIT_READY transition plus asserting the channel is gone after one reconnect (rather than counting reconnects) should be enough.

[vincenzopalazzo]

Thanks, this is the useful kind of review. All five inline comments are addressed in 36cc672; I force-pushed rather than adding fixups because one of the fixes was the commit message itself.

1. You're right about the race, and my commit message was wrong. Rewritten in 36cc672. !peer_fd in dualopen_errmsg() can only come from the "Owning subdaemon %s died" path in lightningd/subd.c, which passes peer_fd=NULL, disconnect=false, warning=false — handle_peer_error() always attaches a peer_fd. So the node that loses is the receiver whose dualopend dies before it reads the error, exactly as you describe, and our own error does get flushed by drain_peer() first.

2. I don't think we can fix it there without regressing resume.

Because !peer_fd only ever means "our dualopend died", it's the one branch in that function where we don't know the peer failed the channel — every other branch is a warning, an error, or an abort we actually saw. And it's the case we deliberately resume: peer_connected_hook_final() restarts dualopend for DUALOPEND_OPEN_COMMIT_READY on reconnect (lightningd/peer_control.c), and wallet_update_channel_commit() moves it to DUALOPEND_OPEN_COMMITTED once their commitment_signed lands. Deleting on subd death means a v2 open interrupted at the commitment exchange can never resume, even when both sides saved it. I can't find a test covering that resume path (test_v2_open_sigs_reconnect_* all resume from DUALOPEND_OPEN_COMMITTED), so it would be a silent regression too.

If you want us self-healing against peers that do hang up (older CLN, LND, eclair), I think the safe shape is bounded retries rather than an unconditional delete: count reestablish attempts for a COMMIT_READY channel and forget it after N. There's no funding tx, so nothing is at stake and N=3 or so is cheap. Happy to add that here or as a follow-up — your call. I'd just rather not land the unconditional version.

Does that address the concern, or did I misread what you were proposing?

3. Regression test added in 592381a, but not as an xfail.

I tried the xfail-first structure and it doesn't hold up: I can't reproduce the loop deterministically. The setup reproduces fine (dev_disconnect -WIRE_TX_COMPLETE on l1: l1 saves DUALOPEND_OPEN_COMMIT_READY, l2 throws its unsaved channel away), but on my machine l1 wins the race against the hangup and forgets the channel even without the patch — the channel-state assertion passes on 199ecd2. @pytest.mark.xfail(strict=True) would just be flaky in the other direction, which is worse than no marker.

So the test pins the behaviour instead: l2 answers the unknown reestablish, l1 reads the error and forgets the channel, and neither side hangs up. That does fail without the patch (only_one(l1.rpc.listpeers()['peers']) raises, because l2 hung up on us).

Which is more evidence for your point 1, I think: the loop only appears when the receiver loses the race, so it's a flake by nature — which is how #8822 was found in the first place.

One thing to flag: tests/test_connection.py::test_disconnect_opener (the test #8822 came from) fails identically for me on 199ecd2 and on this branch — the node never logs "Server started with public key". Looks like local environment noise, but mentioning it in case CI disagrees.