lightningd: store raw failure message so waitsendpay always has raw_message

If a payment's HTLC failure completes before waitsendpay is called,
wait_payment() rebuilds the error from the database, which did not
persist the raw BOLT4 failure message: a 2019 FIXME in that path set
fail->msg = NULL, silently dropping raw_message from the error data.
This is the cause of the test_error_returns_blockheight CI flake -
the test loses the race occasionally, calls waitsendpay after the
failure has landed, and KeyErrors on the missing raw_message.

Add a failmsg column to the payments table (with a downgrade drop),
persist fail->msg when recording the failure, and read it back in
wait_payment.  Local and self-payment failures store NULL as before,
since no onion failure message exists for them; failed payments
recorded before this migration also return NULL, matching the old
behavior.

The flaky test now also calls waitsendpay a second time, which
deterministically takes the database-replay path, so the regression
is covered without any timing dependence.

Changelog-Fixed: JSON-RPC: `waitsendpay` error data now includes `raw_message` even when the payment already failed before the command was called.
Fixes: #9341

Author: ksedgwic

lightningd/pay.c

@@ -652,7 +652,8 @@ void payment_failed(struct lightningd *ld,
 				    fail ? fail->erring_channel : NULL,
 				    NULL,
 				    failstr,
-				    fail ? fail->channel_dir : 0);
+				    fail ? fail->channel_dir : 0,
+				    fail ? fail->msg : NULL);
 
 	tell_waiters_failed(ld, payment_hash, payment, pay_errcode,
 			    failonion, fail, failstr);
@@ -675,6 +676,7 @@ static struct command_result *wait_payment(struct lightningd *ld,
 	struct short_channel_id *failchannel;
 	u8 *failupdate;
 	char *faildetail;
+	u8 *failmsg;
 	struct routing_failure *fail;
 	int faildirection;
 	enum jsonrpc_errcode rpcerrorcode;
@@ -715,7 +717,8 @@ static struct command_result *wait_payment(struct lightningd *ld,
 					    &failchannel,
 					    &failupdate,
 					    &faildetail,
-					    &faildirection);
+					    &faildirection,
+					    &failmsg);
 		/* Old DB might not save failure information */
 		if (!failonionreply && !failnode) {
 			return command_fail(cmd, PAY_UNSPECIFIED_ERROR,
@@ -745,8 +748,7 @@ static struct command_result *wait_payment(struct lightningd *ld,
 				fail->erring_channel = NULL;
 			}
 
-			/* FIXME: We don't store this! */
-			fail->msg = NULL;
+			fail->msg = tal_dup_talarr(fail, u8, failmsg);
 
 			/* Peers which fail directly can hit this! */
 			if (failcode & BADONION)
@@ -1533,7 +1535,7 @@ static struct command_result *self_payment(struct lightningd *ld,
 					    fail->failcode, fail->erring_node,
 					    NULL, NULL,
 					    err,
-					    0);
+					    0, NULL);
 		/* We do this even though there really can't be any waiters,
 		 * since we didn't block. */
 		tell_waiters_failed(ld, rhash, payment, PAY_DESTINATION_PERM_FAIL,

tests/test_pay.py

@@ -2888,6 +2888,15 @@ def test_error_returns_blockheight(node_factory, bitcoind):
     assert (err.value.error['data']['raw_message']
             == '400f{:016x}{:08x}'.format(100, bitcoind.rpc.getblockcount()))
 
+    # A second waitsendpay replays the failure from the database (the
+    # path a waitsendpay racing the failure takes): raw_message must
+    # survive that round trip too.
+    with pytest.raises(RpcError, match=r"INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS.*'erring_index': 1") as err:
+        l1.rpc.waitsendpay('00' * 32, TIMEOUT)
+
+    assert (err.value.error['data']['raw_message']
+            == '400f{:016x}{:08x}'.format(100, bitcoind.rpc.getblockcount()))
+
 
 @unittest.skipIf(TEST_NETWORK != 'regtest', "Invoice is network specific")
 def test_pay_no_secret(node_factory, bitcoind):

wallet/migrations.c

@@ -1182,6 +1182,10 @@ static const struct db_migration dbmigrations[] = {
      * writes stop in the release that removes chaintopology, freezing all
      * the legacy tables at the same height. */
     {NULL, migrate_backfill_bwatch_tables, NULL, NULL},
+    /* Raw BOLT4 failure message, so waitsendpay can report it even
+     * after the failure was recorded (issue #9341). */
+    {SQL("ALTER TABLE payments ADD failmsg BLOB;"), NULL,
+     SQL("ALTER TABLE payments DROP COLUMN failmsg"), NULL},
     /* ^v26.09 */
 };
 

wallet/wallet.c

@@ -4322,7 +4322,8 @@ void wallet_payment_get_failinfo(const tal_t *ctx,
 				 struct short_channel_id **failchannel,
 				 u8 **failupdate,
 				 char **faildetail,
-				 int *faildirection)
+				 int *faildirection,
+				 u8 **failmsg)
 {
 	struct db_stmt *stmt;
 	bool resb;
@@ -4332,6 +4333,7 @@ void wallet_payment_get_failinfo(const tal_t *ctx,
 				 ", failindex, failcode"
 				 ", failnode, failscid"
 				 ", failupdate, faildetail, faildirection"
+				 ", failmsg"
 				 "  FROM payments"
 				 " WHERE payment_hash=? AND partid=? AND groupid=?;"));
 	db_bind_sha256(stmt, payment_hash);
@@ -4366,6 +4368,10 @@ void wallet_payment_get_failinfo(const tal_t *ctx,
 		*faildetail = db_col_strdup(ctx, stmt, "faildetail");
 	else
 		*faildetail = NULL;
+	if (db_col_is_null(stmt, "failmsg"))
+		*failmsg = NULL;
+	else
+		*failmsg = db_col_arr(ctx, stmt, "failmsg", u8);
 
 	tal_free(stmt);
 }
@@ -4381,7 +4387,8 @@ void wallet_payment_set_failinfo(struct wallet *wallet,
 				 const struct short_channel_id *failchannel,
 				 const u8 *failupdate /*tal_arr*/,
 				 const char *faildetail,
-				 int faildirection)
+				 int faildirection,
+				 const u8 *failmsg /*tal_arr*/)
 {
 	struct db_stmt *stmt;
 
@@ -4395,6 +4402,7 @@ void wallet_payment_set_failinfo(struct wallet *wallet,
 					     "     , faildirection=?"
 					     "     , failupdate=?"
 					     "     , faildetail=?"
+					     "     , failmsg=?"
 					     " WHERE payment_hash=?"
 					     " AND partid=?;"));
 	if (failonionreply)
