fix: rfc #740 requires 100% feespike margin

Changelog-Fixed: Use lightning-rfc #740 feespike margin factor of 2

Author: m-schmoock

channeld/full_channel.c

@@ -390,26 +390,31 @@ 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
  * 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. */
+ * 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;
 }
 

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,

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.

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"""