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

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);

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):
     l1, l2, l3 = node_factory.line_graph(3, opts={'commit-time': 100,