| Age | Commit message (Collapse) | Author |
|---|
|
inconvenient backup filename
a951dba3a9d7663f009701140d663338e6c526a4 wallet: default wallet migration, modify inconvenient backup filename (furszy)
Pull request description:
Fixes #29584
On default legacy wallets, the backup filename starts with an "-" due to the wallet name being empty. This is inconvenient for systems who treat what follows the initial "-" character as flags.
Note:
As the user can freely set the wallet name to anything, we could also guard the backup filename against other inconvenient characters in the future (we need to be careful here, because the wallet name could also be a path).
ACKs for top commit:
achow101:
ACK a951dba3a9d7663f009701140d663338e6c526a4
brunoerg:
utACK a951dba3a9d7663f009701140d663338e6c526a4
vasild:
ACK a951dba3a9d7663f009701140d663338e6c526a4
Tree-SHA512: 6347bb12cfb50526a4baad96e4f1df9d82b493f79f0a0f7e0a1c8335a86a1e8e147c7b7f95cec6ede6f4507506a7eaf7972bd35131a2d5ed4cbbf38d94f0a9ca
|
|
d5228efb5391b31a9a0673019e43e7fa2cd4ac07 kernel: Remove dependency on CScheduler (TheCharlatan)
06069b3913dda048f5d640a662b0852f86346ace scripted-diff: Rename MainSignals to ValidationSignals (TheCharlatan)
0d6d2b650d1017691f48c9109a6cd020ab46aa73 scripted-diff: Rename SingleThreadedSchedulerClient to SerialTaskRunner (TheCharlatan)
4abde2c4e3fd9b66394b79874583bdc2a9132c36 [refactor] Make MainSignals RAII styled (TheCharlatan)
84f5c135b8118cbe15b8bfb4db80d61237987f64 refactor: De-globalize g_signals (TheCharlatan)
473dd4b97ae40e43e1a1a97fdbeb40be4855e9bc [refactor] Prepare for g_signals de-globalization (TheCharlatan)
3fba3d5deec6d7bae33823b8da7682f9b03d9deb [refactor] Make signals optional in mempool and chainman (TheCharlatan)
Pull request description:
By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design.
Removing `CScheduler` also allows removing the thread and exception modules from the kernel library.
To make the `CMainSignals` class easier to use from a kernel library perspective, remove its global instantiation and adopt RAII practices.
Renames `CMainSignals` to `ValidationSignals`, which more accurately describes its purpose and scope.
Also make the `ValidationSignals` in the `ChainstateManager` and CTxMemPool` optional. This could be useful in the future for using or testing these classes without having to instantiate any form of signal handling.
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587). It improves the kernel API and removes two modules from the kernel library.
ACKs for top commit:
maflcko:
re-ACK d5228efb5391b31a9a0673019e43e7fa2cd4ac07 🌄
ryanofsky:
Code review ACK d5228efb5391b31a9a0673019e43e7fa2cd4ac07. Just comment change since last review.
vasild:
ACK d5228efb5391b31a9a0673019e43e7fa2cd4ac07
furszy:
diff ACK d5228ef
Tree-SHA512: e93a5f10eb6182effb84bb981859a7ce750e466efd8171045d8d9e7fe46e4065631d9f6f533c5967c4d34c9bb7d7a67e9f4593bd4c5b30cd7b3bbad7be7b331b
|
|
On default legacy wallets, the backup filename starts with an "-" due
to the wallet name being empty. This is inconvenient for systems who
treat what follows the initial "-" character as flags.
|
|
`GetReservedDestination` fails
|
|
|
|
|
|
9d1dbbd4ceb8c04340927f5127195dc306adf3fc scripted-diff: Fix bitcoin_config_h includes (TheCharlatan)
Pull request description:
As mentioned in https://github.com/bitcoin/bitcoin/pull/26924#issuecomment-1403449932 and https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922334399, it is currently not safe to remove `bitcoin-config.h` includes from headers because some unrelated file might be depending on it.
See also #26972 for discussion.
Solve this by including the file directly everywhere it's required, regardless of whether or not it's already included by another header.
There should be no functional change here, but it will allow us to safely remove includes from headers in the future.
~I'm afraid it's a bit tedious to reproduce these commits, but it's reasonably straightforward:~
Edit: See note below
```bash
# All commands executed from the src/ subdir.
# Collect all tokens from bitcoin-config.h.in
# Isolate the tokens and remove blank lines
# Replace newlines with | and remove the last trailing one
# Collect all files which use these tokens
# Filter out subprojects (proper forwarding can be verified from Makefiles)
# Filter out .rc files
# Save to a text file
git grep -E -l `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '^$' | tr '\n' '|' | sed 's/|$//'` | grep -v -e "^leveldb/" -e "^secp256k1/" -e "^crc32c/" -e "^minisketch/" -e "^Makefile" -e "\.rc$" > files-with-config-include.txt
# Find all files from the above list which don't include bitcoin-config.h
git grep -L -E "config/bitcoin-config.h" -- `cat files-with-config-include.txt`
# Include them manually with the exception of some files in crypto:
# crypto/sha256_arm_shani.cpp crypto/sha256_avx2.cpp crypto/sha256_sse41.cpp crypto/sha256_x86_shani.cpp
# These are exceptions which don't use bitcoin-config.h, rather the Makefile.am adds these cppflags manually.
# Commit changes. This should match the first commit of this PR.
# Use the same search as above to find all files which DON'T use any config tokens
git grep -E -L `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '^$' | tr '\n' '|' | sed 's/|$//'` | grep -v -e "^leveldb/" -e "^secp256k1/" -e "^crc32c/" -e "^minisketch/" -e "^Makefile" -e "\.rc$" > files-without-config-include.txt
# Manually remove the includes and commit changes. This should match the second commit of this PR.
```
Edit: I'll keep this old description for posterity, but the manual approach has been replaced with a scripted diff from TheCharlatan
ACKs for top commit:
maflcko:
ACK 9d1dbbd4ceb8c04340927f5127195dc306adf3f 🚪
TheCharlatan:
ACK 9d1dbbd4ceb8c04340927f5127195dc306adf3fc
hebasto:
ACK 9d1dbbd4ceb8c04340927f5127195dc306adf3fc, I have reviewed the code and it looks OK.
fanquake:
ACK 9d1dbbd4ceb8c04340927f5127195dc306adf3fc
Tree-SHA512: f11ddc4ae6a887f96b954a6b77f310558ddb271088a3fda3edc833669c4251b7f392515224bbb8e5f67eb2c799b4ffed3b07d96454e82ec635c686d0df545872
|
|
performance of descriptor wallets
e041ed9b755468d205ed48b29f6c4b9e9df8bc9f wallet: Retrieve ID from loaded DescSPKM directly (Ava Chow)
39640dd34e980e69d13664ddbc2a7612a1888ab4 wallet: Use scriptPubKeyCache in GetSolvingProvider (Ava Chow)
b410f68791143800968f4c628beda1c9f898b4f6 wallet: Use scriptPubKey cache in GetScriptPubKeyMans (Ava Chow)
edf4e73a163739a64eb9a54cd95413583a0e5c1f wallet: Use scriptPubKey cache in IsMine (Ava Chow)
37232332bd7253422ea92a8c9eeb36b12fc84b56 wallet: Cache scriptPubKeys for all DescriptorSPKMs (Ava Chow)
99a0cddbc04e8bfea3748a6cb4c0107392fab94f wallet: Introduce a callback called after TopUp completes (Ava Chow)
b27682593266c99507c720855cb34f5f7d363dd2 bench: Add a benchmark for ismine (Ava Chow)
Pull request description:
Wallets that have a ton of non-ranged descriptors (such as a migrated non-HD wallet) perform fairly poorly due to looping through all of the wallet's `ScriptPubKeyMan`s. This is done in various places, such as `IsMine`, and helper functions for fetching a `ScriptPubKeyMan` and a `SolvingProvider`. This also has a bit of a performance impact on standard descriptor wallets, although less noticeable due to the small number of SPKMs.
As these functions are based on doing `IsMine` for each `ScriptPubKeyMan`, we can improve this performance by caching `IsMine` scriptPubKeys for all descriptors and use that to determine which `ScriptPubKeyMan` to actually use for those things. This cache is used exclusively and we no longer iterate the SPKMs.
Also added a benchmark for `IsMine`.
ACKs for top commit:
ryanofsky:
Code review ACK e041ed9b755468d205ed48b29f6c4b9e9df8bc9f. Just suggested changes since last review
josibake:
ACK https://github.com/bitcoin/bitcoin/pull/26008/commits/e041ed9b755468d205ed48b29f6c4b9e9df8bc9f
furszy:
Code review ACK e041ed9b
Tree-SHA512: 8e7081991a025e682e9dea838b4543b0d179832d1c47397fb9fe7a97fa01eb699c15a5d5a785634926844fc83a46e6ac07ef753119f39d84423220ef8a548894
|
|
Instead of iterating m_spk_managers a DescriptorSPKM has been loaded in
order to get it's ID to compare, have LoadDescriptorSPKM return a
reference to the loaded DescriptorSPKM so it can be queried directly.
|
|
|
|
|
|
|
|
Have CWallet maintain a cache of all known scriptPubKeys for its
DescriptorSPKMs in order to improve performance of the functions that
require searching for scriptPubKeys.
|
|
After TopUp completes, the wallet containing each SPKM will want to know
what new scriptPubKeys were generated. In order for all TopUp calls
(including ones internal the the SPKM), we use a callback function in
the WalletStorage interface.
|
|
f1684bb88a878eb3f54e945db39ea69b14256eef rpc: mention that migratewallet can take a while (Andrew Chow)
9ecff997e164e70c5958116b20ed441404ccd6f3 rpc: Drop migratewallet experimental warning (Andrew Chow)
Pull request description:
The migration process itself hasn't fundamentally changed since it was added, so I think it's reasonable to say that it is no longer experimental.
ACKs for top commit:
maflcko:
lgtm ACK f1684bb88a878eb3f54e945db39ea69b14256eef
josibake:
ACK https://github.com/bitcoin/bitcoin/pull/28037/commits/f1684bb88a878eb3f54e945db39ea69b14256eef
furszy:
ACK https://github.com/bitcoin/bitcoin/commit/f1684bb88a878eb3f54e945db39ea69b14256eef
ryanofsky:
Code review ACK f1684bb88a878eb3f54e945db39ea69b14256eef
willcl-ark:
ACK f1684bb88a878eb3f54e945db39ea69b14256eef
Tree-SHA512: 99b176cddbf3878c76bd4c80c030106200bf03139785e26dbae3341e1a675b623a13cd6dc7a0bb78344335bf859ae7548d97b2b58eb650c6e7b305d7cdc86e40
|
|
|
|
-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
|
|
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.
|
|
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.
|
|
|
