Matt Corallo [Fri, 17 Apr 2020 23:31:24 +0000 (19:31 -0400)]
Drop uneccessary indirection in map-updating in 1107ab06c3
1107ab06c33bd360bdee7ee64f4b690e753003f6 added a Vec of future
updates to apply during a loop, fixing a borrow checker issue that
didn't exist in the merged version of the patch. This simply reverts
that small part of the change.
Matt Corallo [Sat, 21 Mar 2020 22:29:17 +0000 (18:29 -0400)]
Flatten ChannelMonitor substructs that don't add clarity
The new OnchainDetection struct (which is the remnants of the old
KeyStorage enum, which was removed in 1dbda4faedc33506e63176e6a456)
doesn't really add any clarity to ChannelMonitor, so best to just
drop it and move its members into ChannelMonitor directly.
Antoine Riard [Wed, 8 Apr 2020 01:07:55 +0000 (21:07 -0400)]
Add test_update_err_monitor_lockdown
This test tries the new lockdown logic in case of a signed-and-broadcast
local commitment transaction while a concurrent ChannelMonitorUpdate for
a next _local_ commitment is submitted from offchain. Update is rejected
as expected with a ChannelMonitorUpdateErr.
Antoine Riard [Mon, 6 Apr 2020 22:54:45 +0000 (18:54 -0400)]
Monitor should panic on receiving buggy update sequences
Channel shouldn't send a ChannelForceClosed update followed by
a LatestLocalCommitmentTxInfo as it would be a programming error
leading to risk of money loss. Force-closing the channel will
broadcast the local commitment transaction, if the revocation
secret for this one is released after its broadcast, it would
allow remote party to claim outputs on this transaction using
the revocation path.
Antoine Riard [Mon, 6 Apr 2020 22:32:57 +0000 (18:32 -0400)]
Make acces and signature of local commitment transaction unique
Local commitment transaction broadcast can be triggered by a)
a Channel force-close or b) reaching some block height implying
a onchain HTLC-timeout. If one of this condition is fulfilled,
commitment is signed and from then any state update would be
rejected.
ChannelMonitor init at Channel creation need to be refactored
before to make get_fully_signed_local_tx infaillible to avoid
choking in the test framework.
Antoine Riard [Mon, 9 Mar 2020 22:15:35 +0000 (18:15 -0400)]
Move HTLC tx generation in OnchainTxHandler
HTLC Transaction can't be bumped without sighash changes
so their gneeration is one-time for nwo. We move them in
OnchainTxHandler for simplifying ChannelMonitor and to prepare
storage of keys material behind one external signer interface.
Some tests break due to change in transaction broadcaster order.
Number of transactions may vary because of temporary anti-duplicata
tweak can't dissociate between 2- broadcast from different
origins (ChannelMonitor, ChannelManager) and 2-broadcast from same
component.
Antoine Riard [Mon, 23 Mar 2020 05:30:48 +0000 (01:30 -0400)]
Add OnchainTxHandler::get_fully_signed_htlc
In case of channel force-closure, access to local commitment
transactions and its dependent HTLCs is needed. Instead of using
broadcast_by_local_state which registers outpoint to claim and
outputs to watch which are going to be discarded in this case,
we simply ask OnchainTxHandler to build and sign HTLC transactions
through new API.
Antoine Riard [Sat, 21 Mar 2020 22:52:00 +0000 (18:52 -0400)]
Cache per-HTLC data in OnchainTxHandler::HTLCTxCache
Splitting further parsing from transaction generation, we cache
transaction elements needed for local HTLC transaction inside
OnchainTxHandler. Duplicated data will be removed from ChannelMonitor
in future commits.
Antoine Riard [Tue, 7 Apr 2020 22:46:14 +0000 (18:46 -0400)]
Cache HTLC transaction material inside OnchainTxHandler
Splitting further parsing from transaction generation, we cache
transaction elements needed for local HTLC transaction inside
OnchainTxHandler. Duplicated data will be removed from ChannelMonitor
in future commits.
Antoine Riard [Sat, 21 Mar 2020 19:39:19 +0000 (15:39 -0400)]
Remove signing htlc transaction from ChannelMonitor
Extend external signer interface to sign HTLC transactions on its
behalf without seckey passing. This move will allow us to remove
key access access from ChannelMonitor hot memory in further work.
HTLC transactions should stay half-signed by remote until
we need to broadcast them for timing-out/claiming HTLCs onchain.
Antoine Riard [Sat, 21 Mar 2020 19:48:11 +0000 (15:48 -0400)]
Remove per_input_material introduce in commit 0011713
Caching of input material for HTLC transaction was introducted
prevently but since then API (InputMaterial) has changed
between ChannelMonitor and OnchainTxHandler
Antoine Riard [Sat, 21 Mar 2020 02:41:12 +0000 (22:41 -0400)]
Access signed local commitment through OnchainTxHandler
Implementing dynamic fee bumping implied to cache transaction material
including its witness, to generate a bumped version if needed.
ChannelMonitor is slowly rescoped to its parsing function with ongoing
patchset and data duplicata are removed. If signed local commitment tx
access is needed, it's done through OnchainTxHandler extended API
For test framework purpose, we use the test-only method
ChannelMonitor::unsafe_get_latest_local_commitment_txn to intentionally
generate unsafe local commitment to exerce revocation logic.
Antoine Riard [Tue, 24 Mar 2020 23:26:28 +0000 (19:26 -0400)]
Cache current local commitment number in ChannelMonitor.
By caching current local commitment number instead of deciphering
it from local commitment tx, we may remove local commitment tx
from ChannelMonitor in next commit.
Antoine Riard [Tue, 3 Mar 2020 23:51:50 +0000 (18:51 -0500)]
Move local commitment tx generation in OnchainTxHandler
Local Commitment Transaction can't be bumped without anchor outputs
so their generation is one-time for now. We move them in
OnchainTxHandler for simplifying ChannelMonitor and to prepare
storage of keys material behind one external signer interface.
Some tests break due to change in transaction broadcast order but
number of transactions broadcast should stay the same.
Antoine Riard [Fri, 27 Mar 2020 21:53:52 +0000 (17:53 -0400)]
Prevent any update of local commitment transaction once signed
To prevent any unsafe state discrepancy between offchain and onchain,
once local commitment transaction has been signed due to an event
(either block height for HTLC-timeout or channel force-closure), don't
allow any further update of local commitment transaction view
to avoid delivery of revocation secret to counterparty for the
aformentionned signed transaction.
Antoine Riard [Fri, 20 Mar 2020 22:04:01 +0000 (18:04 -0400)]
Remove signing local commitment transaction from ChannelMonitor
Extend external signer interface to sign local commitment transactions
on its behalf without seckey passing. This move will allow us to remove
key access from ChannelMonitor hot memory in further work.
Local commitment transaction should stay half-signed by remote until
we need to broadcast for a channel force-close or a HTLC to timeout onchain.
Add an unsafe test-only version of sign_local_commitment to fulfill our
test_framework needs.
Matt Corallo [Tue, 7 Apr 2020 17:36:58 +0000 (13:36 -0400)]
Pass Route to send_payment as a reference, not move
ChannelManager::send_payment stopped utilizing its ownership of the
Route with MPP (which, for readability, now clone()s the individual
paths when creating HTLCSource::OutboundRoute objects). While this
isn't ideal, it likely also makes sense to ensure that the user has
access to the Route after sending to correlate individual path
failures with the paths in the route or, in the future, retry
individual paths.
Thus, the easiest solution is to just take the Route by reference,
allowing the user to retain ownership.
Matt Corallo [Tue, 7 Jan 2020 01:29:33 +0000 (20:29 -0500)]
Implement multipath sends using payment_secret.
This rather dramatically changes the return type of send_payment
making it much clearer when resending is safe and allowing us to
return a list of Results since different paths may have different
return values.
Matt Corallo [Mon, 30 Mar 2020 20:24:19 +0000 (16:24 -0400)]
Refactor payment-claim logic to ensure MPP-claim atomicity
Previously if we claimed an MPP where a previous-hop channel was
closed while we were waitng for the user to provide us the preimage
we'd simply skip claiming that HTLC without letting the user know.
This refactors the claim logic to first check that all the channels
are still available (which is actually all we need - we really
mostly care about updating the channel monitors, not the channels
themselves) and then claim the HTLCs in the same lock, ensuring
atomicity.
Matt Corallo [Thu, 2 Jan 2020 06:23:48 +0000 (01:23 -0500)]
Impl Base AMP in the receive pipeline and expose payment_secret
Base AMP is centered around the concept of a 'payment_secret` - an
opaque 32-byte random string which is used to authenticate the
sender to the recipient as well as tie the various HTLCs which
make up one payment together. This new field gets exposed in a
number of places, though sadly only as an Option for backwards
compatibility when sending to a receiver/receiving from a sender
which does not support Base AMP.
Sadly a huge diff here, but almost all of it is changing the method
signatures for sending/receiving/failing HTLCs and the
PaymentReceived event, which all now need to expose an
Option<[u8; 32]> for the payment_secret.
It doesn't yet properly fail back pending HTLCs when the full AMP
payment is never received (which should result in accidental
channel force-closures). Further, as sending AMP payments is not
yet supported, the only test here is a simple single-path payment
with a payment_secret in it.
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.