Avoids a hash160 for every key we derive to test. Callers that need the
key re-derive it without the skip flag, so there are no side effects
from this optimization.
Changelog-Changed: core: Processing blocks should now be faster
Signed-off-by: Jon Griffiths <jon_p_griffiths@yahoo.com>
Standardizes the is_xxx script function all take a script length, and changes
their first-level callers to pass it. This has several knock on benefits:
- We remove the repeated tal_count/tal_bytelen calls on the script, in
particular the redundant calls that result when we must check for multiple
types of script - which is almost all cases.
- We remove the dependency on the memory being tal-allocated (It is, in
all cases, but theres no reason we need to require that).
- We remove all cases where we create a copy of the script just to id it.
- We remove all allocations for non-interesting scripts while iterating block
txs in process_getfilteredblock_step1().
- We remove all allocations *including for potentially interesting scripts* in
topo_add_utxos().
Signed-off-by: Jon Griffiths <jon_p_griffiths@yahoo.com>
The is_xxx script functions already perform these checks for us without
allocating. They currently expect their memory to be tal-allocated,
which is why a copy is made. However:
- The memory already *is* tal-allocated since wally is configured to do so.
- The tal-dependency is artifical and is removed in a future commit in
this PR.
This reverts commit c329756723.
Signed-off-by: Jon Griffiths <jon_p_griffiths@yahoo.com>
In some weird situations it may happen that some channel along the route
could have htlcmax=htlcmin, so that the supremum of htlcmin and the
infimum of htlcmax are the same number. In that case there is only one
allowed amount that can go through that route.
Without this patch renepay would not handle correctly this cornercase.
Set up a simple line of channel pairs, where one should be preferred
over the other for our various reasons. Make sure this works, both
using a low-level call to Dijkstra and at a higher level as the pay
plugin does.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I noticed that run-route-infloop chose some worse-looking paths after
routing was fixed, eg the second node:
Before:
Destination node, success, probability, hops, fees, cltv, scid...
02b3aa1e4ed31be83cca4bd367b2c01e39502cb25e282a9b4520ad376a1ba0a01a,1,0.991856,2,1004,40,2572260x39x0/1,2131897x45x0/0
After:
Destination node, success, probability, hops, fees, cltv, scid...
02b3aa1e4ed31be83cca4bd367b2c01e39502cb25e282a9b4520ad376a1ba0a01a,1,0.954540,3,1046,46,2570715x21x0/1,2346882x26x14/1,2131897x45x0/0
This is because although the final costs don't reflect it, routing was taking
into account local channels, and 2572260x39x0/1 has a base fee of 2970.
There's an easy fix: when we the pay plugin creates localmods for our
gossip graph, add all local channels with delay and fees equal to 0.
We do the same thing in our unit test. This improves things across
the board:
Linear success probability (when found): min-max(mean +/- stddev)
Before: 0.487040-0.999543(0.952548+/-0.075)
After: 0.486985-0.999750(0.975978+/-0.053)
Hops:
Before: 1-5(2.98374+/-0.77)
After: 1-5(2.09593+/-0.63)
Fees:
Before: 0-50848(922.457+/-2.7e+03)
After: 0-50041(861.621+/-2.7e+03)
Delay (blocks):
Before: 0-196(65.8081+/-60)
After: 0-190(60.3285+/-60)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Plugins: `pay` route algorithm doesn't bias against our own "expensive" channels any more.
The "path_score" callback was supposed to evaluate the *entire path*,
but that was counter-intuitive and opened the door to a cost function
bug which caused this path cost to be less than the closer path.
In particular, the capacity bias code didn't understand this at all.
1. Rename the function to `channel_score` and remove the "distance"
parameter (always "1" since you're supposed to be evaluating a
single hop).
2. Rename "cost" to the more specific "fee": "score" is our
actual cost function result (we avoid the word "cost" as it
may get confused with satoshi amounts).
3. For capacity biassing, we do want to know the amount, but
explicitly hand that as a separate parameter "total".
4. Fix a minor bug where total handed to scoring function previously
included channel fee (this is wrong: fee is paid before sending into
channel).
5. Remove the now-unused total_delay member from the dijkstra
struct.
Here are the results of our test now (routing 4194303 msat, which
didn't crash the old code, so we could compare). In both cases
we could find routes to 615 nodes:
Linear success probability (when found): min-max(mean +/- stddev)
Before: 0.484764-0.999750(0.9781+/-0.049)
After: 0.487040-0.999543(0.952548+/-0.075)
Hops:
Before: 1-5(2.13821+/-0.66)
After: 1-5(2.98374+/-0.77)
Fees:
Before: 0-50041(2173.75+/-5.3e+03)
After: 0-50848(922.457+/-2.7e+03)
Delay (blocks):
Before: 0-294(83.1642+/-68)
After: 0-196(65.8081+/-60)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: https://github.com/ElementsProject/lightning/issues/7092
Changelog-Fixed: Plugins: `pay` would occasionally crash on routing.
Changelog-Fixed: Plugins: `pay` route algorithm fixed and refined to balance fees and capacity far better.
It gave 0. A lot.
Firstly, rmsat was often very small, because delays are often small. Much
smaller than the actual fee.
We really just want to offset the bias and multiply it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We attempted to introduce a "capacity bias" so we would penalize
channels where we were using a large amount of their total capacity.
Unfortunately, this was both naive, and applied wrongly:
-log((capmsat + 1 - amtmsat) / (capmsat + 1));
As an integer gives 0 up to about 65% capacity. We then applied this
by multiplying:
(cmsat * rmsat * bias) / (cmsat + rmsat + bias + 1);
Giving a total score of 0 in these cases! If we're using the
arithmetic mean we really need to add 1 to bias. We might as well
use a double the whole way through, for a slightly more fine-grained
approach.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The amount is set not to crash by default, but run
"common/test/run-route-infloop 8388607" and you'll see a crash.
Sorry about the 7MB blob, but this testing was quite revealing and
I consider it worth adding.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Wrote a test program which passed num_channel_updates_rejected as NULL
(which we don't usually do), and valgrind complained:
```
==1048302== Conditional jump or move depends on uninitialised value(s)
==1048302== at 0x118B90: update_channel (gossmap.c:550)
==1048302== by 0x119EEE: map_catchup (gossmap.c:663)
==1048302== by 0x11A299: load_gossip_store (gossmap.c:726)
==1048302== by 0x11A352: gossmap_load (gossmap.c:1052)
==1048302== by 0x125362: main (run-route-infloop.c:90)
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
So far we would call `preapproveinvoice` once for each payment split,
i.e., at least once per HTLC, and potentially more often. There is no
point in doing so repeatedly, and especially in remote signer setup
this is actually counterproductive due to the additional roundtrips.
Changelog-Changed pay: Improved performance by removing repeated `preapproveinvoice` calls
We have installation instructions that tell the user to use `poetry`
and then we ourselves think we're clever and install only a known
subset? It was only a matter of time until we broke this.
Changelog-None
Thanks to amazing debugging assistance from grubles, we figured out
that indeed, my memory was correct: write and mmap are not consistent
on all platforms. The easiest fix is to disable mmap on OpenBSD for now:
the better fix is to do in-place updates using the mmap, and only rely
on write() for append (which always causes a remap anyway before it's accessed).
Fixes: https://github.com/ElementsProject/lightning/issues/7109
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This workflow has never had a particularly good signal-to-noise ratio,
and disabling it saves us a couple of GH Actions cycles, with only
minor reduction in test reach. We should rather rethink this and use
the installation instructions to write Dockerfiles for each described
architecture, and then have the CI test-build those Dockerfiles. That
would also cover the docs during testing, rather than having yet another
place where the instructions can bitrot away.
Changelog-None No user-visible change.
Increasing the min version of the hsmd due that we
added new code that required the hsmd to sign an announcements.
One of the solution is to increase the min version in this way
a signer like VLS fails directly during the init phase.
Link: https://github.com/ElementsProject/lightning/issues/7074
Changelog-None: hsmd: increase the min version
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Due to the API ratelimit, this allows cloning a github repo and searching
the result rather than searching via the REST API. If a source has
already been cloned, it is fetched and the default branch checked out.
Fixes a failure reported by @farscapian
Changelog-Fixed: Reckless no longer fails on github API ratelimit.
Github API calls are now ratelimited to 60 per hour. Searching through
the subdirectory contents of lightningd/plugins will hit this limit after
two searches or installs. The simple answer is to look for the directory
rather than verifying a valid entrypoint in a suitably-named directory
prior to cloning the repository.
Also corrects a bug that was flagging submodules as files while
populating directory contents.
We were extracting the output script for all outputs, and discarding
them immediately again if they were not P2WSH outputs which are the
ones of interest to us. This patch move the extraction until after we
have determined it is useful, and so we should save a couple thousand
`tal()` and `tal_free()` calls.
Changelog-Changed: lightningd: Speed up blocksync by not parsing unused parts of the transactions
Processing blocks is rather slow at the moment, but one thing we can
do, is to prevent copying all output scripts, when really all we are
interested in are the couple of outputs that are P2WSH.
This builds the foundation of that by adding a method to introspect
the script without having to clone it first, saving us some
allocations, and deallocations.
Changelog-Changed: core: Processing blocks should now be faster
You otherwise get thousands of these zero count Splice resume checks
spamming your logs while most of the time it is only informing you
of "nothing is happening".
Reduces log noise.
Signed-off-by: Warren Togami <wtogami@gmail.com>
