dualopend: resume saved opens across reconnects - #9461
Draft
niftynei wants to merge 20 commits into
Draft
Conversation
A callback can close a connection while readiness from the current poll call is still being dispatched. Deleting that connection compacts the fd table, and registering a replacement can restore its previous length with different occupants. A snapshot tied to mutable table positions can consequently skip an original event or deliver stale readiness to the replacement. Give each fd a stable registration object and retain that registration while a poll result refers to it. Deletion clears the registration's fd pointer, so pending readiness for a retired connection is ignored, while a replacement receives a distinct registration and cannot inherit the old event. Snapshot only entries with nonzero revents and preserve the IO_ALWAYS marker in the existing fairness order. Reuse a growable snapshot buffer between loop iterations and dispatch directly through the retained registration, avoiding both a fresh allocation on every poll and the repeated linear lookup required by the earlier generation-based approach. Register one process-exit cleanup for that reusable buffer. Retaining it for the daemon lifetime preserves allocation reuse, while releasing it after event dispatch can no longer be active avoids a reachable allocation in Valgrind and LeakSanitizer runs. Changelog-Fixed: io: replacing a file descriptor during poll dispatch no longer delivers stale readiness events to the replacement connection.
Closing a connection while dispatching poll results compacts the fd table. If the callback immediately registers a replacement, the table can return to its original length with a different connection occupying a slot represented in the current poll result. Dispatching readiness by mutable table position can then deliver an old event to the replacement or skip another ready connection. Use a fake poll implementation to make three original connections ready. The connection selected first closes itself and installs a replacement while the other readiness events remain pending. Verify that every original event is delivered exactly once, that the replacement does not inherit stale readiness, and that it is handled normally by the following poll call. Register an exit assertion before enabling protection so atexit LIFO ordering checks that the backend releases its reusable snapshot buffer at process exit. This keeps leak-sensitive unit runs from accepting a permanently reachable allocation. Add the regression to check-units so this CCAN behavior is covered by the project's normal unit-test suite. Changelog-None
Add test_inflight_disconnect_commitment_v2 which triggers a disconnect at +WIRE_COMMITMENT_SIGNED during a dual-funded open. Changelog-None
The error message in ElementsProject#8902 indicates that we're failing to correctly parse an error message from lightningd lightningd-2 2026-02-16T00:50:21.721Z **BROKEN** 038194b5f32bdf0aa59812c86c4ef7ad2f294104fa027d1ace9b469bb6f88cf37b-dualopend-chan#2: STATUS_FAIL_MASTER_IO: Error parsing 7011: 1b5b50656572206572726f7220776974682050534254207369676e6174757265732e00 The openchannel2_sign_hook_cb in lightningd can return error messages, not just the DUALOPEND_SEND_TX_SIGS message at this point. We handle this here. Changelog-Fixed: dualopend: dual-funding signing-hook errors are now reported correctly instead of causing a `dualopend` master-reply parse failure.
Issue ElementsProject#8902 demonstrates that there are races conditions ocurring when we use the peer disconnection notifications. In theory, we don't actually need to listen for peer disconnects, as we're already listening for open attempt failures with both the state_change and the channel_open_failed notifications. Changelog-Fixed: funder: peer disconnects no longer race channel-open failure handling when cleaning up pending dual-funded opens.
opener_commits allocates a temporary penalty base before negotiating the commitment and validating the remote signature. Both failure paths revert the channel state and return without transferring ownership, so they must release that allocation explicitly. Changelog-Fixed: dualopend: failed commitment negotiations no longer leak their temporary penalty-base allocation.
fetch_per_commitment_point previously ignored a failed write and passed a NULL read result to the wire decoder. An HSM disconnect could therefore appear as a malformed reply or crash instead of exposing the underlying transport failure. Check both sides of the synchronous HSM exchange and fail with STATUS_FAIL_HSM_IO at the point of failure. Clear errno before reading so a NULL result reports EOF rather than an unrelated stale error. Changelog-Fixed: dualopend: HSM transport failures are now reported directly instead of appearing as malformed replies or crashes.
hsmd permits only one client for a channel database ID. Starting channeld before the completed openingd owner has exited can therefore race creation of the replacement HSM client, particularly when several v1 opens finish at the same time. Release openingd after preserving its peer endpoint and before asking channeld to acquire the channel's HSM client. This makes owner exit an explicit barrier without blocking the handling of other completed opens. Changelog-Fixed: openingd: completed v1 channel opens no longer race `openingd` teardown against `channeld` HSM client creation.
…race Flake in test_rbf_reconnect_tx_construct indicates that there's a race in the teardown of the old dualopend/HSM code, which causes mismatches in the inflights and BROKEN HSM messages. **BROKEN** dualopend-chan#1: STATUS_FAIL_HSM_IO: Bad get_per_commitment_point_reply Changelog-Fixed: dualopend: restarting `dualopend` after an RBF abort now waits for the previous HSM client to exit, avoiding commitment-point reply mismatches.
After a dualopend route receives tx_abort, connectd hides it from normal dispatch and schedules it to close. If lightningd starts a replacement owner during that interval, it previously had no way to know when connectd had installed the replacement route. The new owner could begin its peer protocol while the route was absent or still belonged to the retiring owner. Add a tracked attachment request and defer its reply until io_new_conn has installed the replacement duplex plan. Validate the peer generation and connection state before accepting it, and report rejection to lightningd so it does not start an owner against a half-live route. A replacement may instead inherit the existing endpoint that was quarantined after tx_abort. Add a resume request which finds that exact route, cancels its close timer, and makes it eligible for dispatch again. Keep ordinary subdaemon attachment on the existing unacknowledged path, so the synchronization barrier applies only to replacement dualopend routes. Changelog-Fixed: connectd: replacement `dualopend` routes are now activated only after `connectd` confirms installation, preventing reconnects from using missing or retiring routes.
Persist enough dual-open state to recreate owners and continue outstanding commands when their peer reconnects. Serialize replay behind retirement of the previous owner and acknowledgement of the replacement connectd route so stale destructors and transport generations cannot displace the new owner. Retain serialized RPC steps once the channel is durable, replay them only after channel_reestablish completes, and bound open attempts while their peer remains disconnected. Preserve completed RBF state across tx_abort so a replacement attempt can resume without losing an inflight the peer may know. Changelog-Fixed: dualopend: saved dual-funded opens and RBF attempts now resume their pending RPCs after peer reconnection instead of becoming stranded.
A disconnect after the opener sends its final tx_complete can leave only that side with a saved channel. On reconnect it sends channel_reestablish, while the peer correctly reports the unknown channel. The opener must consume that error, discard the safe-to-forget half-open, fail the retained fundchannel RPC, and leave the peer connection usable instead of entering a reconnect loop. Cover both outcomes for the saved opener: without reconnection, a short grace period bounds the RPC and channel lifetime; with reconnection, the unknown- channel response clears the stale state. Also stop treating the final '=' directive as an expected disconnect: it merely consumes the directive and was only failing because an earlier half-open remained stranded by ElementsProject#8822. These cases make the original opener-disconnect test deterministic enough to remove its flaky marker. Changelog-None
Stopping the peer returns before the surviving node necessarily processes the transport loss. Mutating the stopped peer's remembered funding txid and restarting it immediately can therefore overlap teardown of the old dualopend route with installation of its replacement. Wait until the surviving node observes the disconnect before restarting the peer. This keeps the test focused on reconciling the deliberate next-funding mismatch rather than an unrelated old-versus-new transport race. Changelog-None
connect() returns once the peer_connected hook completes, before replacement channeld routes have necessarily exchanged channel_reestablish. A transport can fail during that interval and leave one connectd reporting a surviving half-connection, causing the following close RPC to wait on a route that was never fully installed. Require both channelds to receive channel_reestablish before proceeding. If a connection generation fails or stalls, force-disconnect any surviving half, wait until both nodes observe the disconnect, and retry with a fresh generation. Keep the retry bounded because this test is about closing a withheld zeroconf channel. Repeated route-generation failures are exercised separately by the dedicated reconnect tests. Changelog-None
Seeing the dev_disconnect log only proves that the injected fault fired; it does not mean both connectds have finished retiring that transport generation. Reconnecting immediately can let a delayed disconnect from the old generation interfere with the replacement dualopend route and turn this into a route lifecycle race. Wait until both peers report disconnected before reconnecting. This keeps the test focused on resuming the outstanding fundchannel RPC after a disconnect during commitment signing. Changelog-None
A transport failure after dual-open state has been persisted does not mean the operation failed. Returning RpcError at that point leaves callers unable to distinguish a safely retryable request from one whose channel or RBF attempt already has durable commitment state, and retrying can start conflicting work. Expect fundchannel and openchannel_signed to remain pending while replacement dualopends reestablish and retransmit the required messages. Where automatic reconnect is not guaranteed, reconnect explicitly and require the original RPC future—not a replacement call—to complete successfully. Changelog-None
Running several disconnect directives through one RBF attempt makes each fault depend on protocol state left by the earlier generations. A passing test then does not show clearly that every individual wire boundary can recover, and a failure does not identify which boundary is broken. Parameterize tx_init_rbf, tx_ack_rbf, tx_add_input, and tx_add_output so each direction is exercised in a fresh channel. Consume the initial channel-open TX_ADD messages before arming those RBF faults, and leave tx_complete and commitment_signed to the construction test. For each case, wait for the injected generation to retire, reconnect, and require the original openchannel_bump RPC to complete. Use a short retained- command timeout so a broken recovery path fails promptly instead of stalling the suite. Changelog-None
Single-disconnect cases prove that one replacement owner can resume an RBF, but they do not expose state which survives long enough to interfere with a later generation. A stale owner destructor, disconnect notification, route, or connection counter can incorrectly retire generation N+2 even though the first recovery succeeded. Inject the same tx_add_input disconnect through four complete transport and owner generations, waiting for both peers to observe each teardown before reconnecting. Require the original openchannel_bump future to complete after the final replacement, without issuing a caller retry. Keep this repeated-generation regression separate from the parameterized single-wire-fault cases so failures distinguish lifecycle reuse from a problem at one particular message boundary. Changelog-None
The construction reconnect test deliberately carries one RBF attempt through several partially persisted states: one-sided tx_complete, mutual remembered state, and commitment_signed retransmission. Starting the next fault before the previous transport and owner generation has retired turns those state checks into teardown races and can strand the replacement route before its scratch commitment is saved. Run blocking RPCs in futures, wait for both peers to observe each disconnect, and consume the expected RPC result before advancing. After the final commitment_signed fault, wait for connectivity to converge and for the selected inflight's scratch_txid to appear before retrying openchannel_update. Treat injected transport loss as clean owner retirement rather than accepting a BROKEN log. This keeps the test focused on which RBF state survives each construction boundary. Changelog-None
test_rbf_to_chain_before_commit deliberately disconnects a dual-open attempt before the commitment exchange completes, then exercises recovery when a funding transaction reaches the chain. Waiting for the production 600-second disconnect timeout would instead leave the test stalled until its own timeout. Set the retaining node's timeout to three seconds so the expected expiration is prompt and deterministic. This is local test configuration and does not change the production default. Changelog-None
niftynei
force-pushed
the
review/8822-reconnect
branch
from
August 31, 2026 17:12
9afafd5 to
17b7f8d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #9460.
The primary fix for issue #8822.
One issue with openchannel v2 opens is that disconnects can leave state across spenderp, funder, dualopend in various states of half finished/waiting. Here, we work to make disconnects/reconnects work more reliably across all the related subdaemons and plugins that are part of the openchannel process.
One big difference is we now timeout pending opens after 600s (10m). (Configurable via
--dual-open-disconnect-timeout=xxx) The timer starts once a pending dual-open RPC has:When it expires: