db2effaca4 scripted-diff: refactor: CWallet::Create() -> CreateNew() (David Gumberg)
27e021ebc0 wallet: Correctly log stats for encrypted messages. (David Gumberg)
d8bec61be2 wallet: remove loading logic from CWallet::Create (David Gumberg)
f35acc893f refactor: wallet: Factor out `WriteVersion()` from `PopulateWalletFromDB()` (David Gumberg)
e12ff8aca0 test: wallet: Split create and load (David Gumberg)
70dbc79b09 wallet: Use CWallet::LoadExisting() for loading existing wallets. (David Gumberg)
ae66e01164 wallet: Create separate function for wallet load (David Gumberg)
bc69070416 refactor: Wallet stats logging in its own function (David Gumberg)
a9d64cd49c wallet: Remove redundant birth time update (David Gumberg)
b4a49cc727 wallet: Move argument parsing to before DB load (David Gumberg)
b15a94a618 refactor: Split out wallet argument loading (David Gumberg)
a02c4a82d8 refactor: Move -walletbroadcast setting init (David Gumberg)
411caf7281 wallet: refactor: PopulateWalletFromDB use switch statement. (David Gumberg)
a48e23f566 refactor: wallet: move error handling to PopulateWalletFromDB() (David Gumberg)
0972785fd7 wallet: Delete unnecessary PopulateWalletFromDB() calls (David Gumberg)
f0a046094e scripted-diff: refactor: CWallet::LoadWallet->PopulateWalletFromDB (David Gumberg)
Pull request description:
This PR is mostly a refactor which splits out logic used for creating wallets and for loading wallets, both of which are presently contained in `CWallet::Create()` into `CWallet::CreateNew()` and `CWallet::LoadExisting()`
The real win of this PR is that `CWallet::Create()` uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet:
370c592612/src/wallet/wallet.cpp (L2882-L2885)
This heuristic assumes that wallets with no `ScriptPubKeyMans` are being created, which sounds reasonable, but as demonstrated in #32112 and #32111, this can happen when the user tries to load a wallet file that is corrupted, both issues are fixed by this PR and any other misbehavior for wallet files which succeeded the broken heuristic's sniff test for new wallets.
It was already the case that every caller of `CWallet::Create()` knows whether it is creating a wallet or loading one, so we can avoid replacing this bad heuristic with another one, and just shift the burden to the caller.
ACKs for top commit:
achow101:
ACK db2effaca4
polespinasa:
approach ACK db2effaca4
w0xlt:
reACK db2effaca4
murchandamus:
ACK db2effaca4
rkrux:
ACK db2effaca4
Tree-SHA512: c28d60e0a3001058da3fd2bdbe0726c7ebe742a4b900a1dee2e5132eccc22e49619cb747a99b4032b000eafd4aa2fdd4ec244c32be2012aba809fdc94b5f6ecd
There are too many functions in CWallet with names like "Load" and
"Create", disambiguate what CWallet::LoadWallet does by renaming it to
PopulateWalletFromDB.
-BEGIN VERIFY SCRIPT-
sed -i 's|\bLoadWallet()|PopulateWalletFromDB()|g' $(git grep -l 'LoadWallet()' -- ':(exclude)src/wallet/walletdb.cpp')
-END VERIFY SCRIPT-
This function is a duplicate of HasEncryptionKeys().
-BEGIN VERIFY SCRIPT-
sed -i '/bool IsCrypted() const;/d' src/wallet/wallet.h
sed -i '/^bool CWallet::IsCrypted() const$/,/^}$/{/^}$/N;d;}' src/wallet/wallet.cpp
sed -i --regexp-extended 's/IsCrypted\(\)/HasEncryptionKeys()/g' $(git ls-files '*.cpp' '*.h')
-END VERIFY SCRIPT-
d51fbab4b3 wallet, test: Be able to always swap BDB endianness (Ava Chow)
0b753156ce test: Test bdb_ro dump of wallet without reset LSNs (Ava Chow)
c1984f1282 test: Test dumping dbs with overflow pages (Ava Chow)
fd7b16e391 test: Test dumps of other endian BDB files (Ava Chow)
6ace3e953f bdb: Be able to make byteswapped databases (Ava Chow)
d9878903fb Error if LSNs are not reset (Ava Chow)
4d7a3ae78e Berkeley RO Database fuzz test (TheCharlatan)
3568dce9e9 tests: Add BerkeleyRO to db prefix tests (Ava Chow)
70cfbfdadf wallettool: Optionally use BERKELEY_RO as format when dumping BDB wallets (Ava Chow)
dd57713f6e Add MakeBerkeleyRODatabase (Ava Chow)
6e50bee67d Implement handling of other endianness in BerkeleyRODatabase (Ava Chow)
cdd61c9cc1 wallet: implement independent BDB deserializer in BerkeleyRODatabase (Ava Chow)
ecba230979 wallet: implement BerkeleyRODatabase::Backup (Ava Chow)
0c8e728476 wallet: implement BerkeleyROBatch (Ava Chow)
756ff9b478 wallet: add dummy BerkeleyRODatabase and BerkeleyROBatch classes (Ava Chow)
ca18aea5c4 Add AutoFile::seek and tell (Ava Chow)
Pull request description:
Split from #26596
This PR adds `BerkeleyRODatabase` which is an independent implementation of a BDB file parser. It provides read only access to a BDB file, and can therefore be used as a read only database backend for wallets. This will be used for dumping legacy wallet records and migrating legacy wallets without the need for BDB itself.
Wallettool's `dump` command is changed to use `BerkeleyRODatabase` instead of `BerkeleyDatabase` (and `CWallet` itself) to demonstrate that this parser works and to test it against the existing wallettool functional tests.
ACKs for top commit:
josibake:
reACK d51fbab4b3
TheCharlatan:
Re-ACK d51fbab4b3
furszy:
reACK d51fbab4b3
laanwj:
re-ACK d51fbab4b3
theStack:
ACK d51fbab4b3
Tree-SHA512: 1e7b97edf223b2974eed2e9eac1179fc82bb6359e0a66b7d2a0c8b9fa515eae9ea036f1edf7c76cdab2e75ad994962b134b41056ccfbc33b8d54f0859e86657b
It's not necessary to set up an entire CWallet just so we can get access
to the WalletDatabase and read the records. Instead we can go one level
lower and make just a WalletDatabase.
When there is a wallet loading error, it could be a noncritical one so
it is not necessary to make wallet_instance a nullptr. The wallet can
still go on with normal operation in that case, as we do for loading in
bitcoind and bitcoin-qt.
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
The fs.* files are already part of the libbitcoin_util library. With the
introduction of the fs_helpers.* it makes sense to move fs.* into the
util/ directory as well.
0cb6d2aec6 Bugfix: Wallet: Document expectations for AddWalletFlags (now InitWalletFlags) correctly (Luke Dashjr)
Pull request description:
Includes some slight refactoring (return type changed, current status checked)
ACKs for top commit:
achow101:
ACK 0cb6d2aec6
w0xlt:
ACK 0cb6d2aec6
ryanofsky:
Code review ACK 0cb6d2aec6. This is a clarifying change, and should prevent the InitWalletFlags method being called incorrectly. I left a comment suggestion, but feel free to ignore it.
Tree-SHA512: fa18e9471b5e89d35cbc01526e6d4dbe4eee8faa9646847248909af1751b33014a6f9a42fe70a1331c0d73adea79008b8fc3ae2b51a641eba3e36d5c631327f6
3431839c33 util, refactor: Improve headers for bitcoin-wallet tool (Hennadii Stepanov)
Pull request description:
This PR:
- removes unneeded `#include <wallet/wallet.h>` from `<wallet/wallettool.h>`
- introduces class forward declaration in `<wallet/wallettool.h>`
- added `#include <config/bitcoin-config.h>` to `wallet/wallettool.cpp` where the `USE_BDB` macro is used
Top commit has no ACKs.
Tree-SHA512: a0de560d821f8b570ae806a1165b9b382c9e0b339687d932052fa4c38ab2ba493e7e050f19adc02ad7db40c42cf88ac1d37209f9071494a0ab268ed33ff22b9f
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
9c1052a521 wallet: Default new wallets to descriptor wallets (Andrew Chow)
f19ad40463 rpc, wallet: Descriptor wallets are no longer experimental (Andrew Chow)
Pull request description:
Changes the default wallet type from legacy to descriptors. Descriptor wallets will now by the default type. Additionally, descriptor wallets will no longer be marked as experimental.
This follows the timeline proposed in #20160
ACKs for top commit:
lsilva01:
Tested ACK 9c1052a521 on Ubuntu 20.04
prayank23:
tACK 9c1052a521
meshcollider:
Code review ACK 9c1052a521
Tree-SHA512: 834e6fec88e0c18673af7ebe135bd5333694d1be502164eb93a90e3e76c27974165aa4e59426945100c88e4eca07356e16886ef5b05cf789683ecb23fc71a12a
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
