channeld: track feerates.

Handling feerates for the fundee (who only receives fee_update) is
simple: it's practically atomic since we accept commitment and send
revocation, thus they're applied to both sides at once.

Handling feerates for the funder is more complex: in theory we could
have multiple in flight.  However, if we avoid this using the same
logic as we use to suppress multiple commitments in flight, it's
simple again.

We fix the test code to use real feerate manipulation, thus have to
remove an assert about feerate being non-zero.  And now we have
feechanges, we need to rely on the changes_pending flags, as we can
have changes without an HTLCs changing.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld/full_channel.c

@@ -406,8 +406,6 @@ static enum channel_add_err add_htlc(struct channel *channel,
 		u64 dust = dust_limit_satoshis(channel, recipient);
 		size_t untrimmed;
 
-		assert(feerate >= 1);
-		assert(dust >= 1);
 		untrimmed = commit_tx_num_untrimmed(committed, feerate, dust,
 						    recipient)
 			+ commit_tx_num_untrimmed(adding, feerate, dust,
@@ -743,7 +741,11 @@ bool channel_update_feerate(struct channel *channel, u32 feerate_per_kw)
 	if (!can_funder_afford_feerate(channel, feerate_per_kw))
 		return false;
 
+	status_trace("Setting %s feerate to %u",
+		     side_to_str(!channel->funder), feerate_per_kw);
+
 	channel->view[!channel->funder].feerate_per_kw = feerate_per_kw;
+	channel->changes_pending[!channel->funder] = true;
 	return true;
 }
 
@@ -752,11 +754,9 @@ u32 channel_feerate(const struct channel *channel, enum side side)
 	return channel->view[side].feerate_per_kw;
 }
 
