Instead of storing and passing around fixed strings for the purpose of
an address, use an enum.
This also rationalizes the CAddressBookData struct, documenting all fields and
making them public, and simplifying the representation to avoid bugs like
https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134615114 and make
it not possible to invalid address data like change addresses with labels.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
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.
4aebd832a4 db: Change DatabaseCursor::Next to return status enum (Andrew Chow)
d79e8dcf29 wallet: Have cursor users use DatabaseCursor directly (Andrew Chow)
7a198bba0a wallet: Introduce DatabaseCursor RAII class for managing cursor (Andrew Chow)
69efbc011b Move SafeDbt out of BerkeleyBatch (Andrew Chow)
Pull request description:
Instead of having database cursors be tied to a particular `DatabaseBatch` object and requiring its setup and teardown be separate functions in that batch, we can have cursors be separate RAII classes. This makes it easier to create and destroy cursors as well as having cursors that have slightly different behaviors.
Additionally, since reading data from a cursor is a tri-state, this PR changes the return value of the `Next` function (formerly `ReadAtCursor`) to return an Enum rather than the current system of 2 booleans. This greatly simplifies and unifies the code that deals with cursors as now there is no confusion as to what the function returns when there are no records left to be read.
Extracted from #24914
ACKs for top commit:
furszy:
diff ACK 4aebd83
theStack:
Code-review ACK 4aebd832a4
Tree-SHA512: 5d0be56a18de5b08c777dd5a73ba5a6ef1e696fdb07d1dca952a88ded07887b7c5c04342f9a76feb2f6fe24a45dc31f094f1f5d9500e6bdf4a44f4edb66dcaa1
f496528556 walletdb: refactor: drop unused `FindWalletTx` parameter and rename (Sebastian Falbesoner)
Pull request description:
Since commit 3340dbadd3 ("Remove -zapwallettxes"), the `FindWalletTx` helper is only needed to read tx hashes, so drop the other parameter and rename the method accordingly.
ACKs for top commit:
S3RK:
code review ACK f496528556
achow101:
ACK f496528556
vincenzopalazzo:
ACK https://github.com/bitcoin/bitcoin/pull/26702/commits/f496528556a67107d3d75d9c2ae345f7f4565d77
Tree-SHA512: ead85bc724462f9e920f9d7fe89679931361187579ffd6e63427c8bf5305cd5f71da24ed84f3b1bd22a12be46b5abec13f11822e71a3e1a63bf6cf49de950ab5
Next()'s result is a tri-state - failed, more to go, complete. Replace
the way that this is returned with an enum with values FAIL, MORE, and
DONE rather than with two booleans.
Since commit 3340dbadd3 ("Remove
-zapwallettxes"), the `FindWalletTx` helper is only needed to read tx
hashes, so drop the other parameter and rename the method accordingly.
3198e4239e test: check that loading descriptor wallet with legacy entries throws error (Sebastian Falbesoner)
349ed2a0ee wallet: throw error if legacy entries are present on loading descriptor wallets (Sebastian Falbesoner)
Pull request description:
Loading a descriptor wallet currently leads to a segfault if a legacy key type entry is present that can be deserialized successfully and needs SPKman-interaction. To reproduce with a "cscript" entry (see second commit for details):
```
$ ./src/bitcoin-cli createwallet crashme
$ ./src/bitcoin-cli unloadwallet crashme
$ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat
SQLite version 3.38.2 2022-03-26 13:51:10
Enter ".help" for usage hints.
sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00');
$ ./src/bitcoin-cli loadwallet crashme
--- bitcoind output: ---
2022-11-06T13:51:01Z Using SQLite Version 3.38.2
2022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme
2022-11-06T13:51:01Z init message: Loading wallet…
2022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900
Segmentation fault (core dumped)
```
Background: In the wallet key-value-loading routine, most legacy type entries require a `LegacyScriptPubKeyMan` instance after successful deserialization. On a descriptor wallet, creating that (via method `GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a null-pointer dereference crash. E.g. for CSCRIPT: 50422b770a/src/wallet/walletdb.cpp (L589-L594)
~~This PR fixes this by simply ignoring legacy entries if the wallet flags indicate that we have a descriptor wallet. The second commits adds a regression test to the descriptor wallet's functional test (fortunately Python includes sqlite3 support in the standard library).~~
~~Probably it would be even better to throw a warning to the user if unexpected legacy entries are found in descriptor wallets, but I think as a first mitigation everything is obvisouly better than crashing. As far as I'm aware, descriptor wallets created/migrated by Bitcoin Core should never end up in a state containing legacy type entries though.~~
This PR fixes this by throwing an error if legacy entries are found in descriptor wallets on loading.
ACKs for top commit:
achow101:
ACK 3198e4239e
aureleoules:
ACK 3198e4239e
Tree-SHA512: ee43da3f61248e0fde55d9a705869202cb83df678ebf4816f0e77263f0beac0d7bae9490465d1753159efb093ee37182931d76b2e2b6e8c6f8761285700ace1c
At wallet load time, we set the crypted key "checksum_valid" variable always to false.
Which, on every wallet decryption call, forces the process to re-write the entire ckeys to db when
it's not needed.
In the wallet key-value-loading routine, most legacy type entries
require a LegacyScriptPubKeyMan instance after successful
deserialization. On a descriptor wallet, creating that (via method
`GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a
null-pointer dereference crash. Fix this by throwing an error if
if the wallet flags indicate that we have a descriptor wallet and there
is a legacy entry found.
If the descriptor entry is unrecognized/corrupt, the unserialization fails and
`LoadWallet` instead of stop there and return the error, continues reading all
the db records. As other records tied to the unrecognized/corrupted descriptor
are scanned, a fatal error is thrown.
c318211ddd walletdb: fix last client version update (furszy)
bda8ebe608 wallet: don't read db every time that a new WalletBatch is created (furszy)
Pull request description:
Found it while was working on #25297.
We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not.
As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached.
ACKs for top commit:
achow101:
ACK c318211ddd
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/25383/commits/c318211ddd48d44dd81dded553afeee3bc41c89e
Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2
e673d8b475 bench: Enable loading benchmarks depending on what's compiled (Andrew Chow)
4af3547eba bench: Use mock wallet database for wallet loading benchmark (Andrew Chow)
49910f255f sqlite: Use in-memory db instead of temp for mockdb (Andrew Chow)
a1080802f8 walletdb: Create a mock database of specific type (Andrew Chow)
7c0d34476d bench: reduce the number of txs in wallet for wallet loading bench (Andrew Chow)
f85b54ed27 bench: Add transactions directly instead of mining blocks (Andrew Chow)
d94244c4bf bench: reduce number of epochs for wallet loading benchmark (Andrew Chow)
817c051364 bench: use unsafesqlitesync in wallet loading benchmark (Andrew Chow)
9e404a9831 bench: Remove minEpochIterations from wallet loading benchmark (Andrew Chow)
Pull request description:
`minEpochIterations` is probably unnecessary to set, so removing it makes the runtime much faster.
ACKs for top commit:
Rspigler:
tACK e673d8b475
furszy:
Code review ACK e673d8b4, nice PR.
glozow:
Concept ACK e673d8b475. For each commit, verified that there was a performance improvement without negating the purpose of the bench, and made some effort to verify that the code is correct.
Tree-SHA512: 9337352ef846cf18642d5c14546c5abc1674b4975adb5dc961a1a276ca91f046b83b7a5e27ea6cd26264b96ae71151e14055579baf36afae7692ef4029800877
The value was only being updated launching releases with higher version numbers
and not if the user launched a previous release.
Co-authored-by: MacroFake <falke.marco@gmail.com>
