From 53d362e2f512fba8724f773c13cc1aa28bcbd684 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 1 Mar 2021 13:02:05 +1030 Subject: [PATCH 1/3] plugins/bcli: fake minimum fee if we're in regtest mode. Saves a great deal of confusion for regtest users. Signed-off-by: Rusty Russell Changelog-Changed: JSON-RPC: If bitcoind won't give a fee estimate in regtest, use minimum. --- plugins/bcli.c | 25 +++++++++++++++++++++++++ tests/test_connection.py | 2 +- tests/test_misc.py | 3 ++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index 193767e6a239..5aa6d07d1ba8 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -69,6 +69,14 @@ struct bitcoind { /* Percent of CONSERVATIVE/2 feerate we'll use for commitment txs. */ u64 commit_fee_percent; + + /* Whether we fake fees (regtest) */ + bool fake_fees; + +#if DEVELOPER + /* Override in case we're developer mode for testing*/ + bool no_fake_fees; +#endif }; static struct bitcoind *bitcoind; @@ -431,6 +439,11 @@ estimatefees_parse_feerate(struct bitcoin_cli *bcli, u64 *feerate) /* Paranoia: if it had a feerate, but was malformed: */ if (json_get_member(bcli->output, tokens, "feerate")) return command_err_bcli_badjson(bcli, "cannot scan"); + /* Regtest fee estimation is generally awful: Fake it at min. */ + if (bitcoind->fake_fees) { + *feerate = 1000; + return NULL; + } /* We return null if estimation failed, and bitcoin-cli will * exit with 0 but no feerate field on failure. */ return estimatefees_null_response(bcli); @@ -874,6 +887,11 @@ static const char *init(struct plugin *p, const char *buffer UNUSED, const jsmntok_t *config UNUSED) { wait_and_check_bitcoind(p); + + /* Usually we fake up fees in regtest */ + if (streq(chainparams->network_name, "regtest")) + bitcoind->fake_fees = IFDEV(!bitcoind->no_fake_fees, true); + plugin_log(p, LOG_INFORM, "bitcoin-cli initialized and connected to bitcoind."); @@ -938,6 +956,9 @@ static struct bitcoind *new_bitcoind(const tal_t *ctx) bitcoind->rpcport = NULL; bitcoind->max_fee_multiplier = 10; bitcoind->commit_fee_percent = 100; +#if DEVELOPER + bitcoind->no_fake_fees = false; +#endif return bitcoind; } @@ -994,6 +1015,10 @@ int main(int argc, char *argv[]) " closed more often due to fee fluctuations," " large values may result in large fees.", u32_option, &bitcoind->max_fee_multiplier), + plugin_option("dev-no-fake-fees", + "bool", + "Suppress fee faking for regtest", + bool_option, &bitcoind->no_fake_fees), #endif /* DEVELOPER */ NULL); } diff --git a/tests/test_connection.py b/tests/test_connection.py index fd94b1e01eaa..5f44d564484e 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -2282,7 +2282,7 @@ def mock_sendrawtransaction(r): @unittest.skipIf(not DEVELOPER, "needs dev_fail") def test_no_fee_estimate(node_factory, bitcoind, executor): - l1 = node_factory.get_node(start=False) + l1 = node_factory.get_node(start=False, options={'dev-no-fake-fees': True}) # Fail any fee estimation requests until we allow them further down l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', { diff --git a/tests/test_misc.py b/tests/test_misc.py index 1c35ff829758..ee2d6415f36f 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1438,7 +1438,8 @@ def test_ipv4_and_ipv6(node_factory): "FEERATE_FLOOR on testnets, and we test the new API." ) def test_feerates(node_factory): - l1 = node_factory.get_node(options={'log-level': 'io'}, start=False) + l1 = node_factory.get_node(options={'log-level': 'io', + 'dev-no-fake-fees': True}, start=False) l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', { 'error': {"errors": ["Insufficient data or no feerate found"], "blocks": 0} }) From 217700b0c24134e9446e906be39504feada6a7cf Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 1 Mar 2021 14:31:09 +1030 Subject: [PATCH 2/3] chaintopology: fix notification first time fee estimate works. We probably want to notify everyone immediately, rather than waiting for the first change. Signed-off-by: Rusty Russell --- lightningd/chaintopology.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 072a9e698f0b..4f5b192f5728 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -357,17 +357,6 @@ static void add_feerate_history(struct chain_topology *topo, topo->feehistory[feerate][0] = val; } -/* Did the the feerate change since we last estimated it ? */ -static bool feerate_changed(struct chain_topology *topo, u32 old_feerates[]) -{ - for (int f = 0; f < NUM_FEERATES; f++) { - if (try_get_feerate(topo, f) != old_feerates[f]) - return true; - } - - return false; -} - /* We sanitize feerates if necessary to put them in descending order. */ static void update_feerates(struct bitcoind *bitcoind, const u32 *satoshi_per_kw, @@ -379,6 +368,7 @@ static void update_feerates(struct bitcoind *bitcoind, * 2 minutes. The following will do that in a polling interval * independent manner. */ double alpha = 1 - pow(0.1,(double)topo->poll_seconds / 120); + bool feerate_changed = false; for (size_t i = 0; i < NUM_FEERATES; i++) { u32 feerate = satoshi_per_kw[i]; @@ -392,14 +382,18 @@ static void update_feerates(struct bitcoind *bitcoind, /* Initial smoothed feerate is the polled feerate */ if (!old_feerates[i]) { + feerate_changed = true; old_feerates[i] = feerate; init_feerate_history(topo, i, feerate); log_debug(topo->log, "Smoothed feerate estimate for %s initialized to polled estimate %u", feerate_name(i), feerate); - } else + } else { + if (feerate != old_feerates[i]) + feerate_changed = true; add_feerate_history(topo, i, feerate); + } /* Smooth the feerate to avoid spikes. */ u32 feerate_smooth = feerate * alpha + old_feerates[i] * (1 - alpha); @@ -435,7 +429,7 @@ static void update_feerates(struct bitcoind *bitcoind, maybe_completed_init(topo); } - if (feerate_changed(topo, old_feerates)) + if (feerate_changed) notify_feerate_change(bitcoind->ld); next_updatefee_timer(topo); From 6ebbacf935d7d07c69fb1ce6ec5456d28d6f720d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 1 Mar 2021 14:31:56 +1030 Subject: [PATCH 3/3] pytest: test fee estimates which start working after channel establishment. They need to specify fees to get a channel before this, but it's possible. The test revealed no surprises (other than the last change). Signed-off-by: Rusty Russell --- tests/test_connection.py | 51 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/test_connection.py b/tests/test_connection.py index 5f44d564484e..40d26d6ba279 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -2039,6 +2039,57 @@ def test_fee_limits(node_factory, bitcoind): l1.rpc.close(chan) +@unittest.skipIf(not DEVELOPER, "needs dev-no-fake-fees") +def test_update_fee_dynamic(node_factory, bitcoind): + # l1 has no fee estimates to start. + l1 = node_factory.get_node(options={'log-level': 'io', + 'dev-no-fake-fees': True}, start=False) + l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', { + 'error': {"errors": ["Insufficient data or no feerate found"], "blocks": 0} + }) + l1.start() + l2 = node_factory.get_node() + + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + # Fails due to lack of fee estimate. + with pytest.raises(RpcError, match='Cannot estimate fees'): + l1.fundchannel(l2, 10**6) + + # Explicit feerate works. + l1.fundchannel(l2, 10**6, feerate='10000perkw') + + l1.set_feerates((15000, 11000, 7500, 3750)) + + # It will send UPDATE_FEE when it tries to send HTLC. + inv = l2.rpc.invoice(5000, 'test_update_fee_dynamic', 'test_update_fee_dynamic')['bolt11'] + l1.rpc.pay(inv) + + l2.daemon.wait_for_log('peer_in.*UPDATE_FEE') + + # Now we take it away again! + l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', { + 'error': {"errors": ["Insufficient data or no feerate found"], "blocks": 0} + }) + # Make sure that registers! (DEVELOPER means polling every second) + time.sleep(2) + + inv = l2.rpc.invoice(5000, 'test_update_fee_dynamic2', 'test_update_fee_dynamic2')['bolt11'] + l1.rpc.pay(inv) + + # Won't update fee. + assert not l2.daemon.is_in_log('peer_in.*UPDATE_FEE', + start=l2.daemon.logsearch_start) + + # Bring it back. + l1.set_feerates((14000, 10000, 7000, 3000)) + + # It will send UPDATE_FEE when it tries to send HTLC. + inv = l2.rpc.invoice(5000, 'test_update_fee_dynamic3', 'test_update_fee_dynamic')['bolt11'] + l1.rpc.pay(inv) + + l2.daemon.wait_for_log('peer_in.*UPDATE_FEE') + + @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") def test_update_fee_reconnect(node_factory, bitcoind): # Disconnect after commitsig for fee update.