Indeed, we can fall through this if it's not a valid enum value.
gcc-12 (Ubuntu 12.2.0-17ubuntu1) 12.2.0
```
In file included from plugins/commando.c:10:
ccan/ccan/tal/str/str.h: In function ‘rune_altern_to_english’:
ccan/ccan/tal/str/str.h:43:9: error: ‘cond_str’ may be used uninitialized [-Werror=maybe-uninitialized]
43 | tal_fmt_(ctx, TAL_LABEL(char, "[]"), __VA_ARGS__)
| ^~~~~~~~
plugins/commando.c:97:21: note: ‘cond_str’ was declared here
97 | const char *cond_str;
| ^~~~~~~~
cc1: all warnings being treated as errors
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
```
In function ‘peer_reconnect’,
inlined from ‘init_channel’ at channeld/channeld.c:5890:3,
inlined from ‘main’ at channeld/channeld.c:5951:2:
channeld/channeld.c:5028:21: error: ‘next_matches_inflight’ may be used uninitialized [-Werror=maybe-uninitialized]
5027 | if (remote_next_funding && !next_matches_current
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5028 | && !next_matches_inflight) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~
channeld/channeld.c: In function ‘main’:
channeld/channeld.c:4595:36: note: ‘next_matches_inflight’ was declared here
4595 | bool next_matches_current, next_matches_inflight;
| ^~~~~~~~~~~~~~~~~~~~~
channeld/channeld.c:5042:57: error: ‘inflight’ may be used uninitialized [-Werror=maybe-uninitialized]
5042 | &inflight->outpoint.txid),
| ^
channeld/channeld.c:4594:26: note: ‘inflight’ was declared here
4594 | struct inflight *inflight;
| ^~~~~~~~
cc1: all warnings being treated as errors
make: *** [Makefile:300: channeld/channeld.o] Error 1
```
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>
In spec commit 498f104fd399488c77f449d05cb21c0b604636a2 (August 2021),
Bastien Teinturier removed the requirement that the mutual close fee be
less than or equal the final commitment tx.
We adopted that change in v0.10.2, but we made sure to never offer a fee
under the final commitment tx's fee, so we didn't break older nodes.
However, the closing tx can actually be larger than the final commitment tx!
The final commit tx has a 22-byte P2WKH output and a 34-byte P2WSH output;
the closing can have two 34-byte outputs, making it 4*8 = 32 Sipa heavier.
Previously this would only happen if both sides asked for P2WSH outputs,
but now it happens with P2TR, which we now do.
The result is that we create a tx which is below the finally commitment
tx fee, and may be below minrelayfee (as it was in regtest).
So it's time to remove that backwards-compatibility hack.
Changelog-Fixed: Protocol: We may propose mutual close transaction which has a slightly higher fee than the final commitment tx (depending on the outputs, e.g. two taproot outputs).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: #6545
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>
dump_our_gossip() is mainly useful for propagating our gossip when we
are poorly connected, not when we have many peers. @whitslack
reported excessive memory use queueing messages on a large node, so we
limit it beyond the first 5 peers, to 5 channels each.
This assumes we have ~ the same number of peers as channels, which
is probably reasonable.
In the long term, we should move this to connectd, which is properly
equipped to trickle out these messages.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: #6540
This was recommended by @t-bast: if the final spec commits to something
compatible, we can simply advertize and accept both features, but if it
does change in incompatible ways we won't cause problems for nodes
who implement the official spec.
(I split this, so first, we remove the OPT_SPLICE entirely, to make
sure we caught them all. --RR)
Suggested-by: @t-bast
Changelog-None
We've stomped errno, so if exec fails we don't get a reliable result:
```
2023-08-07T17:58:45.713Z **BROKEN** plugin-bcli: bitcoin-cli exec failed: Bad file descriptor
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
EXPERIMENTAL_SPLICING=1 turns it on for *all* tests, to make sure we don't
accidentally break those. But we can (and should!) run the splice test
under every possible CI scenario.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The nomenclature confusion mean that we were ANDING a capability
with a message number (29) which always returned non-zero. We really
do need a new capability which we can hand to channeld to make these
splice txs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I obviously like the word "capabilities" since I reused it to refer
to the HSM's overall features :(
Suggested-by: @ksedgwic
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
See: https://github.com/bitcoindevkit/bdk/issues/1047#issuecomment-1660645669
In general, futures produced by most libraries in the ecosystem of Rust, and bounds placed
on users of famous runtimes like tokio and its spawn method all lack Sync requirements.
Because of this, anyone who creates a callback using any sort of library that returns a
non-Sync future (which most libraries fit this description) inside of it will get some
cryptic error messages (async error messages still leave a lot to be desired).
Removing these Sync requirements will make the library more useful.
Apparently MacOS doesn't always have fdatasync, so use fsync. Even more importantly
check whether it succeeds!
Fixes: #6516
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
