Skip to content

plugins/pay: revert 899a2e64b02d47fc3aa68d70b9f483c2e8d7d685, use routehints in order. - #3866

Closed
rustyrussell wants to merge 2 commits into
ElementsProject:masterfrom
rustyrussell:fix-routehint
Closed

plugins/pay: revert 899a2e64b02d47fc3aa68d70b9f483c2e8d7d685, use routehints in order.#3866
rustyrussell wants to merge 2 commits into
ElementsProject:masterfrom
rustyrussell:fix-routehint

Conversation

@rustyrussell

Copy link
Copy Markdown
Contributor

This uses @cdecker's idea of excluding the routehinted channel from the route,
and also consumes the route hints as it goes so that it makes progress.

I don't know if this is correct, but it reliably passes tests/test_pay.py::test_tlv_or_legacy
now.

Signed-off-by: Rusty Russell rusty@rustcorp.com.au

This uses @cdecker's idea of excluding the routehinted channel from the route,
and also consumes the route hints as it goes so that it makes progress.

I don't know if this is correct, but it reliably passes tests/test_pay.py::test_tlv_or_legacy
now.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell added this to the v0.9.0 milestone Jul 23, 2020
@rustyrussell
rustyrussell requested a review from cdecker July 23, 2020 04:53
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@cdecker

cdecker commented Jul 23, 2020

Copy link
Copy Markdown
Member

Hm, agreed that we need to remove the randomness, however by removing the routehints from the array we're forgetting them forever, including after the split, where they might start working again. I think I have a better idea:

  • Initialize the routehint array in the root payment
  • If an exploratory getroute for 1sat succeeds, i.e., the destination is potentially directly reachable, add a NULL routehint to the array (attempts that hit that will try directly)
  • Each payment indexes into that array, on retry we increment until we find a non-excluded routehint, on split we reset to 0
  • We tell the retry mod that not finding a route is ok until we've wrapped around once, otherwise it might skip retries

This saves us from destroying the routehint array as we run through, save allocations and memory since the array is immutable and can be reused, and we neatly add the "can attempt directly" case into the routehint selection. I'll write this up and see which one performs better.

@cdecker

cdecker commented Jul 23, 2020

Copy link
Copy Markdown
Member

Strange, I also noticed that the PR is mostly empty now, since the second commit reverts almost all changes of the first. I split out the overrun issue into #3869 since that needs to be fixed asap.

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