From 462e62e9c7a43a3464892f0d5a36d90c7b0daada Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 9 Apr 2020 14:30:06 +0930 Subject: [PATCH] channeld: defer first update_fee until we have an HTLC to send. Sending update_fee immediately after channel establishment seems to upset LND, so work around it by deferring it. The reason we increase the fee after establishment is because now we might need to close the channel in a hurry due to htlcs, but until there are htlcs that's unnecessary. Fixes: #3596 Changelog-Changed: Added workaround for lnd rejecting our commitment_signed when we send an update_fee after channel confirmed. Signed-off-by: Rusty Russell --- channeld/channeld.c | 7 ++++++- tests/test_connection.py | 21 +++++++++++++++++---- tests/test_pay.py | 3 +++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index 480dfdb398bd..c56ccec1382f 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -2749,7 +2749,12 @@ static void handle_feerates(struct peer *peer, const u8 *inmsg) */ if (peer->channel->funder == LOCAL) { peer->desired_feerate = feerate; - start_commit_timer(peer); + /* Don't do this for the first feerate, wait until something else + * happens. LND seems to get upset in some cases otherwise: + * see https://github.com/ElementsProject/lightning/issues/3596 */ + if (peer->next_index[LOCAL] != 1 + || peer->next_index[REMOTE] != 1) + start_commit_timer(peer); } else { /* BOLT #2: * diff --git a/tests/test_connection.py b/tests/test_connection.py index 7aac83df5ef9..cc7dfe529919 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -1367,7 +1367,6 @@ def test_update_fee(node_factory, bitcoind): # Make l1 send out feechange. l1.set_feerates((14000, 11000, 7500, 3750)) - l2.daemon.wait_for_log('peer updated fee to 14000') # Now make sure an HTLC works. # (First wait for route propagation.) @@ -1376,6 +1375,8 @@ def test_update_fee(node_factory, bitcoind): # Make payments. l1.pay(l2, 200000000) + # First payment causes fee update. + l2.daemon.wait_for_log('peer updated fee to 14000') l2.pay(l1, 100000000) # Now shutdown cleanly. @@ -1401,6 +1402,9 @@ def test_update_fee(node_factory, bitcoind): def test_fee_limits(node_factory, bitcoind): l1, l2 = node_factory.line_graph(2, opts={'dev-max-fee-multiplier': 5, 'may_reconnect': True}, fundchannel=True) + # Kick off fee adjustment using HTLC. + l1.pay(l2, 1000) + # L1 asks for stupid low fee (will actually hit the floor of 253) l1.stop() l1.set_feerates((15, 15, 15, 15), False) @@ -1430,6 +1434,9 @@ def test_fee_limits(node_factory, bitcoind): l1.rpc.connect(l3.info['id'], 'localhost', l3.port) chan = l1.fund_channel(l3, 10**6) + # Kick off fee adjustment using HTLC. + l1.pay(l3, 1000) + # Try stupid high fees l1.stop() l1.set_feerates((15000 * 10, 11000, 7500, 3750), False) @@ -1449,8 +1456,8 @@ def test_fee_limits(node_factory, bitcoind): @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") def test_update_fee_reconnect(node_factory, bitcoind): - # Disconnect after first commitsig. - disconnects = ['+WIRE_COMMITMENT_SIGNED'] + # Disconnect after commitsig for fee update. + disconnects = ['+WIRE_COMMITMENT_SIGNED*3'] # Feerates identical so we don't get gratuitous commit to update them l1 = node_factory.get_node(disconnect=disconnects, may_reconnect=True, feerates=(15000, 15000, 15000, 3750)) @@ -1460,6 +1467,9 @@ def test_update_fee_reconnect(node_factory, bitcoind): l1.rpc.connect(l2.info['id'], 'localhost', l2.port) chan = l1.fund_channel(l2, 10**6) + # Make an HTLC just to get us to do feechanges. + l1.pay(l2, 1000) + # Make l1 send out feechange; triggers disconnect/reconnect. # (Note: < 10% change, so no smoothing here!) l1.set_feerates((14000, 14000, 14000, 3750)) @@ -1755,13 +1765,16 @@ def test_no_fee_estimate(node_factory, bitcoind, executor): @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. - disconnects = ['-WIRE_COMMITMENT_SIGNED'] + disconnects = ['-WIRE_COMMITMENT_SIGNED*3'] l1 = node_factory.get_node(may_reconnect=True, feerates=(7500, 7500, 7500, 7500)) l2 = node_factory.get_node(disconnect=disconnects, may_reconnect=True) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l1.fund_channel(l2, 10**6) + # Need a payment otherwise it won't update fee. + l1.pay(l2, 10**9 // 2) + # create fee update, causing disconnect. l1.set_feerates((15000, 11000, 7500, 3750)) l2.daemon.wait_for_log(r'dev_disconnect: \-WIRE_COMMITMENT_SIGNED') diff --git a/tests/test_pay.py b/tests/test_pay.py index 5f6617ca16ed..835e0b1c3c88 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -228,6 +228,9 @@ def test_pay_disconnect(node_factory, bitcoind): l1, l2 = node_factory.line_graph(2, opts={'dev-max-fee-multiplier': 5, 'may_reconnect': True}) + # Dummy payment to kick off update_fee messages + l1.pay(l2, 1000) + inv = l2.rpc.invoice(123000, 'test_pay_disconnect', 'description') rhash = inv['payment_hash']