Jeffrey Czyz [Fri, 27 Mar 2020 23:46:57 +0000 (16:46 -0700)]
Test failing backward any pending HTLCs
Upon channel failure, any pending HTLCs in a channel's holding cell must
be failed backward. The added test exercises this behavior and
demonstrates a deadlock triggered within the handle_error!() macro. The
deadlock occurs when the channel_state lock is already held and then
reacquired when finish_force_close_channel() is called.
If we have HTLCs to fail backwards, handle_error!() will call
finish_force_close_channel() which will attempt to lock channel_state
while it is locked at the original caller. Instead, hold the lock for
shorter scopes such that it is not held upon entering handle_error!().
Co-authored-by: Matt Corallo <git@bluematt.me> Co-authored-by: Jeffrey Czyz <jkczyz@gmail.com>
Antoine Riard [Fri, 20 Mar 2020 20:58:13 +0000 (16:58 -0400)]
Remove useless local commitment txn signatures
check_spend_local_transaction is tasked with detection of
onchain local commitment transaction and generate HTLC transaction.
Signing an already onchain tx isn't necessary.
Antoine Riard [Wed, 18 Mar 2020 04:29:26 +0000 (00:29 -0400)]
Add ChanSigner in OnchainTxHandler
Rename ChannelMonitor::Storage to OnchainDetection,
holder of channel state (base_key+per_commitment_point)
to detect onchain transactions accordingly.
Going further between splitting detection and transaction
generation, we endow OnchainTxHandler with keys access.
That way, in latter commits, we may remove secret keys entirely
from ChannelMonitor.
Antoine Riard [Wed, 18 Mar 2020 05:15:28 +0000 (01:15 -0400)]
Remove Watchtower mode from Storage enum and make it a struct
Watchtower will be supported through external signer interface
where a watchtower implementation may differ from a local one
by the scope of key access and pre-signed datas.
Matt Corallo [Thu, 19 Mar 2020 23:15:06 +0000 (19:15 -0400)]
Fail to deserialize ChannelManager if it is ahead of any monitor(s)
If any monitors are out of sync with the Channel, we previously
closed the channel, but we should really only do that if the
monitor is ahead of the channel, opting to call the whole thing
invalid if the channel is ahead of the monitor.
Antoine Riard [Thu, 19 Mar 2020 00:58:05 +0000 (20:58 -0400)]
Implement reorg-safety for SpendableOutputDescriptor detection
We delay SpendableOutputDescriptor until reaching ANTI_REORG_DELAY
to avoid misleading user wallet in case of reorg and alternative
settlement on a channel output.
Previously, we would generate SpendableOutputDescriptor::StaticOutput
in OnchainTxHandler even if our claiming transaction wouldn't confirm
onchain, misbehaving user wallet to think it receives more funds than
in reality.
Matt Corallo [Thu, 19 Mar 2020 01:37:09 +0000 (21:37 -0400)]
Fetch latest commitment txn via monitor, not channel in test
Eventually, we want to remove the Channel's copy of its own
ChannelMonitor, reducing memory footprint and complexity of
ChannelManager greatly.
This removes the last uses of said ChannelMonitor for latest
local commitment transactions (though it is still used for
would_broadcast_at_height(), which is the last remaining use).
If we call get_update_fulfill_htlc (in this case via
ChannelManager::claim_funds_internal ->
Channel::get_update_fulfill_htlc_and_commit) and it finds that we
already have a holding-cell pending HTLC claim, it will return no
monitor update but leave latest_monitor_update_id incremented.
If we later go and add a new monitor update we'll panic as the
updates appear to have been applied out-of-order.
Antoine Riard [Wed, 26 Feb 2020 23:18:27 +0000 (18:18 -0500)]
Watch outputs of revoked HTLC-transactions
Bumping of justice txn on revoked HTLC-Success/HTLC-timeout is triggered
until our claim is confirmed onchain with at least
ANTI_REORG_DELAY_SAFE. Before this patch, we weren't tracking them in
check_spend_remote_htlc, leading us to infinite bumps.
Antoine Riard [Wed, 11 Mar 2020 19:10:29 +0000 (15:10 -0400)]
Fix duplicata of adjusted justice tx generation in OnchainTxHandler
Adjusted tx occurs when a previous aggregated claim tx has
seen one of its outpoint being partially claimed by a remote tx.
To pursue claiming of the remaining outpoint a adjusted claim tx
is generated with leftover of claimable outpoints.
Previously, in case of block-rescan where a partial claim occurs,
we would generate duplicated adjusted tx, wrongly inflating feerate
for next bumps. At rescan, if input has already been dropped from
outpoints map from a claiming request, don't regenerate again
a adjuste tx.
3d640da5c343111f538f006996c13c9a98e0d9e6 changed the indexes for
some enums in ChannelMonitor deserialization but not serialization.
Thus, the chanmon_deser_target fuzz target failed on travis on at
least one PR.
Antoine Riard [Tue, 10 Mar 2020 17:03:10 +0000 (13:03 -0400)]
Make htlc_minimum_msat configurable
Enforce a minimum htlc_minimum_msat of 1.
Instead of computing dynamically htlc_minimum_msat based on feerate,
relies on user-provided configuration value. This let user compute
an economical-driven channel parameter according to network dynamics.
Matt Corallo [Mon, 24 Feb 2020 19:17:04 +0000 (14:17 -0500)]
Fix long-standing race in net-tokio reading after a disconnect event
If rust-lightning tells us to disconnect a socket after we read
some bytes from the socket, but before we actually give those bytes
to rust-lightning, we may end up calling rust-lightning with a
Descriptor that isn't registered anymore.
Sadly, there really isn't a good way to solve this, and it should
be a pretty quick event, so we just busy-wait.
Matt Corallo [Sat, 15 Feb 2020 03:32:30 +0000 (22:32 -0500)]
Update pre-HTLC DataLossProtect to match new spec changes
This was the way DataLossProtect was originally written, however it
didn't match other implementations at the time during testing. It
turns out, other implementations didn't agree with each other
anyway (depending on the exact timeline), so the spec was clarified
somewhat in https://github.com/lightningnetwork/lightning-rfc/pull/550
. This updates us to be in line with the new guidance and appears
to solve out-of-sync issues in testing.
Matt Corallo [Thu, 5 Mar 2020 23:01:06 +0000 (18:01 -0500)]
Use block timestamps as the min for generated update messages.
Fixes issue #493 and should resolve some issues where other nodes
(incorrectly) reject channel_update/node_announcement messages
which have a serial number that is not a relatively recent
timestamp.
Matt Corallo [Fri, 3 Jan 2020 01:32:37 +0000 (20:32 -0500)]
Add ability to broadcast our own node_announcement.
This is a somewhat-obvious oversight in the capabilities of
rust-lightning, though not a particularly interesting one until we
start relying on node_features (eg for variable-length-onions and
Base AMP).
Sadly its not fully automated as we don't really want to store the
list of available addresses from the user. However, with a simple
call to ChannelManager::broadcast_node_announcement and a sensible
peer_handler, the announcement is made.
Matt Corallo [Wed, 4 Mar 2020 22:45:27 +0000 (17:45 -0500)]
Take multiple spent-txn to check_spends! in functional_tests
This reintroduces a check_spends!() removed in 3d640da5c343111f538f
due to check_spends not being able to check a transaction which
spends multiple other transactions.
It also simplifies a few calls in claim_htlc_outputs_single_tx by
using check_spends!().
Matt Corallo [Wed, 4 Mar 2020 22:36:12 +0000 (17:36 -0500)]
Drop redundant .clone() in check_spends calls.
The API to rust-bitcoin to check a transaction correctly spends
another changed some time ago, but we still have a lot of needless
.clone()s in our tests.
Matt Corallo [Wed, 4 Mar 2020 22:27:03 +0000 (17:27 -0500)]
Flatten Vec passed from channelmonitor to onchaintx block_connected
Instead of passing a Vec of Vecs drop them into one as we go in
ChannelMonitor, hopefully avoiding a bit of memory fragmentation
and improving readability.
Antoine Riard [Fri, 24 Jan 2020 16:57:52 +0000 (11:57 -0500)]
Introduce OnchainTxHandler, move bumping and tracking logic
Encapsulates tracking and bumping of in-flight transactions in
its own component. This component may be latter abstracted
to reuse tracking and RBF for new features (e.g dual-funding,
splicing)
Build all transactions generation in one place. Also as fees
and signatures are closely tied, what keys do you have determine
what bumping mode you can use.
Matt Corallo [Mon, 24 Feb 2020 04:26:10 +0000 (23:26 -0500)]
Test Router serialization round-trip in functional_tests.
This tests Router serialization round-trip at the end of each
functional test in the same way we do ChannelMonitors and
ChannelManagers to catch any cases where we were able to get into
a state which would have prevented reading a Router back off disk.
We further walk all of the announcements which both the original
and deserialized Routers would send to peers requesting initial
sync to ensure they match.
Matt Corallo [Mon, 24 Feb 2020 04:25:43 +0000 (23:25 -0500)]
Change Option<T> serialization format to include length
This is a cheap way to fix an error in Router serialization
roundtrip due to us calling read_to_end during the read of
channel/node announcement/updates. During normal message reading,
we only have limited bytes to read (specifically the message buffer)
so this is fine, however when we read them inside Router, we have
more data from other fields of the Router available as well. Thus,
we end up reading the entire rest of the Router into one message
field, and failing to deserialize.
Because such fields are always stored in Option<>s, we can simply
use a LengthLimitingStream in the Option<> serialization format and
make only the correct number of bytes available.
By using a variable-length integer for the new field, we avoid
wasting space compared to the existing serialization format.
Matt Corallo [Mon, 24 Feb 2020 04:12:19 +0000 (23:12 -0500)]
Make Readable::read a templated on the stream, not Readable itself
This makes Readable symmetric with Writeable and makes sense -
something which is Readable should be Readable for any stream which
implements std::io::Read, not only for a stream type it decides on.
This solves some lifetime-compatibility issues in trying to read()
from a LengthLimitingReader in arbitrary Readable impls.