In similar fashion to the previous commit, due to a no longer existing
bug within the wallet, it was possible for change addresses to be
created outside of their intended key scope (the default), so wallets
affected by this now need to ensure upon recovery that they scan the
chain for _all_ existing key scopes, rather than just the default ones,
to reflect their proper balance. Through manual testing, it was shown
that the impact of recovering the additional key scopes is negligible in
most cases for both full nodes and light clients.
Due to a no longer existing bug within the wallet, it was possible for
change addresses to be created outside of their intended key scope (the
default), so wallets affected by this now need to ensure they scan the
chain for all addresses within the default key scopes (as expected), and
all _internal_ addresses (branch used for change addresses) within any
other registered key scopes to reflect their proper balance.
The commit being reverted resulted in the discovery of a bug in which
change addresses could at times be created outside of the default key
scopes, causing us to not properly determine their spends.
Previously, the wallet would determine the key scope to use for change
addresses by locating the one compatible with P2WPKH addresses, but this
wasn't always safe like in the case when multiple key scopes that
supported these addresses existed within the address manager, leading
the change address to be created outside of the intended key scope.
Normally the wallet doesn't wait for the chain backend to be synced
on regtest/simnet because there we cannot be certain if we are at
the chain tip, as there are no other nodes to compare to. For
Neutrino, this is a bit different because we rely on the cfheader
server to tell us what it thinks the chain tip is. For a wallet
recovery on Neutrino we therefore need to make sure we are at least
synced up to what the server thinks is the tip.
It was discovered that the wallet can scan the chain for unnecessary
additional addresses that are derived by higher-level applications using
custom key scopes. This isn't much of an issue for full nodes, but it
can cause light clients to scan more than what's required, triggering
more false positive matches which lead to block retrieval.
Now, we'll only scan the chain for addresses that exist within the
default key scopes, as those are the only ones the wallet should be
concerned about.
In this commit, we move the existing `internal/txsizes` package into its
own package and make it a module along the way. This allows projects
like `neutrino` to depend on a slimmer set of `btcwallet` related
dependencies.
A nil txid could've been returned from publishTransaction even if it was
successful. This was due to the underlying SendRawTransaction call
"failing", e.g., when the transaction being broadcast has already
confirmed, but publishTranasction interpreting such failure as a
success.
In this commit, we speed up creating a fresh wallet when using the btcd
backend. Before this commit, we would need to rescan 10k or so blocks
when creating a new wallet with the btcd backend. btcd will actually
fully scan all the blocks even though we have zero addresses or UTXOs to
look for. As a result, this can take quite some time.
In this commit we modify the starting height of the initial rescan to
start at the birthday height, and only modify it if it's unset, or the
best height of the chain is before this birthday height. As a result, we
won't always have the full 10k block re org safety horizon on disk, but
will tend to this level after we begin to sync forward.
Previously, the wallet would attempt to store the same block it
checkpointed during its initial sync when performing a recovery. This
would cause the previous block existence validation check to be in
place, which would ultimately fail because the previous block was not
stored intentionally.
To address this, we always start/resume our recovery from the wallet's
best height. This also ensures that we do not rescan the same block
again when resuming a recovery after a shutdown.
In this commit we fix a lingering bug in our output sanity checks that
would only show up during time periods of persistently higher fees.
Before this commit we would incorrectly use the fee rate instead of the
min relay fee when checking an output for dust. This would cause us to
mistakenly reject a transaction for having a dust output.
We fix this by falling back to using the current min-relayfee.
We use the recently introduced locateBirthdayBlock function within
birthdaySanityCheck as it serves as a more optimized alternative that
achieves the same purpose.
Currently, wallet rescans start from its known tip of the chain. Since
we no longer store blocks all the way from genesis to the tip of the
chain, performing a rescan would cause us to scan blocks all the way
from genesis, which we want to avoid. To prevent this, we set the
wallet's tip to be the current reorg safe height. This ensures that
we're unable to scan any blocks before it, and that we maintain
MaxReorgDepth blocks stored.
This commit serves as another building point to allow the wallet to not
store blocks all the way from genesis to the tip of chain. We modify the
wallet's recovery logic to now start from either its birthday block, or
the current reorg safe height if it's before the birthday, to ensure the
wallet properly only stores MaxReorgDepth blocks.
We also refactor things a bit in hopes of making the logic a bit more
readable.
We do this as the wallet will no longer store blocks all the way from
genesis to the tip of the chain. Instead, in order to find a reasonable
birthday block, we resort to performing a binary search for a block
timestamp that's within +/-2 hours of the birthday timestamp.
This serves as groundwork for only storing up to MaxReorgDepth blocks
upon initial sync. To do so, we want to make sure the chain backend
considers itself current so that we can only fetch the latest
MaxReorgDepth blocks from it.
This ensures the wallet can properly do an initial sync, a recovery, or
detect if it's on a stale branch before attempting to process new blocks
at tip.
Since the rescan will be triggered synchronously as well, we'll need to
catch the wallet's quit chan when handling rescan batches in order to
allow for clean shutdowns.
This unifies the logic of receiving an error when broadcasting a
confirmed transaction through btcd's/bitcoind's RPC interface. The btcd
dependency update is required in order for it to match bitcoind's
behavior. For older nodes that have yet to update, the confirmed
transaction will still be caught by the "transaction already exists"
case. This is not needed for bitcoind however, because its been sending
the same RPC error code for several major releases now.
In this commit, we address an issue with chains that are not current,
like in the often case of regtest and simnet chains. Syncing the wallet
would fail due to the chain not being current and not finding a suitable
birthday block. We fix this by just using the last synced block as the
birthday block to ensure we can properly sync to the chain.
In this commit, we fix a regression in the wallet when attempting to
sync new developer test chains such as regtest and simnet. The wallet
would block syncing until a block was mined, but in order to mine a
block, an address must be generated by the wallet first. This address
generation would block as the syncing logic was already holding the
database's mutex.
In this commit, we fix an issue with the wallet's initial sync logic
where we'd miss processing all of the blocks in the chain. This can
happen if the backend is considered current while we're still catching
up. To address this, we make sure we update our best height to process
those missed blocks.
Co-authored-by: Roei Erez <roeierez@gmail.com>
By doing this, we defer all error string-matching to happen within
publishTransaction, which allows us to simplify some of the existing
logic and maintain consistency.
In this commit, we rework how publishTransaction works in order to
correctly handle removing invalid transactions from the wallet's
unconfirmed transaction store. This is crucial as otherwise, invalid
transactions can remain within the wallet and be used for further
transactions, causing a chain of inaccurate transactions.
publishTransaction will now only return an error if the transaction
fails to be broadcast and it has not been previously seen in the
mempool/chain. This is intended in order to provide an easier API to
callers. Any other errors when broadcasting the transaction will cause
it to be removed from the wallet's unconfirmed transaction store to
ensure it maintains an accurate view of the chain.
We do this in order to be able to reuse the new publishTransaction
method within other parts of the wallet in order to consolidate all
error string-matching within one place.
This is done to avoid the birthday rescan to fail if the chain backend
reports a bestheight of 0.
Earlier it could happen that we attempted to sync to the birthday, but
since only the genesis block was available, which would be rejected as
birthday block because of the timestamp, it would fail to find a block
and the sync would fail.
In this commit, we consolidate the existing rollback logic to carry out
its duties under one database transaction.
Co-authored-by: Roei Erez <roeierez@gmail.com>
In this commit, we refactor the wallet's syncing logic with
syncWithChain to use the newer, simpler methods: syncToBirthday and
recovery. Along the way, we also fix a bug within the wallet where it
was possible to sync past the birthday, but not sync to tip completely
and restart, which would lead to us starting a rescan from the latest
synced height, rather than from the birthday stamp.
This commit slightly changes the wallet's syncing behavior to the
following:
1. Ensure the wallet is synced to its birthday.
2. Perform a recovery if requested.
3. Check for chain reorgs.
4. Dispatch a rescan from the current synced height.
Co-authored-by: Roei Erez <roeierez@gmail.com>
In this commit, we add a new recovery method to the wallet. This method
attempts to recover any unspent outputs which pay to any of the wallet's
addresses. Most of the logic found within it is heavily borrowed from
the existing syncWithChain method. This method is currently unused, but
it will end up replacing some of the existing sync logic in a later
commit.
In this commit, we add a new syncToBirthday method to the wallet. This
method intends to sync the wallet's point of the view of the chain until
finding its birthday. Most of the logic found within it is heavily
borrowed from the existing syncWithChain method. This method is
currently unused, but it will end up replacing some of the existing sync
logic in a later commit.
Co-authored-by: Roei Erez <roeierez@gmail.com>
In this commit, we address an issue that would cause users to be stuck
in an infinite loop by fetching the same candidate birthday block due to
its height not being updated if the sanity check was attempting to fix
an estimate in the future. We fix this by setting the new candidate
height so that new candidate blocks can be fetched and tested.
In this commit, we remove the wallet dependency from the
birthdaySanityCheck function. Every interaction with the wallet is now
backed by two interfaces, birthdayStore and chainConn. These interfaces
will allow us to increase the test coverage of the birthdaySanityCheck
as now we'll only need to mock out only the necessary functionality.
In this commit, we prevent any further sanity check attempts by the
wallet if its correctness has previously been verified. We do this to
ensure we don't unnecessarily attempt to find a new candidate.
In this commit, we address an issue with the wallet where it would
always request a rescan from the birthday block. This is very crucial
for older wallets, as it'll potentially go through thousands of blocks.
To address this, we'll now only request a rescan from our birthday if
we're recovering our wallet from our seed, the birthday block was rolled
back, or if we're performing our initial sync. Otherwise, we'll request
a rescan from tip.
In this commit, we address a slight regression within the wallet
that was introduced in a previous commit. When attempting to send coins
on-chain, we would never ask the chain backend to notify us of the
transaction upon confirmation. This, along with the rebroadcast of
unconfirmed transactions logic, would result in the wallet becoming out
of sync with the chain.
Below is an example of how this could have happened:
1. Send funds on-chain.
2. Wallet doesn't ask to be notified of the confirmation.
3. Since the wallet is not notified of the confirmation, the
transaction remains in the unconfirmed bucket, even though it might
have already confirmed on-chain.
4. Restart and trigger the rebroadcast of unconfirmed transactions.
5. The unconfirmed transaction is removed from the unconfirmed bucket
due to it already existing on-chain, without it being moved to the
confirmed bucket. Moving to the confirmed bucket would require the
block at which it confirmed, which we don't have at this point.
In this commit, we add a sanity check for the wallet's birthday block
before syncing as a result of the migration that populated it for
existing wallets. This is done as the second part to the migration to
ensure we do not miss any relevant events throughout rescans.
The sanity check performs two main checks: whether the birthday block
timestamp reflects a time before the birthday timestamp and whether the
delta between these two timestamps is a reasonable amount. The birthday
block is then found as the first candidate that satisfies both of these
conditions.
ImportPrivateKey
In this commit, we ensure that when an external private key is imported
into the wallet, that we do not overwrite our existing birthday with the
one provided. If this were to happen and we forced a wallet rescan using
the birthday as our starting point, then we'd miss detecting relevant
on-chain events that occurred between them.
In this commit, we modify the wallet to use the new migration logic
provided by the recently introduced migration package. Additionally,
we'll also perform all of our upgrades within the same database
transaction to guarantee fault-tolerance of the wallet.
In this commit, we relax the initial sync detection logic a bit. We do
this as right now, if a user creates an address during the sync point,
if they restart, then we'll fall back to performing a rescan from that
height as we'll detect that we aren't performing the initial sync, so
won't pick up the birthday timestamp.
To fix this, we now declare that if we have no UTXO's, then we're still
performing the initial sync. This solves this issue as when the user
restarts, we'll continue to wait for the backend to sync, and pick up
the proper birthday height before we attempt to scan forward for the
rescan. However, the one tradeoff is that we'll now always start the
rescan from the birthday height until the wallet has gained it's first
UTXO. I don't think this is too bad, as after all, the point of a wallet
is to manage utxos.
In this commit, we refactor the logic outside of PublishTransaction into
another unexported method. This will pave the road for unifying the
logic between SendOutputs and PublishTransaction.
In this commit, we simplify the logic when broadcasting transactions to
the greater network. Rather than special casing when running with a
Neutrino backend, we'll always add the transaction to the store as
relevant when attempting to broadcast it. This will properly insert it
into the store and update unconfirmed balances. In the event that the
transaction failed to broadcast, it can be removed from the store with
no side-effects, essentially acting as if the transaction was never
added to the store in the first place.
In this commit, we modify the SendOutputs method to also notify new
outgoing transctions for neutriino. For the full node backends, they'll
get this notification when the transactino hits the mempool. However,
for neutrino it will only be notified once the transaction has been
confirmed. This commit ensures that we'll notify on send as well.
In this commit, we avoid notifying clients of transactions that we've
received chain.RelevantTx notifications for, but are not found within
the wallet. This can happen as now we'll prevent adding an unconfirmed
transaction to the wallet that already exists as confirmed. Due to this,
UniqueTxDetails will be unable to find the transaction and return nil,
casuing a panic for potential callers.
This PR moves any address notifications outside of the
db transaction that creates them. This is known to have
resulted in deadlocks, since chainClient.NotifyReceived
could block the db transaction from committing.
Doing so also prevents the situation where we send
notifications about the new addresses, but the db txn
fails to commit and the addresses are in fact never
created.
This commit adds rescanWithTarget, in order to facilitate
rescans beginning a certain height. This is done as a
precursor to fixing a bug in the initial sync, that would
cause us to miss relevant txns if they are confirmed before
starting the initial rescan.
In this commit, we alter the behavior for handling chain notifications
within the wallet. The previous code would assume that the channel would
close, but due to now using a ConcurrentQueue to handle notifications,
this assumption no longer stands. Now, we'll stop handling notifications
either once the wallet has or stopped or once the notifications channel
has been closed.
