From 175db926c200cf204699125b69adbe02e07a234d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Aug 2018 08:57:25 +0930 Subject: [PATCH] chaintopology: expose when we don't actually know feerate. We use feerate in several places, and each one really should react differently when it's not available (such as when bitcoind is still catching up): 1. For general fee-enforcement, we use the broadest possible limits. 2. For closingd, we use it as our opening negotiation point: just use half the last tx feerate. 3. For onchaind, we can use the last tx feerate as a guide for our own txs; it might be too high, but at least we know it was sufficient to be mined. 4. For withdraw and fund_channel, we can simply refuse. Fixes: #1836 Signed-off-by: Rusty Russell --- lightningd/chaintopology.c | 39 +++------------------------ lightningd/chaintopology.h | 4 +-- lightningd/channel_control.c | 19 +++++++++----- lightningd/closing_control.c | 36 +++++++++++++++++++------ lightningd/onchain_control.c | 17 +++++++++++- lightningd/opening_control.c | 13 ++++++--- lightningd/peer_control.c | 34 +++++++++++++++++++----- lightningd/peer_control.h | 7 ++--- tests/test_connection.py | 51 ++++++++++++++++++++++++++++++++++++ wallet/test/run-wallet.c | 6 ++--- wallet/walletrpc.c | 7 ++++- 11 files changed, 162 insertions(+), 71 deletions(-) diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 3fa083ee3..3b5cd5617 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -302,14 +302,14 @@ static void update_feerates(struct bitcoind *bitcoind, u32 feerate = satoshi_per_kw[i]; /* Takes into account override_fee_rate */ - old_feerates[i] = get_feerate(topo, i); + old_feerates[i] = try_get_feerate(topo, i); /* If estimatefee failed, don't do anything. */ if (!feerate) continue; /* Initial smoothed feerate is the polled feerate */ - if (!topo->feerate[i]) { + if (!old_feerates[i]) { old_feerates[i] = feerate; log_debug(topo->log, "Smoothed feerate estimate for %s initialized to polled estimate %u", @@ -361,7 +361,7 @@ static void update_feerates(struct bitcoind *bitcoind, topo->feerate[j] = topo->feerate[i]; } } - if (get_feerate(topo, i) != old_feerates[i]) + if (try_get_feerate(topo, i) != old_feerates[i]) changed = true; } @@ -618,39 +618,8 @@ u32 get_block_height(const struct chain_topology *topo) return topo->tip->height; } -/* We may only have estimate for 2 blocks, for example. Extrapolate. */ -static u32 guess_feerate(const struct chain_topology *topo, enum feerate feerate) +u32 try_get_feerate(const struct chain_topology *topo, enum feerate feerate) { - size_t i = 0; - u32 rate = 0; - - /* We assume each one is half the previous. */ - for (i = 0; i < feerate; i++) { - if (topo->feerate[i]) { - log_info(topo->log, - "No fee estimate for %s: basing on %s rate", - feerate_name(feerate), - feerate_name(i)); - rate = topo->feerate[i]; - } - rate /= 2; - } - - if (rate == 0) { - rate = topo->default_fee_rate >> feerate; - log_info(topo->log, - "No fee estimate for %s: basing on default fee rate", - feerate_name(feerate)); - } - - return rate; -} - -u32 get_feerate(const struct chain_topology *topo, enum feerate feerate) -{ - if (topo->feerate[feerate] == 0) { - return guess_feerate(topo, feerate); - } return topo->feerate[feerate]; } diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index 32e91cace..eb9573b7f 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -131,8 +131,8 @@ size_t get_tx_depth(const struct chain_topology *topo, /* Get highest block number. */ u32 get_block_height(const struct chain_topology *topo); -/* Get fee rate in satoshi per kiloweight. */ -u32 get_feerate(const struct chain_topology *topo, enum feerate feerate); +/* Get fee rate in satoshi per kiloweight, or 0 if unavailable! */ +u32 try_get_feerate(const struct chain_topology *topo, enum feerate feerate); /* Broadcast a single tx, and rebroadcast as reqd (copies tx). * If failed is non-NULL, call that and don't rebroadcast. */ diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index c3302dd0f..409a1c8fc 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -20,11 +20,16 @@ static void update_feerates(struct lightningd *ld, struct channel *channel) { - u8 *msg = towire_channel_feerates(NULL, - get_feerate(ld->topology, - FEERATE_IMMEDIATE), - feerate_min(ld), - feerate_max(ld)); + u8 *msg; + u32 feerate = try_get_feerate(ld->topology, FEERATE_IMMEDIATE); + + /* Nothing to do if we don't know feerate. */ + if (!feerate) + return; + + msg = towire_channel_feerates(NULL, feerate, + feerate_min(ld, NULL), + feerate_max(ld, NULL)); subd_send_msg(channel->owner, take(msg)); } @@ -336,8 +341,8 @@ void peer_start_channeld(struct channel *channel, &channel->our_config, &channel->channel_info.their_config, channel->channel_info.feerate_per_kw, - feerate_min(ld), - feerate_max(ld), + feerate_min(ld, NULL), + feerate_max(ld, NULL), &channel->last_sig, cs, &channel->channel_info.remote_fundingkey, diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index b29054cef..f48f0e5f5 100644 --- a/lightningd/closing_control.c +++ b/lightningd/closing_control.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -24,7 +25,9 @@ static bool better_closing_fee(struct lightningd *ld, const struct bitcoin_tx *tx) { u64 weight, fee, last_fee, min_fee; + u32 min_feerate; size_t i; + bool feerate_unknown; /* Calculate actual fee (adds in eliminated outputs) */ fee = channel->funding_satoshi; @@ -41,17 +44,25 @@ static bool better_closing_fee(struct lightningd *ld, /* Weight once we add in sigs. */ weight = measure_tx_weight(tx) + 74 * 2; - min_fee = get_feerate(ld->topology, FEERATE_SLOW) * weight / 1000; + /* If we don't have a feerate estimate, this gives feerate_floor */ + min_feerate = feerate_min(ld, &feerate_unknown); + + min_fee = min_feerate * weight / 1000; if (fee < min_fee) { log_debug(channel->log, "... That's below our min %"PRIu64 " for weight %"PRIu64" at feerate %u", - min_fee, weight, - get_feerate(ld->topology, FEERATE_SLOW)); + min_fee, weight, min_feerate); return false; } - /* Prefer lower fee: in case of a tie, prefer new over old: this - * covers the preference for a mutual close over a unilateral one. */ + /* In case of a tie, prefer new over old: this covers the preference + * for a mutual close over a unilateral one. */ + + /* If we don't know the feerate, prefer higher fee. */ + if (feerate_unknown) + return fee >= last_fee; + + /* Otherwise prefer lower fee. */ return fee <= last_fee; } @@ -131,6 +142,7 @@ void peer_start_closingd(struct channel *channel, const u8 *channel_reestablish) { u8 *initmsg; + u32 feerate; u64 minfee, startfee, feelimit; u64 num_revocations; u64 funding_msatoshi, our_msatoshi, their_msatoshi; @@ -175,9 +187,17 @@ void peer_start_closingd(struct channel *channel, feelimit = commit_tx_base_fee(channel->channel_info.feerate_per_kw[LOCAL], 0); - minfee = commit_tx_base_fee(get_feerate(ld->topology, FEERATE_SLOW), 0); - startfee = commit_tx_base_fee(get_feerate(ld->topology, FEERATE_NORMAL), - 0); + /* Pick some value above slow feerate (or min possible if unknown) */ + minfee = commit_tx_base_fee(feerate_min(ld, NULL), 0); + + /* If we can't determine feerate, start at half unilateral feerate. */ + feerate = try_get_feerate(ld->topology, FEERATE_NORMAL); + if (!feerate) { + feerate = channel->channel_info.feerate_per_kw[LOCAL] / 2; + if (feerate < feerate_floor()) + feerate = feerate_floor(); + } + startfee = commit_tx_base_fee(feerate, 0); if (startfee > feelimit) startfee = feelimit; diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index 0c277e759..3f29f4b45 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -393,6 +394,7 @@ enum watch_result onchaind_funding_spent(struct channel *channel, struct lightningd *ld = channel->peer->ld; struct pubkey final_key; int hsmfd; + u32 feerate; channel_fail_permanent(channel, "Funding transaction spent"); @@ -437,6 +439,19 @@ enum watch_result onchaind_funding_spent(struct channel *channel, /* This could be a mutual close, but it doesn't matter. */ bitcoin_txid(channel->last_tx, &our_last_txid); + /* We try to use normal feerate for onchaind spends. */ + feerate = try_get_feerate(ld->topology, FEERATE_NORMAL); + if (!feerate) { + /* We have at least one data point: the last tx's feerate. */ + u64 fee = channel->funding_satoshi; + for (size_t i = 0; i < tal_count(channel->last_tx->output); i++) + fee -= channel->last_tx->output[i].amount; + + feerate = fee / measure_tx_weight(tx); + if (feerate < feerate_floor()) + feerate = feerate_floor(); + } + msg = towire_onchain_init(channel, &channel->their_shachain.chain, channel->funding_satoshi, @@ -450,7 +465,7 @@ enum watch_result onchaind_funding_spent(struct channel *channel, * we specify theirs. */ channel->channel_info.their_config.to_self_delay, channel->our_config.to_self_delay, - get_feerate(ld->topology, FEERATE_NORMAL), + feerate, channel->our_config.dust_limit_satoshis, &our_last_txid, p2wpkh_for_keyidx(tmpctx, ld, diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index f643deed7..a507dc4a9 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -737,7 +737,8 @@ void peer_start_openingd(struct peer *peer, cs, &uc->local_basepoints, &uc->local_funding_pubkey, uc->minimum_depth, - feerate_min(peer->ld), feerate_max(peer->ld), + feerate_min(peer->ld, NULL), + feerate_max(peer->ld, NULL), !peer_active_channel(peer), send_msg); subd_send_msg(uc->openingd, take(msg)); @@ -763,7 +764,7 @@ static void json_fund_channel(struct command *cmd, struct pubkey *id; struct peer *peer; struct channel *channel; - u32 feerate_per_kw = get_feerate(cmd->ld->topology, FEERATE_NORMAL); + u32 feerate_per_kw = try_get_feerate(cmd->ld->topology, FEERATE_NORMAL); u8 *msg; fc->cmd = cmd; @@ -778,6 +779,11 @@ static void json_fund_channel(struct command *cmd, if (!json_tok_wtx(&fc->wtx, buffer, sattok, MAX_FUNDING_SATOSHI)) return; + if (!feerate_per_kw) { + command_fail(cmd, LIGHTNINGD, "Cannot estimate fees"); + return; + } + peer = peer_by_id(cmd->ld, id); if (!peer) { command_fail(cmd, LIGHTNINGD, "Unknown peer"); @@ -817,8 +823,7 @@ static void json_fund_channel(struct command *cmd, msg = towire_opening_funder(NULL, fc->wtx.amount, fc->push_msat, - get_feerate(cmd->ld->topology, - FEERATE_NORMAL), + feerate_per_kw, fc->wtx.change, fc->wtx.change_key_index, fc->channel_flags, diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 4fdd8f3a5..327918c15 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -189,16 +189,24 @@ u8 *p2wpkh_for_keyidx(const tal_t *ctx, struct lightningd *ld, u64 keyidx) return scriptpubkey_p2wpkh(ctx, &shutdownkey); } -u32 feerate_min(struct lightningd *ld) +u32 feerate_min(struct lightningd *ld, bool *unknown) { u32 min; + if (unknown) + *unknown = false; + /* We can't allow less than feerate_floor, since that won't relay */ if (ld->config.ignore_fee_limits) min = 1; - else - /* Set this to half of slow rate.*/ - min = get_feerate(ld->topology, FEERATE_SLOW) / 2; + else { + u32 feerate = try_get_feerate(ld->topology, FEERATE_SLOW); + if (!feerate && unknown) + *unknown = true; + + /* Set this to half of slow rate (if unknown, will be floor) */ + min = feerate / 2; + } if (min < feerate_floor()) return feerate_floor(); @@ -211,13 +219,25 @@ u32 feerate_min(struct lightningd *ld) * spent in the future, it's a good idea for the fee payer to keep a good * margin (say 5x the expected fee requirement) */ -u32 feerate_max(struct lightningd *ld) +u32 feerate_max(struct lightningd *ld, bool *unknown) { + u32 feerate; + + if (unknown) + *unknown = false; + if (ld->config.ignore_fee_limits) return UINT_MAX; - return get_feerate(ld->topology, FEERATE_IMMEDIATE) * - ld->config.max_fee_multiplier; + /* If we don't know feerate, don't limit other side. */ + feerate = try_get_feerate(ld->topology, FEERATE_IMMEDIATE); + if (!feerate) { + if (unknown) + *unknown = true; + return UINT_MAX; + } + + return feerate * ld->config.max_fee_multiplier; } static void sign_last_tx(struct channel *channel) diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index 5fce0b0d5..b1c78527a 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -88,9 +88,10 @@ void activate_peers(struct lightningd *ld); void drop_to_chain(struct lightningd *ld, struct channel *channel, bool cooperative); -/* Get range of feerates to insist other side abide by for normal channels. */ -u32 feerate_min(struct lightningd *ld); -u32 feerate_max(struct lightningd *ld); +/* Get range of feerates to insist other side abide by for normal channels. + * If we have to guess, sets *unknown to true, otherwise false. */ +u32 feerate_min(struct lightningd *ld, bool *unknown); +u32 feerate_max(struct lightningd *ld, bool *unknown); void channel_watch_funding(struct lightningd *ld, struct channel *channel); #endif /* LIGHTNING_LIGHTNINGD_PEER_CONTROL_H */ diff --git a/tests/test_connection.py b/tests/test_connection.py index 47b40ed47..1107d7c0f 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -1105,6 +1105,57 @@ def test_fundee_forget_funding_tx_unconfirmed(node_factory, bitcoind): assert len(l2.rpc.listpeers(l1.info['id'])['peers']) == 0 +@unittest.skipIf(not DEVELOPER, "needs dev_fail") +def test_no_fee_estimate(node_factory, bitcoind, executor): + l1 = node_factory.get_node(start=False) + l1.bitcoind_cmd_override(cmd='estimatesmartfee', + failscript="""echo '{ "errors": [ "Insufficient data or no feerate found" ], "blocks": 0 }'; exit 0""") + l1.start() + + l2 = node_factory.get_node() + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + + # Can't fund a channel. + l1.fundwallet(10**7) + with pytest.raises(RpcError, match=r'Cannot estimate fees'): + l1.rpc.fundchannel(l2.info['id'], 10**6) + + # Can't withdraw either. + with pytest.raises(RpcError, match=r'Cannot estimate fees'): + l1.rpc.withdraw(l2.rpc.newaddr()['address'], 'all') + + # But can accept incoming connections. + l2.fund_channel(l1, 10**6) + + # Can do HTLCs. + l2.pay(l1, 10**5) + + # Can do mutual close. + l1.rpc.close(l2.info['id']) + bitcoind.generate_block(100) + + # Can do unilateral close. + l2.rpc.connect(l1.info['id'], 'localhost', l1.port) + l2.fund_channel(l1, 10**6) + l2.pay(l1, 10**9 // 2) + l1.rpc.dev_fail(l2.info['id']) + l1.daemon.wait_for_log('sendrawtx exit 0') + bitcoind.generate_block(5) + l1.daemon.wait_for_log('sendrawtx exit 0') + bitcoind.generate_block(100) + + # Restart estimatesmartfee, wait for it to pass 5000 + l1.set_feerates((15000, 7500, 3750), True) + l1.daemon.wait_for_log('Feerate estimate for Normal set to [567][0-9]{3}') + + # Can now fund a channel. + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1.rpc.fundchannel(l2.info['id'], 10**6) + + # Can withdraw. + l1.rpc.withdraw(l2.rpc.newaddr()['address'], 'all') + + @unittest.skipIf(not DEVELOPER, "needs --dev-disconnect") def test_funder_feerate_reconnect(node_factory, bitcoind): # l1 updates fees, then reconnect so l2 retransmits commitment_signed. diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 9c8cbf5c7..5752a33f2 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -75,9 +75,6 @@ bool fromwire_connect_peer_connected(const tal_t *ctx UNNEEDED, const void *p UN /* Generated stub for fromwire_hsm_sign_commitment_tx_reply */ bool fromwire_hsm_sign_commitment_tx_reply(const void *p UNNEEDED, secp256k1_ecdsa_signature *sig UNNEEDED) { fprintf(stderr, "fromwire_hsm_sign_commitment_tx_reply called!\n"); abort(); } -/* Generated stub for get_feerate */ -u32 get_feerate(const struct chain_topology *topo UNNEEDED, enum feerate feerate UNNEEDED) -{ fprintf(stderr, "get_feerate called!\n"); abort(); } /* Generated stub for invoices_autoclean_set */ void invoices_autoclean_set(struct invoices *invoices UNNEEDED, u64 cycle_seconds UNNEEDED, @@ -357,6 +354,9 @@ u8 *towire_errorfmt(const tal_t *ctx UNNEEDED, /* Generated stub for towire_hsm_sign_commitment_tx */ u8 *towire_hsm_sign_commitment_tx(const tal_t *ctx UNNEEDED, const struct pubkey *peer_id UNNEEDED, u64 channel_dbid UNNEEDED, const struct bitcoin_tx *tx UNNEEDED, const struct pubkey *remote_funding_key UNNEEDED, u64 funding_amount UNNEEDED) { fprintf(stderr, "towire_hsm_sign_commitment_tx called!\n"); abort(); } +/* Generated stub for try_get_feerate */ +u32 try_get_feerate(const struct chain_topology *topo UNNEEDED, enum feerate feerate UNNEEDED) +{ fprintf(stderr, "try_get_feerate called!\n"); abort(); } /* Generated stub for watch_txid */ struct txwatch *watch_txid(const tal_t *ctx UNNEEDED, struct chain_topology *topo UNNEEDED, diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index 92da7c10f..8bbf969ce 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -90,7 +90,7 @@ static void json_withdraw(struct command *cmd, const jsmntok_t *desttok, *sattok; struct withdrawal *withdraw = tal(cmd, struct withdrawal); - u32 feerate_per_kw = get_feerate(cmd->ld->topology, FEERATE_NORMAL); + u32 feerate_per_kw = try_get_feerate(cmd->ld->topology, FEERATE_NORMAL); struct bitcoin_tx *tx; enum address_parse_result addr_parse; @@ -107,6 +107,11 @@ static void json_withdraw(struct command *cmd, if (!json_tok_wtx(&withdraw->wtx, buffer, sattok, -1ULL)) return; + if (!feerate_per_kw) { + command_fail(cmd, LIGHTNINGD, "Cannot estimate fees"); + return; + } + /* Parse address. */ addr_parse = json_tok_address_scriptpubkey(cmd, get_chainparams(cmd->ld),