Fix for #8902 - #9394
Conversation
…ect BROKEN Add test_inflight_disconnect_commitment_v2 which triggers a disconnect at +WIRE_COMMITMENT_SIGNED during a dual-funded open.
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.
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-None
madelinevibes
added
the
Status::Ready for Review
Andezion
left a comment
Andezion
left a comment
There was a problem hiding this comment.
Is it guaranteed that lightningd always fires channel_open_failed when a dual-open aborts due to peer disconnect during commitment signing, for every abort path? If any abort path skips that notification, removing the disconnect handler would leak the PSBT reservation instead of just delaying its cleanup until reconnect/retry
|
|
||
| if (state->our_role == TX_ACCEPTER) | ||
| /* in TX_ACCEPTER case, `msg` could be a failure message */ | ||
| if (msg && (fromwire_peektype(msg) == WIRE_DUALOPEND_FAIL)) { |
There was a problem hiding this comment.
if fromwire_dualopend_fail() fails to parse (malformed payload), msg is left as the raw, undecoded WIRE_DUALOPEND_FAIL bytes and err_reason is left unset. The code then falls into if (!msg) (false) and proceeds to handle_send_tx_sigs(state, msg), which will itself fail to parse msg as WIRE_DUALOPEND_SEND_TX_SIGS and call master_badmsg() - a BROKEN exit again, just one level deeper and with a less accurate error message. The existing handle_failure_fatal() a few lines above handles this correctly (if (!fromwire_dualopend_fail(msg, msg, &err)) master_badmsg(...))
| u8 *msg, | ||
| char *err_reason) | ||
| { | ||
| if (!msg) { |
There was a problem hiding this comment.
Maybe
if (!msg) {
if (err_reason)
negotiation_failed(state, "%s", err_reason);
return false;
}
|
Working on a fix for the CI |
4 participants
There's two issues identified in #8902. This changeset:
funderThis should resolve both issues identified.