From 6f015b69fd2c70d46259a7ce8d293f44f929aef6 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sat, 8 Jun 2019 12:27:27 +0930 Subject: [PATCH] channeld: don't send feerate spam if we can't set it as high as we want. @pm47 gave a great bug report showing c-lightning sending the same UPDATE_FEE over and over, with the final surprise result being that we blamed the peer for sending us multiple empty commits! The spam is caused by us checking "are we at the desired feerate?" but then if we can't afford the desired feerate, setting the feerate we can afford, even though it's a duplicate. Doing the feerate cap before we test if it's what we have already eliminates this. But the empty commits was harder to find: it's caused by a heuristic in channel_rcvd_revoke_and_ack: ``` /* For funder, ack also means time to apply new feerate locally. */ if (channel->funder == LOCAL && (channel->view[LOCAL].feerate_per_kw != channel->view[REMOTE].feerate_per_kw)) { status_trace("Applying feerate %u to LOCAL (was %u)", channel->view[REMOTE].feerate_per_kw, channel->view[LOCAL].feerate_per_kw); channel->view[LOCAL].feerate_per_kw = channel->view[REMOTE].feerate_per_kw; channel->changes_pending[LOCAL] = true; } ``` We assume we never send duplicates, so we detect an otherwise-empty change using the difference in feerates. If we don't set this flag, we will get upset if we receive a commitment_signed since we consider there to be no changes to commit. This is actually hard to test: the previous commit adds a test which spams update_fee and doesn't trigger this bug, because both sides use the same "there's nothing outstanding" logic. Fixes: #2701 Signed-off-by: Rusty Russell --- CHANGELOG.md | 1 + channeld/channeld.c | 23 +++++++++++++---------- tests/test_connection.py | 1 - 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a132ae3d1..58ce3ef58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ changes. - lightningd: fixed occasional hang on `connect` when peer had sent error. - JSON RPC: `decodeinvoice` and `pay` now handle unknown invoice fields properly. - JSON API: `waitsendpay` (PAY_STOPPED_RETRYING) error handler now returns valid JSON +- protocol: don't send multiple identical feerate changes if we want the feerate higher than we can afford. ### Security diff --git a/channeld/channeld.c b/channeld/channeld.c index 2b2631498..c49d93450 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -1133,9 +1133,7 @@ static void send_commit(struct peer *peer) } /* If we wanted to update fees, do it now. */ - if (peer->channel->funder == LOCAL - && peer->desired_feerate != channel_feerate(peer->channel, REMOTE)) { - u8 *msg; + if (peer->channel->funder == LOCAL) { u32 feerate, max = approx_max_feerate(peer->channel); feerate = peer->desired_feerate; @@ -1145,14 +1143,19 @@ static void send_commit(struct peer *peer) if (feerate > max) feerate = max; - if (!channel_update_feerate(peer->channel, feerate)) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Could not afford feerate %u" - " (vs max %u)", - feerate, max); + if (feerate != channel_feerate(peer->channel, REMOTE)) { + u8 *msg; - msg = towire_update_fee(NULL, &peer->channel_id, feerate); - sync_crypto_write(peer->pps, take(msg)); + if (!channel_update_feerate(peer->channel, feerate)) + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "Could not afford feerate %u" + " (vs max %u)", + feerate, max); + + msg = towire_update_fee(NULL, &peer->channel_id, + feerate); + sync_crypto_write(peer->pps, take(msg)); + } } /* BOLT #2: diff --git a/tests/test_connection.py b/tests/test_connection.py index 22cc3e259..27909da8a 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -1692,7 +1692,6 @@ def test_change_chaining(node_factory, bitcoind): l1.rpc.fundchannel(l3.info['id'], 10**7, minconf=0) -@pytest.mark.xfail(strict=True) def test_feerate_spam(node_factory): l1, l2 = node_factory.line_graph(2)