Repository navigation
Fix sendpay failure status - #9731
Conversation
Andezion
left a comment
Andezion
left a comment
There was a problem hiding this comment.
from what i see renepay has /* FIXME shouldn't this be always SENDPAY_FAILED? */ (plugins/renepay/routetracker.c from line 556 to 567). It logs LOG_UNUSUAL "sendpay_failure notification returned status=%s" and forces the status to failed. Today this probably fires for every failed part, but after this pr it should never fire. so maybe remove the workaround in a follow up, or turn it into a plugin_err / LOG_BROKEN, so a regression gets noticed?
|
|
||
| /* Waiters and the notification need the updated status. */ | ||
| payment = wallet_payment_by_hash(tmpctx, ld->wallet, payment_hash, | ||
| partid, groupid); |
There was a problem hiding this comment.
maybe add assert(payment);? same on 1557 line
There was a problem hiding this comment.
I saw that payment_succeeded() but i wasn't sure it could actually be NULL here? Anyways for consistency i added it here.
nGoline
left a comment
nGoline
left a comment
There was a problem hiding this comment.
ACK 0592b38
Checked that the new test fails without the fix.
Side effect worth knowing: for unparseable onion replies the reloaded row now includes erroronion in the notification and the in-flight waitsendpay error, which a late waitsendpay already returned. The schema already allows it.
Out of scope for this PR, but noticed while reading: wallet_payment_set_failinfo() matches on payment_hash and partid only, not groupid.
When an HTLC fails, payment_failed() reads the payment before marking it failed, then hands that stale copy to the sendpay_failure notification and to any waitsendpay already waiting. Both report "status": "pending" without completed_at or updated_index, while a waitsendpay issued afterwards reads the database and says "failed". This is the cause of the test_sendpay_notifications flake: whether its waitsendpay lands before or after the failure decides which version it gets, and the notification never matches the fresh one. Reported-in: ElementsProject#7561
payment_failed() loads the payment, marks it failed in the database, then passes the copy it loaded beforehand to the waiters and the sendpay_failure notification. So they saw "status": "pending" and no completed_at or updated_index, unlike a later waitsendpay or listsendpays. Reload the payment after updating it, as payment_succeeded() does; the self-payment failure path had the same problem. This also fixes the test_sendpay_notifications flake, where the notification was compared against a waitsendpay that read the database. Rusty proposed setting payment->status locally in 2024, but that never landed and would still leave completed_at and updated_index out. Fixes: ElementsProject#7561 Changelog-Fixed: Plugins: `sendpay_failure` notifications and `waitsendpay` errors now report `status` `failed` (with `completed_at` and `updated_index`), not `pending`.
0592b38 to
ad3ecda
Compare
renepay is deprecated so i did not add changes to it to this PR and i don't intend to in another PR.
Good catch, opened #9734 for that. |
Fixes: #7561
and the flake in test_sendpay_notifications: