htlcswitch+invoices: allow settling invoices via multi-path payments#3415
Conversation
joostjager
changed the title
joostjager
force-pushed
the
mpp
branch
3 times, most recently
from
5f1966a to
45a27a3
Compare
joostjager
force-pushed
the
mpp
branch
2 times, most recently
from
9e0b20f to
a4b9fa2
Compare
joostjager
added
incomplete
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
just did a quick pass to refresh my memory of this pr. i know you said don't review, but just leaving some initial comments tho i'm sure the underlying pr has changed a bit
joostjager
changed the title
joostjager
force-pushed
the
mpp
branch
3 times, most recently
from
9c81a17 to
c03862c
Compare
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
so far so good, changes coming together :) should be pretty close to being able to do a full e2e!
joostjager
force-pushed
the
mpp
branch
3 times, most recently
from
9a8df74 to
b75c07f
Compare
There was a problem hiding this comment.
Imho it's cleaner if this is not passed as a closure but an interface. See #3694 for details (clock package).
There was a problem hiding this comment.
We already do it like this in two other locations. I didn't notice much problems with a closure really. As you say, if it is extended with additional functions like time.After it probably makes sense to have an interface
There was a problem hiding this comment.
One nice thing about using the interface that once it's spread, refactor will be simpler as well as it may lead to better code hygene (common test clock implementation maybe?)
There was a problem hiding this comment.
The Now closure or if you choose to instead use the clock.Clock interface as suggested earlier should be an incomit parameter of makeTestDB() function. Otherwise all other tests will depend on the real time.
There was a problem hiding this comment.
It would be nice to make that better, but it is also pre-existing.
There was a problem hiding this comment.
Or other option if you choose to stay with the Now closure is to still inject it in makeTestDB() or create a default Now() function for all tests and modfity makeTestDB() instead.
There was a problem hiding this comment.
invoices/update.go needs unit tests
There was a problem hiding this comment.
It is covered by invoiceregistry_test.go. There is indeed an opportunity to now test this code in isolation, but this PR also didn't introduce this code, just moved it.
There was a problem hiding this comment.
Sorry, missed that this is a dependent PR. Discussed this offline too, but to restate I'm on the opinion that separate units should be tested separately if possible.
There was a problem hiding this comment.
There's quiet a bit of refactor in #3694 to move all test data generation to a separate compulation unit. I'd suggest building on that. Not sure how to do this properly without merging that first though.
There was a problem hiding this comment.
It is mostly rename/move it seems. I think it can be done independently?
There was a problem hiding this comment.
I'm fine merging this if this PR is merged to master earlier. Optionally we could somehow make it part of this PR too (it's in commit: f663b33).
There was a problem hiding this comment.
not sure what a good value would be. Maybe on the order of 1-2 min?
There was a problem hiding this comment.
Then why skip the HTLC if htlc.State != channeldb.HtlcStateSettled ? 🤔
joostjager
force-pushed
the
mpp
branch
7 times, most recently
from
f47ed9b to
f3b071a
Compare
|
Itest fail, problem is that |
|
Rebased on top of @cfromknecht's commits that create the payment address. |
halseth
left a comment
halseth
left a comment
There was a problem hiding this comment.
Nice, I think this one is nearly there 👍 Only a few comments.
There was a problem hiding this comment.
could have a channel signal it is ready instead
There was a problem hiding this comment.
modified so that tick channels also tick immediately when the trigger time is already there.
|
Note to self: don't forget to remove debug commits |
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
did another pass, just one minor nit. is it time to remove debug commits?
|
debug commits removed |
|
@cfromknecht brought up the mpp timeout failure. Will address that in a follow up in coordination with @carlaKC. Without this error returned, senders that actually support sending multiple shards and interpret this new failure will not function optimally. But seems acceptable for where we are right now. |
halseth
left a comment
halseth
left a comment
There was a problem hiding this comment.
All aboard the mpptrain! 🚂 LGTM
There was a problem hiding this comment.
Shouldn't we use either signed or unsigned types for block height everywhere?
There was a problem hiding this comment.
Yes, that is inconsistent at the moment. But if you start changing, you will expand into many different areas and also replace one case by the other sometimes.
There was a problem hiding this comment.
Really nice! May I suggest you to extend clock.Clock interface and use that as a common ground instead? Maybe also move this test clock in that package such that others can use it?
There was a problem hiding this comment.
I don't want to delay this PR with a discussion on where to put that package/file. Given the time those took in past prs I'd rather avoid it (reference class forecasting).
There was a problem hiding this comment.
Looks like the file changed during review. The above comment meant to be here: "I think it's important to test cases where settlement may fail".
There was a problem hiding this comment.
There is a case where the partial payment times out. Which other cases do you mean? Of course the total number of possible test cases is large, but I tried to cover at least the ones that usually occur.
There was a problem hiding this comment.
Can this ever happen though?
There was a problem hiding this comment.
I don't think so. Perhaps we will do invoice cleanup in the future?
This commit introduces PriorityQueue, which is a general, heap based priority queue, and PriorityQueueItem which is an interface that concrete priority queue items must implement. This implementation is encapsulated, users do not need to use any other package for full functionality. PriorityQueue exports the usual public methids: Push, Pop, Top, Empty and Len. For full documentaton consult the priority_queue.go, for usage: priority_queue_test.go
4 participants
Before this PR, MPP fields in the payload were parsed but not processed. This PR adds processing of those fields in the invoice registry. This means that it is now possible to settle an invoice using multiple partial payments.