Age | Commit message (Collapse) | Author |
|
|
|
-BEGIN VERIFY SCRIPT-
regex_string='^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|CHAR_EQUALS_INT8|CLIENT_VERSION_BUILD|CLIENT_VERSION_IS_RELEASE|CLIENT_VERSION_MAJOR|CLIENT_VERSION_MINOR|COPYRIGHT_HOLDERS|COPYRIGHT_HOLDERS_FINAL|COPYRIGHT_HOLDERS_SUBSTITUTION|COPYRIGHT_YEAR|ENABLE_ARM_SHANI|ENABLE_AVX2|ENABLE_EXTERNAL_SIGNER|ENABLE_SSE41|ENABLE_TRACING|ENABLE_WALLET|ENABLE_X86_SHANI|ENABLE_ZMQ|HAVE_BOOST|HAVE_BUILTIN_CLZL|HAVE_BUILTIN_CLZLL|HAVE_BYTESWAP_H|HAVE_CLMUL|HAVE_CONSENSUS_LIB|HAVE_CXX20|HAVE_DECL_BE16TOH|HAVE_DECL_BE32TOH|HAVE_DECL_BE64TOH|HAVE_DECL_BSWAP_16|HAVE_DECL_BSWAP_32|HAVE_DECL_BSWAP_64|HAVE_DECL_FORK|HAVE_DECL_FREEIFADDRS|HAVE_DECL_GETIFADDRS|HAVE_DECL_HTOBE16|HAVE_DECL_HTOBE32|HAVE_DECL_HTOBE64|HAVE_DECL_HTOLE16|HAVE_DECL_HTOLE32|HAVE_DECL_HTOLE64|HAVE_DECL_LE16TOH|HAVE_DECL_LE32TOH|HAVE_DECL_LE64TOH|HAVE_DECL_PIPE2|HAVE_DECL_SETSID|HAVE_DECL_STRERROR_R|HAVE_DEFAULT_VISIBILITY_ATTRIBUTE|HAVE_DLFCN_H|HAVE_DLLEXPORT_ATTRIBUTE|HAVE_ENDIAN_H|HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR|HAVE_FDATASYNC|HAVE_GETENTROPY_RAND|HAVE_GETRANDOM|HAVE_GMTIME_R|HAVE_INTTYPES_H|HAVE_LIBADVAPI32|HAVE_LIBCOMCTL32|HAVE_LIBCOMDLG32|HAVE_LIBGDI32|HAVE_LIBIPHLPAPI|HAVE_LIBKERNEL32|HAVE_LIBOLE32|HAVE_LIBOLEAUT32|HAVE_LIBSHELL32|HAVE_LIBSHLWAPI|HAVE_LIBUSER32|HAVE_LIBUUID|HAVE_LIBWINMM|HAVE_LIBWS2_32|HAVE_MALLOC_INFO|HAVE_MALLOPT_ARENA_MAX|HAVE_MINIUPNPC_MINIUPNPC_H|HAVE_MINIUPNPC_UPNPCOMMANDS_H|HAVE_MINIUPNPC_UPNPERRORS_H|HAVE_NATPMP_H|HAVE_O_CLOEXEC|HAVE_POSIX_FALLOCATE|HAVE_PTHREAD|HAVE_PTHREAD_PRIO_INHERIT|HAVE_STDINT_H|HAVE_STDIO_H|HAVE_STDLIB_H|HAVE_STRERROR_R|HAVE_STRINGS_H|HAVE_STRING_H|HAVE_STRONG_GETAUXVAL|HAVE_SYSCTL|HAVE_SYSCTL_ARND|HAVE_SYSTEM|HAVE_SYS_ENDIAN_H|HAVE_SYS_PRCTL_H|HAVE_SYS_RESOURCES_H|HAVE_SYS_SELECT_H|HAVE_SYS_STAT_H|HAVE_SYS_SYSCTL_H|HAVE_SYS_TYPES_H|HAVE_SYS_VMMETER_H|HAVE_THREAD_LOCAL|HAVE_TIMINGSAFE_BCMP|HAVE_UNISTD_H|HAVE_VM_VM_PARAM_H|LT_OBJDIR|PACKAGE_BUGREPORT|PACKAGE_NAME|PACKAGE_STRING|PACKAGE_TARNAME|PACKAGE_URL|PACKAGE_VERSION|PTHREAD_CREATE_JOINABLE|QT_QPA_PLATFORM_ANDROID|QT_QPA_PLATFORM_COCOA|QT_QPA_PLATFORM_MINIMAL|QT_QPA_PLATFORM_WINDOWS|QT_QPA_PLATFORM_XCB|QT_STATICPLUGIN|STDC_HEADERS|STRERROR_R_CHAR_P|USE_ASM|USE_BDB|USE_DBUS|USE_NATPMP|USE_QRCODE|USE_SQLITE|USE_UPNP|_FILE_OFFSET_BITS|_LARGE_FILES)'
exclusion_files=":(exclude)src/minisketch :(exclude)src/crc32c :(exclude)src/secp256k1 :(exclude)src/crypto/sha256_arm_shani.cpp :(exclude)src/crypto/sha256_avx2.cpp :(exclude)src/crypto/sha256_sse41.cpp :(exclude)src/crypto/sha256_x86_shani.cpp"
git grep --perl-regexp --files-with-matches "$regex_string" -- '*.cpp' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do line_number=$(awk -v my_file="$file" '/\/\/ file COPYING or https?:\/\/www.opensource.org\/licenses\/mit-license.php\./ {line = NR} /^\/\// && NR == line + 1 {while(getline && /^\/\//) line = NR} END {print line+1}' "$file"); sed -i "${line_number}i\\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done;
git grep --perl-regexp --files-with-matches "$regex_string" -- '*.h' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do sed -i "/#define.*_H/a \\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done;
for file in $(git grep --files-with-matches 'bitcoin-config.h' -- '*.cpp' '*.h' $exclusion_files); do if ! grep -q --perl-regexp "$regex_string" $file; then sed -i '/HAVE_CONFIG_H/{N;N;N;d;}' $file; fi; done;
-END VERIFY SCRIPT-
The first command creates a regular expression for matching all bitcoin-config.h symbols in the following form: ^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|...|_LARGE_FILES). It was generated with:
./autogen.sh && printf '^(?!//).*(%s)' $(awk '/^#undef/ {print $2}' src/config/bitcoin-config.h.in | paste -sd "|" -)
The second command holds a list of files and directories that should not be processed. These include subtree directories as well as some crypto files that already get their symbols through the makefile.
The third command checks for missing bitcoin-config headers in .cpp files and adds the header if it is missing.
The fourth command checks for missing bitcoin-config headers in .h files and adds the header if it is missing.
The fifth command checks for unneeded bitcoin-config headers in sources files and removes the header if it is unneeded.
|
|
|
|
Transactions are intended to be started on upper layers rather than
internally by the bdb batch object. This enables us to consolidate
different write operations within a procedure in the same db txn,
improving consistency due to the atomic property of the transaction,
as well as its performance due to the reduction of disk write
operations.
Important Note:
This approach also ensures that the BerkeleyBatch::ErasePrefix
function behaves exactly as the SQLiteBatch::ErasePrefix function,
which does not create a db txn internally.
Furthermore, since the `BerkeleyBatch::ErasePrefix' implementation
erases records one by one (by traversing the db), this change
ensures that the function is always called within an active txn
context. Without this measure, there's a potential risk to consistency;
certain records may be removed while others could persist due to an
internal failure during the procedure.
|
|
'RunWithinTxn()' provides a way to execute db operations within a
transactional context. It avoids writing repetitive boilerplate code for
starting and committing the database transaction.
|
|
9a3c5c8697659e34d0476103af942a4615818f4e scripted-diff: rename ZapSelectTx to RemoveTxs (furszy)
83b762845f5804f23b63526d403b2f327fe99632 wallet: batch and simplify ZapSelectTx process (furszy)
595d50a1032ad7ffa9945464c86aa57f16665e93 wallet: migration, remove extra NotifyTransactionChanged call (furszy)
a2b071f9920c2f4893afcc43a152f593c03966bf wallet: ZapSelectTx, remove db rewrite code (furszy)
Pull request description:
Work decoupled from #28574. Brother of #28894.
Includes two different, yet interconnected, performance and code improvements to the zap wallet transactions process.
1) As the goal of the `ZapSelectTx` function is to erase tx records that match any of the inputted hashes. There is no need to traverse the whole database record by record. We could just check if the tx exist, and remove it directly by calling `EraseTx()`.
2) Instead of performing single write operations per removed tx record, this PR batches them all within a single atomic db txn.
Moreover, these changes will enable us to consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future.
ACKs for top commit:
achow101:
ACK 9a3c5c8697659e34d0476103af942a4615818f4e
josibake:
ACK https://github.com/bitcoin/bitcoin/pull/28987/commits/9a3c5c8697659e34d0476103af942a4615818f4e
Tree-SHA512: fb2ecc48224c400ab3b1fbb32e174b5b13bf03794717727f80f01f55fb183883b067a68c0a127b2de8885564da15425d021a96541953bf38a72becc2e9929ccf
|
|
|
|
when specified
|
|
13161ecf032b7a850686e5942c12222c8f3d0d52 opt: Skip over barren combinations of tiny UTXOs (Murch)
b7672c7cdd87acb105639f475744094b53cc9891 opt: Skip checking max_weight separately (Murch)
1edd2baa37a69ff69c2eaeceaad1028f1968cbab opt: Cut if last addition was minimal weight (Murch)
5248e2a60d243b3d5c77ecd9e4c335daca399a48 opt: Skip heavier UTXOs with same effective value (Murch)
9124c73742287b06dfe6e8a94be56ace25f07b2c opt: Tiebreak UTXOs by weight for CoinGrinder (Murch)
451be19dc10381122f705bbb2127b083f1d598c6 opt: Skip evaluation of equivalent input sets (Murch)
407b1e3432b77511900b77be84d5d7450352f462 opt: Track remaining effective_value in lookahead (Murch)
5f84f3cc043c5fb15072f5072fee752eaa01a2ec opt: Skip branches with worse weight (Murch)
d68bc74fb2e3ae4ae775ab544fe5b4ab46025abb fuzz: Test optimality of CoinGrinder (Murch)
67df6c629a2e9878b06c1903e90b67087eaa3688 fuzz: Add CoinGrinder fuzz target (Murch)
1502231229dbc32c94e80a2fc2959275c5d246ef coinselection: Track whether CG completed (Murch)
7488acc64685ae16e20b78e7ad018137f27fe404 test: Add coin_grinder_tests (Murch)
6cc9a46cd0f4ed80d4523bbef1508142e0c80d27 coinselection: Add CoinGrinder algorithm (Murch)
89d09566431f57034d9a7df32547ceb13d79c62c opt: Tie-break UTXO sort by waste for BnB (Murch)
aaee65823c6e620bef5cc96d8026567e64d822fe doc: Document max_weight on BnB (Murch)
Pull request description:
***Please refer to the [topic on Delving Bitcoin](https://delvingbitcoin.org/t/gutterguard-and-coingrinder-simulation-results/279) discussing Gutter Guard/Coingrinder simulation results.***
Adds a coin selection algorithm that minimizes the weight of the input set while creating change.
Motivations
---
- At high feerates, using unnecessary inputs can significantly increase the fees
- Users are upset when fees are relatively large compared to the amount sent
- Some users struggle to maintain a sufficient count of UTXOs in their wallet
Approach
---
So far, Bitcoin Core has used a balanced approach to coin selection, where it will generate multiple input set candidates using various coin selection algorithms and pick the least wasteful among their results, but not explicitly minimize the input set weight. Under some circumstances, we _do_ want to minimize the weight of the input set. Sometimes changeless solutions require many or heavy inputs, and there is not always a changeless solution for Branch and Bound to find in the first place. This can cause expensive transactions unnecessarily. Given a wallet with sufficient funds, `CoinGrinder` will pick the minimal-waste input set for a transaction with a change output. The current implementation only runs `CoinGrinder` at feerates over 3×long-term-feerate-estimate (by default 30 ṩ/vB), which may be a decent compromise between our goal to reduce costs for the users, but still permit transactions at lower feerates to naturally reduce the wallet’s UTXO pool to curb bloat.
Trade-offs
---
Simulations for my thesis on coin selection ([see Section 6.3.2.1 [PDF]](https://murch.one/erhardt2016coinselection.pdf)) suggest that minimizing the input set for all transactions tends to grind a wallet’s UTXO pool to dust (pun intended): an approach selecting inputs per coin-age-priority (in effect similar to “largest first selection”) on average produced a UTXO pool with 15× the UTXO count as Bitcoin Core’s Knapsack-based Coin Selection then (in 2016). Therefore, I do not recommend running `CoinGrinder` under all circumstances, but only at extreme feerates or when we have another good reason to minimize the input set for other reasons. In the long-term, we should introduce additional metrics to score different input set candidates, e.g. on basis of their privacy and wallet health impact, to pick from all our coin selection results, but until then, we may want to limit use of `CoinGrinder` in other ways.
ACKs for top commit:
achow101:
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52
sr-gi:
ACK [13161ec](https://github.com/bitcoin/bitcoin/pull/27877/commits/13161ecf032b7a850686e5942c12222c8f3d0d52)
sipa:
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52
Tree-SHA512: 895b08b2ebfd0b71127949b7dba27146a6d10700bf8590402b14f261e7b937f4e2e1b24ca46de440c35f19349043ed2eba4159dc2aa3edae57721384186dae40
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/ZapSelectTx/RemoveTxs/g' $(git grep -l 'ZapSelectTx' ./src/wallet)
-END VERIFY SCRIPT-
|
|
The goal of the function is to erase the wallet transactions that
match the inputted hashes. There is no need to traverse the database,
reading record by record, to then perform single entry removals for
each of them.
To ensure consistency and improve performance, this change-set removes
all tx records within a single atomic db batch operation, as well as
it cleans up code, improves error handling and simplifies the
transactions removal process entirely.
This optimizes the removal of watch-only transactions during the wallet
migration process and the 'removeprunedfunds' RPC command.
|
|
This deduplicates code and leads to more consistent and detailed error
messages. Affected are legacy import RPCs (`importpubkey`,
`importmulti`) and other ones where solving data can be provided
(`fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendall`).
|
|
Given a lot of small amount UTXOs it is possible that the lookahead
indicates sufficient funds, but any combination of them would push us
beyond the current best_weight.
We can estimate a lower bound for the minimal necessary weight to reach
target from the maximal amount and minimal weight in the tail of the
UTXO pool: if adding a number of hypothetical UTXOs of this maximum
amount and minimum weight would not be able to beat `best_weight`, we
can SHIFT to the omission branch, and CUT if the last selected UTXO is
not heavier than the minimum weight of the remainder.
|
|
Initialize `best_selection_weight` as `max_weight` allows us to skip the
separate `max_weight` check on every loop.
|
|
In situations where we have UTXO groups of various weight, we can CUT
rather than SHIFT when we exceeded the max_weight or the best
selection’s weight while the last step was equal to the minimum weight
in the lookahead.
|
|
When two successive UTXOs differ in weight but match in effective value,
we can skip the second if the first is not selected, because all input
sets we can generate by swapping out a lighter UTXOs with a heavier UTXO
of matching effective value would be strictly worse.
|
|
|
|
When two successive UTXOs match in effective value and weight, we can
skip the second if the prior is not selected: adding it would create an
equivalent input set to a previously evaluated.
E.g. if we have three UTXOs with effective values {5, 3, 3} of the same
weight each, we want to evaluate
{5, _, _}, {5, 3, _}, {5, 3, 3}, {_, 3, _}, {_, 3, 3},
but skip {5, _, 3}, and {_, _, 3}, because the first 3 is not selected,
and we therefore do not need to evaluate the second 3 at the same
position in the input set.
If we reach the end of the branch, we must SHIFT the previously selected
UTXO group instead.
|
|
Introduces a dedicated data structure to track the total
effective_value available in the remaining UTXOs at each index of the
UTXO pool. In contrast to the approach in BnB, this allows us to
immediately jump to a lower index instead of visiting every UTXO to add
back their eff_value to the lookahead.
|
|
Once we exceed the weight of the current best selection, we can always
shift as adding more inputs can never yield a better solution.
|
|
Co-authored-by: Pieter Wuille <pieter@wuille.net>
|
|
|
|
CoinGrinder may not be able to exhaustively search all potentially
interesting combinations for large UTXO pools, so we keep track of
whether the search was terminated by the iteration limit.
|
|
|
|
CoinGrinder is a DFS-based coin selection algorithm that
deterministically finds the input set with the lowest weight creating a
change output.
|
|
migration process
86960cdb7f75eaa2ae150914c54240d1d5ef96d1 wallet: migration, batch addressbook records removal (furszy)
342c45f80e32b0320829ce380b5854844cd74bc8 wallet: addressbook migration, batch db writes (furszy)
595bbe6e81885d35179aba6137dc63d0e652cc1f refactor: wallet, simplify addressbook migration (furszy)
d0943315b1d00905fe7f4513b2f3f47b88a99e8f refactor: SetAddressBookWithDB, minimize number of map lookups (furszy)
bba4f8dcb55de3ca4963711dc17882b43cb0bc4a refactor: SetAddrBookWithDB, signal only if write succeeded (furszy)
97b075392305becfbad4d497614478cff2d9237f wallet: clean redundancies in DelAddressBook (furszy)
Pull request description:
Commits decoupled from #28574, focused on the address book cloning process
Includes:
1) DB batch operations and flow simplification for the address book migration process.
2) Code improvements to `CWallet::DelAddressBook` and `Wallet::SetAddrBookWithDB` methods.
These changes will let us consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future.
ACKs for top commit:
achow101:
ACK 86960cdb7f75eaa2ae150914c54240d1d5ef96d1
josibake:
reACK https://github.com/bitcoin/bitcoin/commit/86960cdb7f75eaa2ae150914c54240d1d5ef96d1
Tree-SHA512: 10c941df3cd84fd8662b9c9ca6a1ed2c7402d38c677d2fc66b8b6c9edc6d73e827a5821487bbcacb5569d502934fa548fd10699e2ec45185f869e43174d8b2a1
|
|
`SQLiteBatch`s
cfcb9b1ecf9be5267487713fa1e112ca09a1ae55 test: wallet, coverage for concurrent db transactions (furszy)
548ecd11553bea28bc79e6f9840836283e9c4e99 tests: Test for concurrent writes with db tx (Ava Chow)
395bcd245476d7c83abf2d82e31e90b6d0c432f3 sqlite: Ensure that only one SQLiteBatch is writing to db at a time (Ava Chow)
Pull request description:
The way that we have configured SQLite to run means that only one database transaction can be open at a time. Typically, each individual read and write operation will be its own transaction that is opened and committed automatically by SQLite. However, sometimes we want these operations to be batched into a multi-statement transaction, so `SQLiteBatch::TxnBegin`, `SQLiteBatch::TxnCommit`, and `SQLiteBatch::TxnAbort` are used to manage the transaction of the database.
However, once a db transaction is begun with one `SQLiteBatch`, any operations performed by another `SQLiteBatch` will also occur within the same transaction. Furthermore, those other `SQLiteBatch`s will not be expecting a transaction to be active, and will abort it once the `SQLiteBatch` is destructed. This is problematic as it will prevent some data from being written, and also cause the `SQLiteBatch` that opened the transaction in the first place to be in an unexpected state and throw an error.
To avoid this situation, we need to prevent the multiple batches from writing at the same time. To do so, I've implemented added a `CSemaphore` within `SQLiteDatabase` which will be used by any `SQLiteBatch` trying to do a write operation. `wait()` is called by `TxnBegin`, and at the beginning of `WriteKey`, `EraseKey`, and `ErasePrefix`. `post()` is called in `TxnCommit`, `TxnAbort` and at the end of `WriteKey`, `EraseKey`, and `ErasePrefix`. To avoid deadlocking on ` TxnBegin()` followed by a `WriteKey()`, `SQLiteBatch will now also track whether a transaction is in progress so that it knows whether to use the semaphore.
This issue is not a problem for BDB wallets since BDB uses WAL and provides transaction objects that must be used if an operation is to occur within a transaction. Specifically, we either pass a transaction pointer, or a nullptr, to all BDB operations, and this allows for concurrent transactions so it doesn't have this problem.
Fixes #29110
ACKs for top commit:
josibake:
ACK https://github.com/bitcoin/bitcoin/pull/29112/commits/cfcb9b1ecf9be5267487713fa1e112ca09a1ae55
furszy:
ACK cfcb9b1ecf9be5267487713fa1e112ca09a1ae55
ryanofsky:
Code review ACK cfcb9b1ecf9be5267487713fa1e112ca09a1ae55. This looks great and I think it is ready for merge. Just holding off because josibake seemed ready to review https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1930372190 and might have more feedback.
Tree-SHA512: 2dd5a8e76df52451a40e0b8a87c7139d68a0d8e1bf2ebc79168cc313e192dab87cfa4270ff17fea4f7b370060d3bc9b5d294d50f7e07994d9b5a69b40397c927
|
|
Instead of doing one db transaction per removed record,
we now batch all removals in a single db transaction.
Speeding up the process and preventing the wallet from entering
an inconsistent state when any of the intermediate writes fail.
|
|
Optimizing the process performance and consistency.
|
|
Same process written in a cleaner manner.
Removing code duplication.
|
|
|
|
|
|
1) Encode destination only once (instead of three).
2) Fail if the entry's linked data cannot be removed.
3) Don't remove entry from memory if db write fail.
4) Notify GUI only if removal succeeded
|
|
`FuzzedWallet`
b14298c5bca395e5ed6a27fe1758c0d1f4b824ac fuzz: remove unused `args` and `context` from `FuzzedWallet` (brunoerg)
Pull request description:
`ArgsManager args` and `WalletContext context` were previously used to create the wallet into `FuzzedWallet`. After fa15861763df71e788849b587883b3c16bb12229, they are not used anymore. This PR removes them.
ACKs for top commit:
maflcko:
lgtm ACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac
epiccurious:
utACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac
ryanofsky:
Code review ACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac
Tree-SHA512: 164e6a66ba05e11176a0cf68db6257f0ac07459cf7aa01ec4302b303c156c205a68128373a0b8daba0a6dfbff990af7fa14465a6341a296312fb20ea778c7a8c
|
|
instances in spkm's `FillPSBT` methods
e2ad343f69af4f924b22dccf94a52b6431ef6e3c wallet: remove unused `SignatureData` instances in spkm's `FillPSBT` methods (Sebastian Falbesoner)
Pull request description:
These are filled with signature data from a PSBT input, but not used anywhere after, hence they can be removed. Note that the same code is in the `SignPSBTInput` function where the `sigdata` result is indeed used.
ACKs for top commit:
achow101:
ACK e2ad343f69af4f924b22dccf94a52b6431ef6e3c
brunoerg:
crACK e2ad343f69af4f924b22dccf94a52b6431ef6e3c
Tree-SHA512: f0cabcc000bcea6bc7d7ec9d3be2e2a8accbdbffbe35252250ea2305b65a5813fde2b8096fbdd2c7cccdf417ea285183dc325fc2d210d88bce62978ce642930e
|
|
Verifying that a database handler can't commit/abort changes
occurring in a different database handler.
|
|
A SQLiteBatch need to wait for any other batch to finish writing before
it can begin writing, otherwise db txn state may be incorrectly
modified. To enforce this, each SQLiteDatabase has a semaphore which
acts as a lock and is acquired by a batch when it begins a write, erase,
or a transaction, and is released by it when it is done.
To avoid deadlocking on itself for writing during a transaction,
SQLiteBatch also keeps track of whether it has begun a transaction.
|
|
|
|
The wallet decryption process (CheckDecryptionKey() and Unlock())
contains an arg 'accept_no_keys,' introduced in #13926, that has
never been used.
Additionally, this also removes the unimplemented SplitWalletPath
function.
|
|
have both spendable and watchonly outputs
4da76ca24725eb9ba8122317e04a6e1ee14ac846 test: Test migration of tx with both spendable and watchonly (Ava Chow)
c62a8d03a862fb124b4f4b88efd61978e46605f8 wallet: Keep txs that belong to both watchonly and migrated wallets (Ava Chow)
71cb28ea8cb579ac04cefc47a57557c94080d1af test: Make sure that migration test does not rescan on reloading (Ava Chow)
78ba0e6748d2a519a96c41dea851e7c43b82f251 wallet: Reload the wallet if migration exited early (Ava Chow)
9332c7edda79a39bb729b71b6f8db6a9d37343bb wallet: Write bestblock to watchonly and solvable wallets (Ava Chow)
Pull request description:
A transaction does not necessarily have to belong to either the migrated wallet (with the private keys) and the watchonly wallet (with watchonly things), it could have multiple outputs with each isminetype. So we should be putting such transactions in one or the other wallet, but rather putting it in both.
I've added a test for this behavior, however the test also revealed a few other issues. Notably, it revealed that `migratewallet` would have the watchonly wallet rescan from genesis when it is reloaded at the end of migration. This could be a cause for migration appearing to be very slow. This is resolved by first writing best block records to the watchonly and solvable wallets, as well as updating the test to make sure that rescans don't happen.
The change to avoid rescans also found an issue where some of our early exits would result in unloading the wallet even though nothing happened. So there is also a commit to reload the wallet for such early exits.
ACKs for top commit:
ryanofsky:
Code review ACK 4da76ca24725eb9ba8122317e04a6e1ee14ac846. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage.
furszy:
Code review ACK 4da76ca2
Tree-SHA512: 5fc210cff16ca6720d7b2d0616d7e3f295c974147854abc704cf99a3bfaad17572ada084859e7a1b1ca94da647ad130303219678f429b7995f85e040236db35c
|
|
|
|
It is possible for a transaction that has an output that belongs to the
mgirated wallet, and another output that belongs to the watchonly
wallet. Such transaction should appear in both wallets during migration.
|
|
Migration will unload loaded wallets prior to beginning. It will then
perform some checks which may exit early. Such unloaded wallets should
be reloaded prior to exiting.
|
|
When migrating, we should also be writing the bestblock record to the
watchonly and solvable wallets to avoid rescanning on the reload as that
can be slow.
|
|
c11c404281d2d0e22967e30e16c0733db84f4eee tests: Test migration of blank wallets (Andrew Chow)
563b2a60d6a371f26474410397da435547e58786 wallet: Better error message when missing LegacySPKM during migration (Andrew Chow)
b1d2c771d43b802db12768e620588ed179e92b29 wallet: Check for descriptors flag before migration (Andrew Chow)
8c127ff1edb6b9a607bf1ad247893347252a38e3 wallet: Skip key and script migration for blank wallets (Andrew Chow)
Pull request description:
Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a `LegacyScriptPubKeyMan` which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets.
Fixes the issue mentioned in https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809641110
ACKs for top commit:
furszy:
reACK c11c404281d2d0e22967e30e16c0733db84f4eee. CI failure is unrelated.
ryanofsky:
Code review ACK c11c404281d2d0e22967e30e16c0733db84f4eee
Tree-SHA512: 2466fdf1542eb8489c841253191f85dc88365493f0bb3395b67dee3e43709a9993c68b9d7623657b54b779adbe68fc81962d60efef4802c5d461f154167af7f4
|
|
db transactions
b298242c8d495c36072415e1b95eaa7bf485a38a test: sqlite, add coverage for dangling to-be-reverted db txns (furszy)
fc0e747192e98e779c5f31f2df808f62b3fdd071 sqlite: guard against dangling to-be-reverted db transactions (furszy)
472d2ca98170049e0edec830e2d11c5ef23740a4 sqlite: introduce HasActiveTxn method (furszy)
dca874e838c2599bd24433675b291168f8e7b055 sqlite: add ability to interrupt statements (furszy)
fdf9f66909a354a95f4b7c5f092f0e9fbe1baa7c test: wallet db, exercise deadlock after write failure (furszy)
Pull request description:
Discovered while was reviewing #29112, specifically https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1821862931.
If the db handler that initiated the database transaction is destroyed,
the ongoing transaction cannot be left dangling when the db txn fails
to abort. It must be forcefully reverted; otherwise, any subsequent
db handler executing a write operation will dump the dangling,
to-be-reverted transaction data to disk.
This not only breaks the isolation property but also results in the
improper storage of incomplete information on disk, impacting
the wallet consistency.
This PR fixes the issue by resetting the db connection, automatically
rolling back the transaction (per https://www.sqlite.org/c3ref/close.html)
when the handler object is being destroyed and the txn abortion failed.
Testing Notes
Can verify the failure by reverting the fix e5217fea and running the test.
It will fail without e5217fea and pass with it.
ACKs for top commit:
achow101:
ACK b298242c8d495c36072415e1b95eaa7bf485a38a
ryanofsky:
Code review ACK b298242c8d495c36072415e1b95eaa7bf485a38a. Just fix for exec result codes and comment update since last review.
Tree-SHA512: 44ba0323ab21440e79e9d7791bc1c56a8873c8bd3e8f6a85641b91576e1293011fa8032d8ae5b0580f3fb7a949356f7b9676693d7ceffa617aaad9f6569993eb
|
|
|
|
If the handler that initiated the database transaction is destroyed,
the ongoing transaction cannot be left dangling when the db txn fails
to abort. It must be forcefully reversed; otherwise, any subsequent
db handler executing a write operation will dump the dangling,
to-be-reverted transaction data to disk.
This not only breaks the database isolation property but also results
in the improper storage of incomplete information on disk, impacting
the wallet consistency.
|
|
Util function to clean up code and let us
verify, in the following-up commit, that dangling,
to-be-reverted db transactions cannot occur anymore.
|
|
By encapsulating sqlite3_exec into its own standalone method
and introducing the 'SQliteExecHandler' class, we enable the
ability to test db statements execution failures within the
unit test framework.
This is used in the following-up commit to exercise a deadlock
and improve our wallet db error handling code.
Moreover, the future encapsulation of other sqlite functions
within this class will contribute to minimize the impact of
any future API changes.
|