lightningd: fix 209 "Payment failure reason unknown" for sendonion payments

contrib/msggen/msggen/schema.json

@@ -18584,10 +18584,12 @@
         "A routing failure object has the fields below:",
         "",
         "*erring_index*: The index of the node along the route that reported the error. 0 for the local node, 1 for the first hop, and so on.",
-        "*erring_node*: The hex string of the pubkey id of the node that reported the error.",
-        "*erring_channel*: The short channel ID of the channel that has the error, or *0:0:0* if the destination node raised the error.",
+        "*erring_node*: The hex string of the pubkey id of the node that reported the error. This is inferred from the route, so it may be absent in rare cases.",
+        "*erring_channel*: The short channel ID of the channel that has the error. Only present if the channel is known.",
+        "*erring_direction*: The direction of traversing the *erring_channel*. Only present with *erring_channel*.",
         "*failcode*: The failure code, as per BOLT #4.",
-        "*channel_update*: The hex string of the *channel_update* message received from the remote node. Only present if error is from the remote node and the *failcode* has the `UPDATE` bit set, as per BOLT #4."
+        "*failcodename*: The human-readable name corresponding to *failcode*, if known.",
+        "*raw_message*: The hex string of the raw failure message from the node that reported the error. Not present for failures that did not come from a node, such as a local routing failure."
       ],
       "author": [
         "Christian Decker [decker@blockstream.com](mailto:decker@blockstream.com) is mainly responsible."
@@ -32846,10 +32848,12 @@
         "A routing failure object has the fields below:",
         "",
         "*erring_index*: The index of the node along the route that reported the error. 0 for the local node, 1 for the first hop, and so on.",
-        "*erring_node*: The hex string of the pubkey id of the node that reported the error.",
-        "*erring_channel*: The short channel ID of the channel that has the error, or *0:0:0* if the destination node raised the error.",
+        "*erring_node*: The hex string of the pubkey id of the node that reported the error. This is inferred from the route, so it may be absent in rare cases.",
+        "*erring_channel*: The short channel ID of the channel that has the error. Only present if the channel is known.",
+        "*erring_direction*: The direction of traversing the *erring_channel*. Only present with *erring_channel*.",
         "*failcode*: The failure code, as per BOLT #4.",
-        "*channel_update*: The hex string of the *channel_update* message received from the remote node. Only present if error is from the remote node and the *failcode* has the UPDATE bit set, as per BOLT #4.",
+        "*failcodename*: The human-readable name corresponding to *failcode*, if known.",
+        "*raw_message*: The hex string of the raw failure message from the node that reported the error. Not present for failures that did not come from a node, such as a local routing failure.",
         "",
         "The *data* field of errors will include statistics *getroute_tries* and *sendpay_tries*. It will also contain a *failures* field with detailed data about routing errors."
       ],
@@ -35794,9 +35798,11 @@
         "",
         "*erring_index*: The index of the node along the route that reported the error. 0 for the local node, 1 for the first hop, and so on.",
         "*erring_node*: The hex string of the pubkey id of the node that reported the error.",
-        "*erring_channel*: The short channel ID of the channel that has the error, or *0:0:0* if the destination node raised the error. In addition *erring_direction* will indicate which direction of the channel caused the failure.",
+        "*erring_channel*: The short channel ID of the channel that has the error. Only present if the channel is known.",
+        "*erring_direction*: The direction of traversing the *erring_channel*. Only present with *erring_channel*.",
         "*failcode*: The failure code, as per BOLT #4.",
-        "*channel_update*: The hex string of the *channel_update* message received from the remote node. Only present if error is from the remote node and the *failcode* has the UPDATE bit set, as per BOLT #4."
+        "*failcodename*: The human-readable name corresponding to *failcode*, if known.",
+        "*raw_message*: The hex string of the raw failure message from the node that reported the error. Not present for failures that did not come from a node, such as a local routing failure."
       ],
       "author": [
         "Rusty Russell [rusty@rustcorp.com.au](mailto:rusty@rustcorp.com.au) is mainly responsible."
@@ -40975,11 +40981,12 @@
         "A routing failure object has the fields below:",
         "",
         "*erring_index*: The index of the node along the route that reported the error. 0 for the local node, 1 for the first hop, and so on.",
-        "*erring_node*: The hex string of the pubkey id of the node that reported the error.",
-        "*erring_channel*: The short channel ID of the channel that has the error (or the final channel if the destination raised the error).",
-        "*erring_direction*: The direction of traversing the *erring_channel*:",
+        "*erring_node*: The hex string of the pubkey id of the node that reported the error. Only present if the node is known: a payment sent with **sendonion** does not record it.",
+        "*erring_channel*: The short channel ID of the channel that has the error. Only present if the channel is known.",
+        "*erring_direction*: The direction of traversing the *erring_channel*. Only present with *erring_channel*.",
         "*failcode*: The failure code, as per BOLT #4.",
-        "*failcodename*: The human-readable name corresponding to *failcode*, if known."
+        "*failcodename*: The human-readable name corresponding to *failcode*, if known.",
+        "*raw_message*: The hex string of the raw failure message from the node that reported the error. Not present for failures that did not come from a node, such as a local routing failure."
       ],
       "author": [
         "ZmnSCPxj [ZmnSCPxj@protonmail.com](mailto:ZmnSCPxj@protonmail.com) is mainly responsible."
@@ -42905,7 +42912,7 @@
               "erring_direction": {
                 "type": "u32",
                 "description": [
-                  "The direction within the erring channel."
+                  "The direction within the erring channel. Only present with *erring_channel*."
                 ]
               },
               "raw_message": {

doc/schemas/keysend.json

@@ -218,10 +218,12 @@
     "A routing failure object has the fields below:",
     "",
     "*erring_index*: The index of the node along the route that reported the error. 0 for the local node, 1 for the first hop, and so on.",
-    "*erring_node*: The hex string of the pubkey id of the node that reported the error.",
-    "*erring_channel*: The short channel ID of the channel that has the error, or *0:0:0* if the destination node raised the error.",
+    "*erring_node*: The hex string of the pubkey id of the node that reported the error. This is inferred from the route, so it may be absent in rare cases.",
+    "*erring_channel*: The short channel ID of the channel that has the error. Only present if the channel is known.",
+    "*erring_direction*: The direction of traversing the *erring_channel*. Only present with *erring_channel*.",
     "*failcode*: The failure code, as per BOLT #4.",
-    "*channel_update*: The hex string of the *channel_update* message received from the remote node. Only present if error is from the remote node and the *failcode* has the `UPDATE` bit set, as per BOLT #4."
+    "*failcodename*: The human-readable name corresponding to *failcode*, if known.",
+    "*raw_message*: The hex string of the raw failure message from the node that reported the error. Not present for failures that did not come from a node, such as a local routing failure."
   ],
   "author": [
     "Christian Decker [decker@blockstream.com](mailto:decker@blockstream.com) is mainly responsible."

doc/schemas/notification/sendpay_failure.json

@@ -190,7 +190,7 @@
           "erring_direction": {
             "type": "u32",
             "description": [
-              "The direction within the erring channel."
+              "The direction within the erring channel. Only present with *erring_channel*."
             ]
           },
           "raw_message": {

doc/schemas/pay.json

@@ -226,10 +226,12 @@
     "A routing failure object has the fields below:",
     "",
     "*erring_index*: The index of the node along the route that reported the error. 0 for the local node, 1 for the first hop, and so on.",
-    "*erring_node*: The hex string of the pubkey id of the node that reported the error.",
-    "*erring_channel*: The short channel ID of the channel that has the error, or *0:0:0* if the destination node raised the error.",
+    "*erring_node*: The hex string of the pubkey id of the node that reported the error. This is inferred from the route, so it may be absent in rare cases.",
+    "*erring_channel*: The short channel ID of the channel that has the error. Only present if the channel is known.",
+    "*erring_direction*: The direction of traversing the *erring_channel*. Only present with *erring_channel*.",
     "*failcode*: The failure code, as per BOLT #4.",
-    "*channel_update*: The hex string of the *channel_update* message received from the remote node. Only present if error is from the remote node and the *failcode* has the UPDATE bit set, as per BOLT #4.",
+    "*failcodename*: The human-readable name corresponding to *failcode*, if known.",
+    "*raw_message*: The hex string of the raw failure message from the node that reported the error. Not present for failures that did not come from a node, such as a local routing failure.",
     "",
     "The *data* field of errors will include statistics *getroute_tries* and *sendpay_tries*. It will also contain a *failures* field with detailed data about routing errors."
   ],

doc/schemas/sendpay.json

@@ -364,9 +364,11 @@
     "",
     "*erring_index*: The index of the node along the route that reported the error. 0 for the local node, 1 for the first hop, and so on.",
     "*erring_node*: The hex string of the pubkey id of the node that reported the error.",
-    "*erring_channel*: The short channel ID of the channel that has the error, or *0:0:0* if the destination node raised the error. In addition *erring_direction* will indicate which direction of the channel caused the failure.",
+    "*erring_channel*: The short channel ID of the channel that has the error. Only present if the channel is known.",
+    "*erring_direction*: The direction of traversing the *erring_channel*. Only present with *erring_channel*.",
     "*failcode*: The failure code, as per BOLT #4.",
-    "*channel_update*: The hex string of the *channel_update* message received from the remote node. Only present if error is from the remote node and the *failcode* has the UPDATE bit set, as per BOLT #4."
+    "*failcodename*: The human-readable name corresponding to *failcode*, if known.",
+    "*raw_message*: The hex string of the raw failure message from the node that reported the error. Not present for failures that did not come from a node, such as a local routing failure."
   ],
   "author": [
     "Rusty Russell [rusty@rustcorp.com.au](mailto:rusty@rustcorp.com.au) is mainly responsible."

doc/schemas/waitsendpay.json

@@ -217,11 +217,12 @@
     "A routing failure object has the fields below:",
     "",
     "*erring_index*: The index of the node along the route that reported the error. 0 for the local node, 1 for the first hop, and so on.",
-    "*erring_node*: The hex string of the pubkey id of the node that reported the error.",
-    "*erring_channel*: The short channel ID of the channel that has the error (or the final channel if the destination raised the error).",
-    "*erring_direction*: The direction of traversing the *erring_channel*:",
+    "*erring_node*: The hex string of the pubkey id of the node that reported the error. Only present if the node is known: a payment sent with **sendonion** does not record it.",
+    "*erring_channel*: The short channel ID of the channel that has the error. Only present if the channel is known.",
+    "*erring_direction*: The direction of traversing the *erring_channel*. Only present with *erring_channel*.",
     "*failcode*: The failure code, as per BOLT #4.",
-    "*failcodename*: The human-readable name corresponding to *failcode*, if known."
+    "*failcodename*: The human-readable name corresponding to *failcode*, if known.",
+    "*raw_message*: The hex string of the raw failure message from the node that reported the error. Not present for failures that did not come from a node, such as a local routing failure."
   ],
   "author": [
     "ZmnSCPxj [ZmnSCPxj@protonmail.com](mailto:ZmnSCPxj@protonmail.com) is mainly responsible."

lightningd/pay.c

@@ -720,7 +720,7 @@ static struct command_result *wait_payment(struct lightningd *ld,
 					    &faildirection,
 					    &failmsg);
 		/* Old DB might not save failure information */
-		if (!failonionreply && !failnode) {
+		if (!failonionreply && !failnode && !failmsg) {
 			return command_fail(cmd, PAY_UNSPECIFIED_ERROR,
 					    "Payment failure reason unknown");
 		} else if (failonionreply) {
@@ -732,13 +732,14 @@ static struct command_result *wait_payment(struct lightningd *ld,
 					       NULL, faildetail),
 			    NULL);
 		} else {
-			/* Parsed onion error, get its details */
-			assert(failnode);
+			/* Parsed onion error, get its details. A sendonion
+			 * failure has no erring node, only the failmsg. */
+			assert(failnode || failmsg);
 			fail = tal(tmpctx, struct routing_failure);
 			fail->erring_index = failindex;
 			fail->failcode = failcode;
 			fail->erring_node =
-			    tal_dup(fail, struct node_id, failnode);
+			    tal_dup_or_null(fail, struct node_id, failnode);
 
 			if (failchannel) {
 				fail->erring_channel = tal_dup(

plugins/keysend.c

@@ -75,6 +75,7 @@ static struct command_result *keysend_cb(struct keysend_data *d, struct payment
 		   featurebit method.*/
 
 		if (p->result != NULL &&
+		    p->result->erring_node != NULL &&
 		    node_id_eq(p->route_destination, p->result->erring_node) &&
 		    p->result->failcode == WIRE_INVALID_ONION_PAYLOAD) {
 			return payment_abort(

tests/test_pay.py

@@ -3467,12 +3467,23 @@ def test_sendpay_msatoshi_arg(node_factory):
     assert inv['amount_received_msat'] == Millisatoshi(1001)
 
 
-def test_reject_invalid_payload(node_factory):
+@pytest.mark.parametrize(
+    "wait_for_failure",
+    [
+        False,
+        True,
+    ],
+)
+def test_reject_invalid_payload(node_factory, wait_for_failure):
     """Send an onion payload with an unknown even type.
 
     Recipient l2 should reject it the incoming HTLC with an invalid onion
     payload error.
 
+    With wait_for_failure, waitsendpay runs only after the sender has recorded
+    the failure, so it reads the failure from the database rather than from a
+    live waiter. That path used to report "Payment failure reason unknown".
+
     """
 
     l1, l2 = node_factory.line_graph(2)
@@ -3491,6 +3502,8 @@ def test_reject_invalid_payload(node_factory):
                      first_hop=first_hop,
                      payment_hash=inv['payment_hash'],
                      shared_secrets=onion['shared_secrets'])
+    if wait_for_failure:
+        l1.daemon.wait_for_log(r'htlc failed from 0th node')
     with pytest.raises(RpcError, match=r'WIRE_INVALID_ONION_PAYLOAD'):
         l1.rpc.waitsendpay(inv['payment_hash'])