channeld: fix invalid assumption in htlc restore.

A long time ago (93dcd5f), I
simplified the htlc reload code so it adjusted the amounts for HTLCs
in id order.  As we presumably allowed them to be added in that order,
this avoided special-casing overflow (which was about to deliberately
be made harder by the new amount_msat code).

Unfortunately, htlc id order is not canonical, since htlc ids are
assigned consecutively in both directions!  Concretely, we can have two HTLCs:

	HTLC #0 LOCAL->REMOTE: 500,000,000 msat, state RCVD_REMOVE_REVOCATION
	HTLC #0 REMOTE->LOCAL: 10,000 msat, state SENT_ADD_COMMIT

On a new remote-funded channel, in which we have 0 balance, these
commits *only* work in this order.  Sorting by HTLC ID is not enough!
In fact, we'd have to worry about redemption order as well, as that
matters.

So, regretfully, we offset the balances halfway to UINT64_MAX, then check
they didn't underflow at the end.  This loses us this one sanity check,
but that's probably OK.

Signed-off-by: Rusty Russell <[email protected]>

channeld/full_channel.c

@@ -1164,6 +1164,40 @@ static bool adjust_balance(struct channel *channel, struct htlc *htlc)
 	return true;
 }
 
+static bool offset_balances(struct channel *channel)
+{
+	for (enum side view = LOCAL; view < NUM_SIDES; view++) {
+		for (enum side side = LOCAL; side < NUM_SIDES; side++) {
+			struct amount_msat *a = &channel->view[view].owed[side];
+			if (amount_msat_add(a, *a, AMOUNT_MSAT((u64)1 << 63)))
+				continue;
+
+			status_broken("Can't offset %s",
+				      type_to_string(tmpctx, struct amount_msat,
+						     a));
+			return false;
+		}
+	}
+	return true;
+}
+
+static bool unoffset_balances(struct channel *channel)
+{
+	for (enum side view = LOCAL; view < NUM_SIDES; view++) {
+		for (enum side side = LOCAL; side < NUM_SIDES; side++) {
+			struct amount_msat *a = &channel->view[view].owed[side];
+			if (amount_msat_sub(a, *a, AMOUNT_MSAT((u64)1 << 63)))
+				continue;
+
+			status_broken("Can't unoffset %s",
+				      type_to_string(tmpctx, struct amount_msat,
+						     a));
+			return false;
+		}
+	}
+	return true;
+}
+
 bool channel_force_htlcs(struct channel *channel,
 			 const struct added_htlc *htlcs,
 			 const enum htlc_state *hstates,
@@ -1317,16 +1351,21 @@ bool channel_force_htlcs(struct channel *channel,
 			htlc->failed_scid = NULL;
 	}
 
-	/* Now adjust balances.  The balance never goes negative, because
-	 * we do them in id order. */
+	/* Add giant offset so we never go negative here. */
+	if (!offset_balances(channel))
+		return false;
+
+	/* You'd think, since we traverse HTLCs in ID order, this would never
+	 * go negative.  But this ignores the fact that HTLCs ids from each
+	 * side have no correlation with each other. */
 	for (htlc = htlc_map_first(channel->htlcs, &it);
 	     htlc;
 	     htlc = htlc_map_next(channel->htlcs, &it)) {
 		if (!adjust_balance(channel, htlc))
 			return false;
 	}
 
-	return true;
+	return unoffset_balances(channel);
 }
 
 const char *channel_add_err_name(enum channel_add_err e)

tests/test_connection.py

@@ -2198,7 +2198,6 @@ def test_feerate_stress(node_factory, executor):
     assert not l2.daemon.is_in_log('Bad.*signature')
 
 
-@pytest.mark.xfail(strict=True)
 @unittest.skipIf(not DEVELOPER, "need dev_disconnect")
 def test_pay_disconnect_stress(node_factory, executor):
     """Expose race in htlc restoration in channeld: 50% chance of failure"""