This commit isolates preimages of forwarded htlcs from invoice
preimages. The reason to do this is to prevent the incoming contest
resolver from settling exit hop htlcs for which the invoice isn't marked
as settled.
The former tryApplyPreimage function silently ignored invalid preimages.
This could mask potential bugs. This commit makes the logic stricter and
generates an error in case an unexpected mismatch occurs.
New behaviour of the chain notifier to always send the current block
immediately after registration takes away the need to make a separate
GetBestBlock call on ChainIO.
Now that the success resolver preimage field is always populated by the
incoming contest resolver, preimage lookups earlier in the
process (channel and channel arbitrator) can mostly be removed.
One of the first things the incoming contest resolver does is checking
if the preimage is available and if it is, convert itself into a success
resolver.
This behaviour makes it unnecessary to already determine earlier in the
process whether an incoming contest or a success resolver is needed.
By having all incoming htlcs go through the incoming contest resolver,
the number of execution paths is reduced and it becomes easier to
ascertain that the implemented logic is correct.
The only functional change in this commit is that a forwarded htlc for
which is the preimage is known, is no longer settled when the htlc is
already expired. Previously a success resolver would be instantiated
directly, skipping the expiry height check.
This created a risk that the success resolver would never finish,
because an expired htlc could already have been swept by the remote
party and there is no detection of this remote spend in the success
resolver currently.
With the new change, the general direction that an expired htlc
shouldn't be settled and instead given up on is implemented more
consistently.
This commit prepares for fixing edges cases related to hodl
invoice on-chain resolution.
In this commit, we modify the way we detect local force closes. Before
this commit, we would directly check the broadcast commitment's txid
against what we know to be our best local commitment. In the case of DLP
recovery from an SCB, it's possible that the user force closed, _then_
attempted to recover their channels. As a result, we need to check the
outputs directly in order to also handle this rare, but
possible recovery scenario.
The new detection method uses the outputs to detect if it's a local
commitment or not. Based on the state number, we'll re-derive the
expected scripts, and check to see if they're on the commitment. If not,
then we know it's a remote force close. A new test has been added to
exercise this new behavior, ensuring we catch local closes where we have
and don't have a direct output.
In this commit, we speed up the `TestChainWatcherDataLossProtect`
_considerably_ by enumerating relevant tests using table driven tests
rather than generating random tests via the `testing/quick` package.
Each of these test cases are also run in parallel bringing down the
execution time of this test from a few minutes, to a few seconds.
This commit adds logging of the reason to go to chain for a channel.
This can help users to find out the reason why a channels forced closed.
To get all go to chain reasons, an optimization to break early is
removed. This optimization was not significant, because the normal flow
already examined all htlcs. In the exceptional case where we need to go
to chain, it does not weigh up against logging all go to chain reasons.
This commits exposes the various parameters around going to chain and
accepting htlcs in a clear way.
In addition to this, it reverts those parameters to what they were
before the merge of commit d1076271456bdab1625ea6b52b93ca3e1bd9aed9.
In this commit, we modify the `closeObserver` to fast path the DLP
dispatch case if we detect that the channel has been restored. We do
this as otherwise, we may inadvertently enter one of the other cases
erroneously, causing us to now properly look up their dlp commitment
point.
In this commit, we modify the main `closeObserver` dispatch loop to only
look for the local force close if we didn't recover the channel. We do
this, as for a recovered channel, it isn't possible for us to force
close from a recovered channel.
The multiplier doesn't make sense because funds may be equally at risk
by failing to broadcast to chain regardless of whether the HTLC is a
redeem or a timeout.
In this commit, we simplify the existing `htlcTImeoutResolver` with some
newly refactored out methods from the `htlcTimeoutContestResolver`. The
resulting logic is easier to follow as it's more linear, and only deals
with spend notifications rather than both spend _and_ confirmation
notifications.
This commit modifies the invoice registry to handle invoices for which
the preimage is not known yet (hodl invoices). In that case, the
resolution channel passed in from links and resolvers is stored until we
either learn the preimage or want to cancel the htlc.
This commit detaches signaling the invoice registry that an htlc was
locked in from the actually settling of the htlc.
It is a preparation for hodl invoices.
Previously a function pointer was passed to chain arbitrator to avoid a
circular dependency. Now that the routetypes package exists, we can pass
the full invoice registry to chain arbitrator.
This is a preparation to be able to use other invoice registry methods
in contract resolvers.
In this commit, we address a lingering issue within some subsystems that
are responsible for broadcasting transactions. Previously,
ErrDoubleSpend indicated that a transaction was already included in the
mempool/chain. This error was then modified to actually be returned for
conflicting transactions, but its callers were not modified accordingly.
This would lead to conflicting transactions to be interpreted as valid,
when they shouldn't be.
In this commit, we fix an off-by-one error when handling force closes
from the remote party. Before this commit, if the remote party
broadcasts state 2, and we were on state 1, then we wouldn't act at all.
This is due to an extraneous +1 in the comparison, causing us to only
detect this DLP case if the remote party's state is two beyond what we
know atm. Before this commit, the test added in the prior commit failed.
In this commit, we add a new test case to exercise the way we handle the
DLP detection and dispatch within the chain watcher. Briefly, we use
the `testing/quick` package to ensure that the following invariant is
always held: "if we do N state updates, then state M is broadcast, iff M
> N, we'll execute the DLP protocol". We limit the number of iterations
to 10 for now, as the tests can take a bit of time to execute, since it
actually does proper state transitions.
In this commit, we abstract the call to `GetStateNumHint` within the
`closeObserver` method to a function closure in the primary config. This
allows us to feed in an arbitrary broadcast state number within unit
tests.
In this commit, we ensure that we mark the channel as borked before we
remove the link during the force close process. This ensures that if the
peer reconnects right after we remove the link, then it won't be loaded
into memory in `loadActiveChannels`. We'll now:
* mark the channel as borked
* remove the link
* read the channel state from disk
* force close
This ensures that the link (if it's active) is able to commit any
pending changes to disk before we read out the channel to force close.
In this commit, we modify the WitnessCache's
AddPreimage method to accept a variadic number
of preimages. This enables callers to batch
preimage writes in performance critical areas
of the codebase, e.g. the htlcswitch.
Additionally, we lift the computation of the
witnesses' keys outside of the db transaction.
This saves us from having to do hashing inside
and blocking other callers, and limits extraneous
blocking at the call site.
Now that the sweeper is available, it isn't necessary anymore for the
commit resolver to craft its own sweep tx. Instead it can offer its
input to the sweeper and wait for the outcome.
Previously the arbitrator wasn't advanced to the final stage after
the last contract resolved.
Also channel arbitrator now does not ignore a log error anymore
unresolved contracts cannot be retrieved.
This commit is a step to split the lnwallet package. It puts the Input
interface and implementations in a separate package along with all their
dependencies from lnwallet.
In this commit, we modify areas where we need to force close a channel
to use the new FetchChannel method instead of manually scanning. This
dramatically reduces the CPU usage when doing things like closing a
large number of channels within lnd.
In this commit, we extend the htlcSuccessResolver to settle the invoice,
if any, of the corresponding on-chain HTLC sweep. This ensures that the
invoice state is consistent as when claiming the HTLC "off-chain".
Previously, contract resolvers that needed to publish a second level tx,
did not have access to the original htlc amount.
This commit reconstructs this amount from data that is already persisted
in arbitrator log.
Co-authored-by: Joost Jager <joost.jager@gmail.com>
This commit removes the breach transaction from the
arguments passed to NewBreachRetribution. We already
keep all prior remote commitments on disk in the
commitment log, and load that transaction from disk
inside the method. In practice, the one loaded from
disk will be the same one that is passed in by the
caller, so there should be no change in behavior
as we've already derived the appropriate state number.
This changes makes integration with the watchtower
client simpler, since we no longer need to acquire
the breaching commitment transaction to be able to
construct the BreachRetribution. This simplifies
not only the logic surrounding transient backsups,
but also on startup (and later, retroactively
backing up historic updates).
In this commit, we extract the existing determineFeePerKw method on the
RPC server into a new file in the sweep package. Along the way, we
consolidate code by introducing a new FeePreference struct, which allows
the caller to express their fee preference either in blocks to
confirmation, or a direct fee rate. This move takes a small step to
father decoupling calls in the main RPC server.
This commit is a preparation for the implementation of remote spend
detection. Remote spends may happen before we broadcast our own sweep
tx. This calls for accurate height hints.
In this commit, we remove the per channel `sigPool` within the
`lnwallet.LightningChannel` struct. With this change, we ensure that as
the number of channels grows, the number of gouroutines idling in the
sigPool stays constant. It's the case that currently on the daemon, most
channels are likely inactive, with only a hand full actually
consistently carrying out channel updates. As a result, this change
should reduce the amount of idle CPU usage, as we have less active
goroutines in select loops.
In order to make this change, the `SigPool` itself has been publicly
exported such that outside callers can make a `SigPool` and pass it into
newly created channels. Since the sig pool now lives outside the
channel, we were also able to do away with the Stop() method on the
channel all together.
Finally, the server is the sub-system that is currently responsible for
managing the `SigPool` within lnd.
In this commit, we prevent the ChainArbitrator from sending a force
close request for a channel if it has previously already sent one. We do
this to prevent blocking the caller of ForceCloseContract.
We pool the database for the channel commit point with an exponential
backoff. This is meant to handle the case where we are in process of
handling a channel sync, and the case where we detect a channel close
and must wait for the peer to come online to start channel sync before
we can proceed.
In this commit, we modify the newly introduced UtxoSweeper.CreateSweepTx
to accept the confirmation target as a param of the method rather than a
struct level variable. We do this as this allows each caller to decide
at sweep time, what the fee rate should be, rather than using a global
value that is meant to work in all scenarios. For example, anytime
we're sweeping an output with a CLTV lock that's has a dependant
transaction we need to sweep/cancel, we may require a higher fee rate
than a regular force close with a CSV output.
This commit restructures the initialization procedure
for chain watchers such that they can proceed in parallel.
This is primarily to help nodes running with the neutrino
backend, which otherwise forces a serial rescan for each
active channel to check for spentness.
Doing so allows the rescans to take advantage of batch
scheduling in registering for the spend notifications,
ensuring that only one or two passes are made, as opposed
to one for each channel.
Lastly, this commit ensures that the chain arb is properly
shutdown if any of it's chain watchers or channel arbs
fails to start, so as to cancel their goroutines before
exiting.
At ChannelArbitrator startup we now check the database close status of
the channel. If we detect that the channel is closed, but our state
machine hasn't advanced to reflect that (possibly because of a shutdown
before the state transition was finished), we manually trigger the state
transition to recover.
This commit moves the responsibility for closing local and remote force
closes in the database from the chain watcher to the channel arbitrator.
We do this because we previously would close the channel in the
database, before sending the event to the channel arbitrator. This could
lead to a situation where the channel was marked closed, but the channel
arbitrator didn't receive the event before shutdown. As we don't listen
for chain events for channels that are closed, those channels would be
stuck in the pending close state forever, as the channel arbitrator
state machine wouldn't progress.
We fix this by letting the ChannelArbitrator close the channel in the
database. After the contract resolutions are logged (in the state
callback before transitioning to StateContractClosed) we mark the
channel closed in the database. This way we make sure that it is marked
closed only if the resolutions have been successfully persisted.
This commit removes the state callback, and instead logs the contract
resolutions directly after receiving the unilateral close event. The
resolutions won't change so there's not really necessary to wait to log
them, and this greatly simplifies the code.
This commit attempts to fix an inconsistency in when we consider an HTLC
to expire. When we first launched the resolver we would compare the
current block height against the expiry, while for new incoming blocks
we would compare against expiry-1.
This lead to a flake during integration tests, during a call to
RestartNode after _exactly_ enough blocks for the HTLC to expire. In
some cases the resolver would see the new blocks and consider the HTLC
to be expired (because of the -1), while in some cases resolver would
shut down before seeing the new blocks, and upon restart wouldn't act on
the new height because we did not compare against -1.
This commit fixes this by doing the same comparison in both cases.
Due to a recent change within the codebase to return estimated fee rates
in sat/kw, this commit ensures that we use this fee rate properly by
calculing a transaction's fees using its weight. This includes all of
the different transactions that are created within lnd (funding, sweeps,
etc.). On-chain transactions still rely on a sat/vbyte fee rate since it's
required by btcwallet.
This commit fixes a potential race condition during
shutdown, that could allow the chain arb's
activeWatchers or activeChannels map to be modified
while ranging over their contents. We fix this by
copying the contents into new maps with the mutex
held, before releasing the mutex and shutting down
each watcher or channel arbitrator.
This commit makes the chainwatcher attempt to dispatch a remote close
when it detects a remote state with a state number higher than our
known remote state. This can mean that we lost some state, and we check
the database for (hopefully) a data loss commit point retrieved during
channel sync with the remote peer. If this commit point is found in the
database we use it to try to recover our funds from the commitment.
This commit fixes a potential race condition within the
IncomingContestResolver, that could cause us to miss a
preimage that was delivered in time.
Currently we query the db for the preimage, and then
subscribe for notifications. This permits the following
ordering of events:
- query for preimage, returns nothing
- preimage is added and delivered to subscribers
- subscribe to preimages
- preimage never comes through!!
We fix this by reordering to subscribe for preimages and
then query just in case it already exists. The effect is
that the query will always return a valid read of the
preimages that are currently queued for delivery.
In this commit, we alter cooperative channel closures to also use
MarkChannelResolved in order to unify the logic for the different types
of channel closures.
We no longer have to mark the channel as fully closed in the database,
as it is done directly in the chainWatcher. Instead, we stop the watcher
and delete it from the set of active watchers.
This commit makes the dispatchCooperativeClose method mark the channel
fully closed directly, without registering for confirmation
notifications first. We can do this as recent changes to the
contractcourt changed the definition of a closed channel in the database
to have had its closing tx confirmed, and we only dispatch the
cooperative close once the transaction has 1 confirmation.
We also rename the markChanClosed method to notifyChanClosed, to more
clearly indicate that the ChainArbitrator no longer has to mark the
channel fully closed in the database.
In this commit, we attempt to fix a bug that's possible within the
Start() method of the ChainArbiter. We pass the channel pointer directly
into the newActiveChannelArbitrator function causing it to close over
the loop variable. We later use the channel point directly to send
messages to other sub-systems. It's possible that we actually have the
shadowed loop variable and will send an incorrect message. Defensively,
we now re-bind the loop variable in order to ensure we point to the
proper channel.
This commit adds a test for the case where the ChannelArbitrator fails
to broadcast its commitment during a force close because of
ErrDoubleSpend. We test that in this case it will still wait for a
commitment getting confirmed in-chain, then resolve.
This commit adds the new function closure option ContractBreach to the
ChainArbitrator config, a closure that is again used by the ChainWatcher
to reliably handoff a breach event to the breachArbiter.
This commit changes how the ChainWatcher notifies the breachArbiter
about a channel breach. Instead of assuming the breachArbiter is among
the clients subscibing to channel events, it will call a new method
contractBreach(), and assume the breachArbiter has reliably gotten the
breach info when this method returns with a non-nil error.
Since the breachArbiter was the only sybsystem having a sync chain
subsciption, we also remove the (now) unused syncDispatch option.
This move the log message "channel marked pending-closed" to the point
where the channel actually has been marked pending closed, instead of
before the database transaction has been done.
This commit removes the for loop in the closeObserver, as it wasn't
serving any purpose. After receiving a spend notification we would
return, breaking out of the loop. When getting a quit signal we would
also return, making the loop only do one iteration in any case.
This commit makes clients subscribing to channel events that are marked
"sync dispatch" _not_ being deleted from the list of clients when they
call Cancel(). Instead a go routine will be launched that will send an
error on every read of the ProcessACK channel.
This fixes a race in handing off the breach info while lnd was shutting
down. The breach arbiter could end up being shut down (and calling
Cancel()) before while the ChainWatcher was in the process of
dispatching a breach. Since the breach arbiter no longer was among the
registered clients at this point, the ChainWatcher would assume the
breach was handed off successfully, and mark the channel as pending
closed. When lnd now was restarted, the breach arbiter would not know
about the breach, and the ChainWatcher wouldn't attempt to re-dispatch,
as it was already marked as pending closed.
This commit adds MVP unit tests for the following scenarios in the
ChannelArbitrator:
1) A cooperative close is confirmed.
2) A remote force close is confirmed.
3) A local force close is requested and confirmed.
4) A local force close is requested, but a remote force close gets
confirmed.
This commit changes the ChainWatcher to only send a chain event in case
the various spends are _confirmed_ on-chain, not only seen on the
network.
A consequence of this is that we now give the ChainWatcher the
responsibility of marking the channel closed when the closing tx is
confirmed, instead of the ChannelArbitrator.
This commit changes the channel arbitrator state machine to only care
about commitment transactions that are being confirmed on-chain
according to the chain_watcher. This is meant to handles the cases where
we would broadcast our commitment, expecting it to get confirmed, but
instead a competing transaction was confirmed.
This commit readies the ChannelArbitrator state machine for the change
that will make the ChainWatcher only notify on confirmed commitments.
The state machine has gotten a new state, StateCommitmentBroadcasted,
which we'll transition to after we have broadcasted our own commitment.
From this state we'll go to the StateContractClosed state regardless of
which commitment the ChainWatcher notifies about, unifying the contract
resolution betweee the local and remote force close.
This commit adds a new method dispatchLocalClose, which will be called
in case our commitment is detected to spend the funding transaction. In
this case LocalUnilateralCloseInfo will be sent on the
LocalUnilateralClosure channel to all subscribers.
The UnilateralClosure channel is renamed to RemoteUnilateralClosure for
consistency.
This commit removes a short circuit checking if the contract resolver
after a unilateral close is empty. After removing this, the state
machine will advance the state from StateDefault->ContractClosed, in
which the stateCallback will be called, logging the state needed to
advance. Since this logged state is empty, the state machine will go
directly to StateFullyResolved, which will trigger the
MarkChannelResolved call. This means the behaviour is kept.
This commit adds a missing return to the resolveContract method, that
will ensure the goroutine exits if the ChannelArbitrator shuts down.
This fixes a potential deadlock during the integration tests.
We also promote some of the logs to Debug from Trace.
In this commit, we fix a long standing bug where at times a co-op
channel closure wouldn't be properly marked as fully closed in the
database. The culprit was a re-occurring code flaw we've seen many times
in the codebase: a closure variable that closes over a loop iterator
variable. Before this instance, I assumed that this could only pop up
when goroutines bind to the loop iterator within a closure. However,
this instance is the exact same issue, but within a regular closure that
has _delayed_ execution. As the closure doesn't execute until long after
the loop has finished executing, it may still be holding onto the _last_
item the loop iterator variable was assigned to.
The fix for this issue is very simple: re-assign the channel point
before creating the closure. Without this fix, we would go to call
db.MarkChanFullyClosed on a channel that may not have yet actually be in
the pending close state, causing all executions to fail.
Fixes#1054.
Fixes#1056.
Fixes#1075.
This commit renames ForceCloseSummary to LocalForceCloseSummary, and
adds a new method NewLocalForceCloseSummary that can be used to derive a
LocalForceCloseSummary if our commitment transaction gets confirmed
in-chain. It is meant to accompany the NewUnilateralCloseSummary method,
which is used for the same purpose in the event of a remote commitment
being seen in-chain.
This commit mitigates a problem within the ChannelArbitrator, where
after a restart we would start up in the state StateBroadcastCommit but
fail to broadcast out commitment because a conflicting transaction (most
likely our own commitment) was already broadcast. A more complete fix
for this case will be added later, but this commit let the
ChannelArbitrator continue, trying to close out the channel.
In this commit, we fix an existing grouting leak within the contract
court package. If a goroutine dies, but it doesn’t actually cancel the
block epoch notification that it requested, then it’s possible to leak
thousands of gorutines. To remedy this situation, we ensure that we’ll
*always* cancel the epoch notification once the goroutine has exited.
In this commit, we fix an existing flake on Travis related to the new
set of on-chain HTLC tests. In this timing flake, Bob would broadcast
his sweeping transaction, but *mid block mining*. As a result, the
output would never be properly swept, needing an additional block to be
mined. We’ll now wait for both Bob’s sweeping transaction, and Carol’s
sweep transaction to be confirmed before we attempt our assertions.
In this commit, we fix an existing bug in the implementation of the
resolution of the htlcOutgoingContestResolver. Before this commit, we
would _always_ watch the claim outpoint. However, if this is on the
remote party’s commitment transaction, then we would end up watching
the wrong output. We’ll now properly detect this by modifying which
output we watch, based on if we have a second level transaction or not.
In this commit, we add 6 new integration tests to test the various
actions that may need to be performed when either side goes on-chain to
fully resolve HTLC’s. Many of the tests are mirrors of each other as
they test sweeping/resolving HTLC’s from both commitment transactions.
In this commit, we modify the way that notifications are dispatched
within the chainWatcher. Before we would *always* wait for an ack back
before we started to clean up he database state. This would at times
lead to deadlocks. To remedy this, we now allow callers to decide if
they want notifications to be sync or not. The only current caller that
requires this is the breach arbiter.
In this commit, we modify the interaction between the chanCloser
sub-system and the chain notifier all together. This fixes a series of
bugs as before this commit, we wouldn’t be able to detect if the remote
party actually broadcasted *any* of the transactions that we signed off
upon. This would be rejected to the user by having a “zombie” channel
close that would never actually be resolved.
Rather than the chanCloser watching for on-chain closes, we’ll now open
up a co-op close context to the chainWatcher (via a layer of
indirection via the ChainArbitrator), and report to it all possible
closes that we’ve signed. The chainWatcher will then be able to launch
a goroutine to properly update the database state once any of the
possible closure transactions confirms.
In this commit, we add the IsOurAddress field into the config of the
chain arb. With this new function closure, the chain arb is able to
detect co-op on chain closes automatically.
In this commit, we extend the chainWatcher to be able to automatically
detect co-op closes of the channel. With this change, it’s now fully
encompassed so able to detect all types of closes on-chain. We detect a
co-op close due to the sequence number being finalized, as well as
paying to us directly in a regular p2wkh-like output.
