Previously, inserting a transaction as unconfirmed into the store and
later confirming it would leave a lingering unconfirmed input record.
This was discovered as part of
https://github.com/btcsuite/btcwallet/pull/655. This issue would only
affect the wallet if it tracked spent transaction outputs, which it
doesn't. We aim to resolve it in any case for the sake of internal
consistency.
This test ensures that there aren't any lingering unconfirmed records
for a transaction that existed within the store as unconfirmed before
becoming confirmed. At the moment, this is currently failing due to a
gap when moving a transaction from unconfirmed to confirmed within the
store. This will be resolved in a subsequent commit.
This commit ensures the wallet won't enter an inconsitent state
by checking tx confirmation before adding credit.
Without this fix, In the case the existing transaction is already
confirmed on-chain the flow updates the bucketUnminedCredits but
without adding en entry also to the bucketUnmined resulting in
inconsistent state.
In this commit, we address an issue with the wallet store where it'd be
possible for us to keep lingering unconfirmed transaction entries for an
output that has been spent by a different, confirmed transaction. This
was caused due to us removing all spending transaction entries for a
given output when removing conflicts. Since all of the entries would be
removed, we weren't able to retrieve the hashes of the remaining
spending transactions to remove their records as well. Instead, we
propose to only remove the entry for the specified spending transaction.
In this commit, we follow up our previous migration to reset our synced
block to our birthday block with another migration to drop our
transaction history. We'll need to do this to ensure that the
transaction store matches the exact state of our outputs on-chain to
prevent inadvertently crafting any invalid transactions.
In this commit, we remove the old upgrade/migration logic of the
transaction manager as it's been superseded by the new approach using
the migration.Manager interface.
In this commit, we can remove the LatestVersion constant as it's no
longer needed. Instead, we'll now define the latest version as the last
entry in the slice of versions previously defined.
In this commit, we add an implementation of the recently introduced
migration.Manager interface for the transaction manager. With this,
we'll now be able to only expose the things required for the migration
to happen, but have the actual migration logic live at a much higher
level.
There are no existing migrations for the transaction manager, but since
the latest version was already defined as 1, we'll start from there to
be backwards-compatible.
In this commit, we extend TestRemoveUnminedTx to also account for
checking the store's total balance (confirmed and unconfirmed). It
currently ensures that the UTXO state is correct, but as a sanity check,
we'll also ensure that balances are properly updated.
In this commit, we remove the duplicate test case from TestStoreQueries
as we'll no longer allow storing a transaction as unconfirmed if it's
already confirmed.
In this commit, we resolve a lingering bug within the wallet where it's
possible that an output is added as unconfirmed credit after the fact
that it has already confirmed. This would lead to duplicate entries for
the same output within the wallet causing double spend transactions to
be crafted.
In this commit, we add a new test case to the wtxmgr store to ensure
that duplicate outputs don't exists within the store. It's possible for
this to happen if an output is marked as unconfirmed credit, then marked
as confirmed once it confirms, and once again marked as unconfirmed. It
can be marked as unconfirmed again due to the backend notifying the
client about this transaction. Ideally this should not happen, but the
root cause is much more involved. As a stop gap, we'll ensure that
outputs can be marked as unconfirmed credits more than once whatsoever.
As is, the test case fails, which proves that this is an issue. A later
commit will resolve this and the test case should pass.
In this commit, we modify the way we store spending transaction hashes
for unconfirmed spends. Now, rather than only keeping track of one
possible unconfirmed spend, we track multiple in order to ensure we
properly handle transaction replacements, like in the case of RBF,
double spends, etc. With this in, the double spent tests recently added
should now pass.
In this commit, we slightly refactor the existing moveMinedTx method
and split it into two: deleteUnminedTx and updateMinedBalance. We do
this as moveMinedTx is no longer moving the transaction from the unmined
bucket to the mined, instead it just removes it from the unmined bucket.
In this commit, we remove most of the common code between insertMinedTx
and moveMinedTx. Now, all the common logic is handled within
insertMinedTx, and moveMinedTx only contains its unique logic.
In this commit, we add a set of double spend tests to ensure that we can
properly detect and handle them. At this point, we do not do this, but a
follow up commit will address this.
In this commit, we add a new method to the Store object that allows
callers to *manually* remove any conflicting transactions. At times,
it’s the case that while we were offline another transaction was
broadcast that double spends our own, or with the existence of RBF,
another replacement transaction was generated. In this case, when we
come back online, the tx will be rejected. Currently, we have no way of
removing such transaction sot avoid the retransmit-then-reject-dance.
This commit fixes that by adding RemoveUnminedTx.
While making these tests compile and pass, we ended up tripping on the
broken bolt cursor usage painfully discovered in dcrwallet, so i've
ported that fix over as well. Would have learned about that a whole
lot sooner if those tests were never disabled..
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.
This fixes a bug introduced by the new segwit behavior upstream in
btcd. Previously it was a nice optimization to skip the extra
serialization, and compute the txid manually from the serialized
transaction.
However, with segwit, doing that will generate an invalid txid, since
txid’s should ignore any witness data. Therefore, we now fallback to
using msgTx.TxSha() which serializes the transaction without witness
data before calculating the txid.
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.
This changes the wtxmgr.Store.UnminedTxs method to sort transactions,
using the Kahn topological sort algorithm, before returning
transactions to the caller. This is possible because transactions
form a sort of directed acyclic graph (DAG) where transactions
reference other transactions to spend their outputs (multiple
referenced outputs from a single transaction spent by the same
transaction count as a single graph edge).
This prevents the possibility of orphan rejection errors when sending
unmined transactions to a full node at startup. As these transactions
are sent using the sendrawtransaction RPC, which does not permit
orphans, this topological sort is required.
Fixes#156.
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.
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.
