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.

Sign up for GitHub

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

Jump to bottom

pytest: test that we properly apply channel_update. #4808

Closed
wants to merge 2 commits into from
Closed

pytest: test that we properly apply channel_update. #4808

wants to merge 2 commits into from

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Sep 22, 2021 •
edited
Loading

Spoiler: we don't, if it's in a routehint!

See also: #4781

Pay should probably retry in this case?

@cdecker cdecker force-pushed the pay-should-update-fees branch from 809cd54 to 9008160 Compare September 28, 2021 14:49
@cdecker
Copy link
Member

cdecker commented Sep 28, 2021

Ok, debugging this further it took me a while, but it appears l3 is serving a stale channel_update with the old fees, which l1 happily stores, and then tries the exact same path again (being the only one available). The gossmap is refreshed just before computing any route, by calling get_gossmap which refreshes internally, and I made sure we are seeing the channel_update alright, just the fees don't match our expectation.

@rustyrussell: I'm not exactly up to speed on the channel_update caching logic we use to defer updates in some cases, could this be related to that? I'd have expected the setchannelfee command to synchronously create a new update and stash it with lightningd which can then serve it, but we seem to have a stale one in there.

@cdecker cdecker force-pushed the pay-should-update-fees branch from c011cc4 to 9beb76a Compare October 8, 2021 14:24
@cdecker cdecker mentioned this pull request Oct 21, 2021
Merged
@rustyrussell rustyrussell added this to the v0.11 milestone Mar 8, 2022
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Mar 28, 2022
@rustyrussell
tests: add test that pay gets update correct.
64f5291 
This is the part which works: we have another old PR (or simply
uncomment the FIXME section) for the routehint case.

Closes: ElementsProject#4781
See-also: ElementsProject#4808
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell mentioned this pull request Mar 28, 2022
Merged
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Mar 28, 2022
@rustyrussell
tests: add test that pay gets update correct.
674b63b 
This is the part which works: we have another old PR (or simply
uncomment the FIXME section) for the routehint case.

Closes: ElementsProject#4781
See-also: ElementsProject#4808
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell modified the milestones: v0.11, v0.12 Mar 30, 2022
rustyrussell added a commit that referenced this pull request Mar 30, 2022
@rustyrussell
tests: add test that pay gets update correct.
0c3aa35 
This is the part which works: we have another old PR (or simply
uncomment the FIXME section) for the routehint case.

Closes: #4781
See-also: #4808
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Mar 31, 2022
@rustyrussell
tests: add test that pay gets update correct.
c8f7a9d 
This is the part which works: we have another old PR (or simply
uncomment the FIXME section) for the routehint case.

Closes: ElementsProject#4781
See-also: ElementsProject#4808
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@niftynei niftynei modified the milestones: v0.12, v22.10 Aug 2, 2022
rustyrussell and others added 2 commits September 21, 2022 16:15
@rustyrussell @cdecker
pytest: test that we properly apply channel_update.
091ce2d 
Spoiler: we don't, we simply give up!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@cdecker
pytest: Add more details to the channel_update error on feechange
de96a63 
While debugging this I noticed that the issue stems not so much from a
stale gossmap, but rather from the failing node handing out an
outdated `channel_update` which leads us to retry the same exact
failing payment again. The amendments to this test now include
extracting the update, parsing it and making sure we actually are
getting the new fees not the old ones.
@cdecker cdecker force-pushed the pay-should-update-fees branch from 9beb76a to de96a63 Compare September 21, 2022 14:16
@cdecker cdecker removed this from the v22.11 milestone Oct 25, 2022
@cdecker cdecker marked this pull request as draft November 2, 2022 11:39
@ShahanaFarooqui
Copy link
Collaborator

Closing all Draft PRs inactive for more than a year. Please feel free to reopen if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdecker cdecker Awaiting requested review from cdecker

Assignees

@cdecker cdecker

@rustyrussell rustyrussell

Labels
gossip needs-rebase pay-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rustyrussell @cdecker @ShahanaFarooqui @niftynei