Compiler can't tell that we always set have_state[PAY_FLOW_FAILED_FINAL]
when we set this:
```
plugins/renepay/payment.c: In function ‘payment_reconsider’:
plugins/renepay/payment.c:287:25: error: ‘final_error’ may be used uninitialized [-Werror=maybe-uninitialized]
287 | payment_fail(payment, final_error, "%s", final_msg);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
plugins/renepay/payment.c:194:30: note: ‘final_error’ was declared here
194 | enum jsonrpc_errcode final_error, ecode;
| ^~~~~~~~~~~
plugins/renepay/payment.c:287:25: error: ‘final_msg’ may be used uninitialized [-Werror=maybe-uninitialized]
287 | payment_fail(payment, final_error, "%s", final_msg);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
plugins/renepay/payment.c:195:21: note: ‘final_msg’ was declared here
195 | const char *final_msg;
| ^~~~~~~~~
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We cannot carry pointers into the gossmap across localmod addition
or removal.
We didn't notice because the map->chan_arr is not normally resized,
but if we change gossmap.c line 689 to only allocate 1 to start, we see this:
```
VALGRIND=1 valgrind -q --error-exitcode=7 --track-origins=yes --leak-check=full --show-reachable=yes --errors-for-leak-kinds=all plugins/renepay/test/run-mcf > /dev/null
==2349744== Invalid read of size 4
==2349744== at 0x1788C2: gossmap_chan_scid (gossmap.c:558)
==2349744== by 0x1872A2: get_chan_extra_half_by_chan (flow.c:346)
==2349744== by 0x187797: remove_completed_flow (flow.c:488)
==2349744== by 0x187927: remove_completed_flow_set (flow.c:518)
==2349744== by 0x18DF4D: main (run-mcf.c:393)
==2349744== Address 0x4b80f38 is 88 bytes inside a block of size 136 free'd
==2349744== at 0x4848C63: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2349744== by 0x173D71: tal_resize_ (tal.c:744)
==2349744== by 0x177E36: next_free_chan (gossmap.c:336)
==2349744== by 0x177ED3: new_channel (gossmap.c:351)
==2349744== by 0x178441: add_channel (gossmap.c:458)
==2349744== by 0x1798D4: gossmap_apply_localmods (gossmap.c:904)
==2349744== by 0x18DEDB: main (run-mcf.c:388)
==2349744== Block was alloc'd at
==2349744== at 0x4848C63: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2349744== by 0x173D71: tal_resize_ (tal.c:744)
==2349744== by 0x177E36: next_free_chan (gossmap.c:336)
==2349744== by 0x177ED3: new_channel (gossmap.c:351)
==2349744== by 0x178441: add_channel (gossmap.c:458)
==2349744== by 0x178B6D: map_catchup (gossmap.c:635)
==2349744== by 0x178F45: load_gossip_store (gossmap.c:697)
==2349744== by 0x179D71: gossmap_load (gossmap.c:978)
==2349744== by 0x18D22F: main (run-mcf.c:295)
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's not there if it's a local error:
```
{
"code": 202,
"message": "Parsing '{message:%,data:{erring_index:%,failcode:%,raw_message:': object does not have member raw_message"
}
```
Reported-by: https://github.com/daywalker90Fixes: #6553
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Unifies the pay_flow resolve functions, and moves remove_htlc_payflow
and commit_htlc_payflow to the top.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We want to make sure that on every path, we terminate the flow. The simplest
way to do this is encourage the pattern "return pay_flow_xxx(flow)".
Indeed, this caught a few places I missed!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The main function here is payment_reconsider:
* Each payment has a list of pay_flow.
* This is populated in try_paying(), calling add_payflows & sendpay_new_flows.
* When we get a notification, we resolve a pay_flow using one of the pay_flow_failedxxx
or pay_flow_succeeded functions.
* They call payment_reconsider() which cleans up finished flows decides what to do:
often calling try_paying again.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's not required, but it should be there so we might as well use it
(though we sometimes don't put one in, esp if it's a private channel).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Avoids a gratuitous "ctx" field, and the simplified declaration
is now understood by `make update-mocks`.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Treat it just like "PAY_TRY_OTHER_ROUTE", except it is from the final node:
this means we correctly process that it "succeeded".
Add a test: this crashes sometimes, but it's cleaned up soon...
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As recommended by your TODO, a bit simpler: we also make the hash function
return a ptr rather than the (now rather large) struct.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Use json_scan(), and use the new pay_flow_from_notification() routine.
Also, the tal_dup_or_null can be tal_dup, since &preimage is never NULL.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There are a few fields in `struct renepay` which are genuinely
transient, but it makes the code much harder to follow than simply
having a single structure.
More cleanups will follow, but this is the minimal set.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The general pattern for xxx_new is that it should populate all
fields, for encapsulation and so you never can have a half-formed
object.
This means a fair bit of work for now, but it pays off in the next
patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
You cannot refresh the gossmap with localmods applied, nor apply localmods
when others have applied localmods in the same process.
There are optimizations we could do, but for now always apply/unapply before
querying gossmap.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes 32-bit builds:
```
In file included from plugins/renepay/pay.c:5:
./plugins/renepay/pay_flow.h: In function 'fmt_payflow_key':
./plugins/renepay/pay_flow.h:54:17: error: format '%ld' expects argument of type 'long int', but argument 4 has type 'u64' {aka 'long long unsigned int'} [-Werror=format=]
54 | "key: groupid=%ld, partid=%ld, payment_hash=%s",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
55 | k->groupid,k->partid,
| ~~~~~~~~~~
| |
| u64 {aka long long unsigned int}
```
etc
It's expressed in bits, but really it's clearer as a quantity, given
how it's used.
Suggested-by: @Lagrang3
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
- adopt "const <type> *"convention
- remove use_shadow option for some pyln tests
- show prob. information of flows into paynotes
- show prob. of success of entire payment flow in paynotes
- minflow: We were not releasing the memory of flow arrays when replacing
them with a new canditate.
- use memleak_scan_obj in memleak_check
- replace u64 with size_t
Signed-off-by: Lagrang3 <eduardo.quintana@pm.me>
The global is an *internal* hack because dijkstra_item_mover doesn't
take a context arg! It should be used with care.
Easy, since all the accessors exist: we just hand in the struct dijkstra.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
- remove internal gheap checks
- add check for arc_t.chanidx overflow
- remove outdated comments
- check the delta flow bounds before augmenting along a path
- get_flow_paths uses a dynamic tal array instead of a list.
- fix a unit test that depended on the order of returned flows
- fix bug: lightnind doesn't like if I reuse the partid of a failed
flow, therefore use a higher partid than any of the previous attempts.
- plugin_err instead of LOG_BROKEN if sendpay fails and we cannot get a
an error code.
- fix wrong comments.
- remove the background timer.
- This is a bugfix. Previous to this the MCF network was built using the
knowledge of the min and max liquidity but it didn't take into account
pending HTLCs.
- Also remove the min_prob_success option but hardcode a 90% value.
Removing some options that are not relevant to the user, they're kept
for developer mode only:
- base_fee_penalty
- min_prob_success
- prob_cost_factor
- remove heap.h, not used
Signed-off-by: Lagrang3 <eduardo.quintana@pm.me>
