Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invoice round robin #3913

Merged
merged 5 commits into from
Aug 25, 2020
Merged

Conversation

ZmnSCPxj
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj commented Aug 8, 2020

This implements part of my suggestions in #3894.

The drawback is that unpublished nodes with this commit could potentially be harder to pay by other implementations that do not have any MPP at all yet.

This change basically embraces MPP and holds onto it tight and whispers lovingly into its ears "I want the whole world to have MPP". On the other hand it does show that our MPP impl seems to be working nicely enough.

It was implementing this that revealed #3908 , if you are curious. Yet another reason to abolish unpublished channels....

There is yet another bug in our MPP that this code also reveals, which is basically that if we get a blockheight disagreement between payee and payer, we are too aggressive with advancing the routehint pointer. I will make a new test and fix for that latent bug.

@ZmnSCPxj ZmnSCPxj requested a review from cdecker as a code owner August 8, 2020 17:21
@ZmnSCPxj ZmnSCPxj force-pushed the invoice-round-robin branch 4 times, most recently from 7492fab to 5ed2b18 Compare August 9, 2020 06:39
@ZmnSCPxj ZmnSCPxj force-pushed the invoice-round-robin branch 3 times, most recently from abf9504 to 984268f Compare August 11, 2020 03:27
@ZmnSCPxj
Copy link
Collaborator Author

The errors occurring, while they occur in test_mpp_interference_2, occur at the fundchannel stage where we are waiting for certain logs to appear, and before any attempt at MPP payment. Weird. COMPAT=0 tho....

@ZmnSCPxj
Copy link
Collaborator Author

Disabled the test at DEVELOPER=0, turns out it takes 10 minutes to set up the channels if DEVELOPER=0.

@ZmnSCPxj
Copy link
Collaborator Author

Added the routehint randomization suggested here: #3914 (comment)

My initial non-exhaustive tests suggest it improves the success rate and speed of test_mpp_interference_2, but not quite enough yet to remove @flaky from it.

A proper round-robin allocation of routehints would, I think, improve this even more, but the random-base idea is "good enough" in practice IMO and is very very simple to implement.

@ZmnSCPxj
Copy link
Collaborator Author

Rebased now that #3914 is on master.

… method to LightningNode, for when you want to Thanos your channels.
Changelog-Changed: We now make MPP-aware routehints in invoices.
This improves the success rate of `test_mpp_interference_2`, though
still not quite up to the level that we can remove `@flaky` from it.
@ZmnSCPxj
Copy link
Collaborator Author

Rebased again.

@@ -3354,3 +3354,100 @@ def pay(l1, inv):

# pay command should complete without error
fut.result(TIMEOUT)


@unittest.skipIf(VALGRIND, "runs 7 nodes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's enough to mark it as slow (as you do below) which turns off VALGRIND if SLOW_MACHINE.

Otherwise this test will never run under CI, since we test VALGRIND=1 DEVELOPER=1 and VALGRIND=0 DEVELOPER=0...

@rustyrussell
Copy link
Contributor

I like this, just want to combine some logic. We measure the incoming capacity both here and in the listpeers code (which does a more thorough job), so we should combine those.

I also dislike the duplication between the mpp and non-mpp, so I'll refactor that as well while I'm at it.

Just waiting for Travis, which I had to kick...

@rustyrussell
Copy link
Contributor

Ack 6972e1f

@rustyrussell rustyrussell merged commit 128adf0 into ElementsProject:master Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants