Now that the `*Features::known` constructor has been removed, there
is no reason to define feature bits as either optional required in
`features.rs` - that logic now belongs in the modules that are
responsible for the given features.
Instead, we only list all features in each context.
As we move towards specify supported/required feature bits in the
module(s) where they are supported, the global `known` feature set
constructors no longer make sense.
Here we (finally) remove the `known` constructor entirely,
modifying tests in the `features` module as required.
As we move towards specify supported/required feature bits in the
module(s) where they are supported, the global `known` feature set
constructors no longer make sense.
In anticipation of removing the `known` constructor, this commit
removes all remaining references to it outside of features.rs.
As we move towards specify supported/required feature bits in the
module(s) where they are supported, the global `known` feature set
constructors no longer make sense.
Here we stop relying on the `known` method in the channel modules.
As we move towards specify supported/required feature bits in the
module(s) where they are supported, the global `known` feature set
constructors no longer make sense.
Here we stop relying on the `known` method in the
functional_test_utils module.
As we move towards specify supported/required feature bits in the
module(s) where they are supported, the global `known` feature set
constructors no longer make sense.
Here we stop relying on the `known` method in the `routing` module,
which was only used in tests.
Historically, LDK has considered the "set of known/supported
feature bits" to be an LDK-level thing. Increasingly this doesn't
make sense - different message handlers may provide or require
different feature sets.
In a previous PR, we began the process of transitioning with
feature bits sent to peers being sourced from the attached message
handler.
This commit makes further progress by moving the concept of which
feature bits are supported by our ChannelManager into
channelmanager.rs itself, via the new `provided_*_features`
methods, rather than in features.rs via the `known_channel_features`
and `known` methods.
Each test featuring HTLCs had a minimum and maximum feerate case. This
is no longer necessary for the zero HTLC transaction anchors variant as
the commitment feerate does not impact whether HTLCs can be trimmed or
not, only the dust limit does.
With the zero fee HTLC transaction anchors variant, HTLCs can no longer
be trimmed due to their amount being too low to have a mempool valid
HTLC transaction. Now they can only be trimmed based on the dust limit
of each party within the channel.
HTLC transactions from anchor channels are constrained by a CSV of 1
block, so broadcasting them along with the unconfirmed commitment
tranasction will result in them being immediately rejected as premature.
There's no need to broadcast our local commitment transaction if we've
already seen a confirmed one as it'll be immediately rejected as a
duplicate/conflict.
This will also help prevent dispatching spurious events for bumping
commitment and HTLC transactions through anchor outputs (once
implemented in future work) and the dispatch for said events follows the
same flow as our usual commitment broadcast.
As we remove the concept of a global "known/supported" feature set
in LDK, we should also remove the concept of a global "required"
feature set. This does so by moving the checks for specific
required features into handlers.
Specifically, it allows the handler `peer_connected` method to
return an `Err` if the peer should be disconnected. Only one such
required feature bit is currently set - `static_remote_key`, which
is required in `ChannelManager`.
In the next commit we'll enforce counterparty `InitFeatures`
matching our required set in `ChannelManager`, implying they must
be set for many tests where they previously did not need to be (as
they were enforced in `PeerManager`, which is not used in
functional tests).
Currently we entirely ignore the BADONION bit when deciding how to
handle HTLC failures. This opens us up to an attack where a
malicious node always fails HTLCs backwards via
`update_fail_malformed_htlc` with an error code of
`BADONION|NODE|PERM|X`. In this case, we may decide to interpret
this as a permanent node failure for the node encrypting the onion,
i.e. the counterparty of the node who sent the
`update_fail_malformed_htlc` message and ultimately failed the
HTLC.
Thus, any node we route through could cause us to fully remove its
counterparty from our network graph. Luckily we do not do any
persistent tracking of removed nodes, and thus will re-add the
removed node once it is re-announced or on restart, however we are
likely to add such persistent tracking (at least in-memory) in the
future.
For non-gossip-broadcast messages, our current flow is to first
serialize the message into a `Vec`, and then allocate a new `Vec`
into which we write the encrypted+MAC'd message and header.
This is somewhat wasteful, and its rather simple to instead
allocate only one buffer and encrypt the message in-place.
In 47e818f198, forwarding broadcasted
gossip messages was split into a separate per-peer message buffer.
However, both it and the original regular-message queue are
encrypted immediately when the messages are enqueued. Because the
lightning P2P encryption algorithm is order-dependent, this causes
messages to fail their MAC checks as the messages from the two
queues may not be sent to peers in the order in which they were
encrypted.
The fix is to simply queue broadcast gossip messages unencrypted,
encrypting them when we add them to the regular outbound buffer.
I'm not sure why rustc didn't complain about the unused parameter
or why we're allowed to get away without explicitly bounding the
`Sign` in the `KeysInterface`, but the current code requires all
`BlindedPath` construction to explicitly turbofish an unused type.
The bindings generator is pretty naive in its generic resolution
and doesn't like `where` clauses for bounds that are simple traits.
This should eventually change, but for now its simplest to just
inline the relevant generic bounds.
The C bindings generator isn't capable of figuring out if a blanket
impl applies in a given context, and instead opts to always write
out any relevant impl's for a trait. Thus, blanket impls should be
disabled when building with `#[cfg(c_bindings)]`.
Similar to how we OR our InitFeaures and NodeFeatures across both our channel
and routing message handlers, we also want to OR the features of our onion
message handler.
When ChannelMessageHandler implementations wish to return a NodeFeatures which
contain all the known flags that are relevant to channel handling, but not
gossip handling, they currently need to do so by manually constructing a
NodeFeatures with all known flags and then clearing the ones they don't want.
Instead of spreading this logic across the codebase, this consolidates such
construction into one place in features.rs.
When we broadcast a node announcement, the features we support are really a
combination of all the various features our different handlers support. This
commit captures this concept by OR'ing our NodeFeatures across both our channel
and routing message handlers.
