From bf2a42ada53c4a7aeec6acfc80cfea78db135d86 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 6e2a3b21e..f526bd6b0 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -2750,7 +2750,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 7aac83df5..cc7dfe529 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 ba788a141..b490546cb 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']