Fixup pay crashes #3630
Fixup pay crashes #3630
Conversation
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
| @@ -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", | |||
There was a problem hiding this comment.
Hmm, this looks like json_add_string() would be better? add_member is pretty low-level...
|
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... |
|
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. |
|
@niftynei OK, your fix is correct. I've got another PR for the test though. |
|
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", | |||
|
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. |
Fix a couple little crashes found by @fiatjaf