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.
In this commit, ensure that upon restart, if any of the full-node based
backends we support reject the transaction, then we'll properly remove
the now invalid transaction from the tx store. Before this commit, we
could miss a few errors from bitcoind. To remedy this, we explicitly
catch those errors, but then also attempt to precisely catch the set of
generic json RPC errors that can be returned.