@@ -4425,6 +4433,8 @@ void wallet_payment_set_failinfo(struct wallet *wallet,
 	else
 		db_bind_null(stmt);
 
+	db_bind_talarr(stmt, failmsg);
+
 	db_bind_sha256(stmt, payment_hash);
 	db_bind_u64(stmt, partid);
 

wallet/wallet.h

@@ -1049,7 +1049,10 @@ void wallet_payment_set_status(struct wallet *wallet,
  * `payment_hash`.
  *
  * Data is allocated as children of the given context. *faildirection
- * is only set if *failchannel is set non-NULL.
+ * is only set if *failchannel is set non-NULL. *failmsg is NULL when
+ * no raw onion failure message was recorded (local and self-payment
+ * failures, or payments that failed before the failmsg column
+ * existed).
  */
 void wallet_payment_get_failinfo(const tal_t *ctx,
 				 struct wallet *wallet,
@@ -1065,7 +1068,8 @@ void wallet_payment_get_failinfo(const tal_t *ctx,
 				 struct short_channel_id **failchannel,
 				 u8 **failupdate,
 				 char **faildetail,
-				 int *faildirection);
+				 int *faildirection,
+				 u8 **failmsg);
 /**
  * wallet_payment_set_failinfo - Set failure information for a given
  * `payment_hash`.
@@ -1081,7 +1085,8 @@ void wallet_payment_set_failinfo(struct wallet *wallet,
 				 const struct short_channel_id *failchannel,
 				 const u8 *failupdate,
 				 const char *faildetail,
-				 int faildirection);
+				 int faildirection,
+				 const u8 *failmsg);
 
 /**
  * payments_first: get first payment, optionally filtering by status