Matt Corallo [Fri, 1 Jan 2021 01:19:21 +0000 (20:19 -0500)]
[bindings] Use consistent imports for MessageSendEvents traits
Our bindings generator is braindead with respect to the idents
used in a trait definition - it treats them as if they were used
where the trait is being used, instead of where the trait is
defined. Thus, if the idents used in a trait definition are not
also imported the same in the files where the traits are used, we
will claim the idents are bogus.
I spent some time trying to track the TypeResolvers globally
through the entire conversion run so that we could use the original
file's TypeResolver later when using the trait, but it is somewhat
of a lifetime mess. While likely possible, import consistency is
generally the case anyway, so unless it becomes more of an issue in
the future, it likely makes the most sense to just keep imports
consistent.
This commit keeps imports consistent across trait definition files
around `MessageSendEvent` and `MessageSendEventsProvider`.
Matt Corallo [Wed, 25 Nov 2020 21:18:12 +0000 (16:18 -0500)]
Use `KeysInterface::read_chan_signer` for all channel keys deser
This drops any direct calls to a generic `ChannelKeys::read()` and
replaces it with the new `KeysInterface::read_chan_signer()`. Still,
under the hood all of our own `KeysInterface::read_chan_signer()`
implementations simply call out to a `Readable::read()` implemention.
Matt Corallo [Wed, 25 Nov 2020 17:23:47 +0000 (12:23 -0500)]
Add a new method `read_chan_signer` to `KeysInterface`
This adds a new method to the general cross-channel `KeysInterface`
which requires it to handle the deserialization of per-channel
signer objects. This allows the deserialization of per-channel
signers to have more context available, which, in the case of the
C bindings, includes the actual KeysInterface information itself.
Matt Corallo [Wed, 25 Nov 2020 20:03:19 +0000 (15:03 -0500)]
Use Writeable for ChannelMonitor instead of a specific function.
There's no reason to have ChannelMonitor::write_for_disk instead of
just using the Writeable trait anymore. Previously, it was used to
differentiate with `write_for_watchtower`, but support for
watchtower-mode ChannelMonitors was never completed and the partial
bits were removed long ago.
This has the nice benefit of hitting the custom Writeable codepaths
in C bindings instead of trying to hit trait-generics paths.
Matt Corallo [Wed, 25 Nov 2020 17:23:37 +0000 (12:23 -0500)]
Universally Require Writeable for ChannelKeys
It doesn't make sense to ever build a lightning node which doesn't
ever write ChannelMonitors to disk, so having a ChannelKeys object
which doesn't implement Writeable is nonsense.
Here we require Writeable for all ChannelKeys objects, simplifying
code generation for C bindings somewhat.
Matt Corallo [Fri, 30 Oct 2020 21:25:15 +0000 (17:25 -0400)]
Cache fields instead of storing a ChannelKeys in ChannelMonitor
We only actually use two of the fields in ChannelKeys inside a
ChannelMonitor - the holder revocation_basepoint and the
derivation parameters. Both are relatively small, so there isn't
a lot of reason to hold a full copy of the ChannelKeys (with most
of the interaction with it being inside the OnchainTxHandler).
Further, this will avoid calling read on a `ChannelKeys` twice,
which is a somewhat strange API quirk.
CommitmentTransaction maintains the per-commitment transaction fields needed to construct the associated bitcoin transactions (commitment, HTLC). It replaces passing around of Bitcoin transactions. The ChannelKeys API is modified accordingly.
By regenerating the transaction when implementing a validating external signer, this allows a higher level of assurance that all relevant aspects of the transactions were checked for policy violations.
ChannelTransactionParameters replaces passing around of individual per-channel fields that are needed to construct Bitcoin transactions.
Eliminate ChannelStaticData in favor of ChannelTransactionParameters.
Use counterparty txid instead of tx in channelmonitor update.
bmancini55 [Wed, 9 Dec 2020 20:06:54 +0000 (15:06 -0500)]
Remove should_request_full_sync from RoutingMessageHandler
This method was used to set the initial_routing_sync flag when sending
an outbound Init message to a peer. Since we are now relying on
gossip_queries instead of initial_routing_sync, synchronization can be
fully encapsulate into RoutingMessageHandler via sync_routing_table.
This commit removes should_request_full_sync from the trait
RoutingMessageHandler. The implementation is still used in
NetGraphMsgHandler and has been converted into a private method instead
of a trait function.
bmancini55 [Thu, 3 Dec 2020 17:48:40 +0000 (12:48 -0500)]
Change routing table sync to use gossip_queries
This commit changes outbound routing table sync to use gossip_queries
instead of the effectively deprecated initial_routing_sync feature.
This change removes setting of initial_routing_sync in our outbound Init
message. Instead we now call sync_routing_table after receiving an Init
message from a peer. If the peer supports gossip_queries and
should_request_full_sync returns true, we initiate a full gossip_queries
sync.
bmancini55 [Thu, 3 Dec 2020 17:00:36 +0000 (12:00 -0500)]
Pass Init message to sync_routing_table method
This commit modifies sync_routing_table in RoutingMessageHandler to
accept a reference to the Init message received by the peer. This allows
the method to use the Peer's features to drive the operations of the
gossip_queries routing table sync.
bmancini55 [Thu, 3 Dec 2020 16:52:54 +0000 (11:52 -0500)]
Pass gossip_queries messages to handler via ownership
This change modifies gossip_queries methods in RoutingMessageHandler to
move the message instead of passing a reference. This allows the message
handler to be more efficient by not requiring a full copy of SCIDs
passed in messages.
bmancini55 [Wed, 18 Nov 2020 18:32:55 +0000 (13:32 -0500)]
Refactor gossip_queries sync to be stateless
This commit simplifies the sync process for routing gossip messages. When
a sync is initiated, the process is handled statelessly by immediately
issuing SCID queries as channel range replies are received. This greatly
simplifies the state machine at the cost of fully validating and
conforming to the current spec.
bmancini55 [Sun, 29 Nov 2020 20:20:35 +0000 (15:20 -0500)]
Add genesis block hash to NetworkGraph
This changes adds the genesis block hash as a BlockHash to the
NetworkGraph struct. Making the NetworkGraph aware allows the message
handler to validate the chain_hash for received messages. This change
also adds the hash value to the Writeable and Readable methods.
bmancini55 [Thu, 22 Oct 2020 16:44:53 +0000 (12:44 -0400)]
Implement gossip_queries sync methods in NetGraphMsgHandler
To perform a sync of routing gossip messages with a peer requires a two
step process where we first initiate a channel range query to discover
channels in a block range. Next we request the routing gossip messages
for discovered channels.
This code implements logic in NetGraphMsgHandler for performing these two
tasks while taking into account the specification and variance in
implementation.
bmancini55 [Thu, 22 Oct 2020 14:51:54 +0000 (10:51 -0400)]
Add gossip_queries methods to RoutingMessageHandler trait
Defines message handlers for gossip_queries messages in the RoutingMessageHandler
trait. The MessageSendEventsProvider supertrait is added to RoutingMessageHandler
so that the implementor can use SendMessageEvents to send messages to a
peer at the appropriate time.
The trait methods are stubbed in NetGraphMsgHandler which implements
RoutingMessageHandler and return a "not implemented" error.
bmancini55 [Thu, 22 Oct 2020 12:47:24 +0000 (08:47 -0400)]
Add send message events for gossip_queries
This change enables initiating gossip queries with a peer using the
SendMessageEvent enum. Specifically we add an event for sending
query_channel_range to discover the existance of channels and an event
for sending query_short_channel_ids to request routing gossip messages
for a set of channels. These events are handled inside the process_events
method of PeerManager which sends the serialized message to the peer.
bmancini55 [Wed, 21 Oct 2020 21:20:26 +0000 (17:20 -0400)]
Add gossip_queries messages to wire decoding
To enable gossip_queries message decoding, this commit implements the
wire module's Encoding trait for each message type. It also adds these
messages to the wire module's Message enum and the read function to
enable decoding of a buffer.
bmancini55 [Wed, 21 Oct 2020 20:31:22 +0000 (16:31 -0400)]
Add gossip_queries feature flag
Support for the gossip_queries feature flag (bits 6/7) is added to the
Features struct. This feature is available in the Init and Node
contexts. The gossip_queries feature is not fully implemented so this
feature is disabled when sent to peers in the Init message.
Matt Corallo [Tue, 24 Nov 2020 18:38:39 +0000 (13:38 -0500)]
Split `NetworkGraph` message handling fns into unsigned and signed
This takes the now-public `NetworkGraph` message handling functions
and splits them all into two methods - one which takes a required
Secp256k1 context and verifies signatures and one which takes only
the unsigned part of the message and does not take a Secp256k1
context.
This both clarifies the public API as well as simplifies it, all
without duplicating code.
Finally, this adds an assertion in the Router fuzzer to make sure
the constants used for message deserialization are correct.
Matt Corallo [Tue, 24 Nov 2020 17:40:11 +0000 (12:40 -0500)]
Move UTXO-lookup into pub utility function from RoutingMsgHandler
This makes the public utility methods in `NetworkGraph` able to do
UTXO lookups ala `NetworkMsgHandler`'s `RoutingMessageHandler`
implementation, slightly simplifying the public interface.
We also take this opportunity to verify signatures before calling
out to UTXO lookups, under the "do actions in order of
cheapest-to-most-expensive to reduce DoS surface" principle.
Matt Corallo [Thu, 12 Nov 2020 22:19:05 +0000 (17:19 -0500)]
[fuzz] Make router_target a bit easier for fuzzers to explore
It turns out (somewhat obviously) that expecting a fuzzer to
correctly build multiple signatures which verify against multiple
public keys in the same message was a bit too daunting, so we now
skip message signatures in routing messages.
We also take this opportunity to simplify the target itself somewhat,
avoiding reading public keys over and over and instead generating
routes to all the public keys that appeared in messages while running.
Matt Corallo [Thu, 12 Nov 2020 22:16:14 +0000 (17:16 -0500)]
Expose manual-update methods in NetworkGraph.
These functions were created but previously not exported, however
they are useful if we want to skip signature checking when accepting
routing messages (which we really should be doing in the routing
fuzzer).
Matt Corallo [Mon, 23 Nov 2020 18:34:31 +0000 (13:34 -0500)]
Don't create chan-closed mon update for outbound never-signed chans
Like the previous commit for channel-closed monitor updates for
inbound channels during processing of a funding_created message,
this resolves a more general issue for closing outbound channels
which have sent a funding_created but not yet received a
funding_signed.
This issue was also detected by full_stack_target.
To make similar issues easier to detect in testing and fuzzing, an
additional assertion is added to panic on updates to a channel
monitor before registering it.
Matt Corallo [Sun, 15 Nov 2020 21:44:09 +0000 (16:44 -0500)]
Do not generate a channel-closed mon update for never-signed chans
The full_stack_target managed to find a bug where, if we receive
a funding_created message which has a channel_id identical to an
existing channel, we'll end up
(a) having the monitor update for the new channel fail (due to
duplicate outpoint),
(b) creating a monitor update for the new channel as we
force-close it,
(c) panicing due to the force-close monitor update is applied to
the original channel and is considered out-of-order.
Obviously we shouldn't be creating a force-close monitor update for
a channel which can never appear on chain, so we do that here and
add a test which previously failed and checks a few
duplicate-channel-id cases.
Matt Corallo [Thu, 12 Nov 2020 23:59:06 +0000 (18:59 -0500)]
[netgraph] Do not allow capacity_sats * 1000 to overflow-panic
In updating the router fuzzer, it discovered that a remote peer can
cause us to overflow while multiplying the channel capacity value.
Since the value should never exceed 21 million BTC, we just add a
check for that.
Matt Corallo [Thu, 12 Nov 2020 23:57:02 +0000 (18:57 -0500)]
[router] Fix + test routing via next/last-hop hints only
We had code in the router to support sending a payment via a single
hop across channels exclusively provided by the next-/last-hop hints.
However, in updating the fuzzer, I noted that this case not only
didn't work, but paniced in some cases.
Here, we both fix the panic, as well as write a new test which
ensures we don't break support for such routing in the future.
Matt Corallo [Mon, 23 Nov 2020 15:20:10 +0000 (10:20 -0500)]
Use CARGO_PROFILE_RELEASE_LTO to fix bindings build on new cargo
Newer versions of cargo broke `cargo rustc -- -Clto` by passing
`-Cembed-bitcode=no` even in `--release`. Its somewhat unclear why
this is still the case on latest cargo given it broke, at least,
compilation of Firefox, as discovered by Val at [1]. We take the
approach they used there, even though they later walked it back [2]
but the issues noted there, especially in [3] don't seem
particularly concerning in our case.
Matt Corallo [Mon, 26 Oct 2020 01:39:58 +0000 (21:39 -0400)]
[bindings] Update eq/clone trait fns to take object, not this_arg
When a trait is required to implement eq/clone (eg in the case of
`SocketDescriptor`), the generated trait struct contains an
eq/clone function which takes a `this_arg` pointer. Since the trait
object can always be read to get the `this_arg` pointer, there is
no loss of generality to pass the trait object itself, and it
provides a bit more flexibility when the trait could be one of
several implementations (which we use in the Java higher-level
bindings).
CVecTempl previously called Vec.clone_from_slice() on a
newly-allocated Vec, which immediately panics as
[T].clone_from_slice() requires that the Vec/target slice already
has the same length as the source slice. This should have been
Vec.extend_from_slice() which exhibits the correct behavior.
Matt Corallo [Fri, 9 Oct 2020 01:25:25 +0000 (21:25 -0400)]
[bindings] Make Transaction::data *mut instead of *const
When the only reference to the transaction bytes is via
Transaction::data, my understanding of the C const rules is that
it would then be invalid to write to it. While its unlikely this
would ever pose an issue, its not hard to simply make it *mut, so
we do that here.
Matt Corallo [Wed, 18 Nov 2020 02:07:15 +0000 (21:07 -0500)]
[fuzz] Check that channels don't get stuck in chanmon_consistency
This adds a new command string in the chanmon_consistency fuzzer
which tests that, once all pending HTLCs are settled, at least one
side of a channel can still send funds.
While this should have caught the recent(ish) spec bug where
channels could get stuck, I did not attempt to reproduce said bug
with this patch.
Matt Corallo [Sat, 21 Nov 2020 17:09:40 +0000 (12:09 -0500)]
[fuzz] Reduce overuse of macros/Arcs in chanmon_consistency
In previous versions of related commits, the macros in
chanmon_consistency ended up blowing up rustc a bit resulting in
20+GB memory usage and long compile times. Shorter function bodies
by avoiding macros where possible fix this.
Matt Corallo [Tue, 17 Nov 2020 20:24:20 +0000 (15:24 -0500)]
[fuzz] Expand the amounts we can send in chanmon_consistency
This should make it a bit easier for the fuzzer to hit any given
balance breakdown during run as well as tweaks the command strings
to be more bit-pattern friendly.
Matt Corallo [Tue, 17 Nov 2020 20:22:59 +0000 (15:22 -0500)]
[fuzz] Test chanmon_consistency payment-send errors are sane
Instead of simply always considering a payment-send failure as
acceptable (and aborting fuzzing), we check that a payment send
failure is from a list of errors that we know we can hit, mostly
around maxing out our channel balance.
Critically, we keep going after hitting an error, as there's no
reason channels should get out of sync even if a send fails.
Claim HTLC output on-chain if preimage is recv'd after force-close
If we receive a preimage for an outgoing HTLC that solves an output on a
backwards force-closed channel, we need to claim the output on-chain.
Note that this commit also gets rid of the channel monitor redundantly setting
`self.counterparty_payment_script` in `check_spend_counterparty_transaction`.
Co-authored-by: Antoine Riard <ariard@student.42.fr> Co-authored-by: Valentine Wallace <vwallace@protonmail.com>
Now callers will separately retrieve the claim requests/
holder revokable script and the new watched holder outputs.
This will be used in the next commit for times when we
need to get holder claim requests, but don't have access to
the holder commitment transaction.
Helpful for debugging. I also included the change in the provide_preimage method
signature which will be used in an upcoming commit, because commit-wise it was
easier to combine the changes.
If the channel is hitting the chain right as we receive a preimage,
previous to this commit the relevant ChannelMonitor would never
learn of this preimage.
The ChannelMonitor already monitors the chain for counterparties
revealing preimages, and will give the HTLCSources back to the
ChannelManager for claiming. Thus it's unnecessary for the ChannelManager
to monitor these HTLCs itself.
See is_resolving_htlc_output:
- if the counterparty broadcasted and then claimed one of the HTLCs we
offered them, line 2015 is where the ChannelMonitor gives the ChannelManager
the HTLC source
- if we broadcasted and they claimed an HTLC we offered them, line 2025 is
where the ChannelMonitor gives the ChannelManager the HTLC source
Matt Corallo [Thu, 8 Oct 2020 23:35:50 +0000 (19:35 -0400)]
[bindings] Un-Box Tuple mapping
Because the C++ wrappers require being able to memset(0) the C
structs to skip free(), we'd previously mapped tuples with two
pointer indirections. However, because all other types already
support memset(0)'ing to disable free() logic, we can skip the
pointer indirections and the behavior is still correct.
Matt Corallo [Fri, 2 Oct 2020 01:32:39 +0000 (21:32 -0400)]
[bindings] Drop one static-lifetime restriction and check success
In general we should stop enforcing that all lifetimes are static
- we may take references from C and its up to reviewing the diff on
the bindings changes and the user(s) to ensure lifetimes are valid.
Also asserts a success criteria that was missed before.
Matt Corallo [Fri, 2 Oct 2020 01:28:57 +0000 (21:28 -0400)]
[bindings] Handle type X = Y aliasing in type resolution
For non-generic type aliases which are meant as convinient aliases
for more complex types, we need to store the aliased type (with all
paths made absolute) and use that in type resolution.
The most code by far is just making all the paths in a type absolute
but its not too bad either.
Matt Corallo [Thu, 1 Oct 2020 22:42:33 +0000 (18:42 -0400)]
[bindings] Avoid guessing whether resolved type is a ref in blocks
In some cases, things which are a Rust Reference (ie slices), we
may still want to map them as a non-reference and need to put a
"mut " in front of the variable name in a function decl. This
worked fine by just checking for the slice case, except that we
are about to add support for type aliases, which no longer match
the naive case.
Instead, we can just have the types module print out the C type and
check if it begins with a '&' to figure out if it is a reference.
Matt Corallo [Fri, 25 Sep 2020 17:25:35 +0000 (13:25 -0400)]
[bindings] Support mapping slices which contain tuples (with refs)
New work upstream puts tuples in slices, which is a very reasonable
thing to expect, however we don't know how to generate conversions
for such objects. Making it more complicated, upstream changes also
include references to things inside such slices, which requires
special handling to avoid creating dangling references.
This adds support for converting such objects, noting that slices
need to be converted first into Vecs which own their underlying
objects and then need to map any reference types into references.
Matt Corallo [Thu, 24 Sep 2020 23:03:05 +0000 (19:03 -0400)]
[bindings] Give Transaction objects a buffer-is-owned flag.
A lot of our container mapping depends on the `is_owned` flag
which we have for in-crate mapped objects to map references and
non-references into the same container type. Transaction was
mapped to two completely different types (a slice and a Vec type),
which led to a number of edge cases in the bindings generation.
Specifically, I spent a few days trying to map
`[(A, &Transaction)]` properly and came up empty - we map slices
into the same types as Vecs (and rely on the `is_owned` flag to
avoid double-free) and the lack of one for `Transaction` would have
required a special-case in numerous functions.
Instead, we just add a flag in `Transaction` to mirror what we do
for in-crate types and check it before free-ing any underlying
memory.
Note that, sadly, because the c_types objects aren't mapped as a
part of our C++ bindings generation, you have to manually call
`Transaction_free()` even in C++.
Test that Persist temp and perm failures behave as expected.
If a persister returns a temporary failure, the channel monitor should be able
to be put on ice and then revived later. If a persister returns a permanent
failure, the channel should be force closed.
Intended to be a cross-platform implementation of the
channelmonitor::Persist trait.
This adds a new lightning-persister crate, that uses the
newly exposed lightning crate's test utilities.
Notably, this crate is pretty small right now. However, due to
future plans to add more data persistence (e.g. persisting the
ChannelManager, etc) and a desire to avoid pulling in filesystem
usage into the core lightning package, it is best for it to be
separated out.
Note: Windows necessitates the use of OpenOptions with the `write`
permission enabled to `sync_all` on a newly opened channel's
data file.
- The ChainMonitor should:
Whenever a new channel is added or updated, these updates
should be conveyed to the persister and persisted to disk.
Even if the update errors while it's being applied, the
updated monitor still needs to be persisted.