From 9fe6ac8d44f70bc8034ca2050206d2104ac424a9 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 11 Feb 2020 17:55:54 +1030 Subject: [PATCH 1/3] pytest: simplified drain test (xfail) This is inspired by @m-schmook's https://github.com/ElementsProject/lightning/pull/3498 except this is simply a two-channel version which probes for the amount. Signed-off-by: Rusty Russell --- tests/test_pay.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_pay.py b/tests/test_pay.py index b4298a16c5d4..e12677447882 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2333,6 +2333,34 @@ def test_channel_drainage(node_factory, bitcoind): l2.rpc.waitsendpay(payment_hash, TIMEOUT) +@pytest.mark.xfail(strict=True) +def test_lockup_drain(node_factory, bitcoind): + """Try to get channel into a state where funder can't afford fees on additional HTLC, so fundee can't add HTLC""" + l1, l2 = node_factory.line_graph(2, opts={'may_reconnect': True}) + + # l1 sends all the money to l2 until even 1 msat can't get through. + total = 0 + msat = 16**9 + while msat != 0: + try: + l1.pay(l2, msat) + print("l1->l2: {}".format(msat)) + total += msat + except RpcError: + msat //= 2 + + # Even if feerate now increases 1.5x (22500), l2 should be able to send + # non-dust HTLC to l1. + l1.set_feerates([22500] * 3, False) + # Restart forces fast fee adjustment (otherwise it's smoothed and takes + # a very long time!). + l1.restart() + wait_for(lambda: only_one(l1.rpc.listpeers()['peers'])['connected']) + assert(l1.rpc.feerates('perkw')['perkw']['normal'] == 22500) + + l2.pay(l1, total // 2) + + def test_error_returns_blockheight(node_factory, bitcoind): """Test that incorrect_or_unknown_payment_details returns block height""" l1, l2 = node_factory.line_graph(2) From 46d19dc0ff446ad836af51e262c9f570d99764af Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 11 Feb 2020 17:56:14 +1030 Subject: [PATCH 2/3] channeld: trivial refactor of fee_for_htlcs(). Extract out num_untrimmed_htlcs() from inside fee_for_htlcs(), and remove unused view arg. Signed-off-by: Rusty Russell --- channeld/full_channel.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/channeld/full_channel.c b/channeld/full_channel.c index 9e7b39128149..f06d2ac3c7d0 100644 --- a/channeld/full_channel.c +++ b/channeld/full_channel.c @@ -363,8 +363,19 @@ static bool get_room_above_reserve(const struct channel *channel, return true; } +static size_t num_untrimmed_htlcs(enum side side, + struct amount_sat dust_limit, + u32 feerate, + const struct htlc **committed, + const struct htlc **adding, + const struct htlc **removing) +{ + return commit_tx_num_untrimmed(committed, feerate, dust_limit, side) + + commit_tx_num_untrimmed(adding, feerate, dust_limit, side) + - commit_tx_num_untrimmed(removing, feerate, dust_limit, side); +} + static struct amount_sat fee_for_htlcs(const struct channel *channel, - const struct channel_view *view, const struct htlc **committed, const struct htlc **adding, const struct htlc **removing, @@ -374,12 +385,8 @@ static struct amount_sat fee_for_htlcs(const struct channel *channel, struct amount_sat dust_limit = channel->config[side].dust_limit; size_t untrimmed; - untrimmed = commit_tx_num_untrimmed(committed, feerate, dust_limit, - side) - + commit_tx_num_untrimmed(adding, feerate, dust_limit, - side) - - commit_tx_num_untrimmed(removing, feerate, dust_limit, - side); + untrimmed = num_untrimmed_htlcs(side, dust_limit, feerate, + committed, adding, removing); return commit_tx_base_fee(feerate, untrimmed); } @@ -536,7 +543,7 @@ static enum channel_add_err add_htlc(struct channel *channel, */ if (enforce_aggregate_limits) { struct amount_msat remainder; - struct amount_sat fee = fee_for_htlcs(channel, view, + struct amount_sat fee = fee_for_htlcs(channel, committed, adding, removing, @@ -578,7 +585,7 @@ static enum channel_add_err add_htlc(struct channel *channel, /* Should be able to afford both their own commit tx * fee, and other's commit tx fee, which are subtly * different! */ - fee = fee_for_htlcs(channel, view, + fee = fee_for_htlcs(channel, committed, adding, removing, @@ -596,7 +603,7 @@ static enum channel_add_err add_htlc(struct channel *channel, &remainder)); return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; } - fee = fee_for_htlcs(channel, view, + fee = fee_for_htlcs(channel, committed, adding, removing, From dc3a718e09d854fe9c1e1288ec486350e79e43ef Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 11 Feb 2020 20:44:38 +1030 Subject: [PATCH 3/3] channeld: channel drain mitigation. Add new check if we're funder trying to add HTLC, keeping us with enough extra funds to pay for another HTLC the peer might add. We also need to adjust the spendable_msat calculation, and update various tests which try to unbalance channels. We eliminate the now-redundant test_channel_drainage entirely. Changelog-Fixed: Corner case where channel could become unusable (https://github.com/lightningnetwork/lightning-rfc/issues/728) Signed-off-by: Rusty Russell --- channeld/full_channel.c | 61 ++++++++++++++++++++++++++++++++++++ lightningd/peer_control.c | 5 ++- tests/test_connection.py | 6 ++-- tests/test_invoices.py | 4 +-- tests/test_pay.py | 65 +++------------------------------------ 5 files changed, 74 insertions(+), 67 deletions(-) diff --git a/channeld/full_channel.c b/channeld/full_channel.c index f06d2ac3c7d0..efaab0d441ae 100644 --- a/channeld/full_channel.c +++ b/channeld/full_channel.c @@ -391,6 +391,58 @@ static struct amount_sat fee_for_htlcs(const struct channel *channel, return commit_tx_base_fee(feerate, untrimmed); } +/* There is a corner case where the funder can spend so much that the + * non-funder can't add any non-dust HTLCs (since the funder would + * have to pay the additional fee, but it can't afford to). This + * leads to the channel starving at the feast! This was reported by + * ACINQ's @t-bast + * (https://github.com/lightningnetwork/lightning-rfc/issues/728) and + * demonstrated with c-lightning by @m-schmook + * (https://github.com/ElementsProject/lightning/pull/3498). + * + * To mostly avoid this situation, at least from our side, we apply an + * additional constraint when we're funder trying to add an HTLC: make + * sure we can afford one more HTLC, even if fees increase 50%. + * + * We could do this for the peer, as well, by rejecting their HTLC + * immediately in this case. But rejecting a remote HTLC here causes + * us to get upset with them and close the channel: we're not well + * architected to reject HTLCs in channeld (it's usually lightningd's + * job, but it doesn't have all the channel balance change calculation + * logic. So we look after ourselves for now, and hope other nodes start + * self-regulating too. */ +static bool local_funder_has_fee_headroom(const struct channel *channel, + struct amount_msat remainder, + const struct htlc **committed, + const struct htlc **adding, + const struct htlc **removing) +{ + u32 feerate = channel_feerate(channel, LOCAL); + size_t untrimmed; + struct amount_sat fee; + + assert(channel->funder == LOCAL); + + /* How many untrimmed at current feerate? Increasing feerate can + * only *reduce* this number, so use current feerate here! */ + untrimmed = num_untrimmed_htlcs(LOCAL, channel->config[LOCAL].dust_limit, + feerate, + committed, adding, removing); + + /* Now, how much would it cost us if feerate increases 50% and we added + * another HTLC? */ + fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1); + if (amount_msat_greater_eq_sat(remainder, fee)) + return true; + + status_debug("Adding HTLC would leave us only %s:" + " we need %s for another HTLC if fees increase 50%% to %uperkw", + type_to_string(tmpctx, struct amount_msat, &remainder), + type_to_string(tmpctx, struct amount_sat, &fee), + feerate + feerate/2); + return false; +} + static enum channel_add_err add_htlc(struct channel *channel, enum htlc_state state, u64 id, @@ -572,6 +624,15 @@ static enum channel_add_err add_htlc(struct channel *channel, &remainder)); return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; } + + if (sender == LOCAL + && !local_funder_has_fee_headroom(channel, + remainder, + committed, + adding, + removing)) { + return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED; + } } /* Try not to add a payment which will take funder into fees diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 3ccc8dec505c..2e2fdd9def34 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -553,7 +553,10 @@ static struct amount_sat commit_txfee(const struct channel *channel, num_untrimmed_htlcs++; } - return commit_tx_base_fee(local_feerate, num_untrimmed_htlcs); + /* Funder is conservative: makes sure it allows an extra HTLC + * even if feerate increases 50% */ + return commit_tx_base_fee(local_feerate + local_feerate / 2, + num_untrimmed_htlcs + 1); } static void subtract_offered_htlcs(const struct channel *channel, diff --git a/tests/test_connection.py b/tests/test_connection.py index 23781dcbd7e4..77750e84be33 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -2207,7 +2207,7 @@ def test_change_chaining(node_factory, bitcoind): def test_feerate_spam(node_factory, chainparams): l1, l2 = node_factory.line_graph(2) - slack = 25000000 if not chainparams['elements'] else 35000000 + slack = 35000000 # Pay almost everything to l2. l1.pay(l2, 10**9 - slack) @@ -2218,8 +2218,8 @@ def test_feerate_spam(node_factory, chainparams): # Now change feerates to something l1 can't afford. l1.set_feerates((100000, 100000, 100000)) - # It will raise as far as it can (20000) - l1.daemon.wait_for_log('Setting REMOTE feerate to 20000') + # It will raise as far as it can (34000) + l1.daemon.wait_for_log('Setting REMOTE feerate to 34000') l1.daemon.wait_for_log('peer_out WIRE_UPDATE_FEE') # But it won't do it again once it's at max. diff --git a/tests/test_invoices.py b/tests/test_invoices.py index 7eb49f92b018..36ecd8d2ad03 100644 --- a/tests/test_invoices.py +++ b/tests/test_invoices.py @@ -128,7 +128,7 @@ def test_invoice_preimage(node_factory): def test_invoice_routeboost(node_factory, bitcoind): """Test routeboost 'r' hint in bolt11 invoice. """ - l0, l1, l2 = node_factory.line_graph(3, fundamount=2 * (10**4), wait_for_announce=True) + l0, l1, l2 = node_factory.line_graph(3, fundamount=2 * (10**5), wait_for_announce=True) # Check routeboost. # Make invoice and pay it @@ -150,7 +150,7 @@ def test_invoice_routeboost(node_factory, bitcoind): wait_channel_quiescent(l1, l2) # Due to reserve & fees, l1 doesn't have capacity to pay this. - inv = l2.rpc.invoice(msatoshi=2 * (10**7) - 123456, label="inv2", description="?") + inv = l2.rpc.invoice(msatoshi=2 * (10**8) - 123456, label="inv2", description="?") # Check warning assert 'warning_capacity' in inv assert 'warning_offline' not in inv diff --git a/tests/test_pay.py b/tests/test_pay.py index e12677447882..f517165cce65 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -578,7 +578,9 @@ def check_balances(): @unittest.skipIf(TEST_NETWORK != 'regtest', "The reserve computation is bitcoin specific") def test_sendpay_cant_afford(node_factory): - l1, l2 = node_factory.line_graph(2, fundamount=10**6) + # Set feerates the same so we don't have to wait for update. + l1, l2 = node_factory.line_graph(2, fundamount=10**6, + opts={'feerates': (15000, 15000, 15000)}) # Can't pay more than channel capacity. def pay(lsrc, ldst, amt, label=None): @@ -593,7 +595,7 @@ def pay(lsrc, ldst, amt, label=None): pay(l1, l2, 10**9 + 1) # This is the fee, which needs to be taken into account for l1. - available = 10**9 - 13440000 + available = 10**9 - 24030000 # Reserve is 1%. reserve = 10**7 @@ -2275,65 +2277,6 @@ def test_channel_spendable_capped(node_factory, bitcoind): assert l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'] == Millisatoshi(0xFFFFFFFF) -@unittest.skipIf(TEST_NETWORK != 'regtest', 'The numbers below are bitcoin specific') -def test_channel_drainage(node_factory, bitcoind): - """Test channel drainage. - - Test to drains a channels as much as possible, - especially in regards to commitment fee: - - [l1] <=> [l2] - """ - sats = 10**6 - l1, l2 = node_factory.line_graph(2, fundamount=sats, wait_for_announce=True) - - # wait for everyone to see every channel as active - for n in [l1, l2]: - wait_for(lambda: [c['active'] for c in n.rpc.listchannels()['channels']] == [True] * 2 * 1) - - # This first HTLC drains the channel. - amount = Millisatoshi("976559200msat") - payment_hash = l2.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] - route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] - l1.rpc.sendpay(route, payment_hash) - l1.rpc.waitsendpay(payment_hash, 10) - - # wait until totally settled - l1.wait_for_htlcs() - l2.wait_for_htlcs() - - # But we can get more! By using a trimmed htlc output; this doesn't cause - # an increase in tx fee, so it's allowed. - amount = Millisatoshi("2580800msat") - payment_hash = l2.rpc.invoice('any', 'inv2', 'for testing')['payment_hash'] - route = l1.rpc.getroute(l2.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] - l1.rpc.sendpay(route, payment_hash) - l1.rpc.waitsendpay(payment_hash, TIMEOUT) - - # wait until totally settled - l1.wait_for_htlcs() - l2.wait_for_htlcs() - - # Now, l1 is paying fees, but it can't afford a larger tx, so any - # attempt to add an HTLC which is not trimmed will fail. - payment_hash = l1.rpc.invoice('any', 'inv', 'for testing')['payment_hash'] - - # feerate_per_kw = 15000, so htlc_timeout_fee = 663 * 15000 / 1000 = 9945. - # dust_limit is 546. So it's trimmed if < 9945 + 546. - amount = Millisatoshi("10491sat") - route = l2.rpc.getroute(l1.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] - l2.rpc.sendpay(route, payment_hash) - with pytest.raises(RpcError, match=r"Capacity exceeded.*'erring_index': 0"): - l2.rpc.waitsendpay(payment_hash, TIMEOUT) - - # But if it's trimmed, we're ok. - amount -= 1 - route = l2.rpc.getroute(l1.info['id'], amount, riskfactor=1, fuzzpercent=0)['route'] - l2.rpc.sendpay(route, payment_hash) - l2.rpc.waitsendpay(payment_hash, TIMEOUT) - - -@pytest.mark.xfail(strict=True) def test_lockup_drain(node_factory, bitcoind): """Try to get channel into a state where funder can't afford fees on additional HTLC, so fundee can't add HTLC""" l1, l2 = node_factory.line_graph(2, opts={'may_reconnect': True})