Currently, the "best source" for a given node tracked during
Dijkstra's is updated with a different critera from the heap
sorting, resulting in loops in calculated paths.
This adds a test for the specific failure currently seen, utilizing
the new path-htlc-minimum tracking in the heap entries in place of
the per-hop htlc-minimum values which the MPP changeset partially
used.
If we walk the network graph and find a route that meets are
payment amount and has a liquidiy limit far in excess of our
payment amount, we may select the same path several times in the
main path-gathering loop.
Instead, if the path we selected was not limited by the available
liquidity, we set the middle hop in the path's liquidity available
to zero, disabling that channel in future path finding steps.
Previously, we'd happily send funds through a path where, while
generating the path, in some middle hope we reduce the value being
sent to meet an htlc_maximum, making a later hop invalid due to it
no longer meeting its htlc_minimum. Instead, we need to track the
path's htlc-minimum while we're transiting the graph.
The new MPP routing algorithm attempts to build paths with a higher
value than our payment to find paths which may allow us to pay a
fee to meet an htlc_minimum limit. This is great if we're
min-bounded, however it results in us rejecting paths where we are
bounded by a maximum near the end of the path, with fees, and then
bounded by a minimum earlier in the path. Further, it means we will
not find the cheapest path where paths have a lower relative fee
but a higher absolute fee.
Instead, we calculate routes using the actual amount we wish to
send. To maintain the previous behavior of searching for cheaper
paths where we can "pay the difference" to meet an htlc_minimum, we
detect if we were minimum-bounded during graph walking and, if we
are, we walk the graph again with a higher value.
Useful for constructing route hints for private channels in invoices.
Co-authored-by: Valentine Wallace <vwallace@protonmail.com>
Co-authored-by: Antoine Riard <ariard@student.42.fr>
We currently only use it to override the graph-specific features
returned in the route, though we should also use it to enable or
disable MPP.
Note that tests which relied on MPP behavior have had all of their
get_route calls upgraded to provide the MPP flag.
We could have possibly constructed a slightly inconsistent
path: since we reduce value being transferred all the way, we
could have violated htlc_minimum_msat on some channels
we already passed (assuming dest->source direction). Here,
we recompute the fees again, so that if that's the case, we
match the currently underpaid htlc_minimum_msat with fees.
Sadly rust upstream never really figured out the benchmark story,
and it looks like the API we use here may not be long for this
world. Luckily, we can switch to criterion with largely the same
API if that happens before upstream finishes ongoing work with the
custom test framework stuff.
Sadly, it requires fetching the current network graph, which I did
using Val's route-testing script written to test the MPP router.
This (finally) exposes `ChannelManager`/`ChannelMonitor` _write
methods, which were (needlessly) excluded as the structs themselves
have generic parameters. Sadly, we also now need to parse
`(C-not exported)` doc comments on impl blocks as we otherwise try
to expose _write methods for `&Vec<RouteHop>`, which doesn't work
(and isn't particularly interesting for users anyway). We add such
doc comments there.
This changes adds the genesis block hash as a BlockHash to the
NetworkGraph struct. Making the NetworkGraph aware allows the message
handler to validate the chain_hash for received messages. This change
also adds the hash value to the Writeable and Readable methods.
We had code in the router to support sending a payment via a single
hop across channels exclusively provided by the next-/last-hop hints.
However, in updating the fuzzer, I noted that this case not only
didn't work, but paniced in some cases.
Here, we both fix the panic, as well as write a new test which
ensures we don't break support for such routing in the future.
ChainWatchInterface was intended as an interface for watching rather
than accessing the chain. Remove get_chain_utxo and add chain::Access
trait for this behavior. Wrap it with an Option in NetGraphMsgHandler in
order to simplify the error interface.
Because the C bindings maps objects into new structs which contain
only a pointer to the underlying (immovable) Rust type, it cannot
create a list of Rust types which are contiguous in memory. Thus,
in order to allow C clients to call certain Rust functions, we have
to use &[&Type] not &[Type]. This commit fixes this issue for the
get_route function.
* Splits up the monolithic test into smaller unit tests
* Factors out helpers for graph setup
* Changes `id_to_feature_flags` to be a function, there was no
reason why it had to be a macro
* Activates a previously commented-out test that checks for
the failure case in `disable_node_test`
This changes the LICENSE file and adds license headers to most files
to relicense under dual Apache-2.0 and MIT. This is helpful in that
we retain the patent grant issued under Apache-2.0-licensed work,
avoiding some sticky patent issues, while still allowing users who
are more comfortable with the simpler MIT license to use that.
See https://github.com/rust-bitcoin/rust-lightning/issues/659 for
relicensing statements from code authors.
... for ChannelError and APIMisuseError
Before this commit, When rl returns error, we don't know
The actual parameter which caused the error.
By returning parameterised `String` instead of predefined `&'static str`,
We can give a caller improved error message.
TestLogger now has two additional methods
1. `assert_log_contains` which checks the logged messsage
has how many entry which includes the specified string as a substring.
2. `aasert_log_regex` mostly the same with `assert_log_contains`
but it is more flexible that caller specifies regex which has
to be satisfied instead of just a substring.
For regex, tests now includes `regex` as dev-dependency.
This was just an oversight when route calculation was split up into
parts - it makes no sense for get_route to require that we have a
full route message handler, only a network graph (which can always
be accessed from a NetGraphMsgHandler anyway).
This caused a bunch of cascading changes, including
passing loggers down to Channels in function calls
rather than having each Channel have a pointer to the
ChannelManager's Logger (which was a circular reference).
Other structs that the Channel had passed its Logger to also
had their loggers removed. Other newly unused Loggers were
also removed, especially when keeping them would've caused
a bunch of extra test changes to be necessary, e.g. with
the ChainWatchInterfaceUtil's Logger.
