`AtomicCounter` was slightly race-y on 32-bit platforms because it
increments the high `AtomicUsize` independently from the low
`AtomicUsize`, leading to a potential race where another thread
could observe the low increment but not the high increment and see
a value of 0 twice.
This isn't a big deal because (a) most platforms are 64-bit these
days, (b) 32-bit platforms aren't super likely to have their
counter overflow 32 bits anyway, and (c) the two writes are
back-to-back so having another thread read during that window is
very unlikely.
However, we can also optimize the counter somewhat by using the
`target_has_atomic = "64"` cfg flag, which we do here, allowing us
to use `AtomicU64` even on 32-bit platforms where 64-bit atomics
are available.
This changes some test behavior slightly, which requires
adaptation.
Fixes#3000
- Add a test to verify the functionality of the handle_message_received
function.
- Ensure the test covers scenarios where InvoiceRequest messages are retried
for PendingOutboundPayments after a simulated connection loss.
- Introduce the `message_received` function to manage the
behavior when a message is received from any peer.
- This function is used within `ChannelManager` to retry `InvoiceRequest`
messages if we haven't received the corresponding invoice yet.
- This change makes the offer communication robust against sudden
connection drops where the initial attempt to send the message
might have failed.
1. Separate the logic of forming `invoice_request` messages from
`invoice_request` and `reply_paths` and enqueueing them into a
separate function.
2. This logic will be reused in the following commit when reforming
`invoice_request` messages for retrying.
1. To enable the retry of the Invoice Request message, it's necessary
to store the essential data required to recreate the message.
2. A new struct is introduced to manage this data, ensuring the
InvoiceRequest message can be reliably recreated for retries.
3. The addition of an `awaiting_invoice` flag allows tracking of
retryable invoice requests, preventing the need to lock the
`pending_outbound_payment` mutex.
In the next commit we'll move to checking `channel_update`s in
three steps - first we check if the `channel_update` is new and the
latest for a channel, then we check the signature, and finally we
update our local state. This allows us to avoid holding a lock on
`NetworkGraph::channels` while validating the message signature.
Here we do a quick prefactor to make that simpler - moving the
validation logic of `channel_update` that we'll do in step one (and
repeat in step three) into a local function. We also take this
opportunity to do one static check unlocked which we had been doing
while holding a `channel` lock.
`blinded_path_with_custom_tlv` indirectly relied on route CLTV
randomization when sending because nodes were at substantially
different block heights after setup. Instead we make sure all nodes
are at the same height which makes the test more robust.
`InMemorySigner` has various private keys in it which makes
`Debug` either useless or dangerous (because most keys won't log
anything, but if they did we'd risk logging private key material).
In order to maintain interface consistency, we refactor all message
handler interfaces to take `PublicKey` rather than `&PublicKey`, as the
difference in efficiency should be negigible and the former is easier to
handle in binding languages.
Over time, we also want to move (no pun intended) towards all messaging
interfaces using move semantics, so dropping the reference for
`PublicKey` is the first step in this direction.
Previously, some `RoutingMessageHandler::handle_` methods (in particular
the ones handling node and channel announcements, as well as channel
updates, omitted the `their_node_id` argument. This didn't allow
implementors to discern *who* sent a particular method.
Here, we add `their_node_id: Option<&PublicKey>` to have them learn who
sent a message, and set `None` if our own node it the originator of a
broadcast operation.
1. Updated the Offers Test to check the reply paths in BOLT12 Invoices.
2. Changed the `extract_invoice` return type from `Option<BlindedMessagePath>`
to `BlindedMessagePath` since all BOLT12Invoices now have a corresponding
reply path by default.
1. Introduced reply_path in BOLT12Invoices to address a gap in error handling.
Previously, if a BOLT12Invoice sent in the offers flow generated an Invoice Error,
the payer had no way to send this error back to the payee.
2. By adding a reply_path to the Invoice Message, the payer can now communicate
any errors back to the payee, ensuring better error handling and communication
within the offers flow.
Introduce HMAC and nonce calculation when sending Invoice with
reply path, so that if we receive InvoiceError back for the
corresponding Invoice we can verify the payment hash before logging it.
...and drop the old, confusing, "protocol error" message when we
get a duplicate connection from a peer.
This should slightly further disambiguate disconnection reasons,
which are a key debugging tool for assigning blame.
- The trait defines the public method one may define for creating and
verifying the HMAC.
- Using a pub trait to define these method allows the flexibility for
other `OffersMessageHandler` construct to construct the HMAC and
authenticate the message.
If we receive `{channel,node}_announcement` messages which we
already have, we first validate their signatures and then look in
our graph and discover that we should discard the messages. This
avoids a second lock in `node_announcement` handling but does not
impact our locking in `channel_announcement` handling. It also
avoids lock contention in cases where the signatures are invalid,
but that should be exceedingly rare.
For nodes with relatively few peers, this is a fine state to be in,
however for nodes with many peers, we may see the same messages
hundreds of times. This causes a rather substantial waste of CPU
resources validating gossip messages.
Instead, here, we change to checking our network graph first and
then validate the signatures only if we don't already have the
message.
The ChannelMonitor::get_claimable_balances and ChainMonitor::get_claimable_balances
methods provide a more straightforward approach to the balance of a channel, which
satisfies most use cases. The computation of AvailableBalances::balance_msat is
complex and originally had a different purpose that is not applicable
anymore.
Co-authored-by: Willem Van Lint <noreply@wvanlint.dev>
In addressing a followup to test reconnection during closing negotation
with async signing, we change things to only return a `ShutdownResult`
when we actually finish shutting down the channel, i.e. we have the
signature ready to send the final closing signed. This slightly
simplifies the logic where we would shutdown our channel
prematurely before we got the final signature. This also means
that we don't push multiple `ChannelClosed` events if we receive closing
signed, reconnect, and receive closing signed again.
`ChannelId`s are almost always referenced as hex, so having debug
output print the raw bytes is somewhat annoying. Instead, we should
dump them as hex the same way we do for `Display`.
This uses the `hex_conservative` `impl_fmt_macros` which does all
the work for us, like we use for `lightning_types`.
`lightning-transaction-sync`'s `esplora-async` dependency
indirectly depends on `tokio-util`, which, like tokio, recently
bumped its MSRV.
Here we update `ci/ci-tests.sh` to pin `tokio-util` to make MSRV
builds pass.
When a InvoiceError is received for a sent BOLT12Invoice, the
corresponding PaymentHash is to be logged. Introduce hmac construction
and verification function for PaymentHash for this purpose.
`ANCHOR_INPUT_WITNESS_WEIGHT` is too high by two weight units,
likely it was calculated to include the SegWit marker bytes, but
it is used to describe an `Input::satisfaction_weight`, which does
not expect the marker bytes.
This corrects that oversight, reducing the constant by two and
adding the marker bytes back in our own internal weight
calculations. It also fixes a second issue where the constant was
too low by one when `grind_signatures` is not set, as that may
result in a signature being one byte longer than we expect.
We should always select at least as many coins as is required to
meet the feerate target, but its perfectly fine if we overshoot.
Specifically, we may overshoot deliberately if we choose to burn
change to fee instead.
When we do coin selection for channel close anchor spends, we may
do coin selection targeting exactly the input values we need.
However, if coin selection does not include a change output, we may
add an OP_RETURN output, which may cause us to end up with less
fee than we wanted on the resulting package.
Here we address this issue by running coin selection twice - first
without seeking the extra weight of the OP_RETURN output, and again
if we find that we under-selected.
There's not a lot of reason to do this, but sadly the bindings
don't currently resolve wildcard imports at all, and I don't want
to deal with implementing it right now.
In 11ab302087 we accidentally removed
the `not(ldk_bench)` bound before outputting logs to stderr, which
we restore here. Instead of simply ignoring logs in benchmarks,
which we did previously, we instead format logs (in a way that LLVM
will not optimize out).
There's no reason why we should be filtering our logs during tests.
It seems mostly like the `TestLogger` was written more as a sample
logger than a test logger, but these days we have samples, so we
should just drop the filtering and show everything.
