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 <rusty@rustcorp.com.au>

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
 

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:

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)