Failure to update constraints in shadow route modifier

[thestick613]

pay: plugins/libplugin-pay.c:1850: shadow_route_listchannels: Assertion `ok' failed.
pay: FATAL SIGNAL 6 (version v0.9.0rc1-11-gc984376)
0x417e3e send_backtrace
        common/daemon.c:38
0x417ec8 crashdump
        common/daemon.c:51
0x7fba46a534af ???
        ???:0
0x7fba46a53428 ???
        ???:0
0x7fba46a55029 ???
        ???:0
0x7fba46a4bbd6 ???
        ???:0
0x7fba46a4bc81 ???
        ???:0
0x40e7c3 shadow_route_listchannels
        plugins/libplugin-pay.c:1850
0x4091de handle_rpc_reply
        plugins/libplugin.c:547
0x4092b3 rpc_read_response_one
        plugins/libplugin.c:662
0x40933b rpc_conn_read_response
        plugins/libplugin.c:681
0x426ced next_plan
        ccan/ccan/io/io.c:59
0x4271ad do_plan
        ccan/ccan/io/io.c:407
0x4271d8 io_ready
        ccan/ccan/io/io.c:417
0x428973 io_loop
        ccan/ccan/io/poll.c:445
0x409d96 plugin_main
        plugins/libplugin.c:1284
0x407086 main
        plugins/pay.c:2010
0x7fba46a3e82f ???
        ???:0
0x402e08 ???
        ???:0
0xffffffffffffffff ???
        ???:0
pay: FATAL SIGNAL 11 (version v0.9.0rc1-11-gc984376)
0x417e3e send_backtrace
        common/daemon.c:38
0x417ec8 crashdump
        common/daemon.c:51
0x7fba46a534af ???
        ???:0
0x0 ???
        ???:0

[ZmnSCPxj]

Definitely a puzzler. One reason for failing is that the constraints on the shadow route are looser than the constraints on the actual payment, which should not happen since shadow route constraints are 1/4 that of the payment it is for. The only other reason for failing is if we overflowed u64, which should not happen since there are not enough satoshi in the world for that.

It could be that payment_getroute_result is updating the payment constraints, but not the shadow route constraints. Which does not make sense to me since shadow routing should happen before any getroute attempts. I think. @cdecker?

[ZmnSCPxj]

However, looking at the shadowroute modifier particularly shadow_route_init --- it seems a payment gets its shadowroute object from the parent payment (with 1/4 the constraints of the parent payment), whereas each child payment has its own constraint. Other modifiers in the payment can change the constraints on the child payment (especially fee budget when splitting a payment!). Perhaps the parent shadowroute constraint is now looser than child payment constraints if there are sufficient number of child payments?

[cdecker]

Correct each sub payment gets allocated constraints proportional to the percentage of the parent amount. Only the presplit and adaptive splitters adjust the amount, retry copies the amount and constraints from the parent without changing them.

Also worth noting is that the multi-part payments opt out of amount changes, since the recipient is supposed to wait for the exact amount. So the only thing that can fail here are the changes to CLTV and fee budget. So this seems to be a weird route with already extremely high fees or CLTV deltas.

[cdecker]

Regarding a possible interference of getroute that should not happen since the shadow route modifier acts only on the pre-getroute step, since we change the destination. Effectively the constraints are just verified after getroute.

[cdecker]

@rustyrussell found the issue: we were accidentally sharing the modifier context between all payment attempts, which then leads to a situation where attempt a sets up its own constraints, then calls listchannels to find a route extension, in the meantime attempt b comes along, overwrites the constraints in the shared context, and yields control by calling listchannels. Now when a comes back it suddenly finds the overwritten context and happily uses that, not noticing that the invariants are now broken.

See #3857 for the fix.

[thestick613]

Hey guys, should i stop testing large MPP with live funds over real nodes? :)