In a synchronous `BackgroundProcessor`, the exit is done by setting
an atomic flag, which is most likely to happen while we're asleep.
Thus, we previously checked for the exit condition after the sleep
(and after we persisted the `ChannelManager`, if required, though
this is no longer required and dates back to when we didn't do a
re-persist after breaking out of the main loop).
For an async `background-processor`, this is also fine, however
because of the relatively longer sleep time, if the exit flag is
set via a sleep check returning true during event processing, we
may end up delaying exit rather substantially.
In order to avoid this, we simply check for the exit condition both
before and immediately after the sleep in `background-processor`.
In a future commit, we plan to correctly enforce that the spending
transaction has a valid locktime relative to the chain for the node
broascasting it in `TestBroadcaster::broadcast_transaction` to. We catch
up these test node instances to their expected height, such that we do
not fail said enforcement.
Unfortunately, the RAII types used by `RwLock` are not `Send`, which is
why they can't be held over `await` boundaries. In order to allow
asynchronous events processing in multi-threaded environments, we here
allow to process events without holding the `total_consistency_lock`.
This fixes two potential panics within the test if the
`BackgroundProcessor` for `nodes[0]` consumed the `ChannelPending` event
prior to us consuming it manually in `end_open_channel`. The first panic
would happen within the event handler, since `ChannelPending` was not
being handled. The second panic would happen upon expecting the
`ChannelPending` event after handling `nodes[1]`'s `funding_signed` if
the `BackgroundProcessor` handled the event first. To ensure we still
reliably receive a `ChannelPending` event once possible, we let the
`BackgroundProcessor` consume the event and notify it.
If the user's sleep future passed to an async background processor
only returns true for exiting once and then reverts back to false,
we should exit anyway when we get a chance to. We do to this here
by always ensuring we check the exit flag even when only polling
sleep futures with no intent to (yet) exit. This is utilized in the
tests added in the coming commit(s).
If `ChannelManager` is persistable before the async background
processor even starts, it may not even get around to overwriting
the `should_exit` flag before testing it, and the default value is
(incorrectly) true, causing an immediate unconditional exit.
The default value should simply be false.
Fixes#2140
Instead of asserting a `Result` `is_ok`, we should always simply
`unwrap` to get a backgrace, and we should avoid doing so if the
thread is already panicking.
Currently, users don't have good way of being notified when channel open
negotiations have succeeded and new channels are pending confirmation on
chain. To this end, we add a new `ChannelPending` event that is emitted
when send or receive a `funding_signed` message, i.e., at the last
moment before waiting for the confirmation period.
We track whether the event had previously been emitted in `Channel` and
remove it from `internal_funding_created` entirely. Hence, we now
only emit the event after ChannelMonitorUpdate completion, or upon
channel reestablish. This mitigates a race condition where where we
wouldn't persist the event *and* wouldn't regenerate it on restart,
therefore potentially losing it, if async CMU wouldn't complete before
ChannelManager persistence.
Some users have suggested that waking every 100ms can be
CPU-intensive in deployments with hundreds or thousands of nodes
all running on the same machine. Thus, we add an option to the
futures-based `background-processor` to avoid waking every 100ms to
check for iOS having backgrounded our app and cut our TCP sockets.
This cuts the normal sleep time down from 100ms to 10s, for those
who turn it on.
If the `ChainMonitor` gets an async monitor update completion, this
means the `ChannelManager` needs to be polled for event processing.
Here we wake it using the new multi-`Future`-await `Sleeper`, or
the existing `select` block in the async BP.
Fixes#2052.
Rather than having three ways to await a `ChannelManager` being
persistable, this moves to just exposing the awaitable `Future` and
having sleep functions on that.
As `futures` apparently makes no guarantees on MSRVs even in patch
releases we really can't rely on it at all, and while it currently
has an acceptable MSRV without the macros feature, its best to just
remove it wholesale.
Luckily, removing it is relatively trivial, even if it requires
the most trivial of unsafe tags.
`futures` recently broke our MSRV by bumping the `syn` major
version in a patch release. This makes it impractical for us to
use, instead here we replace the usage of its `select_biased` macro
with a trivial enum.
Given its simplicity we likely should have done this without ever
taking the dependency.
This is largely motivated by some follow-up work for anchors that will
introduce an event handler for `BumpTransaction` events, which we can
now include in this new top-level `events` module.
`poll`ing completed futures invokes undefined behavior in Rust
(panics, etc, obviously not memory corruption as its not unsafe).
Sadly, in our futures-based version of
`lightning-background-processor` we have one case where we can
`poll` a completed future - if the timer for the network graph
prune + persist completes without a network graph to prune +
persist we'll happily poll the same future over and over again,
likely panicing in user code.
This field was previous useful in manual retries for users to know when all
paths of a payment have failed and it is safe to retry. Now that we support
automatic retries in ChannelManager and no longer support manual retries, the
field is no longer useful.
For backwards compat, we now always write false for this field. If we didn't do
this, previous versions would default this field's value to true, which can be
problematic because some clients have relied on the field to indicate when a
full payment retry is safe.
Forcing users to pass a genesis block hash has ended up being
error-prone largely due to byte-swapping questions for bindings
users. Further, our API is currently inconsistent - in
`ChannelManager` we take a `Bitcoin::Network` but in `NetworkGraph`
we take the genesis block hash.
Luckily `NetworkGraph` is the only remaining place where we require
users pass the genesis block hash, so swapping it for a `Network`
is a simple change.
The `chain::Access` trait (and the `chain::AccessError` enum) is a
bit strange - it only really makes sense if users import it via the
`chain` module, otherwise they're left with a trait just called
`Access`. Worse, for bindings users its always just called
`Access`, in part because many downstream languages don't have a
mechanism to import a module and then refer to it.
Further, its stuck dangling in the `chain` top-level mod.rs file,
sitting in a module that doesn't use it at all (it's only used in
`routing::gossip`).
Instead, we give it its full name - `UtxoLookup` (and rename the
error enum `UtxoLookupError`) and put it in the a new
`routing::utxo` module, next to `routing::gossip`.
Secrets should not be exposed in-memory at the interface level as it
would be impossible the implement it against a hardware security
module/secure element.
This makes `background-processor` build without `std` at all. This
isn't particularly useful in the general no-std case as
`background-processor` is only useful with the `futures` feature,
and async will generally need `std` in some way or another. Still,
it ensures we don't end up reintroducing a dependency on the
current time, which breaks `wasm` use-cases.
`background-processor` does a number of jobs on various timers.
Instead of doing those by interrogating `std::time::Instant`, this
change swaps to using the existing user-provided sleep future.
Fixes#1864.
`background-processor` does a number of jobs on various timers.
Currently, those are all done by checking the timers every 100ms
by interrogating `std::time::Instant`. This is fine for the
threaded version, but we'd like more flexibility in the `futures`-
based `background-processor`.
Here we swap the `std::time::Instant` interrogation for a lambda
which we will switch out to the user-provided sleeper in the next
commit.
As of HEAD the `ChannelManager` is parametrized by a `Router`, while
`InvoicePayer` also owns a `Router`. In order to allow for a single
object being reused, we make the `InvoicePayer` side `Deref`.
This is purely a refactor that does not change the InitFeatures
advertised by a ChannelManager. This allows users to configure which
features should be advertised based on the values of `UserConfig`. While
there aren't any existing features currently leveraging this behavior,
it will be used by the upcoming anchors_zero_fee_htlc_tx feature.
The UserConfig dependency on provided_init_features caused most
callsites of the main test methods responsible for opening channels to
be updated. This commit foregos that completely by no longer requiring
the InitFeatures of each side to be provided to these methods. The
methods already require a reference to each node's ChannelManager to
open the channel, so we use that same reference to obtain their
InitFeatures. A way to override such features was required for some
tests, so a new `override_init_features` config option now exists on
the test harness.
If no network graph is provided to the `BackgroundProcessor`, we
log every time the processor loop goes around (at least every
100ms, if not more) which fille up logs with useless indications
that we have no network graph.
This adds testing of the `futures` feature to CI. In order to avoid
introducing a dependency on `std`, we switch to using the `futures-util`
crate directly enabling only a subset of features. To this end, we also
switch to using the `select_biased!` macro, which is equivalent to
`select!` except that it doesn't choose ready futures pseudo-randomly
at runtime.
We never had the `NetworkGraph::node_failed` method implemented. The
scorer now handles non-permanent failures by downgrading nodes, so we
don't want that implemented.
The method is renamed to `node_failed_permanent` to explicitly indicate
that this is the only case it handles. We also add tracking in the form
of two maps as fields of `NetworkGraph`, namely, `removed_nodes` and
`removed_channels`. We track these removed graph entries to ensure we
don't just resync them right away from gossip soon after removing them.
We stop tracking these removed nodes whenever `remove_stale_channels_and_tracking()`
is called and the entries have been tracked for longer than
`REMOVED_ENTRIES_TRACKING_AGE_LIMIT_SECS` which is currently set to one
week.
We do this to enable users to create routers that do not need a scorer.
This can be useful if they are running a node the delegates pathfinding.
* Move `Score` type parameterization from `InvoicePayer` and `Router` to
`DefaultRouter`
* Adds a new field, `scorer`, to `DefaultRouter`
* Move `AccountsForInFlightHtlcs` to `DefaultRouter`, which we
will use to wrap the new `scorer` field, so scoring only happens in
`DefaultRouter` explicitly.
* Add scoring related functions to `Router` trait that we used to call
directly from `InvoicePayer`.
* Instead of parameterizing `scorer` in `find_route`, we replace it with
inflight_map so `InvoicePayer` can pass on information about inflight
HTLCs to the router.
* Introduced a new tuple struct, InFlightHtlcs, that wraps functionality
for querying used liquidity.
As we move towards specify supported/required feature bits in the
module(s) where they are supported, the global `known` feature set
constructors no longer make sense.
Here we stop relying on the `known` method in the
`lightning-background-processor` and `lightning-persister` crate
tests.
Some `NodeFeatures` will, in the future, represent features which
are not enabled by the `ChannelManager`, but by other message
handlers handlers. Thus, it doesn't make sense to determine the
node feature bits in the `ChannelManager`.
The simplest fix for this is to change to generating the
node_announcement in `PeerManager`, asking all the connected
handlers which feature bits they support and simply OR'ing them
together. While this may not be sufficient in the future as it
doesn't consider feature bit dependencies, support for those could
be handled at the feature level in the future.
This commit moves the `broadcast_node_announcement` function to
`PeerHandler` but does not yet implement feature OR'ing.
Adds the boilerplate needed for PeerManager and OnionMessenger to work
together, with some corresponding docs and misc updates mostly due to the
PeerManager public API changing.
