This fixes a long-standing force close bug. When we receive a
revocation, store the updates that the remote should sign next under
a new database key. Previously, these were not persisted which would
lead to force closure.
Previously, we could sign a pending commitment for the remote party,
disconnect, and not restore these signed remote updates as having been
removed at the pending commitment height. This commit fixes that to
look up whether the update under the unsigned acked updates key is
present on the pending commitment or not and appropriately set
the remove commit heights.
The `restoreStateLogs` function now properly restores the
`addCommitHeightLocal` field of a settle or fail's parent add.
Previously, any updates' parent in unsignedAckedUpdates would have
the field set to the default value of 0. This would cause a force
closure when receiving a commitment due to our belt-and-suspenders
checks for update logs during commitment validation.
The bug in question occurs because the `addCommitHeightLocal` field
is only populated for a restored add if the add is on the local
commitment. `TestChannelRestoreCommitHeight` is expanded in
`lnwallet/channel_test.go` to demonstrate restoration now works.
The faulty state transition:
```
<----fail----
<----sig-----
-----rev----> (add no longer on Alice's commitment)
*Alice restores* (addCommitHeightLocal of failed htlc is 0)
```
NOTE: Alice dies after sending a revocation but before signing a
commitment. This is possible because there is a select block in the link
that can potentially exit after sending over the revocation but before
signing the next commitment state for the counterparty.
This enforces the _actualized_ fee rate of the commitment transaction,
rather than the fee floor used for estimation. The new value of 250
sat/kw corresponds to 1 sat/byte, rather than 253 which is only rounded
up during estimation to account for the fact that BOLT 3 rounds down to
the nearest satoshi and that the vbyte fee estimation is lossy.
Previously we would incorrectly fail to sign the next commitment even
though the fee was technically high enough. Restarting with this commit
should solve the issue as long as the channel hasn't already gone to
chain.
This commit introduces the Signature interface which will be used by our
witness construction methods instead of passing in raw byte slices. This
will be used later to inject various kinds of mock signatures, e.g.
73-byte signatures for simulating worst-case witness weight.
This commit adds an additional santity check that rejects zero-value
HTLCs, preventing them from being added to the channel state even if the
channel config's minhtlc value is zero.
This commit fixes#4118 by properly sorting the HTLC signatures sent
over the wire to match the BOLT3 BIP69+CLTV sorting of the commitment
outputs.
To do so, we expose the slice of cltv deltas for HTLCs on the unsigned
commitment after applying the commitment sorting. This will be used to
locate the proper output index, as the CLTV serves as a tie breaker
between HTLCs that otherwise have the same payment hash and amount.
Note that #3412 fixed the issue partially by ensuring the commitment was
constructed properly (and the second-level prev outpoint's txid was
correct), but failed to address that the HTLC signatures were still sent
out in the incorrect order. With this, we pass the test case introduce
in the next commit.
We currently write each HTLCs OutputIndex to disk, but we don't use it
when restoring. The restoration is modified to use these directly, since
we will have lost access to the sorting of CLTVs after the initial
signing process.
This commit adds two new channel statuses which indicate the party that
initatited closing the channel. These statuses are set in conjunction
with the existing commit broadcast status so that we do not need to
migrate existing logic to handle multiple types of closes. This status
is set for locally initiated force closes in this commit because they
follow a similar pattern to cooparative closes, marking the commitment
broadcast then proceeding with tx broadcast. Remote force closes are
added in the following commit, as they are handled differently.
Since our HTLC must also be added to the remote commitment, we do the
balance caluclation also from the remote chain perspective and report
our minimum balance from the two commit views as our available balance.
When we send non-dust HTLCs as the non-initiator, the remote party will
have to pay the extra commitment fee. To account for this we figure out
if they can afford paying this fee, if not we report that we only have
balance available for dust HTLCs, since these HTLCs won't increase the
commitment fee.
Since we want to handle the edge case where paying the HTLC fee would
take the initiator below the reserve, we move the subtraction of the
reserve into availableBalance where this calculation will be performed.
This commit adds an extra validation step when adding HTLCs. Previously
we would only validate the remote commitment resulting from adding an
HTLC, which in most cases is enough. However, there are situations where
the dustlimits are different, which could lead to the resulting remote
commitment from adding the HTLC being valid, but not the local
commitment.
Now we also validate the local commitment. A test to trigger the case is
added.
add
To ba able to validate the commitment sanity both for remote and local
commitments, and at the same time predict both our and their add, we let
validateCommitmentSanity take an extra payment descriptor to make this
possible.
This commit checks the commitment sanity when receiving an HTLC so
that if a commitment transaction will overflow from an ADD, it is
caught earlier rather than in ReceiveNewCommitment.
The unit test TestNewBreachRetributionSkipsDustHtlcs triggered a state
transition from Bob, even though it was Alice that had added the HTLCs.
This is wrong since it will lead to Bob still owing Alice a commitment,
which is not accounted for in the unit tests.
We add a sanity check that the add heights has been set for all entries
found in the logs, and return an error otherwise. This won't happen
during normal operation, but it does reveal the mistake in the unit
test, which is fixed by making Alice trigger the transition.
In addition we resolve a long standing TODO by removing a (purposeful)
panic in the channel state machine. Old version of lnd had a bug that
could lead to the parent entries being lost during channel restore. A
panic was added to get to the bottom of if.
This is now fixed, so new nodes shouldn't encounter it. However, to be
on the safe side, instead of panicking we return an error back to
gracefully exit the channel state machine.
Updates were always restored with the same log index. This could cause a
crash when the logs were compacted and possibly other problems
elsewhere.
Extended unit test to cover the crash scenario.
This commit updates the channel state machine to
persistently store remote updates that we have received a
signature for, but that we haven't yet included in a commit
signature of our own.
Previously those updates were only stored in memory and
dropped across restarts. This lead to the production of
an invalid signature and channel force closure. The remote
party expects us to include those updates.
When creating the keyring, the tweak is already calculated in the remote
commitment case. We add the calculation also for our own commitment, so
we can use it in all cases without deriving the tweak.
