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 <rusty@rustcorp.com.au>

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

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,

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.

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

tests/test_pay.py

@@ -578,7 +578,9 @@ def test_sendpay(node_factory):
 
 @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 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 - 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})