The original recursive `FindChallenges` explores the Miniscript node tree using depth-first search. Specifically, it performs a pre-order traversal (processing the node's data, then recursively visiting children from left-to-right). This recursion uses the call stack, which can lead to stack overflows on platforms with limited stack space, particularly noticeable in Windows debug builds.
This change replaces the recursive implementation with an iterative version using an explicit stack. The iterative version also performs a depth-first search and processes the node's data before exploring children (preserving pre-order characteristics), although the children are explored in right-to-left order due to the LIFO nature of the explicit stack.
Critically, both versions collect challenges into a `std::set`, which automatically deduplicates and sorts elements. This ensures that not only the final result, but the actual state of the set at any equivalent point in traversal remains identical, despite the difference in insertion order.
This iterative approach is an alternative to increasing the default stack size (as proposed in #32349) and directly addresses the stack overflow issue reported in #32341 by avoiding deep recursion.
The change is done in two commits:
* add a new iterative `FindChallenges` method and rename the old method to `*_recursive` (to simplify removal in the next commit), asserting that its result matches the original;
* Remove the original recursive implementation.
This approach avoids needing to suppress `misc-no-recursion` warnings and provides a portable, low-risk fix.
Using a `std::set` is necessary for deduplication, matching the original function's behavior. An experiment using an `std::vector` showed duplicate challenges being added, confirming the need for the set:
Example failure with vector:
Recursive (set):
(6, 9070746)
(6, 19532513)
(6, 3343376967)
Iterative (vector attempt):
(6, 19532513)
(6, 9070746)
(6, 3343376967)
(6, 9070746) // Duplicate
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
77e553ab6a build: refactor: hardening flags -> core_interface (David Gumberg)
00ba3ba303 build: Drop option for disabling hardening (David Gumberg)
f57db75e91 build: Use `-z noseparate-code` on NetBSD < 11.0 (David Gumberg)
Pull request description:
Follow up to #32038 which dropped `NO_HARDEN` from depends builds, this PR drops the `ENABLE_HARDENING` build option since disabling hardening of binaries should not be a supported or maintained use case. With this change, hardening flags are always enabled.
Individual hardening flags and options can still be disabled by appending flags, e.g.:
```bash
cmake -B build \
-DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 -fno-stack-protector -fcf-protection=none -fno-stack-clash-protection' \
-DAPPEND_LDFLAGS='-Wl,-z,lazy -Wl,-z,norelro -Wl,-z,noseparate-code'
```
There is an issue with NetBSD 10.0's dynamic linker that makes one of the hardening linker flags, `-z separate-code`, [problematic](https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2589347934), so this PR also introduces a check to prevent the use of this flag in NetBSD versions < 11.0, (where this issue is [fixed](acf7fb3abf)). The fix for this [might be backported](https://mail-index.netbsd.org/tech-userlevel/2023/01/05/msg013670.html) to NetBSD 10.0.
I suggest reviewing the diff with whitespace changes hidden (`git diff -w` or using github's hide whitespace option)
ACKs for top commit:
hebasto:
re-ACK 77e553ab6a.
laanwj:
re-ACK 77e553ab6a
janb84:
ACK [77e553a](77e553ab6a)
vasild:
ACK 77e553ab6a
musaHaruna:
tested ACK [77e553](77e553ab6a)
Tree-SHA512: b149fb0371d12312c140255bf674c2bdc9f5272a5750a5b9ec5f192323364bb2ea8e164af13b9ab981ab3aa7ceb91b7a64785081e7458470e81c2f5228abf7b1
61f238e84a doc: Fix fuzz test_runner.py path (monlovesmango)
Pull request description:
This commit fixes the path listed in the documentation for the fuzz testing test_runner.py. Previously the --help option worked but running fuzz tests from the documented path did not.
ACKs for top commit:
kevkevinpal:
ACK [61f238e](61f238e84a)
maflcko:
lgtm ACK 61f238e84a
mabu44:
Tested ACK 61f238e84a
hebasto:
ACK 61f238e84a.
Tree-SHA512: e8770f38e49a428e0e7f0450db193ec90cc1e66c05bcde307763c065ac7051f3f05923bb3e0eca7a337da9c14cfd17512ff0d01ffa330796159d4f3552103b7f
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
PR #30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block:
```diff
if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
- m_options.nBlockMaxWeight - 4000) {
+ m_options.nBlockMaxWeight - m_options.block_reserved_weight) {
// Give up if we're close to full and haven't succeeded in a while
break;
}
```
But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts.
It doesn't seem very logical to reuse the reserve weight for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.
Homebrew’s `qt@6` package places the `translations` and `plugins`
directories in the `share/qt` subdirectory.
This change updates the `macdeployqtplus` script accordingly.
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
This commit fixes the path listed in the documentation for the fuzz
testing test_runner.py. Previously the --help option worked but running
fuzz tests from the documented path did not.
A bug in 0.21.x and 22.x resulted in some wallets having invalid
derivation paths that are the concatenation of two derivation paths.
These appear only when inactive hd chains are topped up.
Since key metadata is a legacy wallet only record, migrating legacy
wallets to descriptor wallets will fix this issue as all key metadata
records are deleted. The derivation path information is derived
on-the-fly from the descriptor that is produced for the inactive hd
chain.
Thus we only need a test to verify that the derivation paths are good,
and that all key metadata records are deleted from the migrated wallet.
`CConnman::AlreadyConnectedToAddress()` searches the existent nodes by
address or by address-and-port:
```cpp
FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort())
```
but:
* if there is a match by just the address, then the address-and-port
search will not be evaluated and the whole condition will be `true`
* if the there is no node with the same address, then the second search
by address-and-port will not find a match either.
The search by address-and-port is comparing against `CNode::m_addr_name`
which could be a hostname, e.g. `"node.foobar.com:8333"`, but
`addr.ToStringAddrPort()` is always going to be numeric.
3dbd50a576 Fix failing util_time_GetTime test on Windows (VolodymyrBg)
Pull request description:
Remove unreliable steady clock time checking from the test that was causing CI failures primarily on Windows. The test previously tried to verify that steady_clock time increases after a 1ms sleep, but this approach is not reliable on all platforms where such a short sleep interval may not consistently result in observable clock changes.
This addresses issue #32197 where the test was reporting failures in the cross-built Windows CI environment. As noted in the discussion, the test is not critical to the functionality of Bitcoin Core, and removing the unreliable part is the most straightforward solution.
ACKs for top commit:
maflcko:
lgtm ACK 3dbd50a576
achow101:
ACK 3dbd50a576
laanwj:
re-ACK 3dbd50a576
Tree-SHA512: 25c80558d9587c7845d3c14464e8d263c8bd9838a510faf44926e5cda5178aee10b03a52464246604e5d27544011d936442ecfa1e4cdaacb66d32c35f7213902
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.
Remove unreliable steady clock time checking from the test that was causing
CI failures primarily on Windows. The test previously tried to verify that
steady_clock time increases after a 1ms sleep, but this approach is not reliable
on all platforms where such a short sleep interval may not consistently result
in observable clock changes.
This addresses issue #32197 where the test was reporting failures in the
cross-built Windows CI environment. As noted in the discussion, the test is not
critical to the functionality of Bitcoin Core, and removing the unreliable part
is the most straightforward solution.
Rename and refocus util_time_GetTime test to util_mocktime
Co-Authored-By: maflcko <6399679+maflcko@users.noreply.github.com>
facb9b327b scripted-diff: Use bpf_cflags (MarcoFalke)
fa0c1baaf8 test: Add imports for util bpf_cflags (MarcoFalke)
Pull request description:
On some Linux kernel versions, the bpf compiler invoked in the functional tests will issue a `-Wduplicate-decl-specifier` warning.
This seems harmless and should be fixed upstream in the Linux kernel.
Here, simply suppress it for now. Fixes https://github.com/bitcoin/bitcoin/issues/32322
ACKs for top commit:
laanwj:
Code review ACK facb9b327b
hebasto:
ACK facb9b327b, I have reviewed the code and it looks OK.
Tree-SHA512: 53387127e3c2a2dbfe05281b2d2e61efbd3c3adcc3b4bf2f11540042f86e1e8c06637f80d246310bc44ca0612318472f25545c1e1ca3636fda97d04381f9e905
3c3548a70e validation: clarify final |= BLOCK_FAILED_VALID in InvalidateBlock (Matt Corallo)
aac5488909 validation: correctly update BlockStatus for invalid block descendants (stratospher)
9e29653b42 test: check BlockStatus when InvalidateBlock is used (stratospher)
c99667583d validation: fix traversal condition to mark BLOCK_FAILED_CHILD (stratospher)
Pull request description:
This PR addresses 3 issues related to how `BLOCK_FAILED_CHILD` is set:
1. In `InvalidateBlock()`
- Previously, `BLOCK_FAILED_CHILD` was not being set when it should have been.
- This was due to an incorrect traversal condition, which is fixed in this PR.
2. In `SetBlockFailure()`
- `BLOCK_FAILED_VALID` is now cleared before setting `BLOCK_FAILED_CHILD`.
3. In `InvalidateBlock()`
- if block is already marked as `BLOCK_FAILED_CHILD`, don't mark it as `BLOCK_FAILED_VALID` again.
Also adds a unit test to check `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` status in `InvalidateBlock()`.
<details>
<summary><h3>looking for feedback on an alternate approach</h3></summary>
<br>
An alternate approach could be removing `BLOCK_FAILED_CHILD` since even though we have a distinction between
`BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` in the codebase, we don't use it for anything. Whenever we check for BlockStatus, we use `BLOCK_FAILED_MASK` which encompasses both of them. See similar discussion in https://github.com/bitcoin/bitcoin/pull/16856.
I have a branch with this approach in https://github.com/stratospher/bitcoin/commits/2025_02_remove_block_failed_child/.
Compared to the version in #16856, it also resets `BLOCK_FAILED_CHILD` already on disk to `BLOCK_FAILED_VALID` when loading from disk so that we won't be in a dirty state in a no-`BLOCK_FAILED_CHILD`-world.
I'm not sure if it's a good idea to remove `BLOCK_FAILED_CHILD` though. would be curious to hear what others think of this approach.
thanks @ mzumsande for helpful discussion regarding this PR!
</details>
ACKs for top commit:
achow101:
ACK 3c3548a70e
TheCharlatan:
Re-ACK 3c3548a70e
mzumsande:
re-ACK 3c3548a70e
Tree-SHA512: 83e0d29dea95b97519d4868135c965b86f6f43be50b15c0bd8f998b3476388fc7cc22b49c0c54ec532ae8222e57dfc436438f0c8e98f54757b384f220488b6a6
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
Legacy wallets do not have the descriptors flag set. Don't load wallets
without the descriptors flag.
At the same time, we will no longer load BDB databases since they are
only used for legacy wallets.
Removes all legacy wallet specific functional tests.
Also removes the --descriptor and --legacy-wallet options as these are
no longer necessary with the legacy wallet removed.
faca46b042 test: Run all benchmarks in the sanity check (MarcoFalke)
Pull request description:
It is unclear why not all benchmarks are run, given that:
* they only run as a sanity check (fastest version)
* no one otherwise runs them, not even CI
* issues have been missed due to this
ACKs for top commit:
l0rinc:
ACK faca46b042
BrandonOdiwuor:
Code Review ACK faca46b042
Tree-SHA512: 866f1ccff0313017dd313d5a218d7ee088b823601a129b9ed4c5819b0d57fd808d78e3ea28ca00714ae6b209df5312b7b9dea091b2b028821ff46b8ba263c48a
3669ecd4cc doc: Document fuzz build options (Anthony Towns)
c1d01f59ac fuzz: enable running fuzz test cases in Debug mode (Anthony Towns)
Pull request description:
When building with
BUILD_FOR_FUZZING=OFF
BUILD_FUZZ_BINARY=ON
CMAKE_BUILD_TYPE=Debug
allow the fuzz binary to execute given test cases (without actual fuzzing) to make it easier to reproduce fuzz test failures in a more normal debug build.
In Debug builds, deterministic fuzz behaviour is controlled via a runtime variable, which is normally false, but set to true automatically in the fuzz binary, unless the FUZZ_NONDETERMINISM environment variable is set.
ACKs for top commit:
maflcko:
re-ACK 3669ecd4cc🏉
marcofleon:
re ACK 3669ecd4cc
ryanofsky:
Code review ACK 3669ecd4cc with just variable renamed and documentation added since last review
Tree-SHA512: 5da5736462f98437d0aa1bd01aeacb9d46a9cc446a748080291067f7a27854c89f560f3a6481b760b9a0ea15a8d3ad90cd329ee2a008e5e347a101ed2516449e
