Type aliases are now more robustly being exported in the C bindings
generator, which requires ensuring we don't include some type
aliases which make no sense in bindings.
On connection, if our peer supports gossip queries, and we never
send a `gossip_timestamp_filter`, our peer is supposed to never
send us gossip outside of explicit queries. Thus, we'll end up
always having stale gossip information after the first few
connections we make to peers.
The solution is to send a dummy `gossip_timestamp_filter`
immediately after connecting to peers.
Its somewhat strange to have a trait method which is named after
the intended action, rather than the action that occurred, leaving
it up to the implementor what action they want to take.
In 2d3a210897, we increased the
default ping timer in `lightning-background-processor` to ten
seconds from five. However, we didn't change the timer count at
which we disconnect peers if they're not responding, which we
likely should have done. We do so here, as well as update the
documentation for `PeerManager::timer_tick_occurred` to suggest
always ticking the timer every ten seconds instead of five.
Quite some time ago, `UnknownRequiredFeature` was only used when a
gossip message has a missing required feature. These days, its also
used for any required TLV which we do not understand in any
message. However, the handling of it was never updated in
`PeerManager`, leaving it printing a warning about gossip and
ignoring the message entirely.
Instead, we send a warning message and disconnect.
Closes#1236, as caught by @jkczyz.
Even if our gossip hasn't changed, we should be willing to
re-broadcast it to our peers. All our peers may have been
disconnected the last time we broadcasted it.
When a `ChannelUpdate` message is generated for broadcast as a part
of a `BroadcastChannelAnnouncement` event, it may be newer than our
previous `ChannelUpdate` and need to be broadcast. However, if the
`ChannelAnnouncement` had already been seen we wouldn't
re-broadcast either message as the `handle_channel_announcement`
call would fail, short-circuiting the condition to broadcast both.
Instead, we split the broadcast of each message as well as the
conditional so that we always attempt to handle each message and
update our local graph state, then broadcast the message if its
update was processed successfully.
NetworkGraph is owned by NetGraphMsgHandler, but DefaultRouter requires
a reference to it. Introduce shared ownership to NetGraphMsgHandler so
that both can use the same NetworkGraph.
This ensures we don't let a hung connection stick around forever if
the peer never completes the initial handshake.
This also resolves a race where, on receiving a second connection
from a peer, we may reset their_node_id to None to prevent sending
messages even though the `channel_encryptor`
`is_ready_for_encryption()`. Sending pings only checks the
`channel_encryptor` status, not `their_node_id` resulting in an
`unwrap` on `None` in `enqueue_message`.
Associated types in C bindings is somewhat of a misnomer - we
concretize each trait to a single struct. Thus, different trait
implementations must still have the same type, which defeats the
point of associated types.
In this particular case, however, we can reasonably special-case
the `Infallible` type, as an instance of it existing implies
something has gone horribly wrong.
In order to help our bindings code figure out how to do so when
referencing a parent trait's associated type, we specify the
explicit type in the implementation method signature.
When we landed custom messages, we used the empty tuple for the
custom message type for `IgnoringMessageHandler`. This was fine,
except that we also implemented `Writeable` to panic when writing
a `()`. Later, we added support for anchor output construction in
CommitmentTransaction, signified by setting a field to `Some(())`,
which is serialized as-is.
This causes us to panic when writing a `CommitmentTransaction`
with `opt_anchors` set. Note that we never set it inside of LDK,
but downstream users may.
Instead, we implement `Writeable` to write nothing for `()` and use
`core::convert::Infallible` for the default custom message type as
it is, appropriately, unconstructable.
This also makes it easier to implement various things in bindings,
as we can always assume `Infallible`-conversion logic is
unreachable.
In order to avoid significant malloc traffic, messages previously
explicitly stated their serialized length allowing for Vec
preallocation during the message serialization pipeline. This added
some amount of complexity in the serialization code, but did avoid
some realloc() calls.
Instead, here, we drop all the complexity in favor of a fixed 2KiB
buffer for all message serialization. This should not only be
simpler with a similar reduction in realloc() traffic, but also
may reduce heap fragmentation by allocating identically-sized
buffers more often.
MessageSendEvent::PaymentFailureNetworkUpdate served as a hack to pass
an HTLCFailChannelUpdate from ChannelManager to NetGraphMsgHandler via
PeerManager. Instead, remove the event entirely and move the contained
data (renamed NetworkUpdate) to Event::PaymentFailed to be processed by
an event handler.
Latest rustc nightly compiles are filled with warnings like the
following, which we fix here:
```
warning: trailing semicolon in macro used in expression position
--> lightning/src/util/macro_logger.rs:163:114
|
163 | $logger.log(&$crate::util::logger::Record::new($lvl, format_args!($($arg)+), module_path!(), file!(), line!()));
| ^
|
::: lightning/src/chain/chainmonitor.rs:165:9
|
165 | log_debug!(self.logger, "New best block {} at height {} provided via block_connected", header.block_hash(), height);
| -------------------------------------------------------------------------------------------------------------------- in this macro invocation
|
= note: `#[warn(semicolon_in_expressions_from_macros)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #79813 <https://github.com/rust-lang/rust/issues/79813>
= note: this warning originates in the macro `log_internal` (in Nightly builds, run with -Z macro-backtrace for more info)
```
This clarifies the docs for `PeerManager::timer_tick_occurred` to
note that the call rate is entirely up to the user, and also
suggests a faster ping rate of "once every five to ten seconds"
instead of "every 30 seconds". There isn't a lot of reason to want
to ping less often, and faster ping means we detect disconnects
sooner, which is important.
If we are a public node and have a private channel, our
counterparty needs to know the fees which we will charge to forward
payments to them. Without sending them a channel_update, they have
no way to learn that information, resulting in the channel being
effectively useless for outbound-from-us payments.
This commit fixes our lack of channel_update messages to private
channel counterparties, ensuring we always send them a
channel_update after the channel funding is confirmed.
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.
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.
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).
