db: correctly migrate forwards for closed incoming channels.

We have to allow them (as otherwise `fees_collected_msat` in getinfo breaks),
but it means that actually, in_htlc_id might be missing in listforwards
(also, out_htlc_id might be missing, which we didn't catch before).

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

Makefile

@@ -74,7 +74,7 @@ endif
 
 ifeq ($(COMPAT),1)
 # We support compatibility with pre-0.6.
-COMPAT_CFLAGS=-DCOMPAT_V052=1 -DCOMPAT_V060=1 -DCOMPAT_V061=1 -DCOMPAT_V062=1 -DCOMPAT_V070=1 -DCOMPAT_V072=1 -DCOMPAT_V073=1 -DCOMPAT_V080=1 -DCOMPAT_V081=1 -DCOMPAT_V082=1 -DCOMPAT_V090=1 -DCOMPAT_V0100=1
+COMPAT_CFLAGS=-DCOMPAT_V052=1 -DCOMPAT_V060=1 -DCOMPAT_V061=1 -DCOMPAT_V062=1 -DCOMPAT_V070=1 -DCOMPAT_V072=1 -DCOMPAT_V073=1 -DCOMPAT_V080=1 -DCOMPAT_V081=1 -DCOMPAT_V082=1 -DCOMPAT_V090=1 -DCOMPAT_V0100=1 -DCOMPAT_V0121=1
 endif
 
 # (method=thread to support xdist)

cln-grpc/proto/node.proto

@@ -1229,7 +1229,7 @@ message ListforwardsForwards {
 		TLV = 1;
 	}
 	string in_channel = 1;
-	uint64 in_htlc_id = 10;
+	optional uint64 in_htlc_id = 10;
 	Amount in_msat = 2;
 	ListforwardsForwardsStatus status = 3;
 	double received_time = 4;

cln-grpc/src/convert.rs

@@ -900,7 +900,7 @@ impl From<responses::ListforwardsForwards> for pb::ListforwardsForwards {
     fn from(c: responses::ListforwardsForwards) -> Self {
         Self {
             in_channel: c.in_channel.to_string(), // Rule #2 for type short_channel_id
-            in_htlc_id: c.in_htlc_id, // Rule #2 for type u64
+            in_htlc_id: c.in_htlc_id, // Rule #2 for type u64?
             in_msat: Some(c.in_msat.into()), // Rule #2 for type msat
             status: c.status as i32,
             received_time: c.received_time, // Rule #2 for type number

cln-rpc/src/model.rs

@@ -3594,8 +3594,8 @@ pub mod responses {
 	pub struct ListforwardsForwards {
 	    #[serde(alias = "in_channel")]
 	    pub in_channel: ShortChannelId,
-	    #[serde(alias = "in_htlc_id")]
+	    #[serde(alias = "in_htlc_id", skip_serializing_if = "Option::is_none")]
-	    pub in_htlc_id: u64,
+	    pub in_htlc_id: Option<u64>,
 	    #[serde(alias = "in_msat")]
 	    pub in_msat: Amount,
 	    // Path `ListForwards.forwards[].status`

doc/lightning-listforwards.7.md

@@ -25,12 +25,12 @@ RETURN VALUE
 On success, an object containing **forwards** is returned.  It is an array of objects, where each object contains:
 
 - **in\_channel** (short\_channel\_id): the channel that received the HTLC
-- **in\_htlc\_id** (u64): the unique HTLC id the sender gave this
 - **in\_msat** (msat): the value of the incoming HTLC
 - **status** (string): still ongoing, completed, failed locally, or failed after forwarding (one of "offered", "settled", "local_failed", "failed")
 - **received\_time** (number): the UNIX timestamp when this was received
+- **in\_htlc\_id** (u64, optional): the unique HTLC id the sender gave this (not present if incoming channel was closed before ugprade to v22.11)
 - **out\_channel** (short\_channel\_id, optional): the channel that the HTLC (trying to) forward to
-- **out\_htlc\_id** (u64, optional): the unique HTLC id we gave this when sending
+- **out\_htlc\_id** (u64, optional): the unique HTLC id we gave this when sending (may be missing even if out_channel is present, for old forwards before v22.11)
 - **style** (string, optional): Either a legacy onion format or a modern tlv format (one of "legacy", "tlv")
 
 If **out\_msat** is present:
@@ -64,4 +64,4 @@ RESOURCES
 
 Main web site: <https://github.com/ElementsProject/lightning>
 
-[comment]: # ( SHA256STAMP:5b2da52b7f3a28563d0103d3853b9d8f717dc41a9e9c6b395ff19f1b975ca5fd)
+[comment]: # ( SHA256STAMP:15bf997ae8e93ab28b0084d9cc45fc80fb18b2bcf705f690f77617f0b66b069d)

doc/schemas/listforwards.schema.json

@@ -14,7 +14,6 @@
         "required": [
           "in_channel",
           "in_msat",
-          "in_htlc_id",
           "status",
           "received_time"
         ],
@@ -25,7 +24,7 @@
           },
           "in_htlc_id": {
             "type": "u64",
-            "description": "the unique HTLC id the sender gave this"
+            "description": "the unique HTLC id the sender gave this (not present if incoming channel was closed before ugprade to v22.11)"
           },
           "in_msatoshi": {
             "deprecated": true
@@ -54,7 +53,7 @@
           },
           "out_htlc_id": {
             "type": "u64",
-            "description": "the unique HTLC id we gave this when sending"
+            "description": "the unique HTLC id we gave this when sending (may be missing even if out_channel is present, for old forwards before v22.11)"
           },
           "style": {
             "type": "string",
@@ -77,7 +76,6 @@
               "required": [
                 "fee_msat",
                 "out_msat",
-                "out_htlc_id",
                 "out_channel"
               ],
               "properties": {

lightningd/peer_htlcs.c

@@ -2844,7 +2844,11 @@ void json_add_forwarding_object(struct json_stream *response,
 	if (payment_hash)
 		json_add_sha256(response, "payment_hash", payment_hash);
 	json_add_short_channel_id(response, "in_channel", &cur->channel_in);
-	json_add_u64(response, "in_htlc_id", cur->htlc_id_in);
+
+#ifdef COMPAT_V0121
+	if (cur->htlc_id_in != HTLC_INVALID_ID)
+#endif
+		json_add_u64(response, "in_htlc_id", cur->htlc_id_in);
 
 	/* This can be unknown if we failed before channel lookup */
 	if (cur->channel_out.u64 != 0) {

tests/test_db.py

@@ -469,7 +469,6 @@ def test_db_sanity_checks(bitcoind, node_factory):
     assert l1.daemon.is_in_stderr('Wallet sanity check failed')
 
 
-@pytest.mark.xfail(strict=True)
 @unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "Canned db used")
 @unittest.skipIf(not COMPAT, "needs COMPAT to convert obsolete db")
 @unittest.skipIf(TEST_NETWORK != 'regtest', "The DB migration is network specific due to the chain var.")

wallet/db.c

@@ -922,9 +922,7 @@ static struct migration dbmigrations[] = {
 	 ", resolved_time"
 	 ", failcode"
 	 ", forward_style"
-	 " FROM forwarded_payments"
+	 " FROM forwarded_payments"), NULL},
-	 " WHERE"
-	 "  in_htlc_id IS NOT NULL"), NULL},
     {SQL("DROP INDEX forwarded_payments_state;"), NULL},
     {SQL("DROP INDEX forwarded_payments_out_htlc_id;"), NULL},
     {SQL("DROP TABLE forwarded_payments;"), NULL},

wallet/wallet.c

@@ -4635,7 +4635,16 @@ const struct forwarding *wallet_forwarded_payments_get(struct wallet *w,
 		}
 
 		db_col_scid(stmt, "in_channel_scid", &cur->channel_in);
+
+#ifdef COMPAT_V0121
+		/* This can happen due to migration! */
+		if (!db_col_is_null(stmt, "in_htlc_id"))
+			cur->htlc_id_in = db_col_u64(stmt, "in_htlc_id");
+		else
+			cur->htlc_id_in = HTLC_INVALID_ID;
+#else
 		cur->htlc_id_in = db_col_u64(stmt, "in_htlc_id");
+#endif
 
 		if (!db_col_is_null(stmt, "out_channel_scid")) {
 			db_col_scid(stmt, "out_channel_scid", &cur->channel_out);