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.
