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.
