Matt Corallo [Mon, 12 Feb 2024 22:40:49 +0000 (22:40 +0000)]
Drop `ahash` dependency in favor of core's `SipHasher`
https://github.com/tkaitchuck/aHash/pull/196 bumped the MSRV of
`ahash` in a patch release, which makes it rather difficult for us
to have it as a dependency.
Further, it seems that `ahash` hasn't been particularly robust in
the past, notably
https://github.com/tkaitchuck/aHash/issues/163 and
https://github.com/tkaitchuck/aHash/issues/166.
Luckily, `core` provides `SipHasher` even on no-std (sadly its
SipHash-2-4 unlike the SipHash-1-3 used by the `DefaultHasher` in
`std`). Thus, we drop the `ahash` dependency entirely here and
simply wrap `SipHasher` for our `no-std` HashMaps.
Matt Corallo [Mon, 12 Feb 2024 22:37:09 +0000 (22:37 +0000)]
Add a crate which wraps `getrandom` but always compiles
In the next commit we'll drop the `ahash` dependency in favor of
directly calling `getrandom` to seed our hash tables. However,
we'd like to depend on `getrandom` only on certain platforms *and*
only when certain features (no-std) are set.
This introduces an indirection crate to do so, allowing us to
depend on it only when `no-std` is set but only depending on
`getrandom` on platforms which it supports.
Matt Corallo [Fri, 16 Feb 2024 19:26:22 +0000 (19:26 +0000)]
Fix `Route` serialization round-trip
When the `max_total_routing_fee_msat` parameter was added to
`RouteParameters`, the serialization used `map` to get the max fee,
accidentally writing an `Option<Option<u64>>`, but then read it as
an `Option<u64>`. Thus, any `Route`s with a `route_params` written
will fail to be read back.
Luckily, this is an incredibly rarely-used bit of code, so only one
user managed to hit it.
Matt Corallo [Fri, 16 Feb 2024 18:41:54 +0000 (18:41 +0000)]
Fix blinded path serialization in `Route`
`Route`'s blinded_path serialization logic writes a blinded path
`Option` per path hop, however on read we (correctly) only read one
blinded path `Option` per path. This causes serialization of
`Route`s with blinded paths to fail to round-trip.
Here we fix this by writing blinded paths per path.
Matt Corallo [Fri, 16 Feb 2024 18:33:53 +0000 (18:33 +0000)]
Drop the `fails_paying_for_bolt12_invoice` test
`fails_paying_for_bolt12_invoice` tests that we fail to send a
payment if the router returns `Ok` but includes a bogus route (one
with 0-length paths). While this marginally increases our test
coverage, in the next commit we'll be testing that all routes
round-trip serialization, which fails here as bogus routes are not
supported in deserialization.
Because this isn't particularly critical test coverage, we simply
opt to drop the test entirely here.
Matt Corallo [Tue, 13 Feb 2024 22:43:51 +0000 (22:43 +0000)]
Never store more than one `StdWaker` per live `Future`
When an `std::future::Future` is `poll()`ed, we're only supposed to
use the latest `Waker` provided. However, we currently push an
`StdWaker` onto our callback list every time `poll` is called,
waking every `Waker` but also using more and more memory until the
`Future` itself is woken.
Here we fix this by removing any `StdWaker`s stored for a given
`Future` when it is `drop`ped or prior to pushing a new `StdWaker`
onto the list when `poll`ed.
Sadly, the introduction of a `Drop` impl for `Future` means we
can't trivially destructure the struct any longer, causing a few
methods to need to take `Future`s by reference rather than
ownership and `clone` a few `Arc`s.
Matt Corallo [Tue, 13 Feb 2024 22:08:55 +0000 (22:08 +0000)]
Give `Future`s for a `FutureState` an idx and track `StdWaker` idxn
When an `std::future::Future` is `poll()`ed, we're only supposed to
use the latest `Waker` provided. However, we currently push an
`StdWaker` onto our callback list every time `poll` is called,
waking every `Waker` but also using more and more memory until the
`Future` itself is woken.
Here we take a step towards fixing this by giving each `Future` a
unique index and storing which `Future` an `StdWaker` came from in
the callback list. This sets us up to deduplicate `StdWaker`s by
`Future`s in the next commit.
Matt Corallo [Tue, 13 Feb 2024 21:58:46 +0000 (21:58 +0000)]
Split lists of `Waker` and directly-registered `Future` callbacks
In the next commit we'll fix a memory leak due to keeping too many
`std::task::Waker` callbacks in `FutureState` from redundant `poll`
calls, but first we need to split handling of `StdWaker`-based
future wake callbacks from normal ones, which we do here.
Matt Corallo [Tue, 13 Feb 2024 23:43:19 +0000 (23:43 +0000)]
Opportunistically skip log in `update_claims_view_from_matched_txn`
On each block, for each `ChannelMonitor`, we log two status
statements in `OnChainTx::update_claims_view_from_matched_txn`.
This can add up to quite a bit, and is generally not very
interesting when we don't actually do anything if there's no claims
to bump.
Here we drop both logs if we have no claims to work with, but
retain it if we process any claims.
Matt Corallo [Tue, 13 Feb 2024 23:33:18 +0000 (23:33 +0000)]
Opportunistically skip log in `update_claims_view_from_requests`
On each block, for each `ChannelMonitor`, we log a status statement
in `OnChainTx::update_claims_view_from_requests`. This can add up
to quite a bit, and is generally not very interesting when we don't
actually do anything if there's no claims to bump.
Here we drop the log if we have no claims to work with, but retain
it if we process any claims.
Matt Corallo [Tue, 13 Feb 2024 23:00:38 +0000 (23:00 +0000)]
Drop some "Channel does not qualify for a feerate change" logs
On a high-traffic/channel node, `Channel .* does not qualify for a
feerate change.*` is our most common log, and it doesn't provide
much useful information. It's logged in two cases - (a) where the
estimator feerate is less than the current channel feerate but not
by more than half twice and (b) where we'd like to update the
channel feerate but the peer is disconnected or channel not
available for updates.
Because these conditions can persist and we log them once a minute
the volume of logs can add up quickly. Here we simply remove the
log in case (a), though leave (b) as its anticipated to be somewhat
quieter and does indicate a persistent issue that should be
addressed (possibly by closing the channel).
Matt Corallo [Tue, 13 Feb 2024 22:57:18 +0000 (22:57 +0000)]
Make peers sending gossip out of order logging less scary
Multiple times we've had users wonder why they see `Error handling
message from.*; ignoring: Couldn't find channel for update` in
their logs and wonder if its related to their channel
force-closing. While this does indicate a peer is sending us gossip
our of order (and thus misbehaving), its not relevant to channel
operation and the logged message and level should indicate that.
Thus, here, we move the level to Gossip and add "gossip" between
"handling" and "message" (so it reads "Error handling gossip
message from.*").
Elias Rohrer [Tue, 30 Jan 2024 11:41:05 +0000 (12:41 +0100)]
Introduce CI workflow running `cargo audit`
In order to continuously monitor our dependencies for security
vulnerabilities, we introduce a new CI job that will use `cargo audit`
to check for any known vulnerabilities.
This job is run on a daily schedule. For each new advisory, a new issue
will be created.
Wilmer Paulino [Tue, 30 Jan 2024 22:45:46 +0000 (14:45 -0800)]
Allow holder commitment and HTLC signature requests to fail
As part of the ongoing async signer work, our holder signatures must
also be capable of being obtained asynchronously. We expose a new
`ChannelMonitor::signer_unblocked` method to retry pending onchain
claims by re-signing and rebroadcasting transactions. Unfortunately, we
cannot retry said claims without them being registered first, so if
we're not able to obtain the signature synchronously, we must return the
transaction as unsigned and ensure it is not broadcast.
Wilmer Paulino [Tue, 30 Jan 2024 22:45:44 +0000 (14:45 -0800)]
Rework get_latest_holder_commitment_txn to broadcast for users
This method is meant to be used as a last resort when a user is forced
to broadcast the current state, even if it is stale, in an attempt to
claim their funds in the channel. Previously, we'd return the commitment
and HTLC transactions such that they broadcast them themselves. Doing so
required a different code path, one which was not tested, to obtain
these transactions than our usual path when force closing. It's not
worth maintaining both, and it's much simpler for us to broadcast
instead.
Previously, we only had blanket impls for `KVStore`. However, in order
to enable the use of `dyn KVStore + Send + Sync` instead of a `KVStore`
generic, we here also add the corresponding blanket implementations for
said type signature.
Matt Corallo [Fri, 19 Jan 2024 00:42:17 +0000 (00:42 +0000)]
[fuzz] Add additional method calls in full_stack_target
The whole point of full_stack_target is to just expose our entire
API to the fuzzer and see what happens. Sadly, we're really only
exposing a small subset of our API. This improves that by exposing
a handful of other assorted methods from ChannelManager and
PeerManager.
Elias Rohrer [Mon, 5 Feb 2024 13:47:39 +0000 (14:47 +0100)]
Expose `channel_id` / `counterparty_node_id` in `BumpTransaction` event
A client node might choose not to handle `Event::BumptTransaction`
events and leave bumping / Anchor output spending to a trusted
counterparty.
However, `Event::BumptTransaction` currently doesn't offer any clear
indication what channel and/or counterparty it is referring to. In order
to allow filtering these events, we here expose the `channel_id` and
`counterparty_node_id` fields.
Willem Van Lint [Sat, 5 Aug 2023 00:07:04 +0000 (17:07 -0700)]
Include pending HTLCs in ChannelDetails
This exposes details around pending HTLCs in ChannelDetails. The state
of the HTLC in the state machine is also included, so it can be
determined which protocol message the HTLC is waiting for to advance.
Matt Corallo [Thu, 18 Jan 2024 22:39:26 +0000 (22:39 +0000)]
Bump `hashbrown` dependency to 0.13
While this isn't expected to materially improve performance, it
does get us ahash 0.8, which allows us to reduce fuzzing
randomness, making our fuzzers much happier.
Sadly, by default `ahash` no longer tries to autodetect a
randomness source, so we cannot simply rely on `hashbrown` to do
randomization for us, but rather have to also explicitly depend on
`ahash`.
Matt Corallo [Thu, 18 Jan 2024 20:27:57 +0000 (20:27 +0000)]
Stop relying on `Hash{Set,Map}::from_iter` directly
In the next commit we'll bump the `hashbrown` version, which no
longer randomizes its hasher by default. Thus, we'll need to call
a different constructor in no-std builds from std builds.
Here we do a quick prefactor to use wrappers for `FromIterator`
constructors instead of calling the tables directly to make the
version bump changeset smaller.
Matt Corallo [Mon, 1 Jan 2024 21:53:36 +0000 (21:53 +0000)]
Use utility methods to construct `HashMap`s and `HashSet`s
In the next commit we'll bump the `hashbrown` version, which no
longer randomizes its hasher by default. Thus, we'll need to call
a different constructor in no-std builds from std builds.
Here we do a quick prefactor to use wrappers for constructors
instead of calling the tables directly to make the version bump
changeset smaller.
Matt Corallo [Sat, 23 Dec 2023 19:13:34 +0000 (19:13 +0000)]
Use arbitrary config object in full_stack_target fuzzer
As we've added more and more configuration parameters which change
our behavior, we're increasingly missing coverage in the general
`full_stack_target` fuzzer. Sadly, a reachable `unwrap` slipped in
uncaught that should have been stopped by the `full_stack_target`.
Here, we update the `full_stack_target` fuzzer to read a full
config object to substantially increase coverage.
benthecarman [Fri, 2 Feb 2024 14:14:51 +0000 (14:14 +0000)]
Make DirectedChannelInfo `source` and `target` public
We use these params for our HubPreferentialScorer and without these
utility functions then we have to manually do this and calculate which
is the source and which is the target node.
Elias Rohrer [Mon, 29 Jan 2024 15:16:08 +0000 (16:16 +0100)]
Expose `skimmed_fee_msat` in `PaymentForwarded`
We generally allow routing nodes to forward less than the expected HTLC
amount, if the receiver knowingly accepts this and claims the
underpaying HTLC (see `ChannelConfig::accept_underpaying_htlcs`). This
use case is in particular useful for the LSPS2/JIT channel setting where
the intial underpaying HTLC pays for the channel open.
While we previously exposed the withheld amount as
`PaymentClaimable::counterparty_skimmed_fee_msat` on the receiver side,
we did not individually provide it on the forwarding node's side.
Here, we therefore expose this additionally withheld amount via
`PaymentForwarded::skimmed_fee_msat`.
shaavan [Wed, 31 Jan 2024 12:14:58 +0000 (17:44 +0530)]
Add tests to check the introduced behaviour
- The first test make sure that the OutboundV1Channel is not
immediately removed when peers disconnect, but is removed after N timer
ticks.
- The second test makes sure that the SendOpenChannel is rebroadcasted
for the OutboundV1Channel if peer reconnects within time.
shaavan [Sun, 10 Dec 2023 12:47:32 +0000 (18:17 +0530)]
Do not remove Outbound Channel immediately when peer disconnects
- Do not remove channel immediately when peer_disconnect, instead
removed it after some time if peer doesn't reconnect soon (handled in
previous commit).
- Do not mark per ok_to_remove if we have some OutboundV1Channels too.
- Rebroadcast SendOpenChannel for outboundV1Channel when peer
reconnects.
- Update the relevant tests to account for the behavior change.
- Repurpose the test_disconnect_in_funding_batch to test that all
channels in the batch close when one them closes.
Matt Corallo [Thu, 18 Jan 2024 20:53:20 +0000 (20:53 +0000)]
Move RGS `GraphSyncError` into the top-level module
The top-level module is only a few hundred lines, so there's not a
lot of reason to hide the `GraphSyncError` in its own module.
Instead, we simply move it to the top-level `lib.rs`, which doesn't
change the public API as it was previously re-exported at the top
level.