fix: rfc ElementsProject#740 requires 100% feespike margin

Author: m-schmoock

channeld/full_channel.c

@@ -390,7 +390,8 @@ 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
@@ -399,17 +400,30 @@ static struct amount_sat fee_for_htlcs(const struct channel *channel,
  * demonstrated with c-lightning by @m-schmook
  * (https://github.com/ElementsProject/lightning/pull/3498).
  *
+ * This will be mitigated by applying a 2-times fee spike margin:
+ * https://github.com/lightningnetwork/lightning-rfc/issues/740
+ *
+ * BOLT #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 2*feerate_per_kw
+ * while maintaining its channel reserve ("fee spike buffer").
+ *
+ *
  * 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.
+ */
 static bool local_funder_has_fee_headroom(const struct channel *channel,
 					  struct amount_msat remainder,
 					  const struct htlc **committed,
@@ -428,14 +442,14 @@ 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);

lightningd/peer_control.c

@@ -551,10 +551,17 @@ static struct amount_sat commit_txfee_spend(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 #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 2 * feerate_per_kw
+	 * while maintaining its channel reserve ("fee spike buffer").
+	 */
+	return commit_tx_base_fee(2 * local_feerate, num_untrimmed_htlcs + 1);
 }
 
 /* Fee a commitment transaction would currently cost when receiving */
@@ -598,10 +605,17 @@ static struct amount_sat commit_txfee_recv(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(feerate + feerate / 2,
-				  num_untrimmed_htlcs + 1);
+	/*
+	 * BOLT #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 2 * feerate_per_kw
+	 * while maintaining its channel reserve ("fee spike buffer").
+	 */
+	return commit_tx_base_fee(2 * feerate, num_untrimmed_htlcs + 1);
 }
 
 

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,30 @@ 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
+    print(l1.rpc.listpeers())
+    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):
@@ -2354,7 +2347,7 @@ def test_lockup_drain(node_factory, bitcoind):
 
     # But if feerate increase just a little more, l2 should not be able to send
     # non-fust HTLC to l1
-    l1.force_feerates(22502)  # TODO: Why does 22501 fail? off by one in C code?
+    l1.force_feerates(30002)  # TODO: Why does 30001 fail? off by one in C code?
     with pytest.raises(RpcError, match=r".*Capacity exceeded.*"):
         l2.pay(l1, total // 2)