Fix for #8902 - #9394
Conversation
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 |
…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
…disconnect BROKEN
f6c6c0d to
55a0bd6
Compare
|
I removed the |
|
Yes, the dual-fund tests in general are flaky. I'm working on figuring out a bigger move to make them less flaky (and in addition solve #8822). In the meantime, this should work ok. |
Andezion
left a comment
Andezion
left a comment
There was a problem hiding this comment.
Did i understood this correctly, if fromwire_dualopend_fail() fails to parse (malformed payload), msg is left pointing at the raw undecoded WIRE_DUALOPEND_FAIL bytes, err_reason stays unset, and execution falls through to handle_send_tx_sigs(state, msg) for TX_ACCEPTER, which then fails to parse msg as WIRE_DUALOPEND_SEND_TX_SIGS and calls master_badmsg() - the exact BROKEN exit this pr is supposed to fix?
fetch_psbt_changes() a few hundred lines up already has the correct pattern for this - it falls back to master_badmsg() with the actual type when the fail message doesn't parse, instead of silently continuing
if (msg && fromwire_peektype(msg) == WIRE_DUALOPEND_FAIL) {
if (!fromwire_dualopend_fail(msg, msg, &err_reason))
master_badmsg(WIRE_DUALOPEND_FAIL, msg);
msg = tal_free(msg);
}This can't just call handle_failure_fatal()/check_accepter_error() - those are intentionally fatal (open_err_fatal), whereas an RBF failure here should stay recoverable via open_abort(). So the fallback needs to live inline, as above?? What do you think??
5 participants
There's two issues identified in #8902. This changeset:
funderThis should resolve both issues identified.