crash in renepay plugin

[endothermicdev]

Issue and Steps to Reproduce

ran lightning-cli renepay <bolt 11> 50000 40000 to test a 50sat payment
which lead to the following crash:

2023-08-03T14:34:18.711Z UNUSUAL plugin-cln-renepay: onion error WIRE_INCORRECT_CLTV_EXPIRY from node #1 791381x1127x0: failed: WIRE_INCORRECT_CLTV_EXPIRY (reply from remote)
2023-08-03T14:34:18.732Z **BROKEN** plugin-cln-renepay: Underflow subtracting flow->amounts[0] (367309429976msat) from &p->total_sent (0msat)
2023-08-03T14:34:18.763Z INFO    plugin-cln-renepay: Killing plugin: exited during normal operation
2023-08-03T14:34:18.763Z **BROKEN** plugin-cln-renepay: Plugin marked as important, shutting down lightningd!

Running 23.08rc1

[vincenzopalazzo]

We should not make renepay marked as important, I do not think it has enough tests to do it?

Maybe this underflow is easier for @Lagrang3 to debug? assigning to it till I will be away from my desk

[Lagrang3]

First thing I notice is that invoice string comes before the payment parameters otherwise the parser will not let you continue.

Can you upload more details of the logs, to simplify maybe just the lines containing renepay?

[endothermicdev]

Sorry, I did get the BOLT11 in before the parameters in the actual test. The report is updated to reflect that.

That was all the log output I received - there was no traceback available. Will see if I can up the logging level and get some more details.

[Lagrang3]

Actually, I think I don't need any more log messages. I seems like the flow destructor is called twice. I'm looking into it.

[Lagrang3]

We should not make renepay marked as important, I do not think it has enough tests to do it?

Maybe this underflow is easier for @Lagrang3 to debug? assigning to it till I will be away from my desk

As a builtin plugin, the bool important flag is true by default. I am not sure if I can flip it at will. There are conflicting definitions of struct plugin in lightningd/plugin.h and plugins/libplugin.c, the first contains a bool important, the second does not.

[vincenzopalazzo]

If there is no possibility to do it, we should add it. Till now, all the plugins built-in are important (except grpc one), but we should have the possibility to say, "it is fine if this plugin dies"

[Lagrang3]

@rustyrussell just PR this #6523 to avoid crashing the node.

[Lagrang3]

@rustyrussell there is also the fix to the underflow that triggered this issue #6505

[vincenzopalazzo]

My fault I was assuming that this was already merged

[rustyrussell]

I believe this should be fixed by #6538