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.
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.
In this commit, we fix a bug introduced in an earlier commit. Before
this commit, we would *always* remove an unmined transaction if it
failed to be accepted by the network upon restart. Instead, we should
only remove transaction that are actually due to us trying to spend an
output that’s already spent, or an orphan transaction.
In this commit, we extend the PublishTransction method to be a more
general semi reliable transaction broadcast mechanism. We do this by
removing the special casing for neutrino. With this change, we’ll
_always_ write any transactions to be broadcast to disk. A side effect
of this, is that if the transaction doesn’t *directly* involve any
outputs we control, then it’ll linger around until a restart, when we
try to rebroadcast, and observe that it has bene rejected.
This commit makes use of the recently added EstimateVirtualSize
method to estimated the size of a transaction when calculating
fees. This makes fee estimation more accurate when we are spending
segwit outputs, as before we wouldn't account for the witness
descount, resulting in overshooting fee estimates.
This commit adds a new method EstimateVirtualSize that calculates
the worst case estimate vsize for a transaction with a given set
of inputs and outputs. This method is aware of P2PKH, P2WPKH and
P2SH-P2WPKH inputs, and caulculates the transaction vsize with
the witness data included.
In this commit, we do away with the internal relayFee all together.
Instead, we’ll pass in the fee rate when we’re crafting any
transactions. This allows the caller to manually dictate their desired
fee rate.
This commit makes sure the wallet db is closed if the call to
open the wallet fails, as subsequent calls to OpenExistingWallet
would fail to open the already open database.
During the time of initial block hash catch-up, it is possible to
request an address be generated. This commit updates the active
addresses by calling `w.activeData` after the catch-up is complete.
This changes the database access APIs and each of the "manager"
packages (waddrmgr/wstakemgr) so that transactions are opened (only)
by the wallet package and the namespace buckets that each manager
expects to operate on are passed in as parameters.
This helps improve the atomicity situation as it means that many
calls to these APIs can be grouped together into a single
database transaction.
This change does not attempt to completely fix the "half-processed"
block problem. Mined transactions are still added to the wallet
database under their own database transaction as this is how they are
notified by the consensus JSON-RPC server (as loose transactions,
without the rest of the block that contains them). It will make
updating to a fixed notification model significantly easier, as the
same "manager" APIs can still be used, but grouped into a single
atomic transaction.
Remove the addresses field from TransactionDetails.Output. It is
assumed that the caller is able to deserialize the transaction and
encode the output scripts to addresses, so this is unnecessary server
overhead and conflicts with the current API philosophy of not
duplicating data already included in another field.
Since there is no additional data included for outputs not controlled
by the wallet, remove the `mine` specifier from the Output message and
replace it with an output index. Only include messages for controlled
outputs, rather than creating messages for both controlled and
uncontrolled outputs. Rename the repeated field from `outputs` to
`credits` to be consistent with the `debits` field.
Bump major API version as this is a breaking change.
Closes#408.
This commit enabled the wallet to properly spend nested and normal
p2wkh outputs under its control.
For regular p2wkh outputs, spending simply involves presenting the
original pub key, and signature as the witness data.
For nested p2wkh outputs, in addition to the above, the version zero
witness p2wkh witness program is placed in the sigScript in order to
allow clients who are aware of BIP 16 to validate the witness spend.
When spending a segwit output, the wallet also needs the input value of
the previous output script. Therefore when selecting outputs the input
value is now returned. Additionally when validating newly signed
outputs the input value as also passed into `txscript.Engine`
Previously, this would not increment the spendable balance for matured
coinbase outputs and would only increment the immature balance if the
output was still immature.
This updates both btcsuite and external dependencies to their latest
versions. In particular, gRPC was updated to version 1.0.3 and bolt
to 1.3.0.
The walletrpc package needed to be regenerated for the gRPC update.
While here, update the Travis-CI script so this can be tested there.
Since the coinbase maturity is now allowed to be defined per chain and
the old blockchain.CoinbaseMaturity constant has been removed, this
updates the code accordingly.
Also, update glide.lock to use the required version of btcd.
This updates all code to make use of the new chainhash package since the
old wire.ShaHash type and related functions have been removed in favor
of the abstracted package.
Also, while here, rename all variables that included sha in their name
to include hash instead.
Finally, update glide.lock to use the required version of btcd, btcutil,
and btcrpcclient.
Remove the addresses field from TransactionDetails.Output. It is
assumed that the caller is able to deserialize the transaction and
encode the output scripts to addresses, so this is unnecessary server
overhead and conflicts with the current API philosophy of not
duplicating data already included in another field.
Since there is no additional data included for outputs not controlled
by the wallet, remove the `mine` specifier from the Output message and
replace it with an output index. Only include messages for controlled
outputs, rather than creating messages for both controlled and
uncontrolled outputs. Rename the repeated field from `outputs` to
`credits` to be consistent with the `debits` field.
Bump major API version as this is a breaking change.
Closes#408.
Due to the way dust is calculated, if the transaction relay fee is
zero, then a zero output amount is not considered dust. As the
transaction authoring code used this dust check to determine whether a
change output can be included or not, it could create unnecessary
change outputs which return no value back to the wallet. Prevent this
by including an explicit check for zero values.
This commit corrects various things found by the static checkers
(comments, unkeyed fields, return after some if/else).
Add generated files and legacy files to the whitelist to be ignored.
Catch .travis.yml up with btcd so goclean can be run.
This changes the wallet.Open function signature to remove the database
namespace parameters. This is done so that the wallet package itself
is responsible for the location and opening of these namespaces from
the database, rather than requiring the caller to open these ahead of
time.
A new wallet.Create function has also been added. This function
initializes a new wallet in an empty database, using the same
namespaces as wallet.Open will eventually use. This relieves the
caller from needing to manage wallet database namespaces explicitly.
Fixes#397.
These notifications were added to support real time updates for
btcgui. As the btcgui project is no longer being developed, there are
no more consumers of this API, and it makes sense to remove them given
their various issues (the largest being that notifiations are sent
unsubscribed to clients that may never be interrested in them).
A new notification server has already been added to the wallet package
to handle notifications in a RPC-server agnostic way. This server is
the means by which the wallet notifies changes for gRPC clients. If
per-client registered notifications are to be re-added for the
JSON-RPC server, they should be integrated with the new notification
server rather than using this legacy code.
This corrects and simplifies the shutdown logic for interrupts, the
walletrpc.WalletLoaderService/CloseWallet RPC, and the legacy stop RPC
by both stopping all wallet processes and closing the wallet database.
It appears that this behavior broke as part of the wallet package
refactor, causing occasional nil pointer panics and memory faults when
closing the wallet database with active transactions.
Fixes#282.
Fixes#283.
This began as a change to improve the fee calculation code and evolved
into a much larger refactor which improves the readability and
modularity of all of the transaction creation code.
Transaction fee calculations have been switched from full increments
of the relay fee to a proportion based on the transaction size. This
means that for a relay fee of 1e3 satoshis/kB, a 500 byte transaction
is only required to pay a 5e2 satoshi fee and a 1500 byte transaction
only need pay a 1.5e3 fee. The previous code would end up estimating
these fees to be 1e3 and 2e3 respectively.
Because the previous code would add more fee than needed in almost
every case, the transaction size estimations were optimistic
(best/smallest case) and signing was done in a loop where the fee was
incremented by the relay fee again each time the actual size of the
signed transaction rendered the fee too low. This has switched to
using worst case transaction size estimates rather than best case, and
signing is only performed once.
Transaction input signature creation has switched from using
txscript.SignatureScript to txscript.SignTxOutput. The new API is
able to redeem outputs other than just P2PKH, so the previous
restrictions about P2SH outputs being unspendable (except through the
signrawtransaction RPC) no longer hold.
Several new public packages have been added:
wallet/txauthor - transaction authoring and signing
wallet/txfees - fee estimations and change output inclusion
wallet/txrules - simple consensus and mempool policy rule checks
Along with some internal packages:
wallet/internal/txsizes - transaction size estimation
internal/helpers - context free convenience functions
The txsizes package is internal as the estimations it provides are
specific for the algorithms used by these new packages.
Previously, when creating a change address during the process of
creating a new transaction an error case would be hit in the waddrmgr
triggered by attempting to derive a new internal address from under a
waddrmgr.ImportedAddrAccount. To remedy this error, we now use the
default account for change when spending outputs from an imported
key. This approach allows funds under the control of imported
private keys to be protected under the wallet's seed as soon as
they've been partially spent.
Previously, if a nil seed was passed into loader.CreateNewWallet, a
random seed was never generated. This would cause an error within the
waddrmgr due to the seed being of invalid (0) length.
This is a rather monolithic commit that moves the old RPC server to
its own package (rpc/legacyrpc), introduces a new RPC server using
gRPC (rpc/rpcserver), and provides the ability to defer wallet loading
until request at a later time by an RPC (--noinitialload).
The legacy RPC server remains the default for now while the new gRPC
server is not enabled by default. Enabling the new server requires
setting a listen address (--experimenalrpclisten). This experimental
flag is used to effectively feature gate the server until it is ready
to use as a default. Both RPC servers can be run at the same time,
but require binding to different listen addresses.
In theory, with the legacy RPC server now living in its own package it
should become much easier to unit test the handlers. This will be
useful for any future changes to the package, as compatibility with
Core's wallet is still desired.
Type safety has also been improved in the legacy RPC server. Multiple
handler types are now used for methods that do and do not require the
RPC client as a dependency. This can statically help prevent nil
pointer dereferences, and was very useful for catching bugs during
refactoring.
To synchronize the wallet loading process between the main package
(the default) and through the gRPC WalletLoader service (with the
--noinitialload option), as well as increasing the loose coupling of
packages, a new wallet.Loader type has been added. All creating and
loading of existing wallets is done through a single Loader instance,
and callbacks can be attached to the instance to run after the wallet
has been opened. This is how the legacy RPC server is associated with
a loaded wallet, even after the wallet is loaded by a gRPC method in a
completely unrelated package.
Documentation for the new RPC server has been added to the
rpc/documentation directory. The documentation includes a
specification for the new RPC API, addresses how to make changes to
the server implementation, and provides short example clients in
several different languages.
Some of the new RPC methods are not implementated exactly as described
by the specification. These are considered bugs with the
implementation, not the spec. Known bugs are commented as such.
This change introduces additional network activity with the btcd
process to ensure that the network connection is not silently dropped.
Previously, if the connection was lost (e.g. wallet runs on a laptop
and connects to remote btcd, and the laptop is suspended/resumed) the
lost connection would not be detectable since all normal RPC activity
(excluding requests from btcwallet to btcd made by the user) is in the
direction of btcd to wallet in the form of websocket notifications.
sync.Locker cannot be safely used to switch a sync.Mutex to a noop
locker since other goroutines that attempt to lock the mutex will race
on the changing interface. Instead, just statically dispatch
sync.Mutex methods.
Rather than the main package being responsible for opening the address
and transaction managers, the namespaces of these components are
passed as parameters to the wallet.Open function.
Additionally, the address manager Options struct has been split into
two: ScryptOptions which holds the scrypt parameters needed during
passphrase key derivation, and OpenCallbacks which is only passed to
the Open function to allow the caller to provide additional details
during upgrades.
These changes are being done in preparation for a notification server
in the wallet package, with callbacks passed to the Open and Create
functions in waddrmgr and wtxmgr. Before this could happen, the
wallet package had to be responsible for actually opening the managers
from their namespaces.
If a long reorganize occurs farther back than the last saved recent
block hash (currently max 20 are saved) a full rescan is triggered
since there is no guarantee the previous blocks weren't also removed
in the reorg. In this case, the address manager was set unsynced, but
transaction history was not rolled back as well. This commit corrects
this by unconfirming all transactions but those in the genesis block.
