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)
Matt Corallo [Sat, 29 May 2021 18:24:16 +0000 (18:24 +0000)]
Avoid calling libsecp serialization fns when calculating length
When writing out libsecp256k1 objects during serialization in a
TLV, we potentially calculate the TLV length twice before
performing the actual serialization (once when calculating the
total TLV-stream length and once when calculating the length of the
secp256k1-object-containing TLV). Because the lengths of secp256k1
objects is a constant, we'd ideally like LLVM to entirely optimize
out those calls and simply know the expected length. However,
without cross-language LTO, there is no way for LLVM to verify that
there are no side-effects of the calls to libsecp256k1, leaving
LLVM with no way to optimize them out.
This commit adds a new method to `Writeable` which returns the
length of an object once serialized. It is implemented by default
using `LengthCalculatingWriter` (which LLVM generally optimizes out
for Rust objects) and overrides it for libsecp256k1 objects.
As of this commit, on an Intel 2687W v3, the serialization
benchmarks take:
test routing::network_graph::benches::read_network_graph ... bench: 2,035,402,164 ns/iter (+/- 1,855,357)
test routing::network_graph::benches::write_network_graph ... bench: 308,235,267 ns/iter (+/- 140,202)
Matt Corallo [Fri, 28 May 2021 01:40:22 +0000 (01:40 +0000)]
Drop byte_utils in favor of native `to/from_be_bytes` methods
Now that our MSRV supports the native methods, we have no need
for the helpers anymore. Because LLVM was already matching our
byte_utils methods as byteswap functions, this should have no
impact on generated (optimzied) code.
This removes most of the byte_utils usage, though some remains to
keep the patch size reasonable.
Matt Corallo [Fri, 28 May 2021 14:16:20 +0000 (14:16 +0000)]
Add bench profiles to Cargo.toml to force codegen-units=1
This makes a small difference for NetworkGraph deserialization
as it enables more inlining across different files, hopefully
better matching user performance as well.
As of this commit, on an Intel 2687W v3, the serialization
benchmarks take:
test routing::network_graph::benches::read_network_graph ... bench: 2,037,875,071 ns/iter (+/- 760,370)
test routing::network_graph::benches::write_network_graph ... bench: 320,561,557 ns/iter (+/- 176,343)
Matt Corallo [Wed, 21 Apr 2021 20:47:24 +0000 (20:47 +0000)]
Correct Channel outbound HTLC serialization
Channel serialization should happen "as if
remove_uncommitted_htlcs_and_mark_paused had just been called".
This is true for the most part, but outbound RemoteRemoved HTLCs
were being serialized as normal, even though
`remote_uncommitted_htlcs_and_mark_paused` resets them to
`Committed`.
This led to a bug identified by the `chanmon_consistency_target`
fuzzer wherein, if we receive a update_*_htlc message bug not the
corresponding commitment_signed prior to a serialization roundtrip,
we'd force-close the channel due to the peer "attempting to
fail/claim an HTLC which was already failed/claimed".
Matt Corallo [Wed, 21 Apr 2021 17:03:57 +0000 (17:03 +0000)]
[fuzz] Expand chanmon_consistency to do single message delivery
While trying to debug the issue ultimately tracked down to a
`PeerHandler` locking bug in #891, the ability to deliver only
individual messages at a time in chanmon_consistency looked
important. Specifically, it initially appeared there may be a race
when an update_add_htlc was delivered, then a node sent a payment,
and only after that, the corresponding commitment-signed was
delivered.
This commit adds such an ability, greatly expanding the potential
for chanmon_consistency to identify channel state machine bugs.
Matt Corallo [Sun, 30 May 2021 17:04:21 +0000 (17:04 +0000)]
Add new `(C-not implementable)` tag on `EventsProvider`
This makes it so that users cannot usefully implement their own
`EventsProvider`, which would require substantial new logic in the
bindings generator (for generic methods). In the case of
`EventsProvider`, because there are no Rust methods which accept an
`EventsProvider` as an argument, this is perfectly OK as the
generated code would be entirely unused anyway.
Matt Corallo [Wed, 19 May 2021 21:47:42 +0000 (21:47 +0000)]
Delay broadcast of PackageTemplate packages until their locktime
This stores transaction templates temporarily until their locktime
is reached, avoiding broadcasting (or RBF bumping) transactions
prior to their locktime. For those broadcasting transactions
(potentially indirectly) via Bitcoin Core RPC, this ensures no
automated rebroadcast of transactions on the client side is
required to get transactions confirmed.
Matt Corallo [Wed, 26 May 2021 15:47:29 +0000 (15:47 +0000)]
Add assertions to ensure we don't use an invalid package_amount
This somewhat cleans up the public API of PackageSolvingData to
make it harder to get an invalid amount and use it, adding further
debug assertion to check it at test-time.
Matt Corallo [Tue, 25 May 2021 21:18:30 +0000 (21:18 +0000)]
Add a macro which implements Readable/Writeable using TLVs only
This also includes a `VecWriteWrapper` and `VecReadWrapper` which
implements serialization for any `Readable`/`Writeable` type that is
in a Vec. We do this instead of implementing `Readable`/`Writeable`
directly as there isn't always a univerally-defined way to serialize
a Vec and this makes things more explicit.
Finally, this tweaks existing macros (and in the new macros) to
support a trailing `,` after a list, eg
`write_tlv_fields!(stream, {(0, a),}, {});` whereas previously the
trailing `,` after the `(0, a)` would be a compile-error.
Jeffrey Czyz [Thu, 27 May 2021 15:53:14 +0000 (08:53 -0700)]
Cache socket address in HttpClient for reconnect
If the HttpClient attempts to reconnect to bitcoind that is no longer
running, the client fails to get the address from the stream. Cache the
address when connecting to prevent this.
Jeffrey Czyz [Wed, 26 May 2021 17:57:39 +0000 (10:57 -0700)]
Parse RPC errors as JSON content
Bitcoin Core's JSON RPC server returns errors as HTTP error responses
with JSON content in the body. Parse this content as JSON to give a more
meaningful error. Otherwise, "binary" is given because the content
contains ASCII control characters.
Jeffrey Czyz [Wed, 26 May 2021 17:51:16 +0000 (10:51 -0700)]
Define an HttpError for returning error contents
Return an HTTP error response as a status code and contents. This allows
clients to interpret the response as desired (e.g., the contents as a
JSON-formatted error).
Antoine Riard [Fri, 7 May 2021 23:36:50 +0000 (19:36 -0400)]
Introduce PackageTemplae, a replacement of InputMaterial
PackageTemplate aims to replace InputMaterial, introducing a clean
interface to manipulate a wide range of claim types without
OnchainTxHandler aware of special content of each.
Antoine Riard [Tue, 16 Jun 2020 00:11:01 +0000 (20:11 -0400)]
Add package.rs file
Package.rs aims to gather interfaces to communicate between
onchain channel transactions parser (ChannelMonitor) and outputs
claiming logic (OnchainTxHandler). These interfaces are data
structures, generated per-case by ChannelMonitor and consumed
blindly by OnchainTxHandler.