-/* FIXME: Handle fee changes too. */
 bool channel_sending_commit(struct channel *channel,
 			    const struct htlc ***htlcs)
 {
-	int change;
 	const enum htlc_state states[] = { SENT_ADD_HTLC,
 					   SENT_REMOVE_REVOCATION,
 					   SENT_ADD_REVOCATION,
@@ -769,9 +769,8 @@ bool channel_sending_commit(struct channel *channel,
 		return false;
 	}
 
-	change = change_htlcs(channel, REMOTE, states, ARRAY_SIZE(states),
-			      htlcs, "sending_commit");
-	assert(change & HTLC_REMOTE_F_COMMITTED);
+	change_htlcs(channel, REMOTE, states, ARRAY_SIZE(states),
+		     htlcs, "sending_commit");
 	channel->changes_pending[REMOTE] = false;
 
 	assert(!channel->awaiting_revoke_and_ack);
@@ -799,13 +798,23 @@ bool channel_rcvd_revoke_and_ack(struct channel *channel,
 	assert(channel->awaiting_revoke_and_ack);
 	channel->awaiting_revoke_and_ack = false;
 
-	return change & HTLC_LOCAL_F_PENDING;
+	/* 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",
+			     channel->view[REMOTE].feerate_per_kw);
+		channel->view[LOCAL].feerate_per_kw
+			= channel->view[REMOTE].feerate_per_kw;
+		channel->changes_pending[LOCAL] = true;
+	}
+
+	return channel->changes_pending[LOCAL];
 }
 
 /* FIXME: We can actually merge these two... */
 bool channel_rcvd_commit(struct channel *channel, const struct htlc ***htlcs)
 {
-	int change;
 	const enum htlc_state states[] = { RCVD_ADD_REVOCATION,
 					   RCVD_REMOVE_HTLC,
 					   RCVD_ADD_HTLC,
@@ -818,13 +827,11 @@ bool channel_rcvd_commit(struct channel *channel, const struct htlc ***htlcs)
 		return false;
 	}
 
-	change = change_htlcs(channel, LOCAL, states, ARRAY_SIZE(states), htlcs,
-			      "rcvd_commit");
+	change_htlcs(channel, LOCAL, states, ARRAY_SIZE(states), htlcs,
+		     "rcvd_commit");
 
-	assert(change & HTLC_LOCAL_F_COMMITTED);
 	channel->changes_pending[LOCAL] = false;
-
-	return change & HTLC_LOCAL_F_COMMITTED;
+	return true;
 }
 
 bool channel_sending_revoke_and_ack(struct channel *channel)
@@ -842,7 +849,18 @@ bool channel_sending_revoke_and_ack(struct channel *channel)
 	if (change & HTLC_REMOTE_F_PENDING)
 		channel->changes_pending[REMOTE] = true;
 
-	return change & HTLC_REMOTE_F_PENDING;
+	/* For non-funder, sending ack means we apply any fund changes to them */
+	if (channel->funder == REMOTE
+	    && (channel->view[LOCAL].feerate_per_kw
+		!= channel->view[REMOTE].feerate_per_kw)) {
+		status_trace("Applying feerate %u to REMOTE",
+			     channel->view[LOCAL].feerate_per_kw);
+		channel->view[REMOTE].feerate_per_kw
+			= channel->view[LOCAL].feerate_per_kw;
+		channel->changes_pending[REMOTE] = true;
+	}
+
+	return channel->changes_pending[REMOTE];
 }
 
 static bool htlc_awaiting_revoke_and_ack(const struct htlc *h)
@@ -862,20 +880,7 @@ static bool htlc_awaiting_revoke_and_ack(const struct htlc *h)
 
 bool channel_awaiting_revoke_and_ack(const struct channel *channel)
 {
-	struct htlc_map_iter it;
-	struct htlc *h;
-
-	/* FIXME: remove debugging iteration. */
-	for (h = htlc_map_first(channel->htlcs, &it);
-	     h;
-	     h = htlc_map_next(channel->htlcs, &it)) {
-		if (htlc_awaiting_revoke_and_ack(h)) {
-			assert(channel->awaiting_revoke_and_ack);
-			return true;
-		}
-	}
-	assert(!channel->awaiting_revoke_and_ack);
-	return false;
+	return channel->awaiting_revoke_and_ack;
 }
 
 bool channel_has_htlcs(const struct channel *channel)

channeld/test/run-full_channel.c

@@ -298,6 +298,35 @@ static void send_and_fulfill_htlc(struct channel *channel,
 	}
 }
 
+static void update_feerate(struct channel *channel, u32 feerate)
+{
+	bool ret;
+
+	ret = channel_update_feerate(channel, feerate);
+	assert(ret);
+	if (channel->funder == LOCAL) {
+		ret = channel_sending_commit(channel, NULL);
+		assert(ret);
+		ret = channel_rcvd_revoke_and_ack(channel, NULL);
+		assert(ret);
+		ret = channel_rcvd_commit(channel, NULL);
+		assert(ret);
+		ret = channel_sending_revoke_and_ack(channel);
+		assert(!ret);
+	} else {
+		ret = channel_rcvd_commit(channel, NULL);
+		assert(ret);
+		ret = channel_sending_revoke_and_ack(channel);
+		assert(ret);
+		ret = channel_sending_commit(channel, NULL);
+		assert(ret);
+		ret = channel_rcvd_revoke_and_ack(channel, NULL);
+		assert(!ret);
+	}
+	assert(channel_feerate(channel, LOCAL) == feerate);
+	assert(channel_feerate(channel, REMOTE) == feerate);
+}
+
 int main(void)
 {
 	tal_t *tmpctx = tal_tmpctx(NULL);
@@ -513,9 +542,9 @@ int main(void)
 			   rchannel, &local_per_commitment_point, 42, REMOTE);
 	txs_must_be_eq(txs, txs2);
 
-	/* FIXME: Adjust properly! */
-	lchannel->view[LOCAL].feerate_per_kw = feerate_per_kw[LOCAL];
-	rchannel->view[REMOTE].feerate_per_kw = feerate_per_kw[REMOTE];
+	update_feerate(lchannel, feerate_per_kw[LOCAL]);
+	update_feerate(rchannel, feerate_per_kw[REMOTE]);
+
 	htlcs = include_htlcs(lchannel, LOCAL);
 	include_htlcs(rchannel, REMOTE);