Matt Corallo [Sat, 3 Jul 2021 15:27:12 +0000 (15:27 +0000)]
Make channel fields which are from accept_channel Optional
These fields are set with a dummy value, which we should generally
be avoiding since Rust gives us a nice `Option` type to use
instead.
Further, we stop rejecting channel_update messages outright when
the htlc_maximum_msat field includes the reserve values, which
nodes could reasonably do without it meriting a channel closure.
Matt Corallo [Fri, 25 Jun 2021 04:16:35 +0000 (04:16 +0000)]
Create SpendableOutputs events no matter the chain::Confirm order
We had a user who pointed out that we weren't creating
`SpendableOutputs` events when we should have been after they
called `ChannelMonitor::best_block_updated` with a block well
after a CSV locktime and then called
`ChannelMonitor::transactions_confirmed` with the transaction which
we should have been spending (with a block height/hash a ways in
the past).
This was due to `ChannelMonitor::transactions_confirmed` only
calling `ChannelMonitor::block_confirmed` with the height at which
the transactions were confirmed, resulting in all checks being done
against that, not the current height.
Further, in the same scenario, we also would not fail-back and HTLC
where the HTLC-Timeout transaction was confirmed more than
ANTI_REORG_DELAY blocks ago.
To address this, we use the best block height for confirmation
threshold checks in `ChannelMonitor::block_confirmed` and pass both
the confirmation and current heights through to
`OnchainTx::update_claims_view`, using each as appropriate.
Matt Corallo [Mon, 28 Jun 2021 16:23:19 +0000 (16:23 +0000)]
Update `ChannelMonitor::best_block` before calling block_confirmed
No matter the context, if we're told about a block which is
guaranteed by our API semantics to be on the best chain, and it has
a higher height than our current understanding of the best chain,
we should update our understanding. This avoids complexity
in `block_confirmed` by never having a height set which is *higher*
than our current best chain, potentially avoiding some bugs in the
rather-complicated code.
It also requires a minor test tweak as we in some cases now no
longer broadcast a conflicting transaction after the original has
reached the ANTI_REORG_DELAY.
Matt Corallo [Fri, 25 Jun 2021 04:14:50 +0000 (04:14 +0000)]
Avoid calling OnchainTx::block_disconnected if no block was discon'd
There are no visible effects of this, but it seems like good code
hygiene to not call a disconnect function in a different file if no
disconnect happened.
Matt Corallo [Wed, 16 Jun 2021 22:57:38 +0000 (22:57 +0000)]
Consider channels "live" even if they are awaiting a monitor update
We use `Channel::is_live()` to gate inclusion of a channel in
`ChannelManager::list_usable_channels()` and when sending an
HTLC to select whether a channel is available for
forwarding through/sending to.
In both of these cases, we should consider a channel `is_live()` when
they are pending a monitor update. Some clients may update monitors
asynchronously, thus we may simply be waiting a short duration for a
monitor update to complete, and shouldn't fail all forwarding HTLCs
during that time.
After #851, we always ensure any holding cells are free'd when
sending P2P messages, making this change much more trivially
correct - instead of having to ensure that we always free the holding
cell when a channel becomes live again after adding something to the
holding cell, we can simply rely on the fact that it always happens.
Matt Corallo [Mon, 28 Jun 2021 00:54:24 +0000 (00:54 +0000)]
Do not always persist ChannelManager on channel_update messages
If we receive a `channel_update` message for a channel unrelated to
our own, we shouldn't trigger a persistence of our
`ChannelManager`. This avoids significant persistence traffic during
initial node startup.
Matt Corallo [Wed, 30 Jun 2021 02:19:28 +0000 (02:19 +0000)]
Correct test log printing due to inverted comparison
We changed the sort order of log levels to be more natural, but this
comparison wasn't updated accordingly. Likely the reason it was
left strange for so long is it also had the comparison argument
ordering flipped.
Matt Corallo [Wed, 23 Jun 2021 03:32:32 +0000 (03:32 +0000)]
Increase the log level of several channelmonitor/onchain logs.
ChannelMonitor and related log entries can generally lean towards
being higher log levels than they necessarily need to be, as they
should be exceedingly rare, if only because they require
confirmation of an on-chain transaction.
Matt Corallo [Tue, 22 Jun 2021 03:35:52 +0000 (03:35 +0000)]
Update logging in channel and channelmanager to better levels
This updates a number of log sites in channel and channelmanager to
* Be a bit more verbose at the TRACE level,
* Move some error/useful messages to the ERROR/WARN/INFO level,
* Add new logs to always log once at the DEBUG level when we
send/receive a commitment_signed (with some extra data),
* Include the channel id being operated on in more log messages.
Matt Corallo [Tue, 22 Jun 2021 01:33:44 +0000 (01:33 +0000)]
Do not log_debug when we receive duplicate gossip messages
We very often receive duplicate gossip messages, which now causes us
to log at the DEBUG level, which is almost certainly not what a
user wants. Instead, we add a new form of ErrorAction which causes
us to only log at the TRACE level.
Matt Corallo [Mon, 28 Jun 2021 20:38:48 +0000 (20:38 +0000)]
Allow logging to specify an explicit log level instead of a macro
For log entries which may have a variable level, we can't call an
arbitrary macro and need to be able to pass an explicit level. This
does so without breaking the compile-time disabling of certain log
levels.
Further, we "fix" the comparison order of log levels to make more
significant levels sort "higher", which implicitly makes more sense
than sorting "lower".
Finally, we remove the "Off" log level as no log entry should ever
be logged at the "Off" level - that would be nonsensical.
Matt Corallo [Tue, 22 Jun 2021 03:36:36 +0000 (03:36 +0000)]
More consistently log in msg handling, incl full msg logging at trace
This much more consistently logs information about messages
sent/received, including logging the full messages being
sent/received at the TRACE log level. Many other log messages which
are more often of interest were moved to the DEBUG log level.
Matt Corallo [Mon, 21 Jun 2021 18:03:19 +0000 (18:03 +0000)]
Add error logs when a ChannelManager as inconsistent monitor state
We had a client application which provided inconsistent monitor
state when deserializing a ChannelManager, resulting in opaque and
generic "InvalidData" deserialization failures. Instead, we log
some informative (and appropriately scary) warning messages in
such cases.
Matt Corallo [Mon, 28 Jun 2021 23:31:29 +0000 (23:31 +0000)]
Fix bogus `Event::PaymentSent` serialization
`Event::PaymentSent` serialization has a bug where we
double-serialize the payment_preimage field but do not attempt to
read it twice. This results in a failure to read `ChannelManager`s
from disk if we have a pending `Event::PaymentSent` pending
awaiting handling when we serialize.
Instead of attempting to read both versions, we opt to simply fix
the serialization, assuming it is exceedingly rare for such a
scenario to appear (and if it does, we can assist in manual
recovery).
The release notes have been updated to note this inconsistency.
Matt Corallo [Wed, 23 Jun 2021 16:39:27 +0000 (16:39 +0000)]
Workaround lnd sending funding_locked before channel_reestablish
lnd has a long-standing bug where, upon reconnection, if the
channel is not yet confirmed they will not send a
channel_reestablish until the channel locks in. Then, they will
send a funding_locked *before* sending the channel_reestablish
(which is clearly a violation of the BOLT specs). We copy
c-lightning's workaround here and simply store the funding_locked
message until we receive a channel_reestablish.
Previous to this PR, TLV serialization involved iterating from 0 to the highest
given TLV type. This worked until we decided to implement keysend, which has a
TLV type of ~5.48 billion.
So instead, we now specify the type of whatever is being (de)serialized (which
can be an Option, a Vec type, or a non-Option (specified in the serialization macros as "required").
Matt Corallo [Thu, 24 Jun 2021 04:31:33 +0000 (04:31 +0000)]
Drop the cdylib export of lightning-invoice
There are ~zero functions in lightning-invoice that are materially
callable from C, so there isn't any reason to tag it as a cdylib
(and make rustc build it as such). Instead, we have C bindings now.
Matt Corallo [Mon, 21 Jun 2021 17:36:46 +0000 (17:36 +0000)]
Do not generate error messages when we receive our own gossip
When a peer sends us the routing graph, it may include gossip
messages for our channels, despite it not being a party to them.
This is completely fine, but we currently print a somewhat-scary
looking log messages in these cases, eg:
```
ERROR [lightning::ln::channelmanager:4104] Got a message for a channel from the wrong node!
TRACE [lightning::ln::peer_handler:1267] Handling SendErrorMessage HandleError event in peer_handler for node ... with message Got a message for a channel from the wrong node!
```
Instead, we should simply not consider this an "error" condition
and stay silent.
Matt Corallo [Thu, 17 Jun 2021 22:30:09 +0000 (22:30 +0000)]
Do not require that no calls are made post-disconnect_socket
The only practical way to meet this requirement is to block
disconnect_socket until any pending events are fully processed,
leading to this trivial deadlock:
* Thread 1: select() woken up due to a read event
* Thread 2: Event processing causes a disconnect_socket call to
fire while the PeerManager lock is held.
* Thread 2: disconnect_socket blocks until the read event in
thread 1 completes.
* Thread 1: bytes are read from the socket and
PeerManager::read_event is called, waiting on the lock
still held by thread 2.
There isn't a trivial way to address this deadlock without simply
making the final read_event call return immediately, which we do
here. This also implies that users can freely call event methods
after disconnect_socket, but only so far as the socket descriptor
is different from any later socket descriptor (ie until the file
descriptor is re-used).
Matt Corallo [Wed, 16 Jun 2021 18:57:21 +0000 (18:57 +0000)]
Fix bogus reentrancy from read_event -> do_attempt_write_data
`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed
out in a previous discussion that `read_event` can reenter user
code despite the documentation stating explicitly that it will not.
This was addressed in #456 by simply codifying the reentrancy, but
its somewhat simpler to just drop the `do_attempt_write_data` call.
Ideally we could land most of Julian's work, but its still in need
of substantial git history cleanup to get it in a reviewable state
and this solves the immediate issue.
Matt Corallo [Thu, 10 Jun 2021 16:26:33 +0000 (16:26 +0000)]
Drop unused "peer gone" handling in get_peer_for_forwarding!()
We can never assume that messages were reliably delivered whether
we placed them in the socket or not, so there isn't a lot of use in
explicitly handling the case that a peer was not connected when we
went to send it a message.
Two TODOs are left for the generation of a `FundingAbandoned` (or
similar) event, though it ultimately belongs in `ChannelManager`.
Jeffrey Czyz [Thu, 17 Jun 2021 18:40:52 +0000 (13:40 -0500)]
Increase poll::Validate visibility to pub
Previously, poll::Validate was pub(crate) to force external implementors
of Poll to define their implementation in terms of ChainPoller. This was
because ChainPoller::look_up_previous_header checks for consistency
between headers and any errors would be checked at that level rather
than by the caller. Otherwise, if performed by the caller, a Poll
implementation would not be aware if the underlying BlockSource was
providing bad data and therefore couldn't react accordingly.
Widening the visibility to pub relaxes this requirement, although it's
still encourage to use ChainPoller to implement Poll. This permits
either copying or moving lightning-block-sync's test utilities to a
separate crate since they use poll::Validate.
Matt Corallo [Thu, 17 Jun 2021 15:49:17 +0000 (15:49 +0000)]
Make links in message signing docs rustdoc links
Latest rustc warns that "this URL is not a hyperlink" and notes that
"bare URLs are not automatically turned into clickable links". This
resolves those warnings.
Thanks to Joshua Nelson for pointing out the correct syntax for
this.
Jeffrey Czyz [Thu, 10 Jun 2021 22:49:14 +0000 (15:49 -0700)]
Accept multi-hop route hints in get_route
Lightning invoices allow for zero or more multi-hop route hints. Update
get_route's interface to accept such hints, although only the last hop
from each is used for the time being.
Moves RouteHint from lightning-invoice crate to lightning crate. Adds a
PrivateRoute wrapper around RouteHint for use in lightning-invoice.
Matt Corallo [Sun, 30 May 2021 23:24:43 +0000 (23:24 +0000)]
Use TLV instead of explicit length for VecReadWrapper termination
VecReadWrapper is only used in TLVs so there is no need to prepend
a length before writing/reading the objects - we can instead simply
read until we reach the end of the TLV stream.
Matt Corallo [Fri, 28 May 2021 14:07:39 +0000 (14:07 +0000)]
Add additional inline hints in serialization methods
This substantially improves deserialization performance when LLVM
decides not to inline many short methods, eg when not building
with LTO/codegen-units=1.
Even with the default bench params of LTO/codegen-units=1, the
serialization benchmarks on an Intel 2687W v3 take:
test routing::network_graph::benches::read_network_graph ... bench: 1,955,616,225 ns/iter (+/- 4,135,777)
test routing::network_graph::benches::write_network_graph ... bench: 165,905,275 ns/iter (+/- 118,798)
Matt Corallo [Sat, 29 May 2021 18:32:53 +0000 (18:32 +0000)]
Impl `serialized_length()` without `LengthCalculatingWriter`
With the new `serialized_length()` method potentially being
significantly more efficient than `LengthCalculatingWriter`, this
commit ensures we call `serialized_length()` when calculating
length of a larger struct.
Specifically, prior to this commit a call to
`serialized_length()` on a large object serialized with
`impl_writeable`, `impl_writeable_len_match`, or
`encode_varint_length_prefixed_tlv` (and
`impl_writeable_tlv_based`) would always serialize all inner fields
of that object using `LengthCalculatingWriter`. This would ignore
any `serialized_length()` overrides by inner fields. Instead, we
override `serialized_length()` on all of the above by calculating
the serialized size using calls to `serialized_length()` on inner
fields.
Further, writes to `LengthCalculatingWriter` should never fail as
its `write` method never returns an error. Thus, any write failures
indicate a bug in an object's write method or in our
object-creation sanity checking. We `.expect()` such write calls
here.
As of this commit, on an Intel 2687W v3, the serialization
benchmarks take:
test routing::network_graph::benches::read_network_graph ... bench: 2,039,451,296 ns/iter (+/- 4,329,821)
test routing::network_graph::benches::write_network_graph ... bench: 166,685,412 ns/iter (+/- 352,537)