From 174c79acad36ce470956acc595c66d3b22b79a57 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 9 Aug 2018 09:55:29 +0930 Subject: [PATCH] openingd: tell master if funding failed, but don't exit. We don't want to exit just because channel parameter negotiation failed, but we do want to tell the master if it was a channel we were trying to fund. Note that lightningd still needs to fail the funding cmd if it gets a fromwire_opening_fundee (they raced us and won), or an outright failure. Signed-off-by: Rusty Russell --- lightningd/opening_control.c | 35 ++++++- openingd/opening.c | 174 ++++++++++++++++++++++------------- openingd/opening_wire.csv | 4 + 3 files changed, 150 insertions(+), 63 deletions(-) diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index e45b19e01..4f63538ce 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -504,12 +504,35 @@ static void opening_fundee_finished(struct subd *openingd, tal_free(uc); } +static void opening_funder_failed(struct subd *openingd, const u8 *msg, + struct uncommitted_channel *uc) +{ + char *desc; + + if (!fromwire_opening_funder_failed(msg, msg, &desc)) { + log_broken(uc->log, + "bad OPENING_FUNDER_FAILED %s", + tal_hex(tmpctx, msg)); + command_fail(uc->fc->cmd, LIGHTNINGD, + "bad OPENING_FUNDER_FAILED %s", + tal_hex(uc->fc->cmd, msg)); + tal_free(uc); + return; + } + + command_fail(uc->fc->cmd, LIGHTNINGD, "%s", desc); + + /* Clear uc->fc, so we can try again, and so we don't fail twice + * if they close. */ + uc->fc = tal_free(uc->fc); +} + static void opening_channel_errmsg(struct uncommitted_channel *uc, int peer_fd, int gossip_fd, const struct crypto_state *cs, const struct channel_id *channel_id UNUSED, const char *desc, - const u8 *err_for_them) + const u8 *err_for_them UNUSED) { if (peer_fd != -1) { close(peer_fd); @@ -639,6 +662,16 @@ static unsigned int openingd_msg(struct subd *openingd, opening_funder_finished(openingd, msg, fds, uc->fc); return 0; + case WIRE_OPENING_FUNDER_FAILED: + if (!uc->fc) { + log_broken(openingd->log, "Unexpected FUNDER_FAILED %s", + tal_hex(tmpctx, msg)); + tal_free(openingd); + return 0; + } + opening_funder_failed(openingd, msg, uc); + return 0; + case WIRE_OPENING_FUNDEE: if (tal_count(fds) != 2) return 2; diff --git a/openingd/opening.c b/openingd/opening.c index cc506f0ec..1ba5c1cd2 100644 --- a/openingd/opening.c +++ b/openingd/opening.c @@ -73,35 +73,38 @@ struct state { const struct chainparams *chainparams; }; -/* FIXME: Don't close connection in this case, but inform master and - * continue! */ -/* For negotiation failures: we tell them it's their fault. Same - * as peer_failed, with slightly different local and remote wording. */ -static void negotiation_failed(struct state *state, const char *fmt, ...) +/* For negotiation failures: we tell them it's their fault. */ +static void negotiation_failed(struct state *state, bool am_funder, + const char *fmt, ...) { va_list ap; const char *errmsg; u8 *msg; va_start(ap, fmt); - errmsg = tal_vfmt(state, fmt, ap); + errmsg = tal_vfmt(tmpctx, fmt, ap); va_end(ap); + status_debug("%s", errmsg); peer_billboard(true, errmsg); - msg = towire_status_peer_error(NULL, &state->channel_id, - errmsg, &state->cs, - towire_errorfmt(errmsg, - &state->channel_id, - "You gave bad parameters:%s", - errmsg)); - tal_free(errmsg); - status_send_fatal(take(msg), PEER_FD, GOSSIP_FD); - peer_failed(&state->cs, &state->channel_id, - "You gave bad parameters: %s", errmsg); + msg = towire_errorfmt(NULL, &state->channel_id, + "You gave bad parameters: %s", errmsg); + sync_crypto_write(&state->cs, PEER_FD, take(msg)); + + /* If necessary, tell master that funding failed. */ + if (am_funder) { + msg = towire_opening_funder_failed(NULL, errmsg); + wire_sync_write(REQ_FD, take(msg)); + } + + /* Reset state. */ + memset(&state->channel_id, 0, sizeof(state->channel_id)); + state->channel = tal_free(state->channel); } -static void check_config_bounds(struct state *state, - const struct channel_config *remoteconf) +static bool check_config_bounds(struct state *state, + const struct channel_config *remoteconf, + bool am_funder) { u64 capacity_msat; u64 reserve_msat; @@ -112,11 +115,13 @@ static void check_config_bounds(struct state *state, *... * - `to_self_delay` is unreasonably large. */ - if (remoteconf->to_self_delay > state->max_to_self_delay) - negotiation_failed(state, + if (remoteconf->to_self_delay > state->max_to_self_delay) { + negotiation_failed(state, am_funder, "to_self_delay %u larger than %u", remoteconf->to_self_delay, state->max_to_self_delay); + return false; + } /* BOLT #2: * @@ -131,12 +136,14 @@ static void check_config_bounds(struct state *state, /* We accumulate this into an effective bandwidth minimum. */ /* Overflow check before capacity calc. */ - if (remoteconf->channel_reserve_satoshis > state->funding_satoshis) - negotiation_failed(state, + if (remoteconf->channel_reserve_satoshis > state->funding_satoshis) { + negotiation_failed(state, am_funder, "channel_reserve_satoshis %"PRIu64 " too large for funding_satoshis %"PRIu64, remoteconf->channel_reserve_satoshis, state->funding_satoshis); + return false; + } /* Consider highest reserve. */ reserve_msat = remoteconf->channel_reserve_satoshis * 1000; @@ -148,17 +155,19 @@ static void check_config_bounds(struct state *state, if (remoteconf->max_htlc_value_in_flight_msat < capacity_msat) capacity_msat = remoteconf->max_htlc_value_in_flight_msat; - if (remoteconf->htlc_minimum_msat * (u64)1000 > capacity_msat) - negotiation_failed(state, + if (remoteconf->htlc_minimum_msat * (u64)1000 > capacity_msat) { + negotiation_failed(state, am_funder, "htlc_minimum_msat %"PRIu64 " too large for funding_satoshis %"PRIu64 " capacity_msat %"PRIu64, remoteconf->htlc_minimum_msat, state->funding_satoshis, capacity_msat); + return false; + } - if (capacity_msat < state->min_effective_htlc_capacity_msat) - negotiation_failed(state, + if (capacity_msat < state->min_effective_htlc_capacity_msat) { + negotiation_failed(state, am_funder, "channel capacity with funding %"PRIu64" msat," " reserves %"PRIu64"/%"PRIu64" msat," " max_htlc_value_in_flight_msat %"PRIu64 @@ -169,12 +178,16 @@ static void check_config_bounds(struct state *state, remoteconf->max_htlc_value_in_flight_msat, capacity_msat, state->min_effective_htlc_capacity_msat); + return false; + } /* We don't worry about how many HTLCs they accept, as long as > 0! */ - if (remoteconf->max_accepted_htlcs == 0) - negotiation_failed(state, + if (remoteconf->max_accepted_htlcs == 0) { + negotiation_failed(state, am_funder, "max_accepted_htlcs %u invalid", remoteconf->max_accepted_htlcs); + return false; + } /* BOLT #2: * @@ -182,10 +195,12 @@ static void check_config_bounds(struct state *state, *... * - `max_accepted_htlcs` is greater than 483. */ - if (remoteconf->max_accepted_htlcs > 483) - negotiation_failed(state, + if (remoteconf->max_accepted_htlcs > 483) { + negotiation_failed(state, am_funder, "max_accepted_htlcs %u too large", remoteconf->max_accepted_htlcs); + return false; + } /* BOLT #2: * @@ -194,13 +209,17 @@ static void check_config_bounds(struct state *state, * - `dust_limit_satoshis` is greater than `channel_reserve_satoshis`. */ if (remoteconf->dust_limit_satoshis - > remoteconf->channel_reserve_satoshis) - negotiation_failed(state, + > remoteconf->channel_reserve_satoshis) { + negotiation_failed(state, am_funder, "dust_limit_satoshis %"PRIu64 " too large for channel_reserve_satoshis %" PRIu64, remoteconf->dust_limit_satoshis, remoteconf->channel_reserve_satoshis); + return false; + } + + return true; } /* We always set channel_reserve_satoshis to 1%, rounded up. */ @@ -366,10 +385,12 @@ static u8 *funder_channel(struct state *state, * - if `minimum_depth` is unreasonably large: * - MAY reject the channel. */ - if (minimum_depth > 10) - negotiation_failed(state, + if (minimum_depth > 10) { + negotiation_failed(state, true, "minimum_depth %u larger than %u", minimum_depth, 10); + return NULL; + } /* BOLT #2: * @@ -384,21 +405,26 @@ static u8 *funder_channel(struct state *state, * - MUST reject the channel. */ if (state->remoteconf.channel_reserve_satoshis - < state->localconf.dust_limit_satoshis) - negotiation_failed(state, + < state->localconf.dust_limit_satoshis) { + negotiation_failed(state, true, "channel reserve %"PRIu64 " would be below our dust %"PRIu64, state->remoteconf.channel_reserve_satoshis, state->localconf.dust_limit_satoshis); + return NULL; + } if (state->localconf.channel_reserve_satoshis - < state->remoteconf.dust_limit_satoshis) - negotiation_failed(state, + < state->remoteconf.dust_limit_satoshis) { + negotiation_failed(state, true, "dust limit %"PRIu64 " would be above our reserve %"PRIu64, state->remoteconf.dust_limit_satoshis, state->localconf.channel_reserve_satoshis); + return NULL; + } - check_config_bounds(state, &state->remoteconf); + if (!check_config_bounds(state, &state->remoteconf, true)) + return NULL; /* Now, ask create funding transaction to pay those two addresses. */ if (change_satoshis) { @@ -447,9 +473,11 @@ static u8 *funder_channel(struct state *state, */ tx = initial_channel_tx(state, &wscript, state->channel, &state->next_per_commit[REMOTE], REMOTE); - if (!tx) - negotiation_failed(state, + if (!tx) { + negotiation_failed(state, true, "Could not meet their fees and reserve"); + return NULL; + } msg = towire_hsm_sign_remote_commitment_tx(NULL, tx, @@ -518,9 +546,11 @@ static u8 *funder_channel(struct state *state, */ tx = initial_channel_tx(state, &wscript, state->channel, &state->next_per_commit[LOCAL], LOCAL); - if (!tx) - negotiation_failed(state, + if (!tx) { + negotiation_failed(state, true, "Could not meet our fees and reserve"); + return NULL; + } if (!check_tx_sig(tx, 0, NULL, wscript, &their_funding_pubkey, &sig)) { peer_failed(&state->cs, @@ -533,6 +563,8 @@ static u8 *funder_channel(struct state *state, &their_funding_pubkey)); } + peer_billboard(false, "Funding channel: opening negotiation succeeded"); + /* BOLT #2: * * The recipient: @@ -617,33 +649,38 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) */ if (!bitcoin_blkid_eq(&chain_hash, &state->chainparams->genesis_blockhash)) { - negotiation_failed(state, + negotiation_failed(state, false, "Unknown chain-hash %s", type_to_string(tmpctx, struct bitcoin_blkid, &chain_hash)); + return NULL; } /* BOLT #2 FIXME: * * The receiving node ... MUST fail the channel if `funding-satoshis` * is greater than or equal to 2^24 */ - if (state->funding_satoshis > MAX_FUNDING_SATOSHI) - negotiation_failed(state, + if (state->funding_satoshis > MAX_FUNDING_SATOSHI) { + negotiation_failed(state, false, "funding_satoshis %"PRIu64" too large", state->funding_satoshis); + return NULL; + } /* BOLT #2: * * The receiving node MUST fail the channel if: * - `push_msat` is greater than `funding_satoshis` * 1000. */ - if (state->push_msat > state->funding_satoshis * 1000) + if (state->push_msat > state->funding_satoshis * 1000) { peer_failed(&state->cs, &state->channel_id, "Our push_msat %"PRIu64 " would be too large for funding_satoshis %"PRIu64, state->push_msat, state->funding_satoshis); + return NULL; + } /* BOLT #2: * @@ -652,15 +689,19 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) * - it considers `feerate_per_kw` too small for timely processing or * unreasonably large. */ - if (state->feerate_per_kw < state->min_feerate) - negotiation_failed(state, + if (state->feerate_per_kw < state->min_feerate) { + negotiation_failed(state, false, "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, + if (state->feerate_per_kw > state->max_feerate) { + negotiation_failed(state, false, "feerate_per_kw %u above maximum %u", state->feerate_per_kw, state->max_feerate); + return NULL; + } set_reserve(state); @@ -674,21 +715,26 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) * `channel_reserve_satoshis` from the `open_channel` message. */ if (state->localconf.channel_reserve_satoshis - < state->remoteconf.dust_limit_satoshis) - negotiation_failed(state, + < state->remoteconf.dust_limit_satoshis) { + negotiation_failed(state, false, "Our channel reserve %"PRIu64 " would be below their dust %"PRIu64, state->localconf.channel_reserve_satoshis, state->remoteconf.dust_limit_satoshis); + return NULL; + } if (state->localconf.dust_limit_satoshis - > state->remoteconf.channel_reserve_satoshis) - negotiation_failed(state, + > state->remoteconf.channel_reserve_satoshis) { + negotiation_failed(state, false, "Our dust limit %"PRIu64 " would be above their reserve %"PRIu64, state->localconf.dust_limit_satoshis, state->remoteconf.channel_reserve_satoshis); + return NULL; + } - check_config_bounds(state, &state->remoteconf); + if (!check_config_bounds(state, &state->remoteconf, false)) + return NULL; msg = towire_accept_channel(state, &state->channel_id, state->localconf.dust_limit_satoshis, @@ -758,9 +804,11 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) */ local_commit = initial_channel_tx(state, &wscript, state->channel, &state->next_per_commit[LOCAL], LOCAL); - if (!local_commit) - negotiation_failed(state, + if (!local_commit) { + negotiation_failed(state, false, "Could not meet our fees and reserve"); + return NULL; + } if (!check_tx_sig(local_commit, 0, NULL, wscript, &their_funding_pubkey, &theirsig)) { @@ -795,9 +843,11 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) remote_commit = initial_channel_tx(state, &wscript, state->channel, &state->next_per_commit[REMOTE], REMOTE); - if (!remote_commit) - negotiation_failed(state, + if (!remote_commit) { + negotiation_failed(state, false, "Could not meet their fees and reserve"); + return NULL; + } /* FIXME: Perhaps we should have channeld generate this, so we * can't possibly send before channel committed? */ @@ -954,8 +1004,6 @@ static u8 *handle_master_in(struct state *state) change_satoshis, change_keyindex, channel_flags, utxos, &bip32_base); - peer_billboard(false, - "Funding channel: opening negotiation succeeded"); return msg; case WIRE_OPENING_CAN_ACCEPT_CHANNEL: @@ -967,6 +1015,7 @@ static u8 *handle_master_in(struct state *state) case WIRE_OPENING_INIT: case WIRE_OPENING_FUNDER_REPLY: case WIRE_OPENING_FUNDEE: + case WIRE_OPENING_FUNDER_FAILED: break; } @@ -1015,6 +1064,7 @@ int main(int argc, char *argv[]) /* Initially we're not associated with a channel, but * handle_peer_gossip_or_error wants this. */ memset(&state->channel_id, 0, sizeof(state->channel_id)); + state->channel = NULL; wire_sync_write(HSM_FD, take(towire_hsm_get_per_commitment_point(NULL, 0))); diff --git a/openingd/opening_wire.csv b/openingd/opening_wire.csv index 13bbbba40..51ae36777 100644 --- a/openingd/opening_wire.csv +++ b/openingd/opening_wire.csv @@ -58,6 +58,10 @@ opening_funder_reply,,funding_txid,struct bitcoin_txid opening_funder_reply,,feerate_per_kw,u32 opening_funder_reply,,our_channel_reserve_satoshis,u64 +# Openingd->master: we failed to negotiation channel +opening_funder_failed,6004 +opening_funder_failed,,reason,wirestring + # Openingd->master: they offered channel. # This gives their txid and info, means we can send funding_signed: we're done. opening_fundee,6003