From 21d2cc663b577d94fdea12ffcc67352e4ff28612 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 28 Oct 2019 14:03:44 +1030 Subject: [PATCH] lightningd: apply feerate changes correctly. Feerate changes are asymmetric, as they can only be sent by the funder. For FUNDER, the remote feerate is set when upon send of commitment_signed, and the local feerate is set on receipt of revoke_and_ack. For non-funder, the local feerate is set on receipt of commitment_signed, and the remote feerate set on send of revoke_and_ack. In our code, these two happen together. channeld gets this right, but lightningd ignored the funder/fundee distinction, and as a result, receipt of a commitment_signed by the funder altered fees in the database. If there was a reconnection event or restart, then these (incorrect) values would be used, causing us to complain about a 'Bad commit_sig signature' and close the channel. Signed-off-by: Rusty Russell --- CHANGELOG.md | 1 + lightningd/peer_htlcs.c | 24 ++++++++++++++---------- tests/test_connection.py | 1 - 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc6eb9643..9cbb2b053 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ Note: You should always set `allow-deprecated-apis=false` to test for changes. ### Fixed +- Fixed bogus "Bad commit_sig signature" which caused channel closures when reconnecting after updating fees under simultaneous bidirectional traffic. - Relative `--lightning_dir` is now working again. - Build: MacOS now builds again (missing pwritev). diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index f54d7f09a..4c14c31de 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -1343,8 +1343,10 @@ void peer_sending_commitsig(struct channel *channel, const u8 *msg) channel->next_htlc_id += num_local_added; } - /* Update their feerate. */ - channel->channel_info.feerate_per_kw[REMOTE] = feerate; + /* Update remote feerate if we are funder. */ + if (channel->funder == LOCAL) + channel->channel_info.feerate_per_kw[REMOTE] = feerate; + if (feerate > channel->max_possible_feerate) channel->max_possible_feerate = feerate; if (feerate < channel->min_possible_feerate) @@ -1546,12 +1548,13 @@ void peer_got_commitsig(struct channel *channel, const u8 *msg) } } - /* Update both feerates: if we're funder, REMOTE should already be - * that feerate, if we're not, we're about to ACK anyway. */ - channel->channel_info.feerate_per_kw[LOCAL] - = channel->channel_info.feerate_per_kw[REMOTE] - = feerate; - + /* Update both feerates if we're not funder (for funder, receiving + * commitment_signed doesn't alter fees). */ + if (channel->funder == REMOTE) { + channel->channel_info.feerate_per_kw[LOCAL] + = channel->channel_info.feerate_per_kw[REMOTE] + = feerate; + } if (feerate > channel->max_possible_feerate) channel->max_possible_feerate = feerate; if (feerate < channel->min_possible_feerate) @@ -1658,9 +1661,10 @@ void peer_got_revoke(struct channel *channel, const u8 *msg) return; } - /* Update feerate: if we are funder, their revoke_and_ack has set + /* Update feerate if we are funder, their revoke_and_ack has set * this for local feerate. */ - channel->channel_info.feerate_per_kw[LOCAL] = feerate; + if (channel->funder == LOCAL) + channel->channel_info.feerate_per_kw[LOCAL] = feerate; /* FIXME: Check per_commitment_secret -> per_commit_point */ update_per_commit_point(channel, &next_per_commitment_point); diff --git a/tests/test_connection.py b/tests/test_connection.py index 9af104564..4fc84ae6e 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -2148,7 +2148,6 @@ def test_feerate_spam(node_factory, chainparams): l1.daemon.wait_for_log('peer_out WIRE_UPDATE_FEE', timeout=5) -@pytest.mark.xfail(strict=True) @unittest.skipIf(not DEVELOPER, "need dev-feerate") def test_feerate_stress(node_factory, executor): # Third node makes HTLC traffic less predictable.