From af7e87930878117a0633bb7a8cd4488b8e9c5b6d Mon Sep 17 00:00:00 2001 From: Michael Schmoock Date: Thu, 12 Mar 2020 12:29:18 +0100 Subject: [PATCH] fix: rfc #740 requires 100% feespike margin Changelog-Fixed: Use lightning-rfc #740 feespike margin factor of 2 --- channeld/full_channel.c | 23 +++++++++++++--------- lightningd/peer_control.c | 19 +++++++++++++++---- tests/test_connection.py | 6 +++--- tests/test_pay.py | 40 ++++++++++++++++++++------------------- 4 files changed, 53 insertions(+), 35 deletions(-) diff --git a/channeld/full_channel.c b/channeld/full_channel.c index 69107e348..5ceea2583 100644 --- a/channeld/full_channel.c +++ b/channeld/full_channel.c @@ -390,18 +390,19 @@ 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 +/* + * 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 + * demonstrated with c-lightning by @m-schmoock * (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%. + * sure we can afford one more HTLC, even if fees increase by 100%. * * We could do this for the peer, as well, by rejecting their HTLC * immediately in this case. But rejecting a remote HTLC here causes @@ -409,7 +410,11 @@ static struct amount_sat fee_for_htlcs(const struct channel *channel, * 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. */ + * self-regulating too. + * + * This mitigation will become BOLT #2 standard by: + * https://github.com/lightningnetwork/lightning-rfc/issues/740 + */ static bool local_funder_has_fee_headroom(const struct channel *channel, struct amount_msat remainder, const struct htlc **committed, @@ -428,17 +433,17 @@ static bool local_funder_has_fee_headroom(const struct channel *channel, feerate, committed, adding, removing); - /* Now, how much would it cost us if feerate increases 50% and we added + /* Now, how much would it cost us if feerate increases 100% and we added * another HTLC? */ - fee = commit_tx_base_fee(feerate + feerate/2, untrimmed + 1); + fee = commit_tx_base_fee(2 * feerate, 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", + status_debug("Adding HTLC would leave us only %s: we need %s for" + " another HTLC if fees increase by 100%% to %uperkw", type_to_string(tmpctx, struct amount_msat, &remainder), type_to_string(tmpctx, struct amount_sat, &fee), - feerate + feerate/2); + feerate + feerate); return false; } diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 7ed1b61a7..d484a2786 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -550,10 +550,21 @@ static struct amount_sat commit_txfee(const struct channel *channel, 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); + /* + * BOLT-95c74fef2fe590cb8adbd7b848743a229ffe825a #2: + * Adding an HTLC: update_add_htlc + * + * A sending node: + * - if it is responsible for paying the Bitcoin fee: + * - SHOULD NOT offer `amount_msat` if, after adding that HTLC to + * its commitment transaction, its remaining balance doesn't allow + * it to pay the fee for a future additional non-dust HTLC at + * `N*feerate_per_kw` while maintaining its channel reserve + * ("fee spike buffer"), where `N` is a parameter chosen by the + * implementation (`N = 2` is recommended to ensure + * predictability). + */ + return commit_tx_base_fee(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 77750e84b..fd819fc74 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 = 35000000 + slack = 45000000 # 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 (34000) - l1.daemon.wait_for_log('Setting REMOTE feerate to 34000') + # It will raise as far as it can (48000) + l1.daemon.wait_for_log('Setting REMOTE feerate to 48000') 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_pay.py b/tests/test_pay.py index 28274c224..282247949 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -584,37 +584,29 @@ def test_sendpay_cant_afford(node_factory): opts={'feerates': (15000, 15000, 15000)}) # Can't pay more than channel capacity. - def pay(lsrc, ldst, amt, label=None): - if not label: - label = ''.join(random.choice(string.ascii_letters + string.digits) for _ in range(20)) - rhash = ldst.rpc.invoice(amt, label, label)['payment_hash'] - routestep = {'msatoshi': amt, 'id': ldst.info['id'], 'delay': 5, 'channel': '1x1x1'} - lsrc.rpc.sendpay([routestep], rhash) - lsrc.rpc.waitsendpay(rhash) - with pytest.raises(RpcError): - pay(l1, l2, 10**9 + 1) + l1.pay(l2, 10**9 + 1) # This is the fee, which needs to be taken into account for l1. - available = 10**9 - 24030000 + available = 10**9 - 32040000 # Reserve is 1%. reserve = 10**7 # Can't pay past reserve. with pytest.raises(RpcError): - pay(l1, l2, available) + l1.pay(l2, available) with pytest.raises(RpcError): - pay(l1, l2, available - reserve + 1) + l1.pay(l2, available - reserve + 1) # Can pay up to reserve (1%) - pay(l1, l2, available - reserve) + l1.pay(l2, available - reserve) # And now it can't pay back, due to its own reserve. with pytest.raises(RpcError): - pay(l2, l1, available - reserve) + l2.pay(l1, available - reserve) # But this should work. - pay(l2, l1, available - reserve * 2) + l2.pay(l1, available - reserve * 2) def test_decodepay(node_factory): @@ -1561,7 +1553,7 @@ def test_pay_retry(node_factory, bitcoind): """Make sure pay command retries properly. """ def exhaust_channel(funder, fundee, scid, already_spent=0): """Spend all available capacity (10^6 - 1%) of channel""" - maxpay = (10**6 - 10**6 // 100 - 13440) * 1000 - already_spent + maxpay = (10**6 - 10**6 // 100 - 16020) * 1000 - already_spent inv = fundee.rpc.invoice(maxpay, ''.join(random.choice(string.ascii_letters + string.digits) for _ in range(20)), "exhaust_channel") @@ -2293,17 +2285,27 @@ def test_lockup_drain(node_factory, bitcoind): except RpcError: msat //= 2 - # Even if feerate now increases 1.5x (22500), l2 should be able to send + # Even if feerate now increases 2x (30000), l2 should be able to send # non-dust HTLC to l1. - l1.set_feerates([22500] * 3, False) + l1.set_feerates([30000] * 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) + assert(l1.rpc.feerates('perkw')['perkw']['normal'] == 30000) l2.pay(l1, total // 2) + # But if feerate increase just a little more, l2 should not be able to send + # non-dust HTLC to l1 + l1.set_feerates([30002] * 3, False) # TODO: why does 30001 fail, off by one in C code? + l1.restart() + wait_for(lambda: only_one(l1.rpc.listpeers()['peers'])['connected']) + assert(l1.rpc.feerates('perkw')['perkw']['normal'] == 30002) + + with pytest.raises(RpcError, match=r".*Capacity exceeded.*"): + l2.pay(l1, total // 2) + def test_error_returns_blockheight(node_factory, bitcoind): """Test that incorrect_or_unknown_payment_details returns block height"""