dualopend: harden signed inflight and repeated-open recovery - #9462
Draft
niftynei wants to merge 29 commits into
Draft
dualopend: harden signed inflight and repeated-open recovery#9462niftynei wants to merge 29 commits into
niftynei wants to merge 29 commits into
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
Once we have sent tx_signatures, the peer may publish that funding transaction even if a later tx_abort ends negotiation before its tx_signatures reach us. The retained inflight must therefore remain watched until its inputs are spent. If that exact transaction confirms, use its outpoint to select the matching saved commitment. Confirmation does not substitute for the missing funding signatures, so fail the channel permanently and broadcast the commitment instead of proceeding to channel_ready. Changelog-Fixed: dualopend: when a retained funding transaction confirms before peer `tx_signatures` arrive, CLN now selects its matching commitment and force-closes instead of proceeding to `channel_ready`.
test_rbf_non_last_mined fails if l2 has the chance to broadcast its third transaction. Instead of waiting an indeterminate amount of time, explicitly wait for both nodes to finish their mempool broadcast attempts before changing the sendrawtransaction mocks. Changelog-None
Once we have sent tx_signatures, the peer may possess everything needed to publish the funding transaction. A later tx_abort may end the negotiation, but it must not discard that durable inflight until its inputs are spent. Exercise this by making the accepter send tx_signatures first while dropping the peer's reply. Change the peer's remembered funding txid before reconnect so channel_reestablish produces tx_abort naturally, then verify that the original funding inflight and its last_tx remain in the wallet. Mine that exact retained funding transaction as a separate case and verify that confirmation selects its matching commitment. Since the peer never delivered tx_signatures, the node must force-close with that commitment and reach on-chain operation rather than send channel_ready. Changelog-None
test_rbf_non_last_mined is intended to verify that confirmation selects an older, non-tip RBF inflight. Block generation can finish before lightningd has processed the new depth and selected that inflight, so restarting the peer immediately also tests a reconnect-versus-topology-update race. Synchronize the retaining node to the mined block height before bringing its peer back. The reconnect then exercises recovery from the already-selected non-tip inflight, which is the behavior this test is meant to cover. Changelog-None
The post-expiry lease test previously advanced the full protocol-standard 4032-block lease in small batches. That made both nodes process thousands of irrelevant blocks and was particularly expensive in full test runs. Add a developer-only lease-duration override, while retaining 4032 blocks as the normal default. Use a 12-block lease in the test so it still negotiates and signs a real expiry, rejects close before the boundary, advances the regtest chain through expiry, closes normally afterward, and verifies the resulting accounting. Changelog-Fixed: tests: speed up liquidity ad lease duration tests
An initial-open failure previously completed its command and deleted the unsaved channel as soon as dualopend reported the error. The old owner and its connectd route could still be unwinding, so an immediate open to the same peer could collide with the stale route or exclusive HSM client. Track owner exit and route removal as separate teardown barriers. Ask connectd to release the exact peer-generation and channel route only after the old owner exits, and acknowledge the request from the route destructor. Completing the command only after both barriers makes local aborts, peer aborts, hook rejections, validation failures, and owner death safe for immediate retry. Handle release requests which race route installation, and force the matching peer transport down after the configured dual-open timeout so a stuck teardown cannot strand a caller forever. Changelog-Fixed: dualopend: prevent new opens from racing retired initial-open routes
dev-queryrates created and published an unsaved channel before allocating its local socketpair. A socketpair failure therefore returned an RPC error while leaving the half-created channel attached to the peer. Allocate the socketpair first, matching openchannel_init's ordering. Failures now occur before any channel state exists, so no exceptional cleanup path is required. Changelog-Fixed: dualopend: avoid retaining an unsaved channel when dev-queryrates socket setup fails
A single successful retry can miss generation bugs: a stale route from the first abort may only collide after another owner has been installed and retired. Repeat the abort and immediate retry on the same connected peer. This verifies that openchannel_abort returns only after the old owner and route are gone and that connectd cannot mistake an earlier generation for the current one. Changelog-Added: tests: exercise repeated initial-open abort generations
Explicit openchannel_abort coverage does not exercise terminal failures initiated by the accepting peer. Hook rejection sends tx_abort from that side and previously could complete while its initial-open route was still retiring. Stop the rejecting hook after observing the failure and immediately fund a channel over the same connection. A successful retry verifies that peer-originated teardown provides the same owner-and-route barrier as a local abort. Changelog-Added: tests: verify hook-rejected dual opens can retry immediately
niftynei
force-pushed
the
review/dualopen-signed-inflight
branch
from
August 31, 2026 17:12
cce6e9b to
f73319f
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 #9461.
Lots more tests and some fixes. Follow-up correctness and regression coverage discovered while testing #8822.