From 6db6ba6c03ad8da282252f396154c316d8e4ef98 Mon Sep 17 00:00:00 2001 From: niftynei Date: Fri, 26 Mar 2021 17:50:56 -0500 Subject: [PATCH] dual-funding: introduce racy state It's unlikely but possible that a race condition will result in us not being at the 'secured' state yet here. Crashlogs. All required msgs are received (in order) from peers, but the crash suggests they weren't relayed/processed by the spender plugin in the order received. WIRE_TX_SIGNATURES is passed the the plugin via a notification; WIRE_COMMITMENT_SIGNED is returned as the result of an RPC call. ``` 021-03-25T12:12:33.5213247Z lightningd-1: 2021-03-25T11:50:13.351Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-dualopend-chan#3: peer_in WIRE_COMMITMENT_SIGNED 2021-03-25T12:12:33.5221140Z lightningd-1: 2021-03-25T11:50:13.659Z DEBUG 022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59-dualopend-chan#1: peer_in WIRE_COMMITMENT_SIGNED 2021-03-25T12:12:33.5228462Z lightningd-1: 2021-03-25T11:50:14.169Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-dualopend-chan#3: peer_in WIRE_TX_SIGNATURES 2021-03-25T12:12:33.5230957Z lightningd-1: 2021-03-25T11:50:14.375Z DEBUG plugin-spenderp: mfc 275, dest 1: openchannel_update 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d returned. 2021-03-25T12:12:33.5233307Z lightningd-1: 2021-03-25T11:50:14.539Z DEBUG 022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59-dualopend-chan#1: peer_in WIRE_TX_SIGNATURES 2021-03-25T12:12:33.5235120Z lightningd-1: 2021-03-25T11:50:17.240Z INFO plugin-spenderp: Killing plugin: exited during normal operation 2021-03-25T12:12:33.5236707Z lightningd-1: 2021-03-25T11:50:17.260Z **BROKEN** plugin-spenderp: Plugin marked as important, shutting down lightningd! ``` Fixes #4455 --- plugins/spender/multifundchannel.c | 2 ++ plugins/spender/multifundchannel.h | 2 ++ plugins/spender/openchannel.c | 22 ++++++++++++++++++---- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/plugins/spender/multifundchannel.c b/plugins/spender/multifundchannel.c index 68f407dda..3ce71b8bd 100644 --- a/plugins/spender/multifundchannel.c +++ b/plugins/spender/multifundchannel.c @@ -122,6 +122,7 @@ has_commitments_secured(const struct multifundchannel_destination *dest) return false; case MULTIFUNDCHANNEL_COMPLETED: case MULTIFUNDCHANNEL_SECURED: + case MULTIFUNDCHANNEL_SIGNED_NOT_SECURED: case MULTIFUNDCHANNEL_SIGNED: case MULTIFUNDCHANNEL_DONE: return true; @@ -311,6 +312,7 @@ mfc_cleanup_(struct multifundchannel_command *mfc, continue; case MULTIFUNDCHANNEL_SECURED: case MULTIFUNDCHANNEL_SIGNED: + case MULTIFUNDCHANNEL_SIGNED_NOT_SECURED: /* We don't actually *send* the * transaction until here, * but peer isnt going to forget. This diff --git a/plugins/spender/multifundchannel.h b/plugins/spender/multifundchannel.h index cc67857bf..1519ccfa1 100644 --- a/plugins/spender/multifundchannel.h +++ b/plugins/spender/multifundchannel.h @@ -38,6 +38,8 @@ enum multifundchannel_state { MULTIFUNDCHANNEL_SECURED, /* We've recieved the peer sigs for this destination */ MULTIFUNDCHANNEL_SIGNED, + /* We've gotten their sigs, but still waiting for their commit sigs */ + MULTIFUNDCHANNEL_SIGNED_NOT_SECURED, /* The transaction might now be broadcasted. */ MULTIFUNDCHANNEL_DONE, diff --git a/plugins/spender/openchannel.c b/plugins/spender/openchannel.c index 710044d2c..61a4bb5a4 100644 --- a/plugins/spender/openchannel.c +++ b/plugins/spender/openchannel.c @@ -564,8 +564,6 @@ static void json_peer_sigs(struct command *cmd, dest->mfc->id, tal_hexstr(tmpctx, &cid, sizeof(cid))); - assert(dest->state == MULTIFUNDCHANNEL_SECURED); - /* Combine with the parent. Unknown map dupes are ignored, * so the updated serial_id should persist on the parent */ tal_wally_start(); @@ -579,7 +577,19 @@ static void json_peer_sigs(struct command *cmd, dest->mfc->psbt)); tal_wally_end(dest->mfc->psbt); - dest->state = MULTIFUNDCHANNEL_SIGNED; + + /* Bit of a race is possible here. If we're still waiting for + * their commitment sigs to come back, we'll be in + * "UPDATED" still. We check that SIGNED is hit before + * we mark ourselves as ready to send the sigs, so it's ok + * to relax this check */ + if (dest->state == MULTIFUNDCHANNEL_UPDATED) + dest->state = MULTIFUNDCHANNEL_SIGNED_NOT_SECURED; + else { + assert(dest->state == MULTIFUNDCHANNEL_SECURED); + dest->state = MULTIFUNDCHANNEL_SIGNED; + } + check_sigs_ready(dest->mfc); } @@ -727,8 +737,12 @@ openchannel_update_ok(struct command *cmd, /* It's possible they beat us to the SIGNED flag, * in which case we just let that be the more senior * state position */ - if (dest->state != MULTIFUNDCHANNEL_SIGNED) + if (dest->state == MULTIFUNDCHANNEL_SIGNED_NOT_SECURED) + dest->state = MULTIFUNDCHANNEL_SIGNED; + else { + assert(dest->state == MULTIFUNDCHANNEL_UPDATED); dest->state = MULTIFUNDCHANNEL_SECURED; + } } else dest->state = MULTIFUNDCHANNEL_UPDATED;