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.
In this commit, we modify the NewUnilateralCloseSummary to be able to
distinguish between a unilateral closure using the lowest+highest
commitment the remote party possesses. Before this commit, if the remote
party broadcast their highest commitment, when they have a lower
unrevoked commitment, then this function would fail to find the proper
output, leaving funds on the chain.
To fix this, it's now the duty of the caller to pass remotePendingCommit
with the proper value. The caller should use the lowest unrevoked
commitment, and the height hint of the broadcast commitment to discern
if this is a pending commitment or not.
In this commit, we move a set of useful functions for testing channels
into a new file. The old createTestChannels has been improved as it will
now properly set the height hint on the first created commitments, and
also no longer accepts any arguments as the revocation window no longer
exists.
This commit changes the bool `IsBorked` in OpenChannel to a `ChanStatus`
struct, of type ChannelStatus. This is used to indicated that a channel
that is technically still open, is either borked, or has had a
commitment broadcasted, but is not confirmed on-chain yet.
The ChannelStatus type has the value 1 for the status Borked, meaning it
is backwards compatible with the old database format.
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.
In this commit, we fix an existing bug in the NewBreachRetribution
method. Rather than creating the slice to the proper length, we instead
now create it to the proper _capacity_. As we'll now properly filter out
any dust HTLCs, before this commit, even if no HTLCs were added, then
the slice would still have a full length, meaning callers could actually
interact with _blank_ HtlcRetribution structs.
The fix is simple: create the slice with the proper capacity, and append
to the end of it.
In this commit, we fix an existing within lnd. Before this commit,
within NewBreachRetribution the order of the keys when generating the
sender HTLC script was incorrect. As in this case, the remote party is
the sender, their key should be first. However, the order was swapped,
meaning that at breach time, our transaction would be rejected as it had
the incorrect witness script.
The fix is simple: swap the ordering of the keys. After this commit, the
test extension added in the prior commit now passes.
In this commit we add a new error: InvalidHtlcSigError. This error will
be returned when we're unable to validate an HTLC signature sent by the
remote party. This will allow other nodes to more easily debug _why_ the
signature was rejected.
In this commit we add a new command line option (and a sane default) to
allow users to specify the *smallest* inbound channel that they'll
accept. Having a higher-ish limit lets users limit their channels, and
also avoid a series of very low value "spam" channels.
The new option is --minchansize, and expressed in satoshis. If we
receive an inbound channel request for a value smaller than this, then
we'll immediately reject it.
In this commit, we add an additional check within CreateCommitTx to
ensure that we will never create or accept a commitment transaction that
wasn't valid by consensus. To enforce this check, we use the
blockchain.CheckTransactionSanity method.
In this commit, we fix an existing rounding related bug in the codebase.
The RPC interface for btcd and bitcoind return values in BTC rather than
in satoshis. So in several places, we're forced to convert ourselves
manually. The existing logic attempted to do this, but didn't properly
account for rounding. As a result, our values can be off due to not
rounding incorrectly.
The fix for this is easy: simply properly use btcutil.NewAmount
everywhere which does rounding properly.
Fixes#939.
This commit adds a check that will make LightningChannel reject a
received commitment if it is accompanied with too many HTLC signatures.
This enforces the requirement in BOLT-2, saying:
if num_htlcs is not equal to the number of HTLC outputs in the local commitment transaction:
* MUST fail the channel.
A test exercising the behaviour is added.
This commit fixes an issue which would arise in some cases when the
local and remote dust limits would differ, resulting in lnd not
producing the expected number of HTLC signatures. This was a result of
checking dust against the local instead of the remote dust limit.
A test exercising the scenario is added.
This commit fixes an issue where we would blindly accept a commitment
which came without any accompanying HTLC signatures. A test exercising
the scenario is added.
This commit fixes an out of bounds error that would occur in the case
where we received a new commitment where the accompanying HTLC sigs were
too few. Now we'll just reject such an commitment.
A test exercising the behavior is also added.
This commit extends the amount of time we wait
for transaction to enter the mempool from
10 to 30 seconds. The wallet's interface tests
seem to be particularly slow when run with the
race flag, a problem which is only exacerbated
by the slowness of travis.
With 10s and the race flag, I was able to repro
the issues locally fairly consistently.
In this commit, we add an additional check within
validateCommitmentSanity due to the recent change to unsigned integers
for peer balances in the channel state machine. If after evaluation
(just applying HTLC updates), the balances are negative, then we’ll
return ErrBelowChanReserve.
In this commit, we add logic to account for an edge case in the
protocol. If they initiator if unable to pay the fees for a commitment,
then their *entire* output is meant to go to fees. The recent change to
properly interpret balances as unsigned integers (within the protocol)
let to the discovery of this missed edge case.
lnwire.MilliSatoshi is now a signed integer, as a result, we’ll return
a different error if our balances go to negative due to the inability
to pay a the set fee.
In this commit, we fix a bug introduced by the recent change of
lnwire.MilliSatoshi to be an unsigned integer. After this change an
integer underflow was left undetected, as a result we’ll now
momentarily cast to a signed integer in order to ensure that both sides
can pay the proper fee.
In this commit, we modify the way we obtain the current best header
timestamp. In doing this, we fix an intermittent flake that would pop
up at times on the integration tests. This could occur as if the wallet
was lagging behind the chain backend for a re-org, then a hash that the
backend knew of, may not be known by the wallet.
To remedy this, we’ll take advantage of a recent change to btcwallet to
actually include the timestamp in its sync state.
In this commit, we modify the mechanics of the wallet to only allow
derivation of segwit-like addresses. Additionally, the ConfirmedBalance
method on the WalletController now only has a single argument, as it’s
assumed that the wallet is itself only concerned with segwit outputs.
In this commit, we modify the way we generate the secrets for
revocation roots to be fully deterministic. Rather than use a special
key and derive all sub-roots from that (mixing in some “salts”), we’ll
use the proper keychain.KeyFamily instead. This ensures that given a
static description of the channel, we’re able to re-derive our
revocation root properly.
In this commit, we modify the funding flow process to obtain all keys
necessary from the keychain.KeyRing interface. This ensure that all
keys we generate are fully deterministic.
In this commit, we update the SignDescriptor struct to instead use the
keychain.KeyDescriptor struct inplace of a raw public key. We do this
as this allows for a recovery mode, where the caller doesn’t know what
key was used exactly, but remembers the precise KeyDescriptor. A
stateless signer is still able to re-derive the proper key, as it’s
given the full path.
The new version of the internal core of btcwallet now uses KeyScopes
rather than address types to derive particular addresses. As a result,
in this commit, we update our API usage to ensure that proper addresses
are still derived.
In this commit, we remove two methods from the WalletController
interface which were previously used by the funding reservation process
(NewRawKey) and the p2p network (FetchRootKey) in order to derive
various keys for operation. This methods are no longer necessary as the
KeyRing interface implements the functionality in a deterministic
manner.
In this commit, due to the recent changes within lnd itself, it may be
possible that a wallet already exists when the wallet has been signaled
to be created. As a result, *always* open the wallet ourselves, but
allow an existing wallet to already be in place.
Adds an extra case to the select statement to catch
an error produced by btcd. The error is meant to signal
that an output was previously spent, which can appear
under certain race conditions in spending/broadcasting.
This caused our final itest to fail because it would
not try to recraft the justice txn.
This commit adds wallet_best_block_timestamp to the gRPC interface.
This is done in order to allow clients to calculate progress while
lnd syncs to the blockchain. wallet_best_block_timestamp is exposed
via the GetInfo() rpc call. Additionally, IsSynced() returns the
WalletBestBlockTimestamp as the second value in the tuple
that is returned, providing additional detail when querying about the
status of the sync. The BtcWallet interface has also been updated
accordingly.
This commit was created to support the issue to
[Add progress bar for chain sync] (lightninglabs/lightning-app#10) in
lightning-app
This commit introduces changes to the validateCommitmentSanity
function to fully validate all channel constraints.
validateCommitmentSanity now validates that the
MaxPendingAmount, ChanReserve, MinHTLC, & MaxAcceptedHTLCs
limits are all adhered to during the lifetime of a channel.
When applying a set of updates, the channel constraints are
validated from the point-of-view of either the local or the
remote node, to make sure the updates will be accepted.
Co-authored-by: nsa <elzeigel@gmail.com>
This commit moves common logic used to calculate the state
of a commitment after applying a set of HTLC updates, into
the new method computeView. This method can be used when
calculating the available balance, validating the sanity
of a commitment after applying a set of updates, and also
when creating a new commitment, reducing the duplication
of this logic.
This commit adds a new boolean parameter mutateState to
evalueteHTLCView, that let us call it without neccessarily
mutating the addHeight/removeHeight of the HTLCs, which is
useful when evaluating the commitment validity without
mutating the state.
This commit adds some more comments and checks to
reservation.CommitConstraints, including making
MinHTLC value one of the passed constraints.
RemoteChanConstraints is also moved out of
reservation.
This commit adds a test that trigger a case where the balance
could end up being negative when we used the logIndex when
calculating the channel's available balance. This could
happen when the logs got out of sync, and we would use
the balance from a settled HTLC even though we wouldn't
include it when signing the next state.
Appendix C of BOLT 03 contains a series of test vectors asserting that
commitment, HTLC success, and HTLC timeout transactions are created
correctly. Here the test cases are transcribed to Go structs and
verified.
We also break out some logic need to tests that bypass the constructor
and remove some redundant fields.
Before this commit, if the remaining change was small enough, then it
was possible for us to generate a non-std funding transaction. This is
an issue as the txn would fail to propagate, meaning funds could
potentially be stuck in limbo if users didn't manually drop their
transaction history.
To avoid this scenario, we won't create a change output that is dusty.
Instead, we'll add these as miner fees.
Fixes#690.
In this commit, we add the second level witness script to the
HtlcRetribution struct. We do this as it’s possible that we when
attempt to sweep funds after a channel breach, then the remote party
has already gone to the second layer. In this case, we’ll then need to
update our SignDesc and also the witness, in order to do that we need
this script that’ll get us pass the second layer P2WSH check.
In this commit, we add a new witness type to the set of known types.
This new type will be used when we need to sweep an HTLC that the
remote party has taken to the second level.
In this commit, we add a new function that allows a caller to create a
UnilateralCloseSummary with the proper materials. This will be used
within a new sub-system to be added in a later commit to properly
dispatch notifications when on-chain events happen for a channel.
In this PR, we entirely remove the closeObserver from the channel state
machine. It was added very early on before most of the other aspects of
the daemon were built out. This goroutine was responsible for
dispatching notifications to outside parties if the commitment
transaction was spent at all. This had several issues, since it was
linked to the *lifetime* of the channel state machine itself. As a
result of this linkage, we had to do weird stuff like hand off in
memory pointers to the state machine in order to ensure notifications
were properly dispatched.
In this commit, we add a new test case for unilateral channel closes to
ensure that if the remote party closes the commitment on-chain. Then
we’re able to sweep both incoming and outgoing HTLC’s from their
commitment. With this tests, we ensure that the values returned for
HtlcResolutions from the UnilateralCloseSummary are correct and allow
us to sweep all funds properly.
In this commit we add some additional scenarios to the TestForceClose
test. With this expanded test case, we now ensure the the party that
force closes is able to properly sweep both incoming and outgoing
HTLC’s fully with the information contained the HtlcResolution struct.
In this commit, we update the channel state machine tests to use a new
key for each purpose. Before this commit, the same key would be used
the entire time. As a result, a few bugs slipped by that would’ve been
detected if we used fresh keys for each purpose. Additionally, this
reflect the real world case as we always use distinct keys for each
purpose to avoid key re-use.
In this commit, we’ve added a new HtlcResolutions struct to house both
the incoming and outgoing HTLC resolutions. This struct will now be
coupled with the object that returns when we detect that a commitment
transaction was closed on chain. For incoming HTLC’s, we’ll check the
preimage cache to see if we can claim the HTLC on-chain. If we can,
then we’ll copy of the preimage, and make a proper incoming HTLC
resolution.
In this commit, we modify the OutgoingHtlcResolution struct to detect
if this is the remote party’s commitment transaction or not. With this
change, we’ll now be able to properly time out an HTLC that was
detected on the commitment transaction of the remote peer.
Additionally, we now populate the CsvDelay (if local commitment) and
the ClaimOutpoint (as we may be sweeping directly from the commitment
transaction now.
In this commit, we add a new IncomingHtlcResolution struct. This is the
opposite of the existing OutgoingHtlcResolution struct. The items in
this new struct allow callers to sweep an incoming HTLC that we know
the preimage to. These will always be created when a commitment goes
on-chain. However, if we know the preimage, then that will be populated
in place of all zeroes in the Preimage field.
In this commit, we modify both the ForceCloseSummary, and the
UnilateralClosureSummary to return the items needed to sweep the
commitment output distinctly. By doing this, it’s now possible to pass
a dedicated struct to a sub-system in order to allow it to sweep a
commitment output. As the maturity delay is a part of this new struct,
this tells the caller if this was on the local commitment (CSV
required) or on the remote commitment (no CSV required).
In this commit, we’ve added a new method to the channel state machine:
ActiveHtlcs. This method will allow callers to poll the state of the
channel to retrieve the set of HTLC’s active on *both* commitment
transactions.
In this commit, we modify the RevokeCurrentCommitment method to now
return the set of active HTLC’s. This will be used by callers in the
future to update other sub-systems when the set of HTLC’s on the
commitment changes, and can also be used on the RPC level to
synchronize systems level integration tests.
By returning a *TxWeightEstimator from each method, we now all callers
to chain the methods. This adds a bit of nice sugar when interacting
with the struct.
In this commit, we rename several of the existing WitnessType
definitions to be more descriptive than they were previously. We also
add a number of additional types which we need to handle scripts for,
but weren’t yet added before. Finally, we modify the
receiverHtlcSpendTimeout to optionally take an additional parameter to
set the locktime of the spending transaction accordingly. This final
modification allows the caller to specify that the lock time has
already been set on the main transaction.
To implement the BOLT 03 test vectors, a more powerful mockSigner is
required. The new version of mockSigner stores multiple keys and signs
the transaction outputs with the appropriate one.
This commit adds an additional check in GetUtxo that
tests for the nil-ness of the spend report returned by
the neutrino backend. Previously, a nil error and
spend report could be returned if the rescan did not
find the output at or above the start height. This
was observed to have cause a nil pointer dereference
when the returning line attempted to access the output.
This case is now handled by returning a distinct error
signaling that the output was not found.
This commit fixes a nasty bug that has been lingering within lnd, and
has been noticed due to the added retransmission logic. Before this
commit, upon a restart, if we had an active HTLC and received a new
commitment update, then we would re-forward ALL active HTLC’s. This
could at times lead to a nasty cycle:
* We re-forward an HTLC already processed.
* We then notice that the time-lock is out of date (retransmitted
HTLC), so we go to fail it.
* This is detected as a replay attack, so we send an
UpdateMalformedHTLC
* This second failure ends up creating a nil entry in the log,
leading to a panic.
* Remote party disconnects.
* Upon reconnect we send again as we need to retransmit the changes,
this goes on forever.
In order to fix this, we now ensure that we only forward HTLC’s that
have been newly locked in at this next state. With this, we now avoid
the loop described above, and also ensure that we don’t accidentally
attempt an HTLC replay attack on our selves.
Fixes#528.
Fixes#545.
In this commit, we add a new detailed error that’s to be returned
when/if the remote peer sends us an invalid commit signature. The new
error contains the transaction that we attempted to validate the
signature over, the sighs, and the state number. Returning this
additional information will serve to aide in debugging any
cross-implementation issues.
In this commit, add an additional return value to
CompleteCooperativeClose. We’ll now report to the caller our final
balance in the cooperative closure transaction. We report this as
depending on if we’re the initiator or not, our final balance may not
exactly match the balance we had in the last state.
This commit fixes a lingering bug that could at times cause
incompatibilities with other implementations when attempting a
cooperative channel close. Before this commit, we would use a pointer
to the funding txin everywhere. As a result, each time we made a new
state, or verified one, we would modify the sequence field of the main
txin of the commitment transaction. Due to this if we updated the
channel, then went to do a cooperative channel closure, the sequence of
the txin would still be set to the value we used as the state hint.
To remedy this, we now copy the txin each time when making the
commitment transaction, and also the cooperative closure transaction.
This avoids accidentally mutating the txin itself.
Fixes#502.
Previously, some methods on a LightningChannel like SettleHTLC and
FailHTLC would identify HTLCs by payment hash. This would not always
work correctly if there are multiple HTLCs with the same payment hash,
so instead we change these methods to identify HTLCs by their unique
identifiers instead.
Before this commit, during a reservation, we wouldn’t ever specify our
minHTL value. We don’t yet fully validate all channel constrains, but
doing this now serves to ensure that once those features are merged,
we’ll actually be setting a valid value for minHTLC.
In this commit, we extend the ProcessChanSyncMsg to detect a case where
we don’t have the necessary revocation window to send out a new commit.
This can arise if the remote party sends us a new state, but we haven’t
yet fully processed their FundingLocked message yet, so we would be
unable to create a new commitment state.
We fix this by enumerating each of our actions in the case of an error.
If we get ErrNoWindow, then this indicates that we can’t give the
remote party the commitment we would like to optimistically send over.
This isn’t an issue though, as in the next round, we’ll resynchronize
our state.
In this commit, we fix an existing bug that would cause issues within
the switch due to a value not being properly set. Before this commit we
would copy a byte array into a slice without first creating the
necessary capacity for that slice. To fix this, we’ll now ensure that
the blob has the proper capacity before copying over. Several tests
have been updated to always set a fake onion blob.
In this commit, we extend the initial check within SignNextCommitment
to bail out early if we don’t yet know the commitment point of the
remote party. This prevents a class of nil pointer panics if we attempt
to create a new state without yet having received the FundingLocked
message.
In this commit, we fix an existing bug within the EstimateFeePerWeight
method for the BtcdFeeEstimator. If the sat/byte value returned was too
low, then it was possible for us to end up with a zero valued
sat/weight. We correct this issue by detecting, and falling back to the
default fee rate if so.
In this commit, we fix an existing bug within our cooperative channel
closing transaction generation. Before this commit, we wouldn’t account
for the fee already allocated within the commitment transaction. As a
result, we would calculate the evaluated balance considering the fee
incorrectly. In this commit, we fix this by adding the commitment fee
to the balance of the initiator when crafting the closing transaction
In this commit, we fix an existing bug, as only the initiator needs to
validate any new fee updates. If the initiator sends an invalid fee,
then it will be rejected by the responder as it may put them below
their required reserve.
In this commit, we ensure that we reject any UpdateFee messages if
after applying the update, the initiator doesn’t have enough funds to
actually pay for the new commitment state.
A test has been added to exercise this new behavior.
In this commit, we update the retransmission logic to ensure that we
properly retransmit any sent UpdateFee messages as part of a state
transition. When creating a CommitDiff, if we have a pending fee
update, then we’ll add that to the set of logs updates. When restoring
the commit diff from disk, if we encounter an UpdateFee entry, then
we’ll apply that as waiting to be ACK’d and skip adding it as a log
entry.
A new test has been added to excessive this new behavior.
In this commit, we correct the BTC -> SAT conversion in
BtcdFeeEstimator. Previously, we use 10e8 instead of 1e8, causing us to
be off by an order of magnitude.
Adding 99 here didn’t really do anything but obfuscate when we were
trying to compute. We’re just using internet division to calculate 1%
of the channel capacity amount. This is the amount that the remote
party must remain above at all times.
In this commit, we add a new ResetState method to the channel state
machine which will reset the state of the channel to `channelOpen`. We
add this as before this commit, it was possible for a channel to shift
into the closing state, the closing negotiation be cancelled for
whatever reason, resulting the the channel held by the breachArbiter
unable to act to potential on-chain events.
In this commit, we fix an existing bug that had ramifications within
the operation of the lnd daemon. Before this commit, if the Stop()
method was called, then the closeObserver would exit as well. This
means that would no longer be watching for channel breaches on-chain,
and could miss either a cooperative channel closure or an actual
contract breach.
To fix this, we now introduce a new method to stop for closeObserver:
CancelObserver(). This should ONLY be called once either: the contract
has been fully settled on-chain, or whom ever is watching the relevant
signals has a newer version of the channel that it will watch instead.
In this commit, we modify CreateCloseProposal to no longer return the
same fee passed in. In the past, this method accepted a fee rat rather
than an absolute fee, and would return the computed absolute fee. Now
that the method takes the absolute fee directly, this is unnecessary.
In this commit, we modify the funding reservation workflow slightly to
allow callers to specify their own custom fee when initialization a
funding workflow. This gives power-users the ability to control exactly
how much in fees are paid for each new funding transaction.
In this commit, we extend the existing SendOutputs method on the
WalletController interface to be able to accept a custom fee. With
this, users are now able to specify their exact fee, allowing the
wallet to be fee estimation agnostic.
In this commit, we add a new implementation of the FeeEstimator
interface: the BtcdFeeEstimator. This implementation of the
FeeEstimator is backed by an active bcd instance. Any requests to query
for the current fee for a given confirmation target are proxied to this
active bcd instance.
In this commit, we extend the FeeEstimator methods to allow them to
return an error. This is required as most implementations aside from
the static fee estimator will want to be able to return errors to users
to indicate the inability to properly estimate fees.
In this commit, we move the FeeEstimator interface into a distinct file
as follow up commits will begin to flesh out the interface with
additional implementations.
Fix wrong calculation of overshot amount which causes coin select
function to go into infinite loop. If overshoot amount is calculated
by subtraction of totalSatoshis and amtNeeded than on the second
iteration of loop amtNeeded already include required fee inside, which
causes continuation of the coin selection loop.
In this commit, we add fully verification (other than checking the
commitment point matches after the fact) of the new optional fields
added to the lnwire.ChannelReestablish message. Two scenarios can
arise: we realize the remote party is on a prior state (and possibly
lost data), or we realize that *we* are on a prior state with the
remote party verifiably proving that they’re on a newer state.
In this commit we extend the set of fields populated within the
returned lnwire.ChannelReestablish to populate the optional data loss
fields. This entails included the commitment secret of the most
recently revoked remote commitment transaction and also our current
unrevoked commitment point.
In this commit, we update all the key derivation within the state
machine to account for the recent spec change which introduces a
distinct key for usages within all HTLC scripts. This change means that
the commitment payment and delay base points, are only required to be
online in the case that a party is forced to go to chain.
We introduce an additional local tweak to the keyring for the HTLC
tweak. Additionally, two new keys have been added: a local and a remote
HTLC key. Generation of sender/receiver HTLC scripts now use the local
and remote HTLC keys rather than the “payment” key for each party.
Finally, when creating/verifying signatures for second-level HTLC
transactions, we use these the distinct HTLC keys, rather than re-using
the payment keys.
In this commit, we modify the naming and comments of the routines that
create the sender/receiver HTLC scripts to denote that a set of
distinct keys are used. The scripts materially haven’t changed, but the
set of keys expected has.
In this commit we modify the primary InvoiceRegistry interface within
the package to instead return a direct value for LookupInvoice rather
than a pointer. This fixes an existing race condition wherein a caller
could modify or read the value of the returned invoice.
In this method we fix an existing deadlock within the unit tests when
running with the race condition detector on. We don’t need to grab the
mutex within the ProcessChanSyncMsg method as this should be the very
first method called when initializing the channel if a channel state
sync is needed.
In this commit we ensure that the channel is always able to exit by
adding a select statement with a quit case when we’re waiting on the
result of a job that was previously sent into the sigPool.
In this commit, we’ve added a set of unit tests to cover all enumerated
channel sync scenarios, including the case where both nodes deem that
they’re unable to synchronize properly.
In this commit we revert a prior commit
(5240953de02d281be694b2c87d151d6c7dce2cb5) which was added as a stop
gap before we added the proper state needed to recover from cases where
the commitment transactions of both chains had diverged slightly due to
asymmetric dust limits.
In this commit we do away with the existing availableLocalBalance
attribute and instead add a new, more accurate AvailableBalance method.
The new method will compute the available balance within the channel ,
assuming a new state was created at the instance the method was called.
This new method will now properly account for HTLC fees.
AvailableBalance is now called within AddHTLC in order to ensure we
don’t add any HTLC’s that are unable to be paid for from the PoV of the
fees on the commitment transaction.
We no longer need to manually pass in the channel delta to
AppendToRevocationLog (now called AdvanceCommitChainTail) as the
pointers on-disk will be updated atomically.
In this commit we update the RevokeCurrentCommitment method to properly
use the new database UpdateCommitment method along with properly
converting the in-memory commitment to its corresponding on-disk
format.
In this commit we complete the partially completed ReceiveReestablish
method and rename it to ProcessChanSyncMsg. The new version now
properly implements retransmission as defined within BOLT#2.
Additionally, we’ve added a new case which will optimistically try and
force a resynchronization of the commitment states if we detect we can
deliver a new commitment signature sooner than later after realizing
that we need to retransmit our last revocation message when we recevied
a new state transition.
This commit adds a new method: createCommitDiff. The method will, given
a newly constructed commitment, its signature, and HTLC signatures will
create a channeldb.CommitDiff. The CommitDiff created is to be stored
on disk, as it can be used in the case that the remote party didn’t
receive our CommitSig message and also forgot all the updates that we
queued with the update.
In this commit we complexly revamp the process of restoring all channel
state back into memory after a restart. We’ll now properly do the
following: restore the pending “dangling” commit of the remote party
into the in-memory commitment chain, re-populate all active HTLC’s back
into their respective update logs with the proper indexes/counters, and
properly restore the current commitment of the remote party back in
memory.
This commit adds a new method to the updateLog which will be used when
restoring the state of a channel from disk after a restart. This new
method will add an entry to the updateLog without incrementing either
of the counters as the HTLC already comes pre populated with its
historical index.
With these new fields, we’ll be able to properly reconstruct the log
state after a restart, as each commitment will now note both the
current HTLC and log index.
After addition of the retransmission logic in the channel link, we
should make the onion blobs persistant, the proper way to do this is
include the onion blobs in the payment descriptor rather than storing
them in the distinct struct in the channel link.
In this commit BOLT№2 retranmission logic for the channel link have
been added. Now if channel link have been initialised with the
'SyncState' field than it will send the lnwire.ChannelReestablish
message and will be waiting for receiving the same message from remote
side. Exchange of this message allow both sides understand which
updates they should exchange with each other in order sync their
states.
This commit changes the use of SigHash flags in the spend
transactions created in scrit_utils. Instead of always
using SigHashAll for the sweep signature, we instead use
the sighash flag specified by the passed sign descriptor.
Tis commit makes the btcwallet signer implementation use
signDesc.HashType instead of SigHashAll when signing
transactions. This will allow the creator of the transaction
to specify the sighash policy when creating the accompanying
sign descriptior.
In this commit, we fix an existing derivation from the commitment state
machine as defined within the specification. Before this commit, we
only kept a single counter which both HTLC adds and fails/settles would
share. This was valid in the prior pre-spec iteration of the state
machine. However in the current draft of the spec, only a distinct
counter for HTLCs are used throughout.
This would cause an incompatibility, as if we mixed adds and settles
during an exchange, then our counter values would differ with other
implementations. To remedy this, we now introduce a distinct HTLC
counter and index within the updateLog.
Each Add will increment both the log counter, and the HTLC counter.
Each Settle/Fail will only increment the log counter. Inbound
Settle/Fails will index into the HTLC index as to target the proper
HTLC. The PaymentDescriptor type has been extended with an additional
field (HltcIndex) which itself tracks the index of an incoming/outgoing
HTLC.
This moves the commitment transaction generation code out of
fetchCommitmentView into createCommitmentTx. Aside from being a pretty
clean logical split, this allows the transaction generation code to be
unit tested more effectively.
This commit fixes the TestChannelBalanceDustLimit unit test in
channel_test.go. The unit test does not account for the fees
required by adding an HTLC. As a result, Alice's balance according
to her local and remote commitment chains drops below 0 at certain
points. By using the correct fee, this is avoided.
The fee estimation for funding transactions now properly accounts for
different types of UTXOs spent, whereas previously it assumed all
inputs were spending native P2WKH outputs.
Use binary.Read/Write in functions to serialize and deserialize
channel close summary and HTLC boolean data, as well as in
methods to put and fetch channel funding info. Remove lnd
implementations of readBool and writeBool as they are no
longer needed. Also fix a few minor typos.
Use sort.Slice in SignNextCommitment function in lnwallet/channel.go,
as part of the move to use new language features. Remove
sortableSignBatch type wrapper for slice of signJobs since it is
no longer needed to sort jobs according to their output indices.
Also fix a few minor typos in channel.go and sigpool.go.
This commit fixes an existing bug where we attempted to re-use the same
commitTweak value when creating an HTLC resolution. Instead, we now
create the commit tweak value factoring the key that is to be used for
signing.
In this commit, we extend the help message for `newaddress`
to indicate which address types can be used when directly
funding channels. Additionally, we add some additional text
to the insufficient funding error to detail that we don't have
enough witness outputs.
This commit expands the existing TestForceClose test case to add an
HTLC (outgoing) to Alice’s commitment transaction before force closing.
We then ensure that both the pre-signed timeout transaction _and_ the
sign descriptor to sweep the second-level output are fully valid.
This commit fixes an existing bug within extractHtlcResolutions. The
prior code would use an index to assign the returned
OutgoingHtlcResolutions into a single slice. However, this is invalid
as there are two cases where an HTLC might be skipped: if it’s an
incoming HTLC, or if the HLTC itself is dust from the PoV of the
commitment chain.
To fix this, we now instead use append to add items to the slice. This
ensure that we don’t have any “empty” items in between fully populated
items.
This commit extracts the ending dust adherence test case from the
existing TestForceClose test case into a distinct test case. With this
modification, we now ensure that the two new tests are focused and test
a single scenario at at time.
This commit fixes a bug wherein we would use the incorrect csvDelay
when crafting HTLC resolutions after a unilateral channel closure.
Previously, we would always use the csvDelay of the local party, as in
the force close case that’s the correct value. However, a unilateral
channel closure instead requires the _remote_ delay.
This commit fixes an existing bug when crafting the HTLC resolution in
the face of a commitment broadcast. Previously, we we’re using the
localKey which is incorrect, as directly below we properly use the
delayKey when crafting the secondLevelHtlcScript to sign.
This commit adds a new field: MaturityDelay, to the
UnilateralCloseSummary struct. This new field will be required, in an
upcoming update as it’s needed in order to properly sweep the
second-level HTLC outputs after MaturityDelay blocks has passed since
confirmation.
This commit fixes a minor bug (that doesn’t affect anything atm) when
crafting the SignDesc for sweeping breached outputs. Previously, we
would take the p2wkh script and then p2wsh-ify that, placing that into
the SignDesc. This is incorrect as the p2wkh script is “injected” into
the sighash when signing, and thus doesn’t need another encoding layer.
This commit adds an additional return value to SettleHTLC in order to
make way for an upcoming change to modify the way bandwidth status from
the link to the switch is reported.
This commit removes the current active LocalAvailableBalance method
from the channel state machine itself. We still maintain the internal
availableLocalBalance method locally as this is used to ensure that we
don’t add an HTLC which puts our available balance below zero.
This commit also adds an incoming flag to
HtlcRetribution struct to allow the breach arbiter to
generate the appropriate witness based on the htlc's
directionality.
It also ensures that the size of the htlc retribution
slice is now determined by the size of the number of
htlcs present in the revoked snapshot, which fixes a
minor bug that could lead to nil pointer deferences.
This commit is a follow up to the prior commit, as since we reversed
the order of the pubkeys in the multi-sig scripts, then we also need to
reverse the order of the signatures that we use when attempting to
spend the funding output directly.
When creating the script for the funding output, we were reversing the
order of the public keys due to an incorrect assertion of the return
value of the bytes.Compare function. To fix this, we now flip the
order, allowing us to properly create channels as specified within the
specification.
This commit adds to methods to the ChannelReservation struct: one for
generating the channel constraints we require for the remote party, and
one for validating their desired constraints, and committing them to
our ChannelConfig.
With these two new methods, we can now begin to properly store and
adhere to the current set of channel flow control constraints.
This commit fixes an existing w.r.t the way that we constructed all
commitment transactions. We were computing the hash that the obfsucator
was derived form correctly, but we were using the first 6-bytes, rather
than the last 6 bytes.
We no longer attach the RPC client to the lnwallet logging instance as
it can generate a ton of spam in trace mode as it’ll dump the entire
hex encoded blocks, transactions, etc.
This commit fixes a bug within the HTLC construction and commitment
transaction construction that would result in HTLC _values_ within the
commitment transaction being off by a factor of 1000. This was due to
the fact that we failed to convert the amount of an HTLC, in mSAT, to
SAT before placing it as an output within the commitment transaction.
When attempt to locate the output index of a particular half, we use
the unconverted amount, meaning it was unnoticed.
This commit adds a new assertion within the TestSimpleAddSettleWorkflow
test to ensure that the HTLC is found within the commitment transaction
with the proper value in satoshi.
This commit updates the script we use to match the current
specification. The change is minor: we can say an extra byte by moving
the OP_CHECKSIG to the end of the script, and swapping the checks and
seqverify operations in the second clause. However, the witness remains
the same!
Note that this commit is temporary, and should be reverted once #231 is
merged. The reason we need to do this for now, is that we don’t
properly track the exact state of the remote party’s commitment. In
this test case, the resulting HTLC’s added are dust to one party, but
non-dust to another. So upon restart, the states (balance wise) has
diverged.
