From 127d3492cdf840f6ddea50056394e9b9a0b5f616 Mon Sep 17 00:00:00 2001 From: Michael Schmoock Date: Thu, 12 Mar 2020 12:29:18 +0100 Subject: [PATCH] fix: rfc #740 requires 100% feespike margin --- channeld/full_channel.c | 28 +++++++++++++++++++++------- lightningd/peer_control.c | 30 ++++++++++++++++++++++-------- tests/test_pay.py | 2 +- 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/channeld/full_channel.c b/channeld/full_channel.c index 69107e348798..4d562dc74f73 100644 --- a/channeld/full_channel.c +++ b/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,9 +400,21 @@ 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 @@ -409,7 +422,8 @@ static struct amount_sat fee_for_htlcs(const struct channel *channel, * 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); diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index f97d0b8074ef..9f41c8ad9671 100644 --- a/lightningd/peer_control.c +++ b/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); } diff --git a/tests/test_pay.py b/tests/test_pay.py index 090bde4ae160..9ba215a4006d 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2354,7 +2354,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)