WIP

channeld/full_channel.c

@@ -525,7 +525,7 @@ static bool htlc_dust(const struct channel *channel,
  *
  * To mostly avoid this situation, at least from our side, we apply an
  * additional constraint when we're opener trying to add an HTLC: make
- * sure we can afford one more HTLC, even if fees increase by 100%.
+ * sure we can afford one more HTLC, even if fees increase.
  *
  * We could do this for the peer, as well, by rejecting their HTLC
  * immediately in this case.  But rejecting a remote HTLC here causes
@@ -560,39 +560,19 @@ static bool local_opener_has_fee_headroom(const struct channel *channel,
 					option_anchors_zero_fee_htlc_tx,
 					committed, adding, removing);
 
-	/* Scale the feerate up by a margin. This ensures that we have
-	 * some leeway even in rising fees. The calculation starts
-	 * with a 100% margin at very low fees, since they are likely
-	 * to rise, and can do so quickly, whereas on the higher fee
-	 * side, asking for a 100% margin is excessive, so ask for a
-	 * 10% margin. In-between these two regions we interpolate
-	 * linearly. Notice that minfeerate and maxfeerate are just
-	 * the markers of the linear interpolation, they don't have
-	 * to correspond to actual feerates seen in the network.
-	 *
-	 * See [CLN6974] for details and discussion.
-	 *
-	 * [CLN6974]: https://github.com/ElementsProject/lightning/issues/6974
-	 */
-	u64 minfeerate = 253, maxfeerate = 45000,
-	    min = feerate - minfeerate > maxfeerate ? maxfeerate
-						    : feerate - minfeerate;
-	double marginperc = 1 - min / (maxfeerate * 1.1);
-	u64 marginrate = 1 + marginperc;
-
 	/* Now, how much would it cost us if feerate increases 100% and we added
 	 * another HTLC? */
-	fee = commit_tx_base_fee(marginrate, untrimmed + 1,
+	fee = commit_tx_base_fee(marginal_feerate(feerate), untrimmed + 1,
 				 option_anchor_outputs,
 				 option_anchors_zero_fee_htlc_tx);
 	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 by 100%% to %uperkw",
+		     " another HTLC if fees increase from %uperkw to %uperkw",
 		     type_to_string(tmpctx, struct amount_msat, &remainder),
 		     type_to_string(tmpctx, struct amount_sat, &fee),
-		     feerate + feerate);
+		     feerate, marginal_feerate(feerate));
 	return false;
 }
 
@@ -770,6 +750,11 @@ static enum channel_add_err add_htlc(struct channel *channel,
 		if (htlc_fee)
 			*htlc_fee = fee;
 
+		status_debug("htlc_fee for %s %zu adding %zu removing == %s",
+			     recipient == LOCAL ? "us" : "them",
+			     tal_count(adding), tal_count(removing),
+			     type_to_string(tmpctx, struct amount_sat, &fee));
+
 		/* This is a little subtle:
 		 *
 		 * The change is being applied to the receiver but it will
@@ -834,6 +819,11 @@ static enum channel_add_err add_htlc(struct channel *channel,
 					    adding,
 					    removing,
 					    channel->opener);
+			status_debug("htlc_fee for %s %zu adding %zu removing == %s",
+				     recipient == LOCAL ? "us" : "them",
+				     tal_count(adding), tal_count(removing),
+				     type_to_string(tmpctx, struct amount_sat, &fee));
+
 			/* set fee output pointer if given */
 			if (htlc_fee && amount_sat_greater(fee, *htlc_fee))
 				*htlc_fee = fee;

common/fee_states.c

@@ -157,6 +157,27 @@ bool fee_states_valid(const struct fee_states *fee_states, enum side opener)
 	return fee_states->feerate[last_fee_state(opener)] != NULL;
 }
 
