peer: handle case correctly where they can't handle fees.

When they propose an HTLC to us, they need to be able to cover both it,
and the associated fees.  When it gets acked and applied to them, however,
they may no longer be able to afford the fees; this is OK and expected.

So add a flag to say whether they can dig into fees or not: without
this patch the code calls fatal() on the next patch which tests it.

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

daemon/channel.c

@@ -90,7 +90,8 @@ static bool change_funding(uint64_t anchor_satoshis,
 			   int64_t htlc_msat,
 			   struct channel_oneside *a,
 			   struct channel_oneside *b,
-			   size_t num_nondust_htlcs)
+			   size_t num_nondust_htlcs,
+			   bool must_afford_fee)
 {
 	uint64_t fee_msat;
 	uint64_t htlcs_total;
@@ -101,8 +102,11 @@ static bool change_funding(uint64_t anchor_satoshis,
 	fee_msat = calculate_fee_msat(num_nondust_htlcs, fee_rate);
 
 	/* If A is paying, can it afford it? */
-	if (htlc_msat > 0) {
-		if (htlc_msat + fee_msat / 2 > a->pay_msat + a->fee_msat)
+	if (htlc_msat >= 0) {
+		uint64_t cost = htlc_msat;
+		if (must_afford_fee)
+			cost += fee_msat / 2;
+		if (cost > a->pay_msat + a->fee_msat)
 			return false;
 	}
 
@@ -148,7 +152,7 @@ struct channel_state *initial_cstate(const tal_t *ctx,
 	funder->fee_msat = fee_msat;
 
 	/* Make sure it checks out. */
-	assert(change_funding(anchor_satoshis, fee_rate, 0, funder, fundee, 0));
+	assert(change_funding(anchor_satoshis, fee_rate, 0, funder, fundee, 0, false));
 	assert(funder->fee_msat == fee_msat);
 	assert(fundee->fee_msat == 0);
 
@@ -194,7 +198,8 @@ bool force_fee(struct channel_state *cstate, uint64_t fee)
 }
 
 /* Add a HTLC to @creator if it can afford it. */
-bool cstate_add_htlc(struct channel_state *cstate, const struct htlc *htlc)
+bool cstate_add_htlc(struct channel_state *cstate, const struct htlc *htlc,
+		     bool must_afford_fee)
 {
 	size_t nondust;
 	struct channel_oneside *creator, *recipient;
@@ -208,7 +213,8 @@ bool cstate_add_htlc(struct channel_state *cstate, const struct htlc *htlc)
 		nondust++;
 	
 	if (!change_funding(cstate->anchor, cstate->fee_rate,
-			    htlc->msatoshis, creator, recipient, nondust))
+			    htlc->msatoshis, creator, recipient, nondust,
+			    must_afford_fee))
 		return false;
 
 	cstate->num_nondust = nondust;
@@ -235,7 +241,7 @@ static void remove_htlc(struct channel_state *cstate,
 	if (!change_funding(cstate->anchor, cstate->fee_rate,
 			    -(int64_t)htlc->msatoshis,
 			    &cstate->side[beneficiary],
-			    &cstate->side[!beneficiary], nondust))
+			    &cstate->side[!beneficiary], nondust, false))
 		abort();
 
 	/* Actually remove the HTLC. */

daemon/channel.h

@@ -58,12 +58,17 @@ struct channel_state *copy_cstate(const tal_t *ctx,
  * cstate_add_htlc: append an HTLC to cstate if it can afford it
  * @cstate: The channel state
  * @htlc: the htlc pointer.
+ * @must_afford_fee: true if payer must meet fee.
  *
- * If that direction can't afford the HTLC (or still owes its half of the fees),
- * this will return false and leave @cstate unchanged.  Otherwise, pay_msat and
- * fee_msat are adjusted accordingly; true is returned.
+ * If that direction can't afford the HTLC this will return false and
+ * leave @cstate unchanged.  If @must_afford_fee is true, and the
+ * direction can't afford its half of the fees, it will also return
+ * false and leave @cstate unchanged. Otherwise, pay_msat and fee_msat
+ * are adjusted accordingly; true is returned.
  */
-bool cstate_add_htlc(struct channel_state *cstate, const struct htlc *htlc);
+bool cstate_add_htlc(struct channel_state *cstate, const struct htlc *htlc,
+		     bool must_afford_fee);
+
 /**
  * cstate_fail_htlc: remove an HTLC, funds to the side which offered it.
  * @cstate: The channel state

daemon/peer.c

@@ -649,7 +649,7 @@ static void retry_all_routing(struct peer *restarted_peer)
 	}
 }
 
-static void adjust_cstate_side(struct channel_state *cstate,
+static bool adjust_cstate_side(struct channel_state *cstate,
 			       struct htlc *h,
 			       enum htlc_state old, enum htlc_state new,
 			       enum htlc_side side)
@@ -662,7 +662,7 @@ static void adjust_cstate_side(struct channel_state *cstate,
 	 * do that again. */
 	if (old == SENT_ADD_HTLC || old == RCVD_REMOVE_HTLC
 	    || old == RCVD_ADD_HTLC || old == SENT_REMOVE_HTLC)
-		return;
+		return true;
 	
 	old_committed = (oldf & HTLC_FLAG(side, HTLC_F_COMMITTED));
 	new_committed = (newf & HTLC_FLAG(side, HTLC_F_COMMITTED));
@@ -673,20 +673,28 @@ static void adjust_cstate_side(struct channel_state *cstate,
 		else
 			cstate_fail_htlc(cstate, h);
 	} else if (!old_committed && new_committed) {
-		/* FIXME: This can happen; see BOLT */
-		if (!cstate_add_htlc(cstate, h))
-			fatal("Could not afford htlc");
+		if (!cstate_add_htlc(cstate, h, false)) {
+			log_broken_struct(h->peer->log,
+					  "Cannot afford htlc %s",
+					  struct htlc, h);
+			log_add_struct(h->peer->log, " channel state %s",
+				       struct channel_state, cstate);
+			return false;
+		}
 	}
+	return true;
 }
 
 /* We apply changes to staging_cstate when we first PENDING, so we can
  * make sure they're valid.  So here we change the staging_cstate on
  * the revocation receive (ie. when acked). */
-static void adjust_cstates(struct peer *peer, struct htlc *h,
+static bool adjust_cstates(struct peer *peer, struct htlc *h,
 			   enum htlc_state old, enum htlc_state new)
 {
-	adjust_cstate_side(peer->remote.staging_cstate, h, old, new, REMOTE);
-	adjust_cstate_side(peer->local.staging_cstate, h, old, new, LOCAL);
+	return adjust_cstate_side(peer->remote.staging_cstate, h, old, new,
+				  REMOTE)
+		&& adjust_cstate_side(peer->local.staging_cstate, h, old, new,
+				      LOCAL);
 }
 
 static void adjust_cstate_fee_side(struct channel_state *cstate,
@@ -767,8 +775,9 @@ static const char *changestates(struct peer *peer,
 	     h = htlc_map_next(&peer->htlcs, &it)) {
 		for (i = 0; i < n; i++) {
 			if (h->state == table[i].from) {
-				adjust_cstates(peer, h,
-					       table[i].from, table[i].to);
+				if (!adjust_cstates(peer, h,
+						    table[i].from, table[i].to))
+					return "accounting error";
 				if (!htlc_changestate(h, table[i].from,
 						      table[i].to, db_commit))
 					return "database error";
@@ -1081,11 +1090,15 @@ static Pkt *handle_pkt_htlc_add(struct peer *peer, const Pkt *pkt)
 	 * "Fee Calculation" ).  A node SHOULD fail the connection if
 	 * this occurs.
 	 */
-	if (!cstate_add_htlc(peer->local.staging_cstate, htlc)) {
+	if (!cstate_add_htlc(peer->local.staging_cstate, htlc, true)) {
+		u64 id = htlc->id;
+		log_broken_struct(peer->log, "They cannot afford htlc %s",
+				  struct htlc, htlc);
+		log_add_struct(peer->log, " cstate %s",
+			       struct channel_state,
+			       peer->local.staging_cstate);
 		tal_free(htlc);
-		return pkt_err(peer, "Cannot afford %"PRIu64" milli-satoshis"
-			       " in our commitment tx",
-			       htlc->msatoshis);
+		return pkt_err(peer, "Cannot afford htlc %"PRIu64, id);
 	}
 	return NULL;
 }
@@ -1705,7 +1718,6 @@ const char *command_htlc_add(struct peer *peer, u64 msatoshis,
 			     u32 *error_code,
 			     struct htlc **htlc)
 {
-	struct channel_state *cstate;
 	struct abs_locktime locktime;
 
 	if (!blocks_to_abs_locktime(expiry, &locktime)) {
@@ -1750,42 +1762,27 @@ const char *command_htlc_add(struct peer *peer, u64 msatoshis,
 			      msatoshis, rhash, expiry, route, tal_count(route),
 			      src, SENT_ADD_HTLC);
 
-	/* FIXME: BOLT is not correct here: we should say IFF we cannot
-	 * afford it in remote at its own current proposed fee-rate. */
-	/* BOLT #2:
-	 *
-	 * A node MUST NOT offer `amount_msat` it cannot pay for in
-	 * the remote commitment transaction at the current `fee_rate`
-	 */
-	cstate = copy_cstate(peer, peer->remote.staging_cstate);
-	if (!cstate_add_htlc(cstate, *htlc)) {
-		log_unusual(peer->log, "add_htlc: fail: Cannot afford %"PRIu64
-			    " milli-satoshis in their commit tx",
-			    msatoshis);
-		*htlc = tal_free(*htlc);
-		*error_code = SERVICE_UNAVAILABLE_503;
-		return "cannot afford htlc";
-	}
-	tal_free(cstate);
-
-	cstate = copy_cstate(peer, peer->local.staging_cstate);
-	if (!cstate_add_htlc(cstate, *htlc)) {
-		log_unusual(peer->log, "add_htlc: fail: Cannot afford %"PRIu64
-			    " milli-satoshis in our commit tx",
-			    msatoshis);
-		*htlc = tal_free(*htlc);
-		*error_code = SERVICE_UNAVAILABLE_503;
-		return "cannot afford htlc";
-	}
-	tal_free(cstate);
-
 	/* BOLT #2:
 	 *
 	 * The sending node MUST add the HTLC addition to the unacked
 	 * changeset for its remote commitment
 	 */
-	if (!cstate_add_htlc(peer->remote.staging_cstate, *htlc))
-		fatal("Could not add HTLC?");
+	if (!cstate_add_htlc(peer->remote.staging_cstate, *htlc, true)) {
+		/* BOLT #2:
+		 *
+		 * A node MUST NOT offer `amount_msat` it cannot pay for in
+		 * the remote commitment transaction at the current `fee_rate`
+		 */
+ 		log_unusual(peer->log, "add_htlc: fail: Cannot afford %"PRIu64
+ 			    " milli-satoshis in their commit tx",
+ 			    msatoshis);
+		log_add_struct(peer->log, " channel state %s",
+			       struct channel_state,
+			       peer->remote.staging_cstate);
+ 		*htlc = tal_free(*htlc);
+		*error_code = SERVICE_UNAVAILABLE_503;
+		return "cannot afford htlc";
+ 	}
 
 	remote_changes_pending(peer);