71656bdfaa gui: crash fix, disconnect numBlocksChanged() signal during shutdown (furszy)
Pull request description:
Aiming to fixbitcoin-core/gui#862.
The crash stems from the order of the shutdown procedure:
We first unset the client model, then destroy the wallet controller—but we leave
the internal wallet models (`m_wallets`) untouched for a brief period. As a result,
there’s a point in time where views still have connected signals and access to
wallet models that are not connected to any wallet controller.
Now.. since the `clientModel` is only replaced with nullptr locally and not destroyed
yet, signals like `numBlocksChanged` can still emit. Thus, when wallet views receive
them, they see a non-null wallet model ptr, and proceed to call backend functions
from a model that is being torn down.
As the shutdown procedure begins by unsetting `clientModel` from all views. It’s safe
to ignore events when `clientModel` is nullptr.
ACKs for top commit:
maflcko:
lgtm ACK 71656bdfaa
pablomartin4btc:
re-ACK 71656bdfaa
hebasto:
ACK 71656bdfaa, I have reviewed the code and it looks OK.
Tree-SHA512: e6a369c40aad8a5a3da64e92daa10250006f60c53feef353a5580e1bdb17fe8e1ad102abf5419ddeff1caa703b69ab634265ef3b9cfef87e9304f97bfdd2c4aa
edd46566bd qt: Replace stray tfm::format to cerr with qWarning (laanwj)
Pull request description:
GUI warnings should go to the log, not to the console (which may not be connected at all).
ACKs for top commit:
hebasto:
ACK edd46566bd, I have reviewed the code and it looks OK.
Tree-SHA512: 32944e00dae0c62bb23e3d7abd486b63e445702483ca03c74c3057ef942f06e771d4d3d3a58fd728582889d6b638fae11ecc536a25febfd89a28522b7d6d08ba
The crash stems from the order of the shutdown procedure:
We first unset the client model, then destroy the wallet controller—but we leave
the internal wallet models ('m_wallets') untouched for a brief period. As a result,
there’s a point in time where views still have connected signals and access to
wallet models that are not connected to any wallet controller.
Now.. since the clientModel is only replaced with nullptr locally and not destroyed
yet, signals like numBlocksChanged can still emit. Thus, when wallet views receive
them, they see a non-null wallet model ptr, and proceed to call backend functions
from a model that is being torn down.
As the shutdown procedure begins by unsetting clientModel from all views. It’s safe
to ignore events when clientModel is nullptr.
55b931934a removed duplicate calling of GetDescriptorScriptPubKeyMan (Saikiran)
Pull request description:
Removed duplicate call to GetDescriptorScriptPubKeyMan and
Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan
after this fix improved performance of importdescriptor part refs https://github.com/bitcoin/bitcoin/issues/32013.
**Steps to reproduce in testnet environment**
**Input size:** 2 million address in the wallet
**Step1:** call importaddresdescriptor rpc method
observe the time it has taken.
**With the provided fix:**
Do the same steps again
observe the time it has taken.
There is a huge improvement in the performance. (previously it may take 5 to 6 seconds now it will take 1 seconds or less)
main changes i've made during this pr:
1. remove duplicate call to GetDescriptorScriptPubKeyMan method
2. And inside GetDescriptorScriptPubKeyMan method previously we checking **each address linearly** so each time it is calling HasWallet method which has aquired lock.
3. Now i've modified this logic call **find method on the map (O(logn)**) time it is taking, so only once we calling HasWallet method.
**Note:** Smaller inputs in the wallet you may not see the issue but huge wallet size it will definitely impact the performance.
ACKs for top commit:
achow101:
ACK 55b931934a
w0xlt:
ACK 55b931934a
Tree-SHA512: 4a7fdbcbb4e55bd034e9cf28ab4e7ee3fb1745fc8847adb388c98a19c952a1fb66d7b54f0f28b4c2a75a42473923742b4a99fb26771577183a98e0bcbf87a8ca
fa86190e6e rpc: Allow fullrbf fee bump (MarcoFalke)
Pull request description:
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.
This is done after fullrbf is always on (https://github.com/bitcoin/bitcoin/pull/30592)
ACKs for top commit:
1440000bytes:
reACK fa86190e6e
achow101:
ACK fa86190e6e
w0xlt:
ACK fa86190e6e
rkrux:
reACK fa86190e6e
glozow:
ACK fa86190e6e
Tree-SHA512: b2ffe8dcadbe71e9be767a16cf8aa0bf383c2de7aa1aee9438d125f444e24f3f7e4f02ddb28981bd3b8b645b6a24a407b4ad6bb0b21946ae637e78f6386e05bf
Also, fix the incorrect documention of the 'replaceable' RPC argument
with respect to sequence number handling. The docs were incorrect
before, so the fix could be extracted, but it seems fine to include here
as well.
babb9f5db6 depends: remove non-native libmultiprocess build (Cory Fields)
5d105fb8c3 depends: Switch libmultiprocess packages to use local git subtree (Ryan Ofsky)
9b35518d2f depends, moveonly: split up int_get_build_id function (Ryan Ofsky)
2d373e2707 lint: Add exclusions for libmultiprocess subtree (Ryan Ofsky)
e88ab394c1 doc: Update documentation to explain libmultiprocess subtree (Ryan Ofsky)
d4bc563982 cmake: Fix clang-tidy "no input files" errors (Ryan Ofsky)
abdf3cb645 cmake: Fix warnings from boost headers (Ryan Ofsky)
8532fcb1c3 cmake: Fix ctest mptest "Unable to find executable" errors (Ryan Ofsky)
d597ab1dee cmake: Support building with libmultiprocess subtree (Ryan Ofsky)
69f0d4adb7 scripted-diff: s/WITH_MULTIPROCESS/ENABLE_IPC/ in cmake (Ryan Ofsky)
a2f28e4be9 Squashed 'src/ipc/libmultiprocess/' content from commit 35944ffd23fa (Ryan Ofsky)
d6244f85c5 depends: Update libmultiprocess library to simplify cmake subtree build (Ryan Ofsky)
Pull request description:
This adds the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library and code generator as a subtree in `src/ipc/libmultiprocess` and allows it to be built with the cmake `-DENABLE_IPC` option, which is disabled by default.
This PR does not entirely remove the depends system [libmultiprocess package](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/native_libmultiprocess.mk) because the package is useful when cross compiling. (A cross-compiling cmake build cannot easily build and run a native code generation tool.) However, it does update the depends package to build from the new git subtree, instead of being downloaded separately from github, so the same sources are used to build both the runtime library and the code generator.
This PR includes the following manual changes (not created automatically with `git subtree add`) which just update the build system and documentation:
- [`d6244f85c509` depends: Update libmultiprocess library to simplify cmake subtree build](d6244f85c5)
- [`69f0d4adb72c` scripted-diff: s/WITH_MULTIPROCESS/ENABLE_IPC/ in cmake](69f0d4adb7)
- [`d597ab1dee6b` cmake: Support building with libmultiprocess subtree](d597ab1dee)
- [`8532fcb1c30d` cmake: Fix ctest mptest "Unable to find executable" errors](8532fcb1c3)
- [`abdf3cb6456f` cmake: Fix warnings from boost headers](abdf3cb645)
- [`d4bc5639829f` cmake: Fix clang-tidy "no input files" errors](d4bc563982)
- [`e88ab394c163` doc: Update documentation to explain libmultiprocess subtree](e88ab394c1)
- [`2d373e27071f` lint: Add exclusions for libmultiprocess subtree](2d373e2707)
- [`9b35518d2f3f` depends, moveonly: split up int_get_build_id function](9b35518d2f)
- [`5d105fb8c3ff` depends: Switch libmultiprocess packages to use local git subtree](5d105fb8c3)
- [`babb9f5db641` depends: remove non-native libmultiprocess build](babb9f5db6)
---
Previous minisketch subtree PR #23114 may be useful for comparison
Instructions for subtree verification can be found:
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees
- https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh
TL&DR:
```sh
git remote add --fetch libmultiprocess https://github.com/chaincodelabs/libmultiprocess.git
test/lint/git-subtree-check.sh -r src/ipc/libmultiprocess
```
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
Sjors:
re-ACK babb9f5db6
TheCharlatan:
tACK babb9f5db6
vasild:
ACK babb9f5db6
Tree-SHA512: 43d4eecca5aab63e55c613de935965666eaced327f9fe859a0e9c9b85f7685dc16c5c8d6e03e09ca998628c5d468633f4f743529930b037049abe8e0101e0143
ec81a72b36 net: Add randomized prefix to Tor stream isolation credentials (laanwj)
c47f81e8ac net: Rename `_randomize_credentials` Proxy parameter to `tor_stream_isolation` (laanwj)
Pull request description:
Add a class TorsStreamIsolationCredentialsGenerator that generates unique credentials based on a randomly generated session prefix and an atomic counter. Use this in `ConnectThroughProxy` instead of a simple atomic int counter.
This makes sure that different launches of the application won't share the same credentials, and thus circuits, even in edge cases.
Example with `-debug=proxy`:
```
2025-03-31T16:30:27Z [proxy] SOCKS5 sending proxy authentication 0afb2da441f5c105-0:0afb2da441f5c105-0
2025-03-31T16:30:31Z [proxy] SOCKS5 sending proxy authentication 0afb2da441f5c105-1:0afb2da441f5c105-1
```
Thanks to hodlinator in https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020973352 for the idea.
ACKs for top commit:
hodlinator:
re-ACK ec81a72b36
jonatack:
ACK ec81a72b36
danielabrozzoni:
tACK ec81a72b36
Tree-SHA512: 195f5885fade77545977b91bdc41394234ae575679cb61631341df443fd8482cd74650104e323c7dbfff7826b10ad61692cca1284d6810f84500a3488f46597a
Replace `uint256::SetHexDeprecated()` calls with `Txid::FromHex()`
in four locations:
- TransactionTableModel::updateTransaction
- TransactionView::contextualMenu
- TransactionView::abandonTx
- TransactionView::bumpFee
The input strings are generally expected to be valid hex strings
from `GetHex()`. However, due to the potentially unpredictable return
value of `.data(TransactionTableModel::TxHashRole)`, check the
`Txid::FromHex` result in `contextualMenu` and return early if the
transaction hash is invalid. The other two functions, `abandonTx`
and `bumpFee` will only be called if the context menu is enabled.
Rename WITH_MULTIPROCESS to ENABLE_IPC, because ENABLE_IPC is a more accurate
name for the feature. It controls whether the src/ipc/ directory is built and
whether IPC features like -ipcbind, -ipcconnect, and -ipcfd are available. It
does NOT currently enable multiprocess features which are implemented in #10102
building on top of the IPC features. It will also no longer (as of the next
commit), control whether a find_package call is made so the "WITH_" prefix is
also inappropriate.
-BEGIN VERIFY SCRIPT-
git grep -l WITH_MULTIPROCESS | xargs sed -i s/WITH_MULTIPROCESS/ENABLE_IPC/g
-END VERIFY SCRIPT-
Rename the `_randomize_credentials` parameter to Proxy's constructor to
`tor_stream_isolation` to make it more clear, and more specific what its
purpose is.
Also change all call sites to use a named parameter.
Checking for IsArgSet before calling GetArg while providing an arbitrary
default value as fallback is both confusing and fragile.
It is confusing, because the provided fallback is dead code. So it would
be better to just call GetArg without a fallback.
Even better would be to provide the true fallback value and sanitize it
as if it were user-input, but this can be done in a follow-up.
Removing the redundant call to IsArgSet will have to be done either way,
so do it now.
Removed duplicate call to GetDescriptorScriptPubKeyMan and
Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan
after this fix improved performance of importdescriptor part refs #32013.
Require that sqlite is available in order to compile the wallet. Removes
instances of USE_SQLITE since it is no longer possible to not have
sqlite available.
The NO_SQLITE option is dropped from depends.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
568fcdddae scripted-diff: Adjust documentation per top-level target output location (Hennadii Stepanov)
026bb226e9 cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
This PR sets the target output locations to the `bin` and `lib` subdirectories within the build tree, creating a directory structure that mirrors that of the installed targets.
This approach is widely adopted by the large projects, such as [LLVM](e146c1867e/lldb/cmake/modules/LLDBStandalone.cmake (L128-L130)):
```cmake
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
```
The `libsecp256k1` project has also recently [adopted](https://github.com/bitcoin-core/secp256k1/pull/1553) this approach.
With this PR, all binaries are conveniently located. For example, run:
```
$ ./build/bin/fuzz
```
instead of:
```
$ ./build/src/test/fuzz/fuzz
```
On Windows, all required DLLs are now located in the same directory as the executables, allowing to run `bitcoin-chainstate.exe` (which loads `bitcoinkernel.dll`) without the need to copy DLLs or modify the `PATH` variable.
The idea was briefly discussed among the build team during the recent CoreDev meeting.
---
**Warning**: This PR changes build locations of newly built executables like `bitcoind` and `test_bitcoin` from `src/` to `bin/` without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.
ACKs for top commit:
theStack:
Light re-ACK 568fcdddae
ryanofsky:
Code review ACK 568fcdddae. Only change since last review was rebasing. I'm ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to \*silently\* break an everyday developer workflow like `git pull; make bitcoind`. I wouldn't have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
TheCharlatan:
Re-ACK 568fcdddae
theuni:
ACK 568fcdddae
Tree-SHA512: 1aa5ecd3cd49bd82f1dcc96c8e171d2d19c58aec8dade4bc329df89311f9e50cbf6cf021d004c58a0e1016c375b0fa348ccd52761bcdd179c2d1e61c105e3b9f
The translations for the following languages, which appear to be the
result of a mistake or an act of vandalism, have been discarded:
- Czech (cs)
- Danish (da)
- Dutch (nl)
Changes to the Thai (th) translation have been discarded due to multiple
unsolicited pronunciation notes.
Some sources might be generated, and while they likely do not contain
any translatable strings, this change generalizes the approach to
include generated sources in the translation process as well.
3e97ff9c5e gui, psbt: Use SIGHASH_DEFAULT when signing PSBTs (Ava Chow)
Pull request description:
SIGHASH_DEFAULT should be used to indicate SIGHASH_DEFAULT for taproot inputs, and SIGHASH_ALL for all other input types. This avoids adding an unnecessary byte to the end of all Taproot signatures added to PSBTs signed in the GUI.
See also bitcoin/bitcoin#22514
ACKs for top commit:
Sjors:
utACK 3e97ff9c5e
pablomartin4btc:
utACK 3e97ff9c5e
hebasto:
ACK 3e97ff9c5e, I have reviewed the code and it looks OK.
Tree-SHA512: f96f26b3a6959865cf23039afb5ffb7e454fb52ee39c510583851caf00a8a383cde69bc7e90db536addbdd498a02f4b001cbaf509d6d53c5f8601b3933786f6c
2a92702baf init: Use size_t consistently for cache sizes (TheCharlatan)
65cde3621d kernel: Move default cache constants to caches (TheCharlatan)
8826cae285 kernel: Move non-kernel db cache size constants (TheCharlatan)
e758b26b85 kernel: Move kernel-specific cache size options to kernel (TheCharlatan)
d5e2c4a409 fuzz: Add fuzz test for checked and saturating add and left shift (TheCharlatan)
c03a2795a8 util: Add integer left shift helpers (TheCharlatan)
8bd5f8a38c [refactor] init: Simplify coinsdb cache calculation (TheCharlatan)
5db7d4d3d2 doc: Correct docstring describing max block tree db cache (TheCharlatan)
Pull request description:
Carrying non-kernel related fields in the cache sizes for the indexes is confusing for kernel library users. The cache sizes are set currently with magic numbers in bitcoin-chainstate. The comments for the cache size calculations are not completely clear. The constants for the cache sizes are also currently in `txdb.h`, which is not an ideal place for holding all cache size related constants.
Solve these things by moving the kernel-specific cache size fields to their own struct and moving the constants to either the node or the kernel cache sizes.
This slightly changes the way the cache is allocated if (and only if) the txindex and/or blockfilterindex is used. Since they are now given precedence over the block tree db cache, this results in a bit less cache being allocated to the block tree db, coinsdb and coins caches. The effect is negligible though, i.e. cache sizes with default dbcache reported through the logs are:
master:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.0 MiB for transaction index database
* Using 49.0 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 335.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
this PR:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.2 MiB for transaction index database
* Using 49.2 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 334.5 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
stickies-v:
re-ACK 2a92702baf
ryanofsky:
Code review ACK 2a92702baf. Changes since last review are fixing size options to use size_t instead of int64_t again, simplifying CheckedLeftShift more, and making other minor suggested cleanups
hodlinator:
re-ACK 2a92702baf
Tree-SHA512: 98376eaa0660b1b8c096a5ce1f3e7c8c30e7cd6644de36856c2d3e573108cfc9473c93ebb3952b7881047b5ae6c85c5b096e6726f30f35be58b98eca07c8c785
This avoids having to rely on implicit casts when passing them to the
various functions allocating the caches.
This also ensures that if the requested amount of db_cache does not fit
in a size_t, it is clamped to the maximum value of a size_t.
Also take this opportunity to make the total amounts of cache in the
chainstate manager a size_t too.
They are not related to the txdb, so a better place for them is the
new kernel and node cache file. Re-use the default amount of kernel
cache for the default node cache.
SIGHASH_DEFAULT should be used to indicate SIGHASH_DEFAULT for taproot
inputs, and SIGHASH_ALL for all other input types. This avoids adding an
unnecessary byte to the end of all Taproot signatures added to PSBTs
signed in the GUI.
This makes code more consistent and makes it easier to add compile-time checking to
enforce that format strings contain the right specifiers, because it stops
using Untranslated() to create the format string, so the Untranslated()
function will not need to get involved in formatting.
-BEGIN VERIFY SCRIPT-
quote='"[^"]+"'
quotes="(?:$quote|\\s)*"
nonparens="[^()]*"
single_level_paren="\($nonparens\)"
double_level_paren="\($nonparens\($nonparens\)$nonparens\)"
exprs="(?:$double_level_paren|$single_level_paren|$nonparens)*"
git grep -l 'Untranslated' | xargs perl -0777 -i -pe "s/strprintf\((\\W*)Untranslated\(($quotes)\)($exprs)(\))/Untranslated(\1strprintf(\2\3))/gs"
-END VERIFY SCRIPT-
95a0104f2e test: Add tests for directories in place of config files (Hodlinator)
e85abe92c7 args: Catch directories in place of config files (Hodlinator)
e4b6b1822c test: Add tests for -noconf (Hodlinator)
483f0dacc4 args: Properly support -noconf (Hodlinator)
312ec64cc0 test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning (Hodlinator)
7402658bc2 test: -norpccookiefile (Hodlinator)
39cbd4f37c args: Support -norpccookiefile for bitcoind and bitcoin-cli (Hodlinator)
e82ad88452 logs: Use correct path and more appropriate macros in cookie-related code (Hodlinator)
6e28c76907 test: Harden testing of cookie file existence (Hodlinator)
75bacabb55 test: combine_logs.py - Output debug.log paths on error (Hodlinator)
bffd92f00f args: Support -nopid (Hodlinator)
12f8d848fd args: Disallow -nodatadir (Hodlinator)
6ff9662760 scripted-diff: Avoid printing version information for -noversion (Hodlinator)
e8a2054edc doc args: Document narrow scope of -color (Hodlinator)
Pull request description:
- Document `-color` as only applying to `-getinfo`, to be less confusing for bitcoin-cli users.
- No longer print version information when getting passed `-noversion`.
- Disallow `-nodatadir` as we cannot run without one. It was previously interpreted as a mix of unset and as a relative path of "0".
- Support `-norpccookiefile`
- Support `-nopid`
- Properly support `-noconf` (instead of working by accident). Also detect when directories are specified instead of files.
Prompted by investigation in https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013.
ACKs for top commit:
l0rinc:
utACK 95a0104f2e
achow101:
ACK 95a0104f2e
ryanofsky:
Code review ACK 95a0104f2e. Looks good! Thanks for all your work on this breaking the changes down and making them simple.
Tree-SHA512: 5174251e6b9196a9c6d135eddcb94130295c551bcfccc78e633d9e118ff91523b1be0d72828fb49603ceae312e6e1f8ee2651c6a2b9e0f195603a73a9a622785
