Matt Corallo [Sat, 29 Apr 2023 18:45:59 +0000 (18:45 +0000)]
Expose a trait impl'd for all `PeerManager` for use as a bound
A while back, in tests, we added a `AChannelManager` trait, which
is implemented for all `ChannelManager`s, and can be used as a
bound when we need a `ChannelManager`, rather than having to
duplicate all the bounds of `ChannelManager` everywhere.
Here we do the same thing for `PeerManager`, but make it public and
use it to clean up `lightning-net-tokio` and
`lightning-background-processor`.
We should likely do the same for `AChannelManager`, but that's left
as a followup.
Duncan Dean [Mon, 1 May 2023 20:52:30 +0000 (22:52 +0200)]
Remove `OptionalField` and move `shutdown_scriptpubkey` into TLV stream
As pointed out in https://github.com/lightning/bolts/pull/754/commits/6656b70,
we can move the `shutdown_scriptpubkey` field into the TLV streams of
`OpenChannel` and `AcceptChannel` without affecting the resulting encoding.
We use `WithoutLength` encoding here to ensure that we do not encode a
length prefix along with `Script` as is normally the case.
Duncan Dean [Mon, 1 May 2023 20:23:20 +0000 (22:23 +0200)]
Make `DataLossProtect` fields required and remove wrappers
The fields provided by `DataLossProtect` have been mandatory since
https://github.com/lightning/bolts/pull/754/commits/6656b70, regardless
of whether `option_dataloss_protect` or `option_remote_key` feature bits
are set.
We move the fields out of `DataLossProtect` to make encoding definitions
more succinct with `impl_writeable_msg!` and to reduce boilerplate.
This paves the way for completely removing `OptionalField` in subsequent
commits.
Matt Corallo [Sat, 29 Apr 2023 17:58:15 +0000 (17:58 +0000)]
Move the `CustomMessageHandler` into the `MessageHandler` struct
`PeerManager` takes a `MessageHandler` struct which contains all
the known message handlers for it to pass messages to. It then,
separately, takes a `CustomMessageHandler`. This makes no sense, we
should simply include the `CustomMessageHandler` in the
`MessageHandler` struct for consistency.
Matt Corallo [Fri, 28 Apr 2023 16:19:04 +0000 (16:19 +0000)]
Fix overflow in `awaiting_pong_timer...` with too many peers
If we have more than
127 / `MAX_BUFFER_DRAIN_TICK_INTERVALS_PER_PEER` (31) peers,
`awaiting_pong_timer_tick_intervals` can overflow before we hit
the limit. This isn't super harmful, we'll still disconnect peers
as long as they don't send *any* messages between two pings, but it
does cause us to not disconnect peers which are extremely slow in
responding to messages, e.g. because they are overloaded.
Duncan Dean [Wed, 26 Apr 2023 12:57:18 +0000 (14:57 +0200)]
Use `env::temp_dir()` for BP tests
Currently `BackgroundProcessor` tests create persister directories in the
current working directory and rely on cleaning up in a `Drop` implementation.
Unfortunately, it seems that in the async tests that nodes are not
`drop()`ed for some reason and so the directories created by those
tests remain behind in the current working directory.
This commit at least ensures that these test directories are created in
a temporary location for the OS using `temp_dir()`. It doesn't aim to
solve the lack of cleanup in the async tests.
Partial fix for #2224 but I believe it's enough to resolve it as these
temp directories that do remain will be purged by the OS at some stage
and are overwritten by subsequent tests if there is a conflict.
Matt Corallo [Wed, 26 Apr 2023 05:01:13 +0000 (05:01 +0000)]
Fix a leak in `FutureState` when a `Notifier` is dropped un-woken
If a `Notifier` has an internal `FutureState` which gathers some
sleeper callbacks, but is never actaully woken, those callbacks
will leak due to a circular `Arc` reference when the `Notifier` is
`drop`'d.
Because `Notifier`s are rarely `drop`'d in production this isn't a
huge deal, but shows up materially in bindings tests as they spawn
many nodes over the course of a short test.
Matt Corallo [Sun, 23 Apr 2023 16:17:29 +0000 (16:17 +0000)]
Don't remove nodes if there's no channel_update for a temp failure
Previously, we were requiring any `UPDATE` onion errors to include
a `channel_update`, as the spec mandates[1]. If we see an onion
error which is missing one we treat it as a misbehaving node that
isn't behaving according to the spec and simply remove the node.
Sadly, it appears at least some versions of CLN are such nodes, and
opt to not include `channel_update` at all if they're returning a
`temporary_channel_failure`. This causes us to completely remove
CLN nodes from our graph after they fail to forward our HTLC.
While CLN is violating the spec here, there's not a lot of reason
to not allow it, so we go ahead and do so here, treating it simply
as any other failure by letting the scorer handle it.
[1] The spec says `Please note that the channel_update field is
mandatory in messages whose failure_code includes the UPDATE flag`
however doesn't repeat it in the requirements section so its not
crazy that someone missed it when implementing.
Matt Corallo [Mon, 24 Apr 2023 03:48:42 +0000 (03:48 +0000)]
Check for `background-processor` exit condition before+after sleep
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`.
While these transactions were still valid, we incorrectly assumed that
they would propagate with a locktime of `current_height + 1`, when in
reality, only those with a locktime strictly lower than the next height
in the chain are allowed to enter the mempool.
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.
Use current height when generating claims on block_disconnected
The `height` argument passed to `OnchainTxHandler::block_disconnected`
represents the height being disconnected, and not the current height.
Due to the incorrect assumption, we'd generate a claim with a locktime
in the future.
Ultimately, we shouldn't be generating claims within
`block_disconnected`. Rather, we should retry the claim at a later block
height, since the bitcoin blockchain does not ever roll back without
connecting a new block. Addressing this is left for future work.
Implement pending claim rebroadcast on force-closed channels
This attempts to rebroadcast/fee-bump each pending claim a monitor is
tracking for a force-closed channel. This is crucial in preventing
certain classes of pinning attacks and ensures reliability if
broadcasting fails. For implementations of `FeeEstimator` that also
support mempool fee estimation, we may broadcast a fee-bumped claim
instead, ensuring we can also react to mempool fee spikes between
blocks.
Extend OnchainTxHandler::generate_claim to optionally force feerate bump
In the next commit, we plan to extend the `OnchainTxHandler` to retry
pending claims on a timer. This timer may fire with much more frequency
than incoming blocks, so we want to avoid manually bumping feerates
(currently by 25%) each time our fee estimator provides a lower feerate
than before.
Elias Rohrer [Fri, 21 Apr 2023 16:02:54 +0000 (18:02 +0200)]
Allow events processing without holding `total_consistency_lock`
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`.
Matt Corallo [Fri, 21 Apr 2023 14:39:01 +0000 (14:39 +0000)]
Clarify the error message when we disconnect a peer
We very regularly receive confusion over the super generic
"Peer sent invalid data or we decided to disconnect due to a
protocol error" message, which doesn't say very much. Usually, we
end up disconnecting because we have a duplicate connection with a
peer, which doesn't merit such a scary message.
Instead, here we clarify the error message to just refer to the
fact that we're disconnecting, and note that its usually a dup
connection in a parenthetical.
To match the local signatures found in test vectors, we must make sure
we don't use any additional randomess when generating signatures, as
we'll arrive at a different signature otherwise.
Generate local signatures with additional randomness
Previously, our local signatures would always be deterministic, whether
we'd grind for low R value signatures or not. For peers supporting
SegWit, Bitcoin Core will generally use a transaction's witness-txid, as
opposed to its txid, to advertise transactions. Therefore, to ensure a
transaction has the best chance to propagate across node mempools in the
network, each of its broadcast attempts should have a unique/distinct
witness-txid, which we can achieve by introducing random nonce data when
generating local signatures, such that they are no longer deterministic.
For offers where the signing pubkey is derived, the keys need to be
extracted from the Offer::metadata in order to sign an invoice.
Parameterize InvoiceBuilder such that a build_and_sign method is
available for this situation.
Jeffrey Czyz [Mon, 6 Feb 2023 21:30:44 +0000 (15:30 -0600)]
Stateless verification of Invoice for Refund
Stateless verification of Invoice for Offer
Verify that an Invoice was produced from a Refund constructed by the
payer using the payer metadata reflected in the Invoice. The payer
metadata consists of a 128-bit encrypted nonce and possibly a 256-bit
HMAC over the nonce and Refund TLV records (excluding the payer id)
using an ExpandedKey.
Thus, the HMAC can be reproduced from the refund bytes using the nonce
and the original ExpandedKey, and then checked against the metadata. If
metadata does not contain an HMAC, then the reproduced HMAC was used to
form the signing keys, and thus can be checked against the payer id.
Jeffrey Czyz [Mon, 6 Feb 2023 21:10:07 +0000 (15:10 -0600)]
Refund metadata and payer id derivation
Add support for deriving a transient payer id for each Refund from an
ExpandedKey and a nonce. This facilitates payer privacy by not tying any
Refund to any other nor to the payer's node id.
Additionally, support stateless Invoice verification by setting payer
metadata using an HMAC over the nonce and the remaining TLV records,
which will be later verified when receiving an Invoice response.
Jeffrey Czyz [Mon, 30 Jan 2023 20:57:43 +0000 (14:57 -0600)]
Stateless verification of Invoice for Offer
Verify that an Invoice was produced from an InvoiceRequest constructed
by the payer using the payer metadata reflected in the Invoice. The
payer metadata consists of a 128-bit encrypted nonce and possibly a
256-bit HMAC over the nonce and InvoiceRequest TLV records (excluding
the payer id) using an ExpandedKey.
Thus, the HMAC can be reproduced from the invoice request bytes using
the nonce and the original ExpandedKey, and then checked against the
metadata. If metadata does not contain an HMAC, then the reproduced HMAC
was used to form the signing keys, and thus can be checked against the
payer id.
Jeffrey Czyz [Mon, 30 Jan 2023 20:56:42 +0000 (14:56 -0600)]
InvoiceRequest metadata and payer id derivation
Add support for deriving a transient payer id for each InvoiceRequest
from an ExpandedKey and a nonce. This facilitates payer privacy by not
tying any InvoiceRequest to any other nor to the payer's node id.
Additionally, support stateless Invoice verification by setting payer
metadata using an HMAC over the nonce and the remaining TLV records,
which will be later verified when receiving an Invoice response.
Jeffrey Czyz [Fri, 10 Mar 2023 23:12:12 +0000 (17:12 -0600)]
Refactor InvoiceRequestContents fields into a sub-struct
InvoiceRequestBuilder has a field containing InvoiceRequestContents.
When deriving the payer_id from the remaining fields, a struct is needed
without payer_id as it not optional. Refactor InvoiceRequestContents to
have an inner struct without the payer_id such that
InvoiceRequestBuilder can use it instead.
Jeffrey Czyz [Wed, 8 Feb 2023 01:15:44 +0000 (19:15 -0600)]
Stateless verification of InvoiceRequest
Verify that an InvoiceRequest was produced from an Offer constructed by
the recipient using the Offer metadata reflected in the InvoiceRequest.
The Offer metadata consists of a 128-bit encrypted nonce and possibly a
256-bit HMAC over the nonce and Offer TLV records (excluding the signing
pubkey) using an ExpandedKey.
Thus, the HMAC can be reproduced from the offer bytes using the nonce
and the original ExpandedKey, and then checked against the metadata. If
metadata does not contain an HMAC, then the reproduced HMAC was used to
form the signing keys, and thus can be checked against the signing
pubkey.