+/* The calculation starts with a 100% margin at very low fees, since
+ * they are likely to rise, and can do so quickly, whereas on the
+ * higher fee side, asking for a 100% margin is excessive, so ask for
+ * a 10% margin. In-between these two regions we interpolate
+ * linearly. Notice that minfeerate and maxfeerate are just the
+ * markers of the linear interpolation, they don't have to correspond
+ * to actual feerates seen in the network. */
+u32 marginal_feerate(u32 current_feerate)
+{
+	const u32 minfeerate = 253, maxfeerate = 45000;
+
+	assert(current_feerate >= minfeerate);
+	if (current_feerate > maxfeerate)
+		return current_feerate * 1.1;
+
+	/* min gives 1, max gives 0.1 */
+	double proportion = 1.0 - ((double)current_feerate - minfeerate) / (maxfeerate - minfeerate) * 0.9;
+
+	return current_feerate + proportion * current_feerate;
+}
+
 static const char *fmt_fee_states(const tal_t *ctx,
 				  const struct fee_states *fee_states)
 {

common/fee_states.h

@@ -90,4 +90,14 @@ bool fee_states_valid(const struct fee_states *fee_states, enum side opener);
 bool feerate_changes_done(const struct fee_states *fee_states,
 			  bool ignore_uncommitted);
 
+/**
+ * https://github.com/lightningnetwork/lightning-rfc/issues/740 indicates
+ * we should not allow an HTLC if a fee increase of 100% would cause us to
+ * get stuck.
+ *
+ * This proved overzealous: if fees are already high there is less chance
+ * they increase by another 100%, so we scale the factor.
+ */
+u32 marginal_feerate(u32 current_feerate);
+
 #endif /* LIGHTNING_COMMON_FEE_STATES_H */

common/test/run-marginal_feerate.c

@@ -0,0 +1,127 @@
+#include "config.h"
+#include <assert.h>
+#include <bitcoin/chainparams.h>
+#include <common/amount.h>
+#include <common/setup.h>
+#include <stdio.h>
+#include "../fee_states.c"
+
+/* AUTOGENERATED MOCKS START */
+/* Generated stub for amount_asset_is_main */
+bool amount_asset_is_main(struct amount_asset *asset UNNEEDED)
+{ fprintf(stderr, "amount_asset_is_main called!\n"); abort(); }
+/* Generated stub for amount_asset_to_sat */
+struct amount_sat amount_asset_to_sat(struct amount_asset *asset UNNEEDED)
+{ fprintf(stderr, "amount_asset_to_sat called!\n"); abort(); }
+/* Generated stub for amount_feerate */
+ bool amount_feerate(u32 *feerate UNNEEDED, struct amount_sat fee UNNEEDED, size_t weight UNNEEDED)
+{ fprintf(stderr, "amount_feerate called!\n"); abort(); }
+/* Generated stub for amount_sat */
+struct amount_sat amount_sat(u64 satoshis UNNEEDED)
+{ fprintf(stderr, "amount_sat called!\n"); abort(); }
+/* Generated stub for amount_sat_add */
+ bool amount_sat_add(struct amount_sat *val UNNEEDED,
+				       struct amount_sat a UNNEEDED,
+				       struct amount_sat b UNNEEDED)
+{ fprintf(stderr, "amount_sat_add called!\n"); abort(); }
+/* Generated stub for amount_sat_eq */
+bool amount_sat_eq(struct amount_sat a UNNEEDED, struct amount_sat b UNNEEDED)
+{ fprintf(stderr, "amount_sat_eq called!\n"); abort(); }
+/* Generated stub for amount_sat_greater_eq */
+bool amount_sat_greater_eq(struct amount_sat a UNNEEDED, struct amount_sat b UNNEEDED)
+{ fprintf(stderr, "amount_sat_greater_eq called!\n"); abort(); }
+/* Generated stub for amount_sat_sub */
+ bool amount_sat_sub(struct amount_sat *val UNNEEDED,
+				       struct amount_sat a UNNEEDED,
+				       struct amount_sat b UNNEEDED)
+{ fprintf(stderr, "amount_sat_sub called!\n"); abort(); }
+/* Generated stub for amount_sat_to_asset */
+struct amount_asset amount_sat_to_asset(struct amount_sat *sat UNNEEDED, const u8 *asset UNNEEDED)
+{ fprintf(stderr, "amount_sat_to_asset called!\n"); abort(); }
+/* Generated stub for amount_tx_fee */
+struct amount_sat amount_tx_fee(u32 fee_per_kw UNNEEDED, size_t weight UNNEEDED)
+{ fprintf(stderr, "amount_tx_fee called!\n"); abort(); }
+/* Generated stub for fromwire */
+const u8 *fromwire(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, void *copy UNNEEDED, size_t n UNNEEDED)
+{ fprintf(stderr, "fromwire called!\n"); abort(); }
+/* Generated stub for fromwire_bool */
+bool fromwire_bool(const u8 **cursor UNNEEDED, size_t *max UNNEEDED)
+{ fprintf(stderr, "fromwire_bool called!\n"); abort(); }
+/* Generated stub for fromwire_fail */
+void *fromwire_fail(const u8 **cursor UNNEEDED, size_t *max UNNEEDED)
+{ fprintf(stderr, "fromwire_fail called!\n"); abort(); }
+/* Generated stub for fromwire_secp256k1_ecdsa_signature */
+void fromwire_secp256k1_ecdsa_signature(const u8 **cursor UNNEEDED, size_t *max UNNEEDED,
+					secp256k1_ecdsa_signature *signature UNNEEDED)
+{ fprintf(stderr, "fromwire_secp256k1_ecdsa_signature called!\n"); abort(); }
+/* Generated stub for fromwire_sha256 */
+void fromwire_sha256(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct sha256 *sha256 UNNEEDED)
+{ fprintf(stderr, "fromwire_sha256 called!\n"); abort(); }
+/* Generated stub for fromwire_tal_arrn */
+u8 *fromwire_tal_arrn(const tal_t *ctx UNNEEDED,
+		       const u8 **cursor UNNEEDED, size_t *max UNNEEDED, size_t num UNNEEDED)
+{ fprintf(stderr, "fromwire_tal_arrn called!\n"); abort(); }
+/* Generated stub for fromwire_u32 */
+u32 fromwire_u32(const u8 **cursor UNNEEDED, size_t *max UNNEEDED)
+{ fprintf(stderr, "fromwire_u32 called!\n"); abort(); }
+/* Generated stub for fromwire_u64 */
+u64 fromwire_u64(const u8 **cursor UNNEEDED, size_t *max UNNEEDED)
+{ fprintf(stderr, "fromwire_u64 called!\n"); abort(); }
+/* Generated stub for fromwire_u8 */
+u8 fromwire_u8(const u8 **cursor UNNEEDED, size_t *max UNNEEDED)
+{ fprintf(stderr, "fromwire_u8 called!\n"); abort(); }
+/* Generated stub for fromwire_u8_array */
+void fromwire_u8_array(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, u8 *arr UNNEEDED, size_t num UNNEEDED)
+{ fprintf(stderr, "fromwire_u8_array called!\n"); abort(); }
+/* Generated stub for htlc_state_flags */
+int htlc_state_flags(enum htlc_state state UNNEEDED)
+{ fprintf(stderr, "htlc_state_flags called!\n"); abort(); }
+/* Generated stub for htlc_state_name */
+const char *htlc_state_name(enum htlc_state s UNNEEDED)
+{ fprintf(stderr, "htlc_state_name called!\n"); abort(); }
+/* Generated stub for towire */
+void towire(u8 **pptr UNNEEDED, const void *data UNNEEDED, size_t len UNNEEDED)
+{ fprintf(stderr, "towire called!\n"); abort(); }
+/* Generated stub for towire_bool */
+void towire_bool(u8 **pptr UNNEEDED, bool v UNNEEDED)
+{ fprintf(stderr, "towire_bool called!\n"); abort(); }
+/* Generated stub for towire_secp256k1_ecdsa_signature */
+void towire_secp256k1_ecdsa_signature(u8 **pptr UNNEEDED,
+			      const secp256k1_ecdsa_signature *signature UNNEEDED)
+{ fprintf(stderr, "towire_secp256k1_ecdsa_signature called!\n"); abort(); }
+/* Generated stub for towire_sha256 */
+void towire_sha256(u8 **pptr UNNEEDED, const struct sha256 *sha256 UNNEEDED)
+{ fprintf(stderr, "towire_sha256 called!\n"); abort(); }
+/* Generated stub for towire_u32 */
+void towire_u32(u8 **pptr UNNEEDED, u32 v UNNEEDED)
+{ fprintf(stderr, "towire_u32 called!\n"); abort(); }
+/* Generated stub for towire_u64 */
+void towire_u64(u8 **pptr UNNEEDED, u64 v UNNEEDED)
+{ fprintf(stderr, "towire_u64 called!\n"); abort(); }
+/* Generated stub for towire_u8 */
+void towire_u8(u8 **pptr UNNEEDED, u8 v UNNEEDED)
+{ fprintf(stderr, "towire_u8 called!\n"); abort(); }
+/* Generated stub for towire_u8_array */
+void towire_u8_array(u8 **pptr UNNEEDED, const u8 *arr UNNEEDED, size_t num UNNEEDED)
+{ fprintf(stderr, "towire_u8_array called!\n"); abort(); }
+/* AUTOGENERATED MOCKS END */
+
+int main(int argc, char *argv[])
+{
+	common_setup(argv[0]);
+
+	/* At minimum, it's 100% */
+	assert(marginal_feerate(253) == 253 * 2);
+
+	/* At maximum, it's 10% */
+	assert(marginal_feerate(45000) == (u32)(45000 * 1.1));
+
+	/* Above maximum, still 10% */
+	assert(marginal_feerate(500000) == (u32)(500000 * 1.1));
+
+	/* Half way, it's 55% */
+	u32 half = (45000 + 253)/2;
+	assert(marginal_feerate(half) == (u32)(half * 1.55));
+
+	common_shutdown();
+}

lightningd/peer_control.c

@@ -673,9 +673,13 @@ static struct amount_sat commit_txfee(const struct channel *channel,
 	 *   reserve. It is recommended that this "fee spike buffer" can
 	 *   handle twice the current `feerate_per_kw` to ensure
 	 *   predictability between implementations.
-	*/
-	fee = commit_tx_base_fee(2 * feerate, num_untrimmed_htlcs + 1,
+	 */
+	fee = commit_tx_base_fee(marginal_feerate(feerate), num_untrimmed_htlcs + 1,
 				 option_anchor_outputs, option_anchors_zero_fee_htlc_tx);
+	log_debug(channel->log, "Before anchors: fee for %zu untrimmed at feerate %u (based on %u) will be %s",
+		  num_untrimmed_htlcs + 1,
+		  marginal_feerate(feerate), feerate,
+		  type_to_string(tmpctx, struct amount_sat, &fee));
 
 	if (option_anchor_outputs || option_anchors_zero_fee_htlc_tx) {
 		/* BOLT #3:
@@ -686,6 +690,8 @@ static struct amount_sat commit_txfee(const struct channel *channel,
 		 */
 		if (!amount_sat_add(&fee, fee, AMOUNT_SAT(660)))
 			; /* fee is somehow astronomical already.... */
+		log_debug(channel->log, "After anchors: fee is %s",
+			  type_to_string(tmpctx, struct amount_sat, &fee));
 	}
 
 	return fee;

tests/test_pay.py

@@ -754,7 +754,8 @@ def test_wait_sendpay(node_factory, executor):
 @pytest.mark.parametrize("anchors", [False, True])
 def test_sendpay_cant_afford(node_factory, anchors):
     # Set feerates the same so we don't have to wait for update.
-    opts = {'feerates': (15000, 15000, 15000, 15000)}
+    opts = {'feerates': (15000, 15000, 15000, 15000),
+            'commit-feerate-offset': 0}
     if anchors is False:
         opts['dev-force-features'] = "-23"
 
@@ -771,22 +772,28 @@ def test_sendpay_cant_afford(node_factory, anchors):
     # minimum = 1
     # maximum = 10**9
     # while maximum - minimum > 1:
-    #     l1, l2 = node_factory.line_graph(2, fundamount=10**6,
-    #                                      opts={'feerates': (15000, 15000, 15000, 15000)})
+    #     l1, l2 = node_factory.line_graph(2, fundamount=10**6, opts=opts)
     #     try:
     #         l1.pay(l2, (minimum + maximum) // 2)
+    #         print("XXX Pay {} WORKED!".format((minimum + maximum) // 2))
     #         minimum = (minimum + maximum) // 2
     #     except RpcError:
+    #         print("XXX Pay {} FAILED!".format((minimum + maximum) // 2))
     #         maximum = (minimum + maximum) // 2
     #     print("{} - {}".format(minimum, maximum))
-    # assert False
+
+    #     # Make sure not HTLCs left to interfere
+    #     wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == [])
+    #     wait_for(lambda: only_one(l2.rpc.listpeerchannels()['channels'])['htlcs'] == [])
+    # assert False, "Max we can pay == {}".format(minimum)
+    # # Currently this gives: 976560000 for non-anchors, 969900000 for anchors
+    # # Add reserve to this result to derive `available`
 
     # This is the fee, which needs to be taken into account for l1.
-    if anchors:
-        # option_anchor_outputs
-        available = 10**9 - 44700000
+    if anchors: 
+        available = 969900000 + reserve
     else:
-        available = 10**9 - 32040000
+        available = 976560000 + reserve
 
     # Can't pay past reserve.
     with pytest.raises(RpcError):
@@ -2507,11 +2514,15 @@ def test_setchannel_startup_opts(node_factory, bitcoind):
     assert result[1]['htlc_maximum_msat'] == Millisatoshi(5)
 
 
-def test_channel_spendable(node_factory, bitcoind):
+@pytest.mark.parametrize("anchors", [False, True])
+def test_channel_spendable(node_factory, bitcoind, anchors):
     """Test that spendable_msat is accurate"""
     sats = 10**6
+    opts={'plugin': os.path.join(os.getcwd(), 'tests/plugins/hold_invoice.py'), 'holdtime': '30'}
+    if anchors is False:
+        opts['dev-force-features'] = "-23"
     l1, l2 = node_factory.line_graph(2, fundamount=sats, wait_for_announce=True,
-                                     opts={'plugin': os.path.join(os.getcwd(), 'tests/plugins/hold_invoice.py'), 'holdtime': '30'})
+                                     opts=opts)
 
     inv = l2.rpc.invoice('any', 'inv', 'for testing')
     payment_hash = inv['payment_hash']