Fix listpays still pending when using xpay/injectpaymentonion

lightningd/pay.c

@@ -2076,14 +2076,6 @@ static struct command_result *json_injectpaymentonion(struct command *cmd,
 	if (command_check_only(cmd))
 		return command_check_done(cmd);
 
-	register_payment_and_waiter(cmd,
-				    payment_hash,
-				    *partid, *groupid,
-				    *destination_msat, *msat, AMOUNT_MSAT(0),
-				    label, invstring, local_invreq_id,
-				    &shared_secret,
-				    destination);
-
 	/* If unknown, we set this equal (so accounting logs 0 fees) */
 	if (amount_msat_eq(*destination_msat, AMOUNT_MSAT(0)))
 		*destination_msat = *msat;
@@ -2098,6 +2090,16 @@ static struct command_result *json_injectpaymentonion(struct command *cmd,
 				    "Could not send to first peer: %s",
 				    onion_wire_name(fromwire_peektype(failmsg)));
 	}
+
+	/* Now HTLC is created, we can add the payment as pending */
+	register_payment_and_waiter(cmd,
+				    payment_hash,
+				    *partid, *groupid,
+				    *destination_msat, *msat, AMOUNT_MSAT(0),
+				    label, invstring, local_invreq_id,
+				    &shared_secret,
+				    destination);
+
 	return command_still_pending(cmd);
 }
 

tests/test_pay.py

@@ -6755,6 +6755,49 @@ def test_injectpaymentonion_failures(node_factory, executor):
     assert 'onionreply' in err.value.error['data']
 
 
+def test_injectpaymentonion_peerfail(node_factory, executor):
+    l1, l2 = node_factory.line_graph(2,
+                                     opts=[{'may_reconnect': True,
+                                            'dev-no-reconnect': None,
+                                            'disconnect': ['=WIRE_UPDATE_ADD_HTLC', '-WIRE_COMMITMENT_SIGNED']},
+                                           {'may_reconnect': True,
+                                            'dev-no-reconnect': None}])
+    blockheight = l1.rpc.getinfo()['blockheight']
+
+    inv1 = l2.rpc.invoice(1000, "test_injectpaymentonion_peerfail", "test_injectpaymentonion_peerfail")
+
+    # First hop for injectpaymentonion is self.
+    hops = [{'pubkey': l1.info['id'],
+             'payload': serialize_payload_tlv(1000, 18 + 6, first_scid(l1, l2), blockheight).hex()},
+            {'pubkey': l2.info['id'],
+             'payload': serialize_payload_final_tlv(1000, 18, 1000, blockheight, inv1['payment_secret']).hex()}]
+    onion = l1.rpc.createonion(hops=hops, assocdata=inv1['payment_hash'])
+
+    l1.rpc.disconnect(l2.info['id'], force=True)
+    with pytest.raises(RpcError, match='WIRE_TEMPORARY_CHANNEL_FAILURE'):
+        l1.rpc.injectpaymentonion(onion=onion['onion'],
+                                  payment_hash=inv1['payment_hash'],
+                                  amount_msat=1000,
+                                  cltv_expiry=blockheight + 18 + 6,
+                                  partid=1,
+                                  groupid=0)
+    # In fact, it won't create any sendpays entry, since it fails too early.
+    assert l1.rpc.listsendpays() == {'payments': []}
+
+    # This will hang, since we disconnect once committed.  But provides another
+    # (legitimately) pending payment for our migration code to test.
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+    executor.submit(l1.rpc.injectpaymentonion,
+                    onion=onion['onion'],
+                    payment_hash=inv1['payment_hash'],
+                    amount_msat=1000,
+                    cltv_expiry=blockheight + 18 + 6,
+                    partid=2,
+                    groupid=0)
+    l1.daemon.wait_for_log("dev_disconnect: =WIRE_UPDATE_ADD_HTLC")
+    assert [p['status'] for p in l1.rpc.listsendpays()['payments']] == ['pending']
+
+
 def test_parallel_channels_reserve(node_factory, bitcoind):
     """Tests wether we are able to pay through parallel channels concurrently.
     To do that we need to enable strict-forwarding."""

tests/test_wallet.py

@@ -2467,3 +2467,12 @@ def test_old_htlcs_cleanup(node_factory, bitcoind):
     # Now they're not
     assert l1.db_query('SELECT COUNT(*) as c FROM channel_htlcs')[0]['c'] == 0
     assert l1.rpc.listhtlcs() == {'htlcs': []}
+
+
+@unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "Makes use of the sqlite3 db")
+@unittest.skipIf(TEST_NETWORK != 'regtest', "sqlite3 snapshot is regtest")
+def test_pending_payments_cleanup(node_factory, bitcoind):
+    bitcoind.generate_block(1)
+    l1 = node_factory.get_node(dbfile='l1-pending-sendpays-with-no-htlc.sqlite3.xz', options={'database-upgrade': True})
+    assert [p['status'] for p in l1.rpc.listsendpays()['payments']] == ['failed', 'pending']
+    assert [p['status'] for p in l1.rpc.listpays()['pays']] == ['pending']

wallet/db.c

@@ -84,6 +84,8 @@ static void migrate_convert_old_channel_keyidx(struct lightningd *ld,
 					       struct db *db);
 static void migrate_initialize_channel_htlcs_wait_indexes_and_fixup_forwards(struct lightningd *ld,
 									     struct db *db);
+static void migrate_fail_pending_payments_without_htlcs(struct lightningd *ld,
+							struct db *db);
 
 /* Do not reorder or remove elements from this array, it is used to
  * migrate existing databases from a previous state, based on the
@@ -1087,7 +1089,8 @@ static struct migration dbmigrations[] = {
     {SQL("CREATE INDEX chain_moves_utxo_idx ON chain_moves (utxo)"), NULL},
     {NULL, migrate_from_account_db},
     /* We accidentally allowed duplicate entries */
-    {NULL, migrate_remove_chain_moves_duplicates}
+    {NULL, migrate_remove_chain_moves_duplicates},
+    {NULL, migrate_fail_pending_payments_without_htlcs},
 };
 
 /**
@@ -2117,3 +2120,25 @@ static void migrate_convert_old_channel_keyidx(struct lightningd *ld,
 	db_bind_int(stmt, channel_state_in_db(CLOSED));
 	db_exec_prepared_v2(take(stmt));
 }
+
+static void migrate_fail_pending_payments_without_htlcs(struct lightningd *ld,
+							struct db *db)
+{
+	/* If channeld died or was offline at the right moment, we
+	 * could register a payment as pending, but then not create an
+	 * HTLC.  Clean those up. */
+	struct db_stmt *stmt;
+
+	stmt = db_prepare_v2(db, SQL("UPDATE payments AS p"
+				     " SET status = ?"
+				     " WHERE p.status = ?"
+				     "   AND NOT EXISTS ("
+				     "     SELECT 1"
+				     "     FROM channel_htlcs AS h"
+				     "     WHERE h.payment_hash = p.payment_hash"
+				     "       AND h.groupid = p.groupid"
+				     "       AND h.partid  = p.partid);"));
+	db_bind_int(stmt, payment_status_in_db(PAYMENT_FAILED));
+	db_bind_int(stmt, payment_status_in_db(PAYMENT_PENDING));
+	db_exec_prepared_v2(take(stmt));
+}