channeld: don't ever send back-to-back feerate changes.

There are several reports of desynchronization with LND here; a simple
approach is to only have one feerate change in flight at any time.

Even if this turns out to be our fault, it's been a historic area of
confusion, so this restriction seems reasonable.

Changelog-Fixed: Protocol: Don't create more than one feerate change at a time, as this seems to desync with LND.
Fixes: #4152
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld/channeld.c

@@ -944,6 +944,28 @@ static struct bitcoin_signature *unraw_sigs(const tal_t *ctx,
 	return sigs;
 }
 
+/* Do we want to update fees? */
+static bool want_fee_update(const struct peer *peer, u32 *target)
+{
+	u32 max, val;
+
+	if (peer->channel->opener != LOCAL)
+		return false;
+
+	max = approx_max_feerate(peer->channel);
+	val = peer->desired_feerate;
+
+	/* FIXME: We should avoid adding HTLCs until we can meet this
+	 * feerate! */
+	if (val > max)
+		val = max;
+
+	if (target)
+		*target = val;
+
+	return val != channel_feerate(peer->channel, REMOTE);
+}
+
 static void send_commit(struct peer *peer)
 {
 	u8 *msg;
@@ -954,6 +976,7 @@ static void send_commit(struct peer *peer)
 	const struct htlc **htlc_map;
 	struct wally_tx_output *direct_outputs[NUM_SIDES];
 	struct penalty_base *pbase;
+	u32 feerate_target;
 
 #if DEVELOPER
 	/* Hack to suppress all commit sends if dev_disconnect says to */
@@ -1002,27 +1025,21 @@ static void send_commit(struct peer *peer)
 	}
 
 	/* If we wanted to update fees, do it now. */
-	if (peer->channel->opener == LOCAL) {
-		u32 feerate, max = approx_max_feerate(peer->channel);
-
-		feerate = peer->desired_feerate;
-
-		/* FIXME: We should avoid adding HTLCs until we can meet this
-		 * feerate! */
-		if (feerate > max)
-			feerate = max;
-
-		if (feerate != channel_feerate(peer->channel, REMOTE)) {
+	if (want_fee_update(peer, &feerate_target)) {
+		/* FIXME: We occasionally desynchronize with LND here, so
+		 * don't stress things by having more than one feerate change
+		 * in-flight! */
+		if (feerate_changes_done(peer->channel->fee_states)) {
 			u8 *msg;
 
-			if (!channel_update_feerate(peer->channel, feerate))
+			if (!channel_update_feerate(peer->channel, feerate_target))
 				status_failed(STATUS_FAIL_INTERNAL_ERROR,
 					      "Could not afford feerate %u"
 					      " (vs max %u)",
-					      feerate, max);
+					      feerate_target, approx_max_feerate(peer->channel));
 
 			msg = towire_update_fee(NULL, &peer->channel_id,
-						feerate);
+						feerate_target);
 			sync_crypto_write(peer->pps, take(msg));
 		}
 	}

common/fee_states.c

@@ -73,6 +73,16 @@ u32 get_feerate(const struct fee_states *fee_states,
 	abort();
 }
 
+/* Are feerates all agreed by both sides? */
+bool feerate_changes_done(const struct fee_states *fee_states)
+{
+	size_t num_feerates = 0;
+	for (size_t i = 0; i < ARRAY_SIZE(fee_states->feerate); i++) {
+		num_feerates += (fee_states->feerate[i] != NULL);
+	}
+	return num_feerates == 1;
+}
+
 void start_fee_update(struct fee_states *fee_states,
 		      enum side opener,
 		      u32 feerate_per_kw)

common/fee_states.h

@@ -85,4 +85,8 @@ struct fee_states *fromwire_fee_states(const tal_t *ctx,
  * Is this fee_state struct valid for this side?
  */
 bool fee_states_valid(const struct fee_states *fee_states, enum side opener);
+
+/* Are therre no more fee changes in-flight? */
+bool feerate_changes_done(const struct fee_states *fee_states);
+
 #endif /* LIGHTNING_COMMON_FEE_STATES_H */