Channel lockup corner case workaround

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,16 +385,64 @@ 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);
 }
 
+/* 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,
@@ -536,7 +595,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,
@@ -565,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
@@ -578,7 +646,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 +664,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,

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 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,62 +2277,31 @@ 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)
+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):