Always notify if a sendpay payment attempt is resolved
#3405
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZmnSCPxj
reviewed
Jan 9, 2020
cdecker
commented
Jan 9, 2020
There was a problem hiding this comment.
Noticed the
draftness a bit late, decided to just publish my comments anyway.
No worries, I was in a rush and didn't check my implementation of the inversion of ownership too well, so I just wanted to avoid this being merged before I can cross-check it once more :-)
Feedback is always welcome ^^
ZmnSCPxj
approved these changes
Jan 9, 2020
cdecker
force-pushed
the
notify-sendpay
branch
2 times, most recently
from
8b465f8 to
dd925d8
Compare
cdecker
force-pushed
the
notify-sendpay
branch
from
dd925d8 to
27aa8dc
Compare
…ith no waiter We clone the test above, but this time we don't attach waiters (they'd be racy anyway), and we wait for the notification to be called. This fails, but is fixed in the next two commits.
We're about to move it a bit up in the call-graph, so encapsulate it in its own function.
Changelog-Changed: plugin: `notify_sendpay_success` and `notify_sendpay_failure` are now always called, even if there is no command waiting on the result.
`wallet_payment_store` frees the unstored payment after it has stored it, but we still need that instance for our notifications. This is the smallest possible fix, but I plan to refactor this out.
`wallet_payment_store` would free the `wallet_payment` instance which would then cause us to reload it from the DB. Instead of doing the store->free->load dance we now tell `wallet_payment_store` whether it should take ownership and leave it alone if not. Passing the payment around instead of referencing it through payment_hash and partid is a nice side-effect.
cdecker
force-pushed
the
notify-sendpay
branch
from
27aa8dc to
e1264f7
Compare
ZmnSCPxj
approved these changes
Jan 13, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sendpay notifications would get sent only if we have an active watcher, and multiple times if we have more than one watcher. This kind of defeats the point since we can't just fire and forget sendpay and wait for the notification if that notification is bound to the watchers.
This PR fixes that by pulling the notification out of the watcher loop, concentrating the error message allocation in a single place, and inverting ownership of
wallet_payment_store. This also reduces a couple of useless allocations, since we don't store, free, then reload thewallet_paymentanymore.Fixes #3403