This commit handles a racy condition within the breacharbiter's justice
tx procedure. For backends that have no mempool we would check if an
HTLC output was spent and then try broadcasting the justice tx, but this
would fail since we wouldn't detect the spend before it was in a block.
The result was that we would continuously attempt to broadcast the
transaction, effectively ending up in an endless (until the second-level
tx actually comfirmed) loop.
Instead we now register for spend notifications in case broadcasting the
transaction fails, and then wait for any of the notifications to be
sent before trying again.
This is a necessary step to be able to make lnd work well only with
confimed transactions, and was a better solution than introducing
timeouts within the broadcast loop (which complicates integration
tests).
This commit adds a poll 20 ms interval to WaitPredicate, similar to what
is done for WaitInvariant. This makes the predicate not being checked
super-rapidly, potentially filling the logs with useless info over the
wait predicate interval.
This commit adds the helper method shutdownAndAssert, and makes us shut
down nodes created temporarily during the test cases using defers
instead of at the end of the test. This makes it easier to remember to
shut them down, as it is done right after they are started.
We remove the internale broadcastMessage method, and instead handle the
mutex handling within BroadcastMessage. This lets us hold the mutex only
when neccessary.
This commit removes the sendToPeer method from the server, and instead
moves the necessary logic into SendToPeer. This let's us make the mutex
acquisition more fine-grained, only holding it while reading from the
peer map. Earlier it was required to be held during the whole call to
sendToPeer, as the method would access the map internally.
We check if the channel is FullySynced instead of comparing the local
and remote commit chain heights, as the heights might not be in sync.
Instead we call FullySynced which recently was modified to use compare
the message indexes instead, which is _should_ really be in sync between
the chains.
The test TestChanSyncOweRevocationAndCommitForceTransition is altered to
ensure the two chains at different heights before the test is started, to
trigger the case that would previously fail to resend the commitment
signature.
This commit changes cancelReservationCtx to gold the resMtx from start
to finish. Earlier it would lock at different times only when accessing
the maps, meaning that other goroutines (I'm looking at you
PeerTerminationWatcher) could come in and grab the context in between
locks, possibly leading to a race.
This commit moves the responsibility of sending a funding error on the
reservation error channel inside failFundingFlow, reducing the places we
need to keep track of sending it.
This commit adds a test which will restore a channel from an OpenChannel
struct at various stages of the state transation cycle, ensuring the
HTLC local and remote add heights are restored properly.
This commit fixes a bug which would cause the add heights of the HTLCs
in the update log to be set wrongly. At times, an add height could be
incorrecly set, leading to the HTLCs not being accounted for correctly
during evaluating the HTLC views. This was caused by the assumption that
if the HTLC was not on the pending remote commit, then it was locked in
on both the local and the remote commit, which is not always true.
Instead of making this assumption, we instead now inspect the three
commits: the local, remote and pending remote; and set the add heights
accordingly. This should ensure that HTLCs are subtracted from the
balances only when they are first added.
This commit adds an integration test that checks that in case a channel
counterparty tries to settle an HTLC with the wrong preimage, the
channel is failed and force closed.
This commit makes the peer aware of the LinkFailureErrors that can
happen during link operation, and making it start a goroutine to
properly remove the link and force close the channel.
Adds a new closure OnChannelFailure to the link config, which is called
when the link fails. This function closure should use the given
LinkFailureError to properly force close the channel, send an error to
the peer, and disconnect the peer.
This commit introduces a new error type LinkFailureError which is used
to distinguish the different kinds of errors that we can encounter
during link operation. It encapsulates the information necessary to
decide how we should handle the error.
The pending state definitin in ChannelCloseSummary was slightly changed
in such a way that channels that has had their commitment broadcasted
now is no longer considered "pending close". They now instead stay in
the open chan bucket with the ChanStatus "CommitmentBroadcasted" until
their commitment is confirmed. This commit updates the IsPending godoc
to reflect this.
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.
This commit stops the chan closer from sending the potential coop close
transactions to the chainwatcher, as this is no longer needed. The
chainwatcher recently was modified to watch for any potential close, and
will because of this handle the close regardless of which one appears in
chain.
When the chancloser broadcast the final close transaction, we mark it as
CommitmentBroadcasted in the database.
This commit removes redundant HTLC restoring. We don't have to restore
outgoing HTLCs from the local commitment, as we _know_ they will always
be added to the remote commitment first. Also, when receiving
Settles/Fails, they will be removed from the local commitment first.
This way we can be sure that outgoing HTLCs found on the local
commitment always will be found on the remote commitment
Similarly we don't have to restore incoming HTLCs from the remote
commitment, as they will be added to the local commitment first.
This commit removes the stage during updateLog restoration where we
would attempt to restore incoming HLTCs from the pendingRemoteCommit, in
addition to update our log and htlc counter to reflect this state. The
reason we can safely remove this is to observe that a pending remote
commit is always created from a commitDiff which only contains updates
made by _us_, and thus only taken from the localUpdateLog. The same can
be said for the counters, when creating a commitDiff we'll always use
the remoteACKedIndex as the index into the remoteUpdateLog, meaning that
all potential updates will already be included in the remote commit that
has been ACKed.
This commit adds a test that runs through a scenario where an HTLC is
added then failed, making sure the update logs are properly restored at
any point during the process.
This commit adds a test ensuring that the fix applied in the previous
commit works as expected. The test exercises the scenario where the
HTLCs on the local, remote and pending remote commitment differ, and we
attempt to restore the update logs. We now check that in this case the
logs before and after restart are equivalent.
remoteUpdateLog from localCommit
This commit fixes a bug within channel.go that would lead to the
content of the update logs and their indexes getting out of sync during
restores.
The scenario that could occur was that the localUpdateLog was initiated
with a log index taken from the localCommitment. Updates we send (which
are added to the localUpdateLog) will be added to the remote commitment
first. The problem happened when an update was sent and added to the
remote commitment, but not ACKed. Since it was not ACKed, we would not
add it to our local commitment. During a restart/restore we would init
the localUpdateLog with a height too low, such that when going through
the outgoing HTLCs on the remote commitment, we would restore an HTLC at
an index higher than our local log HTLC counter.
The symmetric change is done to the remoteUpdateLog.
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 attempts to fix a flake we have seen, where a routing hints
for an inactive channel was unexpectedly added to the invoice. This
happened because of a race between shutting down the endpoint of this
channel (Eve) and creating the invoice. This commit fixes this by moving
the call to AddInvoice with corresponding route hint check inside a
WaitPredicate.
This commit makes sure old log files are deleted before running the
integration tests. Previously new logs would be added to the existing
ones, causing a big mess. This was especially messy during flake
hunting, as all logs would be kept.
In addition, we now print the date before every start of the integration
tests, which is useful to see how long they have been running, for
instance in case of a deadlock.
This commit adds a test where we trigger a situation which would
previously make the link think it was never in sync, and potentially
create a lot of empty state updates. This would happen if we were
waiting for a revocation, while still receiving updates from the remote.
Since in this case we could not ACK the updates because of the exhausted
revocation window, our local commitchain would extend, while the remote
chain would stall. When we finally got the revocation the local
commitment height would be far larger than the remote, and FullySynced
would return false from that point on.
This commit adds a new test that makes sure we don't try to send
commitments for states where there are now new updates. Before the
recent change to FullySynced we would do this in this test scenario, as
the local an remote commitment heights would differ.
The test makes the local commitment chain extend by 1 vs the remote,
which would earlier trigger another state update, and checks taht no
such state update is made.
This commit removes a faulty check we did to determine if the channel
commitments were fully synced. We assumed that if out local commitment
chain had a height higher than the remote, then we would have state
updates present in our chain but not in theirs, and owed a commitment.
However, there were cases where this wasn't true, and we would send a
new commitment even though we had no new updates to sign. This is a
protocol violation.
Now we don't longer check the heights to determine if we are fully
synced. A consequence of this is that we also need to check if we have
any pending fee updates that are nopt yet signed, as those are
considered non-empty updates.
This commit make us return an error in case a restored HTLC from a
pending remote commit has an index that is different from our local
update log index. It is appended with the assumption that these indexes
are the same, and if they are not we cannot really continue.
This commit adds a call to panic in case the HTLC we are looking for is
not found in the update log. It _should_ always be there, but we have
seen crashes resulting from it not being found. Since it will crash with
a nil pointer dereference below, we instead call panic giving us a bit
more information to work with.
This commit make the server populate the ChainArbitrator's
ContractBreach method, by a method that will reliably handoff the breach
event ot the breachArbiter. The server will now forward the breach event
to the breachArbiter, and only let the closure return a non-nil error
when the breachArbiter ACKs this event.
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 commit changes how the breachArbiter gets notified about channel
breaches. Previously it would need to SubscribeToChannelEvents to get
get notified if any breach happened, now we send all seen breaches on a
new channel ContractBreaches.
By having the breachArbiter subscribe to channel events, we risked
events getting lost when we were either starting up or shutting down,
since events could happen before we had been able to subscribe, or right
after we had cancelled our subscription.
Now it is the server's responsibility to reliably forward events from
the ChainArbitrator to the breachArbiter, and forward the ACK the
breachArbiter responds with. This makes sure that the messages aren't
lost in the event of starting up or shutting down, since the connection
between the subsystems now are static.
A result of this change is that the internals of the breachArbiter can
be simplified significantly, as we will get all channel breaches
forwarded on one channel. This lets us get rid of the observer
goroutines, and we spin up goroutines handling the channel breaches only
when they happen.
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 removes the mining of a block within the integration tests,
that would lead to a flake during runs. The reason was that the sweeoing
transaction was actually broadcast already at this point, and this block
was mined with the assumption that it wasn't. However, test would pass
more often than not because the UTXO nursery would be working on
graduating new heights, whcih is a bit slow, so in most cases the block
would be mined before it had broadcast the sweeping transaction.
By removing this block we ensure that the sweeping transaction will be
found for sure in the waitForTxInMempool call below.
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 modifies the integration tests to work with the recent
changes to the ChannelArbitrator, where it will only act on commitments
that has been confirmed. Main changes involving when to look for
transactions in the mempool and in blocks, and using the new RPC for
getting channels in the "waiting close" phase when they are waiting for
the commitment to confirm.
This commit adds a new method FetchWaitingCloseChannels to the database,
used for fetching OpenChannels that have a ChanStatus != Default. These
are channels that are borked, or have had a commitment broadcasted, and
is now waiting for it to confirm.
The fetchChannels method is rewritten to return channels exclusively
based on wheter they are pending or waitingClose.
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.
Johan T. Halseth