Skip to content

Fixed assertion in pay plugin - #3856

Closed
vincenzopalazzo wants to merge 2 commits into
ElementsProject:masterfrom
vincenzopalazzo:pay_plugin_assertion_fix
Closed

Fixed assertion in pay plugin#3856
vincenzopalazzo wants to merge 2 commits into
ElementsProject:masterfrom
vincenzopalazzo:pay_plugin_assertion_fix

Conversation

@vincenzopalazzo

@vincenzopalazzo vincenzopalazzo commented Jul 19, 2020

Copy link
Copy Markdown
Collaborator

Hi all,

I don't know if this type of PR is welcome, if no, I'm so sorry.

This PR includes the fix discussed on PR #3855. This fix was tested with the use case described inside the issue and worked.

@ZmnSCPxj

ZmnSCPxj commented Jul 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

It is acceptable of course.

You need a Changelog entry. Since this fixes a bug that does not exist in previous version, it would be Changelog-None. Just add to commit message (use git commit --amend to edit the commit message) then push --force to your branch.

You also probably want to add the text "Fixes: #3855" to the commit message and the original comment you put above, so that merging this change auto-closes #3855

I am writing a test to tickle this bug, I suggest putting it in as well, please wait..

ZmnSCPxj and others added 2 commits July 20, 2020 00:27
@ZmnSCPxj
tests/test_pay.py: Replicate ElementsProject#3855.
c534539
@vincenzopalazzo @ZmnSCPxj
Fixed assertion in pay plugin
1e62ac0 
This PR includes the fix discussed on PR ElementsProject#3855. This fix was tested with the use case described inside the issue and worked.

Fixes: ElementsProject#3855

Changelog-None
@ZmnSCPxj

ZmnSCPxj commented Jul 19, 2020

Copy link
Copy Markdown
Contributor

Hi @vincenzopalazzo I have a branch with the proposed fixes to your commit message, as well as the test, which I have confirmed catches this bug and also confirms the fix is working. ZmnSCPxj@1e62ac0

Will push into this PR.

@ZmnSCPxj
ZmnSCPxj force-pushed the pay_plugin_assertion_fix branch from cb9569e to 1e62ac0 Compare July 19, 2020 17:00
@vincenzopalazzo
vincenzopalazzo requested a review from cdecker as a code owner July 19, 2020 17:00
@ZmnSCPxj

ZmnSCPxj commented Jul 19, 2020

Copy link
Copy Markdown
Contributor

Force pushed. Let us leave to @cdecker for review.

@vincenzopalazzo

vincenzopalazzo commented Jul 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Hi @ZmnSCPxj

Thanks and sorry if I missed the test. In addition, Should I add the message that you suggest in the message below?

You need a Changelog entry. Since this fixes a bug that does not exist in previous version, it would be Changelog-None. Just add to commit message (use git commit --amend to edit the commit message) then push --force to your branch.

I think no 😄

@ZmnSCPxj

ZmnSCPxj commented Jul 19, 2020

Copy link
Copy Markdown
Contributor

Already added when I force-pushed into your PR. Moar tests == better as well.

@ZmnSCPxj ZmnSCPxj added this to the v0.9.0 milestone Jul 19, 2020
@ZmnSCPxj ZmnSCPxj linked an issue Jul 19, 2020 that may be closed by this pull request
Pay plugin stop to work after call "pay an_bolt11" #3855
Closed
@cdecker

cdecker commented Jul 19, 2020

Copy link
Copy Markdown
Member

Excellent work @vincenzopalazzo and @ZmnSCPxj, this is very much appreciated, but I could hit myself for not thinking of this myself :-)

Anyway, this works greate and I'll ACK it as soon as CI reports that it is working 👍

@ZmnSCPxj

ZmnSCPxj commented Jul 20, 2020

Copy link
Copy Markdown
Contributor

CI seems to catch another bug, where we somehow trigger the paymod system to split the payment into tiny, tiny, tiny parts and then things start collapsing due to the load of handling lots of tiny tiny parts.

@ZmnSCPxj

ZmnSCPxj commented Jul 20, 2020

Copy link
Copy Markdown
Contributor

I suggest adding a reset of some kind to paymod, where after we split too many times into tiny tiny pieces, we just clear the entire payment tree, erase our memory of excluded channels and nodes, and retry with the parent payment.

@cdecker

cdecker commented Jul 20, 2020

Copy link
Copy Markdown
Member

Having a split limiter is definitely a good idea, though we already have that indirectly via the fee and cltv budgets that get split into smaller and smaller pieces.

I think it's probably best not to automatically retry, since that would most likely lead down the same path. Rather we should optimize the first tree in such a way that any retry of the root would be superfluous anyway. There's just loads of parameters we can tweak here, and we're just getting started :-)

@cdecker

cdecker commented Jul 20, 2020

Copy link
Copy Markdown
Member

Rusty and I are working on fixing those other issues, reported by testers, and fix up the CI runs btw, so I'll rebase this PR on top of those so we can make sure this works as expected (which I'm pretty sure looking at the change anyway)

@ZmnSCPxj

ZmnSCPxj commented Jul 20, 2020

Copy link
Copy Markdown
Contributor

Suggest cherry-picking this on top of #3857 and then closing this then, seems we need a bit more tests and so on for paymod.

@cdecker

cdecker commented Jul 20, 2020

Copy link
Copy Markdown
Member

Will do 👍

@cdecker cdecker mentioned this pull request Jul 20, 2020
Merged
@cdecker

cdecker commented Jul 20, 2020

Copy link
Copy Markdown
Member

Cherry-picked on top of #3857 as suggested

@cdecker cdecker closed this Jul 20, 2020
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

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.9.0

Development

Successfully merging this pull request may close these issues.

Pay plugin stop to work after call "pay an_bolt11"

3 participants

@vincenzopalazzo @ZmnSCPxj @cdecker