Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
3efaf83c75 wallet: deactivate descriptor (S3RK)
6737d9655b test: wallet importdescriptors update existing (S3RK)
586f1d53d6 wallet: maintain SPK consistency on internal flag change (S3RK)
f1b7db1474 wallet: don't mute exceptions in importdescriptors (S3RK)
bf68ebc1cd wallet: allow to import same descriptor twice (S3RK)
Pull request description:
Rationale: allow updating existing descriptors with `importdescriptors` command.
Currently if you run same `importdescriptors` command twice with a descriptor containing private key you will get very confusing error — `Missing required fields`. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get `DB_KEYEXIST (-30995)` from BerkelyDB. Please note, that we set `DB_NOOVERWRITE` (I guess not to lose some keys accidentally). The exception is caught in `catch (...)` in rpcdump.cpp with a generic error.
With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index.
For the range only expansion is allowed (range start can only decrease, range end increase).
ACKs for top commit:
achow101:
re-ACK 3efaf83c75
meshcollider:
Code review ACK 3efaf83c75
jonatack:
Light ACK 3efaf83c75 per `git range-diff a000cb0 5d96704 3efaf83` and as a sanity check, re-debug-built on debian with gcc 10.2.1 and clang 11, ran wallet_importdescriptors.py
Tree-SHA512: 122c4b621d64ec8a3b625f3aed9f01a2b5cbaf2029ad0325b5ff38d67fff5cd35324335fabe2dd5169548b01b267c81be6ae0f5c834342f3d5f6eeed515c4843
No change in behavior. Just remove a little bit of code, reduce macro usage,
remove duplicative functions, and make BDB and SQLite implementations more
consistent with each other.
9b74461fa2 refactor: Assert before dereference in CWallet::GetDatabase (João Barbosa)
021feb3187 refactor: Drop redudant CWallet::GetDBHandle (João Barbosa)
Pull request description:
ACKs for top commit:
achow101:
Code Review ACK 9b74461fa2
meshcollider:
utACK 9b74461fa2
ryanofsky:
Code review ACK 9b74461fa2. Changes since last review: rebasing due to conflict, dropping wallet path commit c6a5cd7a64c78b162f545a3467d0fea7dcaadfcc as suggested in discussion, making GetDatabase() const in the earlier commit. Giving more descriptive title like
Tree-SHA512: 68cf3b5e9fe0acb3a5cd081086629989f213f1904cc344e5775767b56759a7d905b1e1c303afbe40f172ff81bf07f3719b59d8f6ec2de3fdd53cd0e2d220fb25
d52f502b1e Fix mock SQLiteDatabases (Andrew Chow)
99309ab3e9 Allow disabling BDB in configure with --without-bdb (Andrew Chow)
ee47f11f73 GUI: Force descriptor wallets when BDB is not compiled (Andrew Chow)
71e40b33bd RPC: Require descriptors=True for createwallet when BDB is not compiled (Andrew Chow)
6ebc41bf9c Enforce salvage is only for BDB wallets (Andrew Chow)
a58b719cf7 Do not compile BDB things when USE_BDB is defined (Andrew Chow)
b33af48210 Include wallet/bdb.h where it is actually being used (Andrew Chow)
Pull request description:
Adds a `--without-bdb` option to `configure` which disables the compilation of the BDB stuff. Legacy wallets will not be created when BDB is not compiled. A legacy-sqlite wallet can be loaded, but we will not create them.
Based on #20156 to resolve the situation where both `--without-sqlite` and `--without-bdb` are provided. In that case, the wallet is disabled and `--disable-wallet` is effectively set.
ACKs for top commit:
laanwj:
Code review ACK d52f502b1e
Tree-SHA512: 5a92ba7a542acc2e27003e9d4e5940e0d02d5c1f110db06cdcab831372bfd83e8d89c269caff31dd5bff062c1cf5f04683becff12bd23a33be731676f346553d
7486e2771e Tests: Unit test related to WalletDB ReadKeyValue (Bushstar)
32def8d1c2 Catch ios_base::failure specifically (Peter Bushnell)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/2950 a hash of the pubkey and private was added to speed up key import, this was made backwards compatible by reading the hash in a try block with an ellipses catch all in case the hash was not present.
CDataStream::read() specifically throws std::ios_base::failure, backwards compatibility expects only that error to be thrown, if something else gets thrown we should not be catching it. The change in this commit is to catch that exception only. If any other exception is thrown other than std::ios_base::failure it will be caught by the wider try block and an error written to the log and/or console.
CDataStream::read() throwing std::ios_base::failure.
2c364fde42/src/streams.h (L191)
Wider catch statements that pick up all others exceptions other than ios_base::failure.
2c364fde42/src/wallet/walletdb.cpp (L425)2c364fde42/src/wallet/walletdb.cpp (L430)
ACKs for top commit:
laanwj:
Code review ACK 7486e2771e
Tree-SHA512: 5364bf935af8ec603bf5b8fef8c23b5cdaa4fe3506090cff988413221f2eaa99f7a91929afb42a35f8881ce2328744a0d32052da51ca0a5b2e65b6809e97f604
Checks are now consolidated in MakeBerkeleyDatabase function instead of
happening in higher level code.
This commit does not change behavior except for error messages which now
include more complete information.
New function is not currently called but will be called in upcoming commits. It
moves database path checking, and existence checking, and already-loaded
checking, and verification into a single function so this logic does not need
to be repeated all over higher level wallet code, and so higher level code does
not need to change when SQLite support is added in
https://github.com/bitcoin/bitcoin/pull/19077. This also lets higher level
wallet code make fewer assumptions about the contents of wallet directories.
This commit just adds the new function and does not change behavior in any way.
0bbe26a1af wallet: filter for keys only before record deser in salvage (Andrew Chow)
544e12a4e8 walletdb: Add KeyFilterFn to ReadKeyValue (Andrew Chow)
Pull request description:
When salvaging a wallet, the only things that matter are the private keys. It is not necessary to attempt to deserialize any other records, especially if those records are corrupted too.
This PR adds a `KeyFilterFn` function callback to `ReadKeyValue` that salvage uses to filter for only the records that it wants. Of course doing it this way also lets us do other filters in the future from other places should we so desire.
ACKs for top commit:
ryanofsky:
Code review ACK 0bbe26a1af. Looks great! This should make the recovery code more robust. Normally it'd be good to have a test case for the problem this fixes, but Marco already wrote one in #19078, so I think we're covered
laanwj:
Code review ACK 0bbe26a1af
Tree-SHA512: 8e3ee283a22a79273915711c4fb751f3c9b02ce94e6bf08dc468f1cfdf9fac35c693bbfd2435ce43c3a06c601b9b0a67e209621f6814bedfe3bc7a7ccc37bb01
-zapwallettxes is made a hidden option to inform users that it is
removed and they should be using abandontransaction to do the stuck
transaction thing.
Add a KeyFilterFn callback to ReadKeyValue which allows the caller to
specify which types to actually deserialize. A KeyFilterFn takes the
type as the parameter and returns a bool indicating whether
deserialization should continue.
b82f0ca4d5 walletdb: Add MakeBatch function to BerkeleyDatabase and use it (Andrew Chow)
eac9200814 walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (Andrew Chow)
Pull request description:
In order to support alternative database systems, we need to have a generic `Batch` class. This PR adds a `DatabaseBatch` abstract class which is implemented by `BerkeleyBatch`. `DatabaseBatch` is now the class that is used by `WalletBatch` to interact with the database. To be able to get the correct type of `DatabaseBatch`, `BerkeleyDatabase` now has a `MakeBatch` function which returns a newly constructed `std::unique_ptr<DatabaseBatch>`. For `BerkeleyDatabase`, that will be `std::unique_ptr<BerkeleyBatch>`.
The `Read`, `Write`, `Erase`, and `Exists` template functions are moved from `BerkeleyBatch`.
Part of #18971
Requires #19308 and #19324
ACKs for top commit:
Sjors:
re-utACK b82f0ca4d5
MarcoFalke:
ACK b82f0ca4d5🌘
meshcollider:
LGTM, utACK b82f0ca4d5
Tree-SHA512: 6d2d41631c0983391dbecd702e881c6775b155c90b275df97f7157e42608ed251744f9d7ce5173d02a6c5cc38d90b611880fac7fa635d3d8c4d590681f56ac6a
a66a7a1a70 walletdb: don't reinitialize desc cache with multiple cache entries (Andrew Chow)
Pull request description:
When loading descriptor caches, we would accidentally reinitialize the descriptor cache when seeing that one already exists. This should have only been initializing the cache when one does not exist. However this code itself is unnecessary as the act of looking up the cache to add to it will initialize it if it didn't already exist.
This issue could be hit by trying to load a wallet that had imported a multisig descriptor. The wallet would fail to load.
A test has been added to wallet_importdescriptors.py to catch this case. Another test case has also been added to check that loading a wallet with only single key descriptors works.
ACKs for top commit:
hugohn:
tACK [a66a7a1](a66a7a1a70)
jonatack:
ACK a66a7a1a70
meshcollider:
Code review ACK a66a7a1a70
Tree-SHA512: 3df746421a008708eaa3bbbdd12b9ddd3e2ec111d54625a212dca7414b971cc1f6e2b1757b3232c31a2f637d1b1ef43bf3ffa4ac4216646cf1e92db5f79954f1
fa73493930 refactor: Use C++11 range-based for loop (MarcoFalke)
fa7b164d62 wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off (MarcoFalke)
faf8401c19 wallet: Pass unused args to StartWallets (MarcoFalke)
fa6c186436 gui tests: Limit life-time of dummy testing setup (MarcoFalke)
fa28a61897 test: Add smoke test to check that wallets are flushed by default (MarcoFalke)
Pull request description:
User-facing, this is a refactor. Internally, the scheduler does not have to call a mostly empty function every half a second.
ACKs for top commit:
jnewbery:
utACK fa73493930
meshcollider:
utACK fa73493930
ryanofsky:
Code review ACK fa73493930. Just rebased since last review
Tree-SHA512: 99e1fe1b2c22a3f4b19de3e566241d38693f4fd8d5a68ba1838d86740aa6c08e3325c11a072e30fd262a8861af4278bed52eb9374c85179b8f536477f528247c
3a9aba21a4 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow)
d9cd095b59 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow)
0122fbab4c Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow)
Pull request description:
`SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in https://github.com/bitcoin/bitcoin/pull/17681#discussion_r427633081. This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet.
`AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet.
`LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk.
ACKs for top commit:
jnewbery:
Code review ACK 3a9aba21a4
ryanofsky:
Code review ACK 3a9aba21a4. Only changes since last review tweaks making m_wallet_flags updates more safe
meshcollider:
utACK 3a9aba21a4
Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f
d8e9ca66d1 walletdb: Move Rewrite into BerkeleyDatabase (Andrew Chow)
91d109156d walletdb: Move PeriodicFlush into WalletDatabase (Andrew Chow)
8f1bcf8b7b walletdb: Combine VerifyDatabaseFile and VerifyEnvironment (Andrew Chow)
Pull request description:
The `BerkeleyBatch` class has 4 static functions that operate on `BerkeleyDatabase` or `BerkeleyEnvironment`. It doesn't make sense for these to be standalone nor for them to be static functions. So instead, move them from `BerkeleyBatch` into `BerkeleyDatabase` and make them member functions instead of static.
`BerkeleyBatch::VerifyEnvironment` and `BerkeleyBatch::VerifyDatabaseFile` are combined into a single `BerkeleyDatabase::Verify` function that operates on that `BerkeleyDatabase` object.
`BerkeleyBatch::Rewrite` and `BerkeleyBatch::PeriodicFlush` both took a `BerkeleyDatabase` as an argument and did stuff on it. So we just make it a member function so it doesn't need to take a database as an argument.
Part of #18971
ACKs for top commit:
MarcoFalke:
re-ACK d8e9ca66d1 only change is test fixup 🤞
promag:
Code review ACK d8e9ca66d1, good stuff.
Tree-SHA512: 9847e55b13d98bf4e5636cc14bc3f5351d56737f7e320fafffaed128606240765599e5400382c5aecac06690f7e36265ca3e1031f3f6d8a9688f6d5cb1bacd2a
When loading descriptor caches, we would accidentally reinitialize the
descriptor cache when seeing that one already exists. This should have
only been initializing the cache when one does not exist. However this
code itself is unnecessary as the act of looking up the cache to add to
it will initialize it if it didn't already exist.
This issue could be hit by trying to load a wallet that had imported a
multisig descriptor. The wallet would fail to load.
A test has been added to wallet_importdescriptors.py to catch this case.
Another test case has also been added to check that loading a wallet
with only single key descriptors works.
ca24edfbc1 walletdb: Handle cursor internally (Andrew Chow)
Pull request description:
Instead of returning a Dbc (BDB cursor object) and having the caller deal with the cursor, make BerkeleyBatch handle the cursor internally.
Split from #18971
ACKs for top commit:
ryanofsky:
Code review ACK ca24edfbc1. Changes since last review: StartCursor rename, moving CloseCursor calls near returns
promag:
Code review ACK ca24edfbc1.
Tree-SHA512: f029b498c7f275aedca53ce7ade7cb99c82975fd6cad17346a4990fb3bcc54e2a5309b32053bd13def9ee464d331b036ac79abb8fc4fa561170c6cfc85283447
Instead of returning a Dbc (BDB cursor object) and having the caller
deal with the cursor, make BerkeleyBatch handle the cursor internally.
This prepares BerkeleyBatch to work with other database systems as Dbc
objects are BDB specific.
951bca61d7 tests: feature_backwards_compatibility.py test 0.16 up/downgrade (Andrew Chow)
3a03a11e8c Skip hdKeypath of 'm' (Andrew Chow)
Pull request description:
Previously the seed was stored with keypath 'm' so we need to skip this as well when determining inactive seeds.
Fixes#19051
ACKs for top commit:
Sjors:
ACK 951bca61d7
instagibbs:
re-utACK 951bca61d7
ryanofsky:
Code review ACK 951bca61d7. No significant changes since last review, just updated comment and some test tweaks
Tree-SHA512: 930f77e7097c9cf4f1012e540bd2b1a72fd279262517f10c1531b2ad48c632ef95e0dd4edea81bcc3b3db306479d34e5e79e5d6c4ed31dfa4b77a4231436436e
