diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index ed23a5ae0..957f399b0 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -561,22 +561,25 @@ opening_funder_failed_cancel_commands(struct uncommitted_channel *uc, */ uc->fc = tal_free(uc->fc); } -static void opening_funder_failed(struct subd *openingd, const u8 *msg, - struct uncommitted_channel *uc) + +static void openingd_failed(struct subd *openingd, const u8 *msg, + struct uncommitted_channel *uc) { char *desc; - if (!fromwire_openingd_funder_failed(msg, msg, &desc)) { + if (!fromwire_openingd_failed(msg, msg, &desc)) { log_broken(uc->log, - "bad OPENING_FUNDER_FAILED %s", + "bad OPENINGD_FAILED %s", tal_hex(tmpctx, msg)); - was_pending(command_fail(uc->fc->cmd, LIGHTNINGD, - "bad OPENING_FUNDER_FAILED %s", - tal_hex(uc->fc->cmd, msg))); + if (uc->fc) + was_pending(command_fail(uc->fc->cmd, LIGHTNINGD, + "bad OPENINGD_FAILED %s", + tal_hex(uc->fc->cmd, msg))); tal_free(uc); return; } + /* Noop if we're not funder. */ opening_funder_failed_cancel_commands(uc, desc); } @@ -853,14 +856,8 @@ static unsigned int openingd_msg(struct subd *openingd, } opening_funder_start_replied(openingd, msg, fds, uc->fc); return 0; - case WIRE_OPENINGD_FUNDER_FAILED: - if (!uc->fc) { - log_unusual(openingd->log, "Unexpected FUNDER_FAILED %s", - tal_hex(tmpctx, msg)); - tal_free(openingd); - return 0; - } - opening_funder_failed(openingd, msg, uc); + case WIRE_OPENINGD_FAILED: + openingd_failed(openingd, msg, uc); return 0; case WIRE_OPENINGD_FUNDEE: diff --git a/openingd/openingd.c b/openingd/openingd.c index ce6db7eee..0370d186b 100644 --- a/openingd/openingd.c +++ b/openingd/openingd.c @@ -102,10 +102,9 @@ struct state { struct feature_set *our_features; }; -/*~ If we can't agree on parameters, we fail to open the channel. If we're - * the opener, we need to tell lightningd, otherwise it never really notices. */ -static void negotiation_aborted(struct state *state, bool am_opener, - const char *why) +/*~ If we can't agree on parameters, we fail to open the channel. + * Tell lightningd why. */ +static void NORETURN negotiation_aborted(struct state *state, const char *why) { status_debug("aborted opening negotiation: %s", why); /*~ The "billboard" (exposed as "status" in the JSON listpeers RPC @@ -116,29 +115,14 @@ static void negotiation_aborted(struct state *state, bool am_opener, * status. */ peer_billboard(true, why); - /* If necessary, tell master that funding failed. */ - if (am_opener) { - u8 *msg = towire_openingd_funder_failed(NULL, why); - wire_sync_write(REQ_FD, take(msg)); - } - - /* Default is no shutdown_scriptpubkey: free any leftover ones. */ - state->upfront_shutdown_script[LOCAL] - = tal_free(state->upfront_shutdown_script[LOCAL]); - state->upfront_shutdown_script[REMOTE] - = tal_free(state->upfront_shutdown_script[REMOTE]); - - /*~ Reset state. We keep gossipping with them, even though this open - * failed. */ - memset(&state->channel_id, 0, sizeof(state->channel_id)); - state->channel = tal_free(state->channel); - - state->channel_type = tal_free(state->channel_type); + /* Tell master that funding failed. */ + wire_sync_write(REQ_FD, take(towire_openingd_failed(NULL, why))); + exit(0); } /*~ For negotiation failures: we tell them the parameter we didn't like. */ -static void negotiation_failed(struct state *state, bool am_opener, - const char *fmt, ...) +static void NORETURN negotiation_failed(struct state *state, + const char *fmt, ...) { va_list ap; const char *errmsg; @@ -152,7 +136,7 @@ static void negotiation_failed(struct state *state, bool am_opener, "You gave bad parameters: %s", errmsg); peer_write(state->pps, take(msg)); - negotiation_aborted(state, am_opener, errmsg); + negotiation_aborted(state, errmsg); } /* We always set channel_reserve_satoshis to 1%, rounded down. */ @@ -177,7 +161,6 @@ static void set_reserve(struct state *state, const struct amount_sat dust_limit) /*~ Handle random messages we might get during opening negotiation, (eg. gossip) * returning the first non-handled one, or NULL if we aborted negotiation. */ static u8 *opening_negotiate_msg(const tal_t *ctx, struct state *state, - bool am_opener, const struct channel_id *alternate) { /* This is an event loop of its own. That's generally considered poor @@ -219,7 +202,7 @@ static u8 *opening_negotiate_msg(const tal_t *ctx, struct state *state, tal_free(msg); continue; } - negotiation_aborted(state, am_opener, + negotiation_aborted(state, tal_fmt(tmpctx, "They sent %s", err)); /* Return NULL so caller knows to stop negotiating. */ @@ -384,7 +367,7 @@ static u8 *funder_channel_start(struct state *state, u8 channel_flags) "Funding channel start: offered, now waiting for accept_channel"); /* ... since their reply should be immediate. */ - msg = opening_negotiate_msg(tmpctx, state, true, NULL); + msg = opening_negotiate_msg(tmpctx, state, NULL); if (!msg) return NULL; @@ -426,7 +409,7 @@ static u8 *funder_channel_start(struct state *state, u8 channel_flags) if (accept_tlvs->channel_type && !featurebits_eq(accept_tlvs->channel_type, state->channel_type->features)) { - negotiation_failed(state, true, + negotiation_failed(state, "Return unoffered channel_type: %s", fmt_featurebits(tmpctx, accept_tlvs->channel_type)); @@ -447,7 +430,7 @@ static u8 *funder_channel_start(struct state *state, u8 channel_flags) if (amount_sat_greater(state->remoteconf.dust_limit, state->localconf.channel_reserve)) { - negotiation_failed(state, true, + negotiation_failed(state, "dust limit %s" " would be above our reserve %s", type_to_string(tmpctx, struct amount_sat, @@ -468,7 +451,7 @@ static u8 *funder_channel_start(struct state *state, u8 channel_flags) state->their_features, OPT_ANCHOR_OUTPUTS), &err_reason)) { - negotiation_failed(state, true, "%s", err_reason); + negotiation_failed(state, "%s", err_reason); return NULL; } @@ -579,7 +562,7 @@ static bool funder_finalize_channel_setup(struct state *state, if (!*tx) { /* This should not happen: we should never create channels we * can't afford the fees for after reserve. */ - negotiation_failed(state, true, + negotiation_failed(state, "Could not meet their fees and reserve: %s", err_reason); goto fail; } @@ -644,7 +627,7 @@ static bool funder_finalize_channel_setup(struct state *state, * transaction. Note that errors may refer to the temporary channel * id (state->channel_id), but success should refer to the new * "cid" */ - msg = opening_negotiate_msg(tmpctx, state, true, &cid); + msg = opening_negotiate_msg(tmpctx, state, &cid); if (!msg) goto fail; @@ -696,7 +679,7 @@ static bool funder_finalize_channel_setup(struct state *state, &state->first_per_commitment_point[LOCAL], LOCAL, direct_outputs, &err_reason); if (!*tx) { - negotiation_failed(state, true, + negotiation_failed(state, "Could not meet our fees and reserve: %s", err_reason); goto fail; } @@ -842,7 +825,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) state->our_features, state->their_features); if (!state->channel_type) { - negotiation_failed(state, false, + negotiation_failed(state, "Did not support channel_type %s", fmt_featurebits(tmpctx, open_tlvs->channel_type)); @@ -861,7 +844,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) * that is unknown to the receiver. */ if (!bitcoin_blkid_eq(&chain_hash, &chainparams->genesis_blockhash)) { - negotiation_failed(state, false, + negotiation_failed(state, "Unknown chain-hash %s", type_to_string(tmpctx, struct bitcoin_blkid, @@ -879,7 +862,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) if (!feature_negotiated(state->our_features, state->their_features, OPT_LARGE_CHANNELS) && amount_sat_greater(state->funding_sats, chainparams->max_funding)) { - negotiation_failed(state, false, + negotiation_failed(state, "funding_satoshis %s too large", type_to_string(tmpctx, struct amount_sat, &state->funding_sats)); @@ -911,14 +894,14 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) * unreasonably large. */ if (state->feerate_per_kw < state->min_feerate) { - negotiation_failed(state, false, + negotiation_failed(state, "feerate_per_kw %u below minimum %u", state->feerate_per_kw, state->min_feerate); return NULL; } if (state->feerate_per_kw > state->max_feerate) { - negotiation_failed(state, false, + negotiation_failed(state, "feerate_per_kw %u above maximum %u", state->feerate_per_kw, state->max_feerate); return NULL; @@ -938,7 +921,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) */ if (amount_sat_greater(state->remoteconf.dust_limit, state->localconf.channel_reserve)) { - negotiation_failed(state, false, + negotiation_failed(state, "Our channel reserve %s" " would be below their dust %s", type_to_string(tmpctx, struct amount_sat, @@ -949,7 +932,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) } if (amount_sat_greater(state->localconf.dust_limit, state->remoteconf.channel_reserve)) { - negotiation_failed(state, false, + negotiation_failed(state, "Our dust limit %s" " would be above their reserve %s", type_to_string(tmpctx, struct amount_sat, @@ -971,7 +954,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) state->their_features, OPT_ANCHOR_OUTPUTS), &err_reason)) { - negotiation_failed(state, false, "%s", err_reason); + negotiation_failed(state, "%s", err_reason); return NULL; } @@ -1001,7 +984,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) /* If they give us a reason to reject, do so. */ if (err_reason) { - negotiation_failed(state, false, "%s", err_reason); + negotiation_failed(state, "%s", err_reason); tal_free(err_reason); return NULL; } @@ -1044,7 +1027,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) "Incoming channel: accepted, now waiting for them to create funding tx"); /* This is a loop which handles gossip until we get a non-gossip msg */ - msg = opening_negotiate_msg(tmpctx, state, false, NULL); + msg = opening_negotiate_msg(tmpctx, state, NULL); if (!msg) return NULL; @@ -1130,7 +1113,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) LOCAL, NULL, &err_reason); /* This shouldn't happen either, AFAICT. */ if (!local_commit) { - negotiation_failed(state, false, + negotiation_failed(state, "Could not meet our fees and reserve: %s", err_reason); return NULL; } @@ -1189,7 +1172,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) &state->first_per_commitment_point[REMOTE], REMOTE, direct_outputs, &err_reason); if (!remote_commit) { - negotiation_failed(state, false, + negotiation_failed(state, "Could not meet their fees and reserve: %s", err_reason); return NULL; } @@ -1353,7 +1336,7 @@ static u8 *handle_master_in(struct state *state) msg = towire_errorfmt(NULL, &state->channel_id, "Channel open canceled by us"); peer_write(state->pps, take(msg)); - negotiation_aborted(state, true, "Channel open canceled by RPC"); + negotiation_aborted(state, "Channel open canceled by RPC"); return NULL; case WIRE_OPENINGD_DEV_MEMLEAK: #if DEVELOPER @@ -1365,7 +1348,7 @@ static u8 *handle_master_in(struct state *state) case WIRE_OPENINGD_FUNDER_REPLY: case WIRE_OPENINGD_FUNDER_START_REPLY: case WIRE_OPENINGD_FUNDEE: - case WIRE_OPENINGD_FUNDER_FAILED: + case WIRE_OPENINGD_FAILED: case WIRE_OPENINGD_GOT_OFFER: case WIRE_OPENINGD_GOT_OFFER_REPLY: case WIRE_OPENINGD_GOT_REESTABLISH: diff --git a/openingd/openingd_wire.csv b/openingd/openingd_wire.csv index b8f0fc5b9..aa014e010 100644 --- a/openingd/openingd_wire.csv +++ b/openingd/openingd_wire.csv @@ -104,8 +104,8 @@ msgdata,openingd_funder_complete,channel_type,channel_type, msgtype,openingd_funder_cancel,6013 # Openingd->master: we failed to negotiation channel -msgtype,openingd_funder_failed,6004 -msgdata,openingd_funder_failed,reason,wirestring, +msgtype,openingd_failed,6004 +msgdata,openingd_failed,reason,wirestring, # Openingd->master: they offered channel. # This gives their txid and info, means we can send funding_signed: we're done. diff --git a/tests/test_connection.py b/tests/test_connection.py index 0af389818..9e1e2105c 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -998,13 +998,9 @@ def test_funding_fail(node_factory, bitcoind): with pytest.raises(RpcError, match=r'to_self_delay \d+ larger than \d+'): l1.rpc.fundchannel(l2.info['id'], int(funds / 10)) - # dual-funded channels disconnect on failure - if not l1.config('experimental-dual-fund'): - assert only_one(l1.rpc.listpeers()['peers'])['connected'] - assert only_one(l2.rpc.listpeers()['peers'])['connected'] - else: - assert len(l1.rpc.listpeers()['peers']) == 0 - assert len(l2.rpc.listpeers()['peers']) == 0 + # channels disconnect on failure + assert len(l1.rpc.listpeers()['peers']) == 0 + assert len(l2.rpc.listpeers()['peers']) == 0 # Restart l2 without ridiculous locktime. del l2.daemon.opts['watchtime-blocks'] @@ -1217,7 +1213,9 @@ def test_funding_external_wallet_corners(node_factory, bitcoind): l1.rpc.txdiscard(wrongaddr['txid']) l1.rpc.fundchannel_cancel(l2.info['id']) - # Should be able to 'restart' after canceling + + # Cancelling causes disconnection. + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) amount2 = 1000000 funding_addr = l1.rpc.fundchannel_start(l2.info['id'], amount2)['funding_address']