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.
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.
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.
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).
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.
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.
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.
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.
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.
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.
Previously we'd ignored generic arguments in traits, leading to
bogus code generation after the Persister trait was added in #681.
This adds minimal support for it, fixing code generation on latest
upstream.
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.
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.
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.
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.
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
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.
