From 607d4bf9d27e3b83ec5f087d31bf3f333507ed0c Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Aug 2018 08:57:17 +0930 Subject: [PATCH] channel: update fees after lockin. We don't respond to fee changes until we're locked in: make sure we catch up at that point. Note that we use NORMAL fees during opening, but IMMEDIATE after, so this often sends a fee update. The tests which break, we set those feerates to be equal. This (sometimes) changes the behavior of test_permfail, as we now get an immediate commit, so that is fixed too so we always wait for that to complete. Signed-off-by: Rusty Russell --- lightningd/channel_control.c | 4 ++++ tests/test_closing.py | 29 ++++++++++++++++++++--------- tests/test_connection.py | 21 +++++++++++++++------ tests/test_misc.py | 16 ++++++++++++---- tests/test_pay.py | 10 +++++++--- 5 files changed, 58 insertions(+), 22 deletions(-) diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index d27bf7400..14a853d94 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -25,6 +25,10 @@ static void lockin_complete(struct channel *channel) /* We set this once they're locked in. */ assert(channel->remote_funding_locked); channel_set_state(channel, CHANNELD_AWAITING_LOCKIN, CHANNELD_NORMAL); + + /* Fees might have changed (and we use IMMEDIATE once we're funded), + * so update now. */ + try_update_feerates(channel->peer->ld, channel); } /* We were informed by channeld that it announced the channel and sent diff --git a/tests/test_closing.py b/tests/test_closing.py index 75bab6f3a..ca2526cd1 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -284,7 +284,8 @@ def test_closing_negotiation_reconnect(node_factory, bitcoind): def test_penalty_inhtlc(node_factory, bitcoind, executor): """Test penalty transaction with an incoming HTLC""" # We suppress each one after first commit; HTLC gets added not fulfilled. - l1 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED-nocommit'], may_fail=True) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED-nocommit'], may_fail=True, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED-nocommit']) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -355,7 +356,8 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor): def test_penalty_outhtlc(node_factory, bitcoind, executor): """Test penalty transaction with an outgoing HTLC""" # First we need to get funds to l2, so suppress after second. - l1 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED*3-nocommit'], may_fail=True) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED*3-nocommit'], may_fail=True, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED*3-nocommit']) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -522,7 +524,8 @@ def test_onchain_unwatch(node_factory, bitcoind): def test_onchaind_replay(node_factory, bitcoind): disconnects = ['+WIRE_REVOKE_AND_ACK', 'permfail'] options = {'watchtime-blocks': 201, 'cltv-delta': 101} - l1 = node_factory.get_node(options=options, disconnect=disconnects) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(options=options, disconnect=disconnects, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(options=options) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -574,7 +577,8 @@ def test_onchain_dust_out(node_factory, bitcoind, executor): """Onchain handling of outgoing dust htlcs (they should fail)""" # HTLC 1->2, 1 fails after it's irrevocably committed disconnects = ['@WIRE_REVOKE_AND_ACK', 'permfail'] - l1 = node_factory.get_node(disconnect=disconnects) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(disconnect=disconnects, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node() l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -638,7 +642,8 @@ def test_onchain_timeout(node_factory, bitcoind, executor): """Onchain handling of outgoing failed htlcs""" # HTLC 1->2, 1 fails just after it's irrevocably committed disconnects = ['+WIRE_REVOKE_AND_ACK', 'permfail'] - l1 = node_factory.get_node(disconnect=disconnects) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(disconnect=disconnects, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node() l2 = node_factory.get_node() @@ -869,7 +874,8 @@ def test_onchain_all_dust(node_factory, bitcoind, executor): # We need 2 to drop to chain, because then 1's HTLC timeout tx # is generated on-the-fly, and is thus feerate sensitive. disconnects = ['-WIRE_UPDATE_FAIL_HTLC', 'permfail'] - l1 = node_factory.get_node(options={'dev-no-reconnect': None}) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(options={'dev-no-reconnect': None}, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(disconnect=disconnects) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -985,7 +991,8 @@ def test_onchain_different_fees(node_factory, bitcoind, executor): def test_permfail_new_commit(node_factory, bitcoind, executor): # Test case where we have two possible commits: it will use new one. disconnects = ['-WIRE_REVOKE_AND_ACK', 'permfail'] - l1 = node_factory.get_node(options={'dev-no-reconnect': None}) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(options={'dev-no-reconnect': None}, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(disconnect=disconnects) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -1022,7 +1029,8 @@ def test_permfail_new_commit(node_factory, bitcoind, executor): def test_permfail_htlc_in(node_factory, bitcoind, executor): # Test case where we fail with unsettled incoming HTLC. disconnects = ['-WIRE_UPDATE_FULFILL_HTLC', 'permfail'] - l1 = node_factory.get_node(options={'dev-no-reconnect': None}) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(options={'dev-no-reconnect': None}, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(disconnect=disconnects) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -1066,7 +1074,8 @@ def test_permfail_htlc_out(node_factory, bitcoind, executor): # Test case where we fail with unsettled outgoing HTLC. disconnects = ['+WIRE_REVOKE_AND_ACK', 'permfail'] l1 = node_factory.get_node(options={'dev-no-reconnect': None}) - l2 = node_factory.get_node(disconnect=disconnects) + # Feerates identical so we don't get gratuitous commit to update them + l2 = node_factory.get_node(disconnect=disconnects, feerates=(7500, 7500, 7500)) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l2.daemon.wait_for_log('openingd-{} chan #1: Handed peer, entering loop'.format(l1.info['id'])) @@ -1127,9 +1136,11 @@ def test_permfail(node_factory, bitcoind): l1.pay(l2, 200000000) # Make sure l2 has received sig with 0 htlcs! + l2.daemon.wait_for_log('Received commit_sig with 1 htlc sigs') l2.daemon.wait_for_log('Received commit_sig with 0 htlc sigs') # Make sure l1 has final revocation. + l1.daemon.wait_for_log('Sending commit_sig with 1 htlc sigs') l1.daemon.wait_for_log('Sending commit_sig with 0 htlc sigs') l1.daemon.wait_for_log('peer_in WIRE_REVOKE_AND_ACK') diff --git a/tests/test_connection.py b/tests/test_connection.py index ef4fffdd8..6664025fb 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -375,8 +375,10 @@ def test_reconnect_sender_add1(node_factory): '+WIRE_UPDATE_ADD_HTLC-nocommit', '@WIRE_UPDATE_ADD_HTLC-nocommit'] + # Feerates identical so we don't get gratuitous commit to update them l1 = node_factory.get_node(disconnect=disconnects, - may_reconnect=True) + may_reconnect=True, + feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(may_reconnect=True) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -408,8 +410,10 @@ def test_reconnect_sender_add(node_factory): '-WIRE_REVOKE_AND_ACK', '@WIRE_REVOKE_AND_ACK', '+WIRE_REVOKE_AND_ACK'] + # Feerates identical so we don't get gratuitous commit to update them l1 = node_factory.get_node(disconnect=disconnects, - may_reconnect=True) + may_reconnect=True, + feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(may_reconnect=True) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -436,7 +440,8 @@ def test_reconnect_receiver_add(node_factory): '-WIRE_REVOKE_AND_ACK', '@WIRE_REVOKE_AND_ACK', '+WIRE_REVOKE_AND_ACK'] - l1 = node_factory.get_node(may_reconnect=True) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(may_reconnect=True, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(disconnect=disconnects, may_reconnect=True) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -719,7 +724,8 @@ def test_channel_persistence(node_factory, bitcoind, executor): # Start two nodes and open a channel (to remember). l2 will # mysteriously die while committing the first HTLC so we can # check that HTLCs reloaded from the DB work. - l1 = node_factory.get_node(may_reconnect=True) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(may_reconnect=True, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED-nocommit'], may_reconnect=True) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -915,7 +921,9 @@ def test_fee_limits(node_factory): def test_update_fee_reconnect(node_factory, bitcoind): # Disconnect after first commitsig. disconnects = ['+WIRE_COMMITMENT_SIGNED'] - l1 = node_factory.get_node(disconnect=disconnects, may_reconnect=True) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(disconnect=disconnects, may_reconnect=True, + feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(may_reconnect=True) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) chan = l1.fund_channel(l2, 10**6) @@ -1129,7 +1137,8 @@ def test_funder_feerate_reconnect(node_factory, bitcoind): l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l1.fund_channel(l2, 10**6) - l1.rpc.dev_setfees('14000') + # lockin will cause fee update, causing disconnect. + bitcoind.generate_block(5) l2.daemon.wait_for_log('dev_disconnect: \-WIRE_COMMITMENT_SIGNED') # Wait until they reconnect. diff --git a/tests/test_misc.py b/tests/test_misc.py index ed217eac3..14146c3c8 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -154,7 +154,9 @@ def test_ping(node_factory): def test_htlc_sig_persistence(node_factory, executor): """Interrupt a payment between two peers, then fail and recover funds using the HTLC sig. """ - l1 = node_factory.get_node(options={'dev-no-reconnect': None}) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(options={'dev-no-reconnect': None}, + feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(disconnect=['+WIRE_COMMITMENT_SIGNED']) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -202,8 +204,10 @@ def test_htlc_out_timeout(node_factory, bitcoind, executor): # HTLC 1->2, 1 fails after it's irrevocably committed, can't reconnect disconnects = ['@WIRE_REVOKE_AND_ACK'] + # Feerates identical so we don't get gratuitous commit to update them l1 = node_factory.get_node(disconnect=disconnects, - options={'dev-no-reconnect': None}) + options={'dev-no-reconnect': None}, + feerates=(7500, 7500, 7500)) l2 = node_factory.get_node() l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -259,8 +263,10 @@ def test_htlc_in_timeout(node_factory, bitcoind, executor): # HTLC 1->2, 1 fails after 2 has sent committed the fulfill disconnects = ['-WIRE_REVOKE_AND_ACK*2'] + # Feerates identical so we don't get gratuitous commit to update them l1 = node_factory.get_node(disconnect=disconnects, - options={'dev-no-reconnect': None}) + options={'dev-no-reconnect': None}, + feerates=(7500, 7500, 7500)) l2 = node_factory.get_node() l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -781,7 +787,9 @@ def test_reserve_enforcement(node_factory, executor): @unittest.skipIf(not DEVELOPER, "needs dev_disconnect") def test_htlc_send_timeout(node_factory, bitcoind): """Test that we don't commit an HTLC to an unreachable node.""" - l1 = node_factory.get_node(options={'log-level': 'io'}) + # Feerates identical so we don't get gratuitous commit to update them + l1 = node_factory.get_node(options={'log-level': 'io'}, + feerates=(7500, 7500, 7500)) # Blackhole it after it sends HTLC_ADD to l3. l2 = node_factory.get_node(disconnect=['0WIRE_UPDATE_ADD_HTLC'], options={'log-level': 'io'}) diff --git a/tests/test_pay.py b/tests/test_pay.py index 436a38690..e43e5e2c7 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -189,9 +189,11 @@ def test_pay_optional_args(node_factory): @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") def test_payment_success_persistence(node_factory, executor): # Start two nodes and open a channel.. die during payment. + # Feerates identical so we don't get gratuitous commit to update them l1 = node_factory.get_node(disconnect=['+WIRE_COMMITMENT_SIGNED'], options={'dev-no-reconnect': None}, - may_reconnect=True) + may_reconnect=True, + feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(may_reconnect=True) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -233,9 +235,11 @@ def test_payment_success_persistence(node_factory, executor): @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") def test_payment_failed_persistence(node_factory, executor): # Start two nodes and open a channel.. die during payment. + # Feerates identical so we don't get gratuitous commit to update them l1 = node_factory.get_node(disconnect=['+WIRE_COMMITMENT_SIGNED'], options={'dev-no-reconnect': None}, - may_reconnect=True) + may_reconnect=True, + feerates=(7500, 7500, 7500)) l2 = node_factory.get_node(may_reconnect=True) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -450,7 +454,7 @@ def test_sendpay_cant_afford(node_factory): pay(l1, l2, 10**9 + 1) # This is the fee, which needs to be taken into account for l1. - available = 10**9 - 6720 + available = 10**9 - 13440 # Reserve is 1%. reserve = 10**7