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

Fixup pay crashes #3630

Merged
merged 2 commits into from
Apr 8, 2020
Merged

Conversation

niftynei
Copy link
Collaborator

@niftynei niftynei commented Apr 7, 2020

Fix a couple little crashes found by @fiatjaf

Fixes (?) ElementsProject#3607

Changelog-Fixed: Fix crash in pay when attempting to find cheaper route with exemptfee
`json_add_member` requires quotes for string types

Changelog-Fixed: `pay` would crash on expired waits with tried routes
@niftynei niftynei added this to the 0.8.2 milestone Apr 7, 2020
@@ -262,7 +262,7 @@ static struct command_result *waitsendpay_expired(struct command *cmd,
for (size_t i = 0; i < tal_count(pc->ps->attempts); i++) {
json_object_start(data, NULL);
if (pc->ps->attempts[i].route)
json_add_member(data, "route", false, "%s",
json_add_member(data, "route", true, "%s",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this looks like json_add_string() would be better? add_member is pretty low-level...

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, but fixed in #3632

@rustyrussell
Copy link
Contributor

But I don't understand how this happens.

I tried the example in #3607 (and hacked my node to ignore expiry). It didn't crash.

It took a long time, however: the routehint itself requires more fee than we're prepared to pay, and we flail through excluding everything else until we run out of routes, but it does eventually.

In particular, find_worst_channel() can only return NULL if there is less than two hops in the route. Which implies it could only happen if the node was a peer, and for some reason we didn't select it first due to the routeboost. Let me try that...

@rustyrussell
Copy link
Contributor

OK, I can't reproduce this. In theory, we could add a channel between pay attempts, but that wouldn't be reproducible like this seems to be.

@rustyrussell
Copy link
Contributor

OK, @fiatjaf notes that he is a peer in #3607 and I finally reproduced. Now to track down...

@rustyrussell
Copy link
Contributor

@niftynei OK, your fix is correct. I've got another PR for the test though.

@rustyrussell
Copy link
Contributor

Ack 069ffc9

Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK 069ffc9

@@ -262,7 +262,7 @@ static struct command_result *waitsendpay_expired(struct command *cmd,
for (size_t i = 0; i < tal_count(pc->ps->attempts); i++) {
json_object_start(data, NULL);
if (pc->ps->attempts[i].route)
json_add_member(data, "route", false, "%s",
json_add_member(data, "route", true, "%s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, but fixed in #3632

@fiatjaf
Copy link
Contributor

fiatjaf commented Apr 8, 2020

Oops, sorry for the irreproducibility, I forgot invoices expired. I tried to reproduce myself (without hacking my node to ignore the expiry) and failed too.

Also sorry for implying that I was a peer. I wasn't a peer of the unannounced channel, the unannounced channel doesn't really exist. In fact I was a peer of the last peer before the unannounced channel, but I didn't think this was important when creating the issue. And it seems Rusty found that out before I could even realize.

Anyway, thank you all for looking at this super corner case.

@cdecker cdecker merged commit b51772f into ElementsProject:master Apr 8, 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.

5 participants