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})