pay: Handle payment failures resulting from sendonion correctly

We are breaking with a couple of assumptions, namely that we have the
`path_secrets` to decode the error onion. If this happens we just want it to
error out.

lightningd/pay.c

@@ -453,30 +453,25 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout,
 					   &hout->payment_hash));
 		return;
 	}
-
-	/* FIXME: Prior to 299b280f7, we didn't put route_nodes and
-	 * route_channels in db.  If this happens, it's an old payment,
-	 * so we can simply mark it failed in db and return. */
-	if (!payment->route_channels) {
-		log_unusual(hout->key.channel->log,
-			    "No route_channels for htlc %s:"
-			    " was this an old database?",
-			    type_to_string(tmpctx, struct sha256,
-					   &hout->payment_hash));
-		wallet_payment_set_status(ld->wallet, &hout->payment_hash,
-					  PAYMENT_FAILED, NULL);
-		return;
-	}
 #else
 	assert(payment);
-	assert(payment->route_channels);
 #endif
+	assert((payment->path_secrets == NULL) == (payment->route_nodes == NULL));
 
 	/* This gives more details than a generic failure message */
 	if (localfail) {
 		fail = local_routing_failure(tmpctx, ld, hout, payment);
 		failmsg = localfail;
 		pay_errcode = PAY_TRY_OTHER_ROUTE;
+	} else if (payment->path_secrets == NULL) {
+		/* This was a payment initiated with `sendonion`, we therefore
+		 * don't have the path secrets and cannot decode the error
+		 * onion. Let's store it and hope whatever called `sendonion`
+		 * knows how to deal with these. */
+
+		pay_errcode = PAY_UNPARSEABLE_ONION;
+		fail = NULL;
+		failmsg = NULL;
 	} else {
 		/* Must be remote fail. */
 		assert(!hout->failcode);

tests/test_pay.py

@@ -2519,3 +2519,27 @@ def test_sendonion_rpc(node_factory):
                      payment_hash=inv['payment_hash'])
 
     l1.rpc.waitsendpay(payment_hash=inv['payment_hash'])
+    invs = l4.rpc.listinvoices(label="lbl")['invoices']
+    assert(len(invs) == 1 and invs[0]['status'] == 'paid')
+
+    pays = l1.rpc.listsendpays()['payments']
+    assert(len(pays) == 1 and pays[0]['status'] == 'complete'
+           and pays[0]['payment_hash'] == inv['payment_hash'])
+
+    # And now for a failing payment, using a payment_hash that doesn't match an
+    # invoice
+    payment_hash = "00" * 32
+    onion = l1.rpc.createonion(hops=hops, assocdata=payment_hash)
+    l1.rpc.sendonion(onion=onion['onion'], first_hop=first_hop,
+                     payment_hash=payment_hash)
+
+    try:
+        l1.rpc.waitsendpay(payment_hash=payment_hash)
+        raise ValueError()
+    except RpcError as e:
+        assert(e.error['code'] == 202)
+        assert(e.error['message'] == "Malformed error reply")
+
+    pays = l1.rpc.listsendpays(payment_hash=payment_hash)['payments']
+    assert(len(pays) == 1 and pays[0]['status'] == 'failed'
+           and pays[0]['payment_hash'] == payment_hash)

wallet/wallet.c

@@ -2220,6 +2220,10 @@ static struct wallet_payment *wallet_stmt2payment(const tal_t *ctx,
 		payment->route_nodes = db_column_node_id_arr(payment, stmt, 8);
 		payment->route_channels =
 		    db_column_short_channel_id_arr(payment, stmt, 9);
+	} else {
+		payment->path_secrets = NULL;
+		payment->route_nodes = NULL;
+		payment->route_channels = NULL;
 	}
 
 	db_column_amount_msat(stmt, 10, &payment->msatoshi_sent);
@@ -2385,8 +2389,11 @@ void wallet_payment_get_failinfo(const tal_t *ctx,
 		*failupdate = tal_arr(ctx, u8, len);
 		memcpy(*failupdate, db_column_blob(stmt, 6), len);
 	}
-	*faildetail =
-	    tal_strndup(ctx, db_column_blob(stmt, 7), db_column_bytes(stmt, 7));
+	if (!db_column_is_null(stmt, 7))
+		*faildetail = tal_strndup(ctx, db_column_blob(stmt, 7),
+					  db_column_bytes(stmt, 7));
+	else
+		*faildetail = NULL;
 
 	tal_free(stmt);
 }
@@ -2448,7 +2455,11 @@ void wallet_payment_set_failinfo(struct wallet *wallet,
 	else
 		db_bind_null(stmt, 6);
 
-	db_bind_text(stmt, 7, faildetail);
+	if (faildetail != NULL)
+		db_bind_text(stmt, 7, faildetail);
+	else
+		db_bind_null(stmt, 7);
+
 	db_bind_sha256(stmt, 9, payment_hash);
 
 	db_exec_prepared_v2(take(stmt));