In this commit, we use the recently added `chanvalidate` package to
verify channels once they have been confirmed in the funding manager. We
expose a new method on the `LightningWallet` struct: `ValidateChannels`
which calls the new shared 1st party verification code.
After the channel is fully confirmed in the funding manager, we'll now
use this newly exposed method to handle all validation. As a result, we
can remove the existing validation code in the funding manager, and rely
on the new code in isolation.
In this commit, we create a new `chanvalidate` package which it to house
all logic required for 1st and 3rd party channel verification. 1st party
verification occurs when we find a channel in the chain that is
allegedly ours, while 3rd party verification will occur when a peer
sends us a channel proof of a new channel.
In the scope of the recent CVE, we actually fully verified 3rd party
channels, but failed to also include those checks in our 1st party
verification code. In order to unify this logic, and prevent future
issues, in this PR we move to concentrate all validation logic into a
single function. Both 1st and 3rd party validation will then use this
function. Additionally, having all the logic in a single place makes it
easier to audit, and also write tests against.
In this commit, we move to make a full deep copy of the commitment
transaction in `getSignedCommitTx` to ensure that we don't mutate the
commitment on disk, possibly resulting in a "hot commitment".
In this commit, we consolidate the number of areas where we derive our
commitment keys. Before this commit, the `isOurCommitment` function in
the chain watcher used a custom routine to derive the expected
scripts/keys for our commitment at that height. With the recent changes,
we now have additional logic in `DeriveCommitmentKeys` that wasn't
copied over to this area. As a result, the prior logic would erroneously
detect if it was our commitment that had hit the chain or not.
In this commit, we remove the old custom code, and use
`DeriveCommitmentKeys` wihtin the chain watcher as well. This ensures
that we only need to maintain the key derivation code in a single place,
preventing future bugs of this nature.
In this commit, we update the brar logic in the channel state machine,
and also the brar itself to be aware of the new commitment format.
Similar to the unilateral close summary, we'll now blank out the
SingleTweak field in `NewBreachRetribution` if it's a tweakless
commitment. The brar will then use this to properly identify the
commitment type, to ensure we use the proper witness generation function
when we're handling our own breach.
In this commit, we update the funding workflow to be aware of the new
channel type that doesn't tweak the remote party's output within the
non-delay script on their commitment transaction. To do this, we now
allow the caller of `InnitChannelReservation` to signal if they want the
old or new (tweakless) commitment style.
The funding tests are also updated to test both funding variants, as
we'll still need to understand the legacy format for older nodes.
In this commit, we update the channel state machine to be aware of
tweakless commits. In several areas, we'll now check the channel's type
to see if it's `SingleFunderTweakless`. If so, then we'll opt to use the
remote party's non-delay based point directly in the script, skipping
any additional cryptographic operations. Along the way we move the
`validateCommitmentSanity` method to be defined _before_ it's used as is
cutomary within the codebase.
Notably, within the `NewUnilateralCloseSummary` method, we'll now _blank
out_ the `SingleTweak` value if the commitment is tweakless. This
indicates to callers the witness type they should map to, as the value
isn't needed at all any longer when sweeping a non-delay output.
We also update the signing+verification tests to also test that we're
able to properly generate a valid witness for the new tweakless
commitment format.
Instead of marking the database state when processing the channel
reestablishment message, we wait for the result of this processing to
arrive in the link, and mark it accordingly in the database here.
We do this move the logic determining whether we should force close the
channel or not, and what state to mark it in the DB, to the same place,
as these need to be consistent.
This commit converts the ErrCommitSyncLocalDataLoss error into a struct,
that also holds the received last unrevoked commit point from the remote
party.
Checks that we get ErrDoubleSpend as expected when publishing a
conflicting mempool transaction with the same fee as the existing one,
and that we can publish a replacement with a higher fee successfully.
error
Since btcwallet will return typed errors now, we can simplify the
matching logic in order to return ErrDoubleSpend.
In case a transaction cannot be published since it did not satisfy the
requirements for a valid replacement, return ErrDoubleSpend to indicate
it was not propagated.
The cache wasn't really serving a purpose as FetchInputInfo isn't known
to be a hot path. Also, with a planned addition of returning the
confirmation status of an output within FetchInputInfo in a later
commit, caching won't be useful as we'll have to go to disk anyway to
determine the confirmation status.
In this commit, we address an edge case that can happen a user rescans
w/ their seed, while retaining their existing `channel.db`. Once they
rescan, if they go to sign for a channel sweep for example, the
commitment key family (actually an account) may not yet have been
created, causing the signing attempt to fail.
We remedy this always creating the account if we go to sign, and the
account isn't found. The change has been structured to make this the
exception, so we'll avoid always needing to do 2 DB hits (check if
account exists, sign), each time we sign.
A new test has been added to exercise this behavior. If the diff from
the `signer.go` file is removed, then the test will fail.
This commit adds a SubtractFees option to the funding request, letting
the caller specify that the fees should be deducted from the funding
amount. This paves the way for letting the funding manager spend up to a
given amount when creating a channel, like the rest of the funds in the
wallet.
This makes the method independent of the ChannelContribution struct.
We also add a function closure to the return of selectCoinsAndChange,
that let is unlock the selected output in case of error.
In this commit, we fix an existing bug that would cause us to be unable
to derive the very first key in a key family if the wallet hadn't
already derived it in the past. This can happen if a user keeps their
same `channel.db`, but restores their wallet resulting in fresh
`wallet.db` state.
This is an existing issue due to the fact that we don't properly
distinguish between an empty key locator, and the very first key in a
`KeyFamily`: `(0, 0)`. Atm, `KeyLoactor{0, 0}.IsEmpty() == True`,
causing us to be unable to retrieve this key in certain cases since we
fall through and attempt address based derivation.
In order to remedy this, we add a new special case (until we upgrade
`KeyLoactor` formats, but needed for legacy reasons) to _try_ a regular
`KeyLoactor` based derivation if we fail to derive via address, and this
is an "empty" key loc. This has been tested in the field and shown to
work, with the one downside that in this "hot swap restoration" case,
we'll hit the database twice to derive the key.
In this commit, we fix a logic flaw in the testCreateSimpleTx test case
which emerged once we the bug fix for dust outputs landed. Before this
commit, we would erroneously fail during valid test execution.
In this commit we fix a hidden bug in the transaction creating logic
that was only manifested recently due to higher fees on Bitcoin's
mainnet. Before this commit, we would use the target fee rate to
determine if an output was dust or not. However, this is incorrect, as
instead the relay fee should be used as this matches the policy checks
widely deployed in Bitcoin full node today.
To fix this issue we now properly use the relay fee when computing dust.
This fixes the issue for the `EstimateFee` call, but the `SendOutputs`
call also has a similar issue. However, this must be fixed within
`btcwallet` itself, so it has been left out of this commit
Fixes#3217.
In this commit, we patch a small bug in the newly added raw tx hex field
for ListTransactions. We now ensure that we also set the raw tx hex
field for unconfirmed transactions.
In this commit, we fix a lingering TOOD statement in the channel arb.
Before this commitment, we would simply wipe our our local HTLC set of
the HTLC set that was on the remote commitment transaction on force
close. This was incorrect as if our commitment transaction had an HTLC
that the remote commitment didn't, then we would fail to cancel that
back, and cause both channels to time out on chain.
In order to remedy this, we introduce a new `HtlcSetKey` struct to track
all 3 possible in-flight set of HTLCs: ours, theirs, and their pending.
We also we start to tack on additional data to all the unilateral close
messages we send to subscribers. This new data is the CommitSet, or the
set of valid commitments at channel closure time. This new information
will be used by the channel arb in an upcoming commit to ensure it will
cancel back HTLCs in the case of split commitment state.
Finally, we start to thread through an optional *CommitSet to the
advanceState method. This additional information will give the channel
arb addition information it needs to ensure it properly cancels back
HTLCs that are about to time out or may time out depending on which
commitment is played.
Within the htlcswitch pakage, we modify the `SignNextCommitment` method
to return the new set of pending HTLCs for the remote party's commitment
transaction and `ReceiveRevocation` to return the latest set of
commitment transactions on the remote party's commitment as well. This
is a preparatory change which is part of a larger change to address a
lingering TODO in the cnct.
Additionally, rather than just send of the set of HTLCs after the we
revoke, we'll also send of the set of HTLCs after the remote party
revokes, and we create a pending commitment state for it.
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.
This enables users to specify an external API for fee estimation.
The API is expected to return fees in the JSON format:
`{
fee_by_block_target: {
a: x,
b: y,
...
c: z
}
}`
where a, b, c are block targets and x, y, z are fees in sat/kb.
Note that a, b, c need not be contiguous.
In this commit, we add a new interface which will allow callers to drop
in an arbitrary Web API for fee estimation with an arbitrary
request/response schema.
Co-authored-by: Valentine Wallace <vwallace@protonmail.com>
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.
In this commit, we modify the `ChanSyncMsg` to send an invalid
commitment secret in `ChanSyncMsg`. We do this in order to force the
remote party to force close off-chain, if we're restoring a channel from
scratch and we never had any state updates within the channel. We need
to do this, as otherwise the remote party will think we can resume as
they're able to verify their own commit secret for state zero.
The checks to determine whether the transaction broadcast failed due to
it already existing in the mempool/chain are no longer needed since the
underlying btcwallet PublishTransaction call will not return an error
when running into these cases.
In this commit, we update the `TestChanSyncFailure` method to pass given
the new behavior around updating borked channel states. In order to do
this, we add a new method to allow the test to clear an existing channel
state. This method may be of independent use in other areas in the
codebase in the future as well.
In this commit, we add a new test: `TestForceCloseBorkedState`. This
ensures that it isn't possible to update the channel state once a
channel has been marked as borked. This assumes that all calls to
`ForceClose` will also mark the channel as borked. This isn't the case
yet, so this test fails as is.
In this commit, we add a new `LastUnusedAddress` method to the
`WalletController` interface. Callers can use this new method to graph
the last unused address, which can be useful for UIs that want to
refresh the address, but not cause nearly unbounded address generation.
The implementation for `btcwallet` uses the existing `CurrentAddress`
method. We've also added a new integration tests to exercise the new
functionality.
In this commit, we set a default max HTLC in the forwarding
policies of newly open channels.
The ForwardingPolicy's MaxHTLC field (added in this commit)
will later be used to decide whether an HTLC satisfies our policy before
forwarding it.
To ensure the ForwardingPolicy's MaxHTLC default matches the max HTLC
advertised in the ChannelUpdate sent out for this channel, we also add
a MaxPendingAmount() function to the lnwallet.Channel.
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.
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.
To avoid more bugs slipping through where the logIndex is not set, we
panic to catch this. This was earlier done for Adds and the htlcCounter,
which did lead us to find the resulting retoration bug.
Earlier versions did not write the log index to disk for fee updates, so
they will be unset. To account for this we set them to to current update
log index.
This reverts commit 4aa52d267f000f84caf912c62fc14a5b8e7cacb5.
It turns out that the other implementations set values for this field
which aren't based on the actual capacity of the channel. As a result,
we'll no reject most of their channel offerings, since they may offer a
value of a max `uint64` or something else hard coded that's above the
size of the channel. As a result, we're reverting this check for now to
maintain proper compatibility.
In this commit, we ensure that if a channel is detected to have local
data loss, then we don't allow a force close attempt, as this may not be
possible, or cause us to play an invalid state.
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).
This tests make sure we don't reset our expected fee upate after signing
our next commitment. This test would fail without the previous set of
commits.
Instead of special casing the UpdateFee messages, we instead add them to
the update logs like any other HTLC update message. This lets us avoid
having to keep an extra set of variables to keep track of the fee
updates, and instead reuse the commit/ack logic used for other updates.
This fixes a bug where we would reset the pendingFeeUpdate variable
after signing our next commitment, which would make us calculate the new
fee incorrectly if the remote sent a commitment concurrently.
When restoring state logs, we also make sure to re-add any fee updates.
When compacting the update logs we remove any fee updates when they
remove height is passed. We do this since we'll assume fee updates are
added and removed at the same commit height, as they will apply for all
commitments following the fee update.
This commit adds conversion between the lnwire.UpdateFee message and the
new FeeUpdate PaymentDescriptor. We re-purpose the existing Amount field
in the PaymentDescriptor stuct to hold the feerate.
This commit adds a new updateType that can be used for
PaymentDescriptors: FeeUpdate. We repurpose the fields of the existing
PaymentDescriptor struct such that we can easily re-use the commit/ack
logic used for other update types also for fee updates.
In this commit, we add a new method WithCoinSelectLock. This method will
allow us to fix bugs in the project atm that can arise if a channel
funding is attempted (either manually or by autopilot) while a users is
attempting to send an on-chain transaction. If this happens
concurrently, then both contexts will grab the set of UTXOs and attempt
to lock them one by one. However, since they didn't obtain an exclusive
snapshot of the UTXO set of the wallet, they may both attempt to lock
the same input.
We also ensure that calls to SendMany cannot run into this issue by
using the WithCoinSelectLock synchronization when attempting to instruct
the internal wallet to send payments.
In this commit, we extend the WitnessGenerator type to now return an
InputScript. This allows it to be more encompassing, as now callers can
expect a sigScript to be populated if the input being swept requires a
sigScript field.
Along the way, we've also renamed input.BuildWitness to
input.CraftInputScript. We also take a step towards allowing the
sweeper to sweep transactions for n2pwkh outputs. We do so by modifying
the BuiltWitness method to instead return an InputScript. Additionally,
when populating inputs if a sigScript is present, it will now be
populated.
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.
Returns a brief json summary of each utxo found by calling
ListUnspentWitness in the wallet. The two arguments are the
minimum and maximum number of conrfirmations (0=include
unconfirmed)
One way applications built on top of lnd can estimate sync percentage is
through comparing the current time to the best known timestamp of the
lnd wallet's sync state. Therefore, we should always return this
information even if the the wallet is not synced.
In this commit, we add an additional check to btcwallet's IsSynced
method to ensure that it is not currently undergoing a rescan. We do
this to block upon starting the server and all other dependent
subsystems until the rescan is complete.
In this commit, we add the lightning address scope before the wallet
starts to prevent a race condition between the wallet syncing and adding
the scope itself. This became more apparent with the recent btcwallet
fixes, as several database transactions now occur between the wallet
being started and it syncing.
In this commit, we add a new test to the existing set of wallet tests to
ensure we can properly detect the confirmation of transactions that
spend our change outputs. We do this as a measure to prevent future
regressions from happening where the wallet doesn't request its backend
to be notified of when an on-chain transaction pays to a change address,
like with the recently discovered SendOutputs bug.
As is, this test will not pass until we update the btcwallet dependency
in the next commit.
In this commit, we add an additional check to btcwallet's FetchInputInfo
method to ensure the output is actually under control of the wallet.
Previously, the wallet would assume the output was under its control if
the txid of the output was found within the wallet. This is not a safe
assumption to make however, because if we happened to be the sender of
this transaction, it would be found within the wallet but it's not
actually under our control. To fix this, we explicitly check that there
exists an address in our wallet for this output.
This change was inspired by #1984 - the underlying call to
ListUnspent supports a (min, max) range so it makes sense that
the WalletController interface can also support this; a
default no-maximum can be expressed using a MaxInt32 value.
In this commit, we fix an existing bug wherein we wouldn't set the short
channel ID for the close summary in the database in the case that the
remote party force closed. The fix is simple, ensure that within
NewUnilateralCloseSummary we properly set the short channel ID. A test
has also been added in this commit, which fails without the
modifications to lnwallet/channel.go.
Fixes#2072.
In this commit, we add a new test to ensure that all backends will
properly send out notifications when an unconfirmed transcation that we
send is inserted into the tx store. Before we updated the btcwallet
build commit in dep, this would fail for neutrino but now passes.
In this commit, we fix a bug in the arguments to GetTransactions for the
btcwallet implementation of the WalletController interface. Before this
commit, we wouldn't properly return unconfirmed transactions. The issue
was that we didn't specify the special mempool height of "-1", as the
ending height. The mempool height is actually internally converted to
the highest possible height that can fit into a int32.
In this commit, we set the start to zero, and end to -1 (actually
2^32-1) to properly scan for unconfirmed transactions.
Fixes#1422.
In this commit, we add a new test to the set of lnwallet integration
tests. In this new test, we aim to ensure that all backends are able to
display unconfirmed transactions in ListChainTransactions. As of this
commit, this test fails as no backends will return unconfirmed
transactions properly.
In this commit, we add an additional degree of isolation to the set of
integration tests. A bug was recently fixed to ensure that the wallet
always starts rescans from _after_ it's birthday. In the past it would
miss some funds that were deposited _right_ before the birthday of the
wallet. Fixing this bug exposed a test flake wherein the btcd node would
itself rescan back and collect some of the funds that were last sent to
the bitcoind node.
In order to fix this, we now ensure that each backend will use a unique
HD seed such that the tests are still deterministic for each backend and
role.
In this commit, we fix a slight bug by ensuring that the revocation info
at the final state of the channel, as well as the local chan config is
properly set within the channel close summary created within
NewUnilateralCloseSummary. Before this commit, for all cooperative close
transactions, this state would _only_ include the pubkey itself, which
in some cases may not be sufficient to re-derive the key if needed.
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.
In this commit, we modify our FeeEstimator interface to return an
estimated fee rate in sat/kw. Recently, due to low fees on the network,
users have been experiencing failures broadcasting transactions due to
not meeting specific fee requirements. This was happening more often
than not, as the estimated fee returned by backend nodes (bitcoind and
btcd) only takes into account vbytes, rather than weight. The fees
returned are also expressed in sat/kb, so we must take care that we do
not lose precision while converting to sat/kw. In the event that this
happens, a fee floor of 253 sat/kw has been added. This fee rate
originates from bitcoind rounding up the conversion from weight to
vbytes.
In this commit, we introduce a nice optimization with regards to lnd's
interaction with a bitcoind backend. Within lnd, we currently have three
different subsystems responsible for watching the chain: chainntnfs,
lnwallet, and routing/chainview. Each of these subsystems has an active
RPC and ZMQ connection to the underlying bitcoind node. This would incur
a toll on the underlying bitcoind node and would cause us to miss ZMQ
events, which are crucial to lnd. We remedy this issue by sharing the
same connection to a bitcoind node between the different clients within
lnd.
In this commit, we update the NewBreachRetribution method to include
pkScripts for htlc outputs. We do this now, as the breach arbiter will
need the raw pkScript when attempting to request spend notifications for
each HTLC.
In this commit, we export WitnessScriptHash and GenMultiSigScript as
external sub-systems may now need to use these methods in order to be
able to watch for confirmations based on the script of a transaction.
This commit adds a check for the LocalUnrevokedCommitPoint sent to us by
the remote during channel reestablishment, ensuring it is the same point
as they have previously sent us.
This commit enumerates the various error cases we can encounter when we
compare our remote commit chain to the view the remote communicates to us
via msg.NextLocalCommitHeight.
We now compare this height to our remote tail and tip height, returning
relevant error in case of a unrecoverable desync, and re-send a
commitment signature (including log updates) in case we owe one.
This commit enumerates the various error cases we can encounter when we
compare our local commit chain to the view the remote communicates to us
via msg.RemoteCommitTailHeight.
We now compare this height to our local tail height (note that there's
never a local "tip" at this point), returning relevant error in case of
a unrecoverable desync, and re-send a revocation in case we owe one.
This commit defines a few new errors that we can potentially encounter
during channel reestablishment:
* ErrInvalidLocalUnrevokedCommitPoint
* ErrCommitSyncLocalDataLoss
* ErrCommitSyncRemoteDataLoss
in addition to the already defined errors
* ErrInvalidLastCommitSecret
* ErrCannotSyncCommitChains
This commit moves the responsibility for publishing the funding tx to
the network from the wallet to the funding manager. This is done to
distinguish the failure of completing the reservation within the wallet
and failure of publishing the transaction.
Earlier we could fail to broadcast the transaction, which would cause us
to fail the funding flow. This is not something we can do directly,
since the CompeteReservation call will mark the channel IsPending in the
databas.e
In this commit, we correct our size estimates for to-local scripts,
which are used on the commitment transaction and the htlc
success/timeout transactions. There have been observed cases of
transactions getting stuck because our estimates were too low, and cause
the transactions to not be relayed.
Our previous estimate for the commitment to-local script was derived
from an older version of the script. Though the estimate is greater than
the actual size, this has been updated with the current estimate of 79
bytes.
This estimates makes the assumption that CSV delays will be at most
4 bytes when serialized. Since this value is expressed in relative block
heights, this should be more than sufficient for our needs, even though
the maximum possible size for the little-endian int64 is 9 bytes (plus
an OP_DATA).
The other correction is to use the ToLocalScriptSize as our estimate for
htlc timeout/success scripts, as they are the same script. Previously,
our estimate was derived from the proper script, though we were 6 bytes
shy of the new to-local estimate, since we counted the csv_delay as 1
byte, and missed some other OP_DATAs.
All derived estimates have been updating depending on the new and
improved ToLocalScriptSize estimate, and fix some estimates that did not
include the witness length in the estimate.
Finally, we correct some weight miscalculations in:
- AcceptedHtlcTimeoutWitnessSize: missing data push lengths
- OfferedHtlcSuccessWitnessSize: extra 73 byte sig, missing data push lengths
- OfferedHtlcPenaltyWitnessSize: missing 33 byte pubkey
Makes the helper methods for constructing witness script
hash and to-local outputs. This will allow watchtowers to
import and reuse this logic when sweeping outputs.
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 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.
In this commit, we add a new index to the HTLC log. This new index is
meant to ensure that we don't attempt to modify and HTLC twice. An HTLC
modification is either a fail or a settle. This is the first in a series
of commits to fix an existing bug in the state machine that can cause a
panic if a remote node attempts to settle an HTLC twice.
In this commit, we add a precautionary assertion at the end of
createCommitmentTx. This assertion is meant to ensure that we don't
accept or propose a commitment transaction that attempts to send out
more than it was funded with.
In this commit, we add a series of additional balance assertions to
ensure that the balance of the two channels at each stage match up with
our expectations. Additionally, we also fix a bug at the end of the test
which would result in Alice accidentally overdrawing her balance in the
channel. The issue was that the test attempted to settle HTLCs that
weren't yet fully locked in. We fix this by adding an additional state
transition before settling the final set of HTLCs.
In this commit, we move the check to CheckTransactionSanity into
createCommitmentTx. We do this as within wallet.go (during the funding
process) we actually end up calling this helper function twice, and also
moving it up until right when we create the fully commitment transaction
ensures we making our assertion against the final version.
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.
In this commit, we fix an issue where sometimes transactions wouldn't
provide enough of a fee to be relayed by the backend node. This would
especially cause issues when sweeping outputs after a contract breach,
etc.
Now, we'll fetch the minimum relay fee from the backend node and ensure
it is set as a lower bound if the estimated fee ends up dipping below
it.
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.
