I encountered this issue while running my code with Valgrind today.
Below is part of the Valgrind error message:
```
==1578139== 472 bytes in 1 blocks are still reachable in loss record 1 of 1
==1578139== at 0x4848899: malloc (...)
==1578139== by 0x4B3AF62: fdopen@@GLIBC_2.2.5 (...)
==1578139== by 0x118B09: subprocess::Popen::execute_process() (...)
```
I noticed that a similar fix had been proposed by another contributor
previously. I did not mean to scoop their work, but merely hoping to fix
it sooner so other people don't get confused by it just as I did today.
Github-Pull: arun11299/cpp-subprocess#106
Rebased-From: 3afe581c1f22f106d59cf54b9b65251e6c554671
e976bd3045010ee217aa0f2dca4c962aabb789d5 validation: add randomness to periodic write interval (Andrew Toth)
2e2f41068128c38120a5b44d24ee30f71970455a refactor: replace m_last_write with m_next_write (Andrew Toth)
b557fa7a175f139614932fbb3a4ad0af8271c73c refactor: rename fDoFullFlush to should_write (Andrew Toth)
d73bd9fbe483ad1397f62dc1d580314202351ace validation: write chainstate to disk every hour (Andrew Toth)
0ad7d7abdbcffc11a46413545a214a716f56dc95 test: chainstate write test for periodic chainstate flush (Andrew Toth)
Pull request description:
Since #28233, periodically writing the chainstate to disk every 24 hours does not clear the dbcache. Since #28280, periodically writing the chainstate to disk is proportional only to the amount of dirty entries in the cache. Due to these changes, it is no longer beneficial to only write the chainstate to disk every 24 hours. The periodic flush interval was necessary because every write of the chainstate would clear the dbcache. Now, we can get rid of the periodic flush interval and simply write the chainstate along with blocks and block index at least every hour.
Three benefits of doing this:
1. For IBD or reindex-chainstate with a combination of large dbcache setting, slow CPU, slow internet speed/unreliable peers, it could be up to 24 hours until the chainstate is persisted to disk. A power outage or crash could potentially lose up to 24 hours of progress. If there is a very large amount of dirty cache entries, writing to disk when a flush finally does occur will take a very long time. Crashing during this window of writing can cause https://github.com/bitcoin/bitcoin/issues/11600. By syncing every hour in unison with the block index we avoid this problem. Only a maximum of one hour of progress can be lost, and the window for crashing during writing is much smaller. For IBD with lower dbcache settings, faster CPU, or better internet speed/reliable peers, chainstate writes are already triggered more often than every hour so this change will have no effect on IBD.
2. Based on discussion in #28280, writing only once every 24 hours during long running operation of a node causes IO spikes. Writing smaller chainstate changes every hour like we do with blocks and block index will reduce IO spikes.
3. Faster shutdown speeds. All dirty chainstate entries must be persisted to disk on shutdown. If we have a lot of dirty entries, such as when close to 24 hours or if we sync with a large dbcache, it can take a long time to shutdown. By keeping the chainstate clean we avoid this problem.
Inspired by [this comment](https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2121088705).
Resolves https://github.com/bitcoin/bitcoin/issues/11600
ACKs for top commit:
achow101:
ACK e976bd3045010ee217aa0f2dca4c962aabb789d5
davidgumberg:
utACK e976bd3045
sipa:
utACK e976bd3045010ee217aa0f2dca4c962aabb789d5
l0rinc:
ACK e976bd3045010ee217aa0f2dca4c962aabb789d5
Tree-SHA512: 5bccd8f1dea47f9820a3fd32fe3bb6841c0167b3d6870cc8f3f7e2368f124af1a914bca6acb06889cd7183638a8dbdbace54d3237c3683f2b567eb7355e015ee
97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3 util: Remove `fsbridge::get_filesystem_error_message()` (Hennadii Stepanov)
Pull request description:
The `fsbridge::get_filesystem_error_message()` function exhibits several drawbacks:
1. It was introduced in https://github.com/bitcoin/bitcoin/pull/14192 to account for platform-specific variations in
`boost::filesystem::filesystem_error::what()`. Since [migrating](https://github.com/bitcoin/bitcoin/pull/20744) to `std::filesystem`, those discrepancies no longer exist.
2. It fails to display UTF-8 paths correctly on Windows:
```
> build\bin\Release\bitcoind.exe -datadir="C:\Users\hebasto\dd_₿_🏃" -regtest
...
2025-04-30T00:17:48Z DeleteAuthCookie: Unable to remove random auth cookie file: remove: Access is denied.: "C:\Users\hebasto\dd_?_??\regtest\.cookie"
...
```
3. It relies on `std::wstring_convert`, which was deprecated in C++17 and removed in C++26 (also see https://github.com/bitcoin/bitcoin/issues/32361).
This PR removes the obsolete `fsbridge::get_filesystem_error_message()` function, thereby resolving all of the above issues.
ACKs for top commit:
maflcko:
lgtm re-ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3
davidgumberg:
untested crACK 97eaadc3bf
achow101:
ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3
laanwj:
Code review ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3
Tree-SHA512: 3c7378a9b143ac2a71add967318a13c346ae3bccbec6e9879d7873083f3fa469b3eef529b2c9c142b2489ba9563e4e12f685745c09a8a219d58b384f7ecf1be1
The `fsbridge::get_filesystem_error_message()` function exhibits several
drawbacks:
1. It was introduced in https://github.com/bitcoin/bitcoin/pull/14192 to
account for platform-specific variations in
`boost::filesystem::filesystem_error::what()`. Since migrating to
`std::filesystem`, those discrepancies no longer exist.
2. It fails to display UTF-8 paths correctly on Windows.
3. It relies on `std::wstring_convert`, which was deprecated in C++17
and removed in C++26.
This change removes the `fsbridge::get_filesystem_error_message()`
function, thereby resolving all of the above issues.
Additionally, filesystem error messages now use the "Warning" log level.
a8333fc9ff9adaa97a1f9024f5783cc071777150 scripted-diff: wallet: rename plain and encrypted master key variables (Sebastian Falbesoner)
5a92077fd5317f936da2fa0aa45e0173248f765b wallet: refactor: dedup master key decryption (Sebastian Falbesoner)
846545947cd3b993c40362b9d0afcd7b4f5f05bd wallet: refactor: dedup master key encryption / derivation rounds setting (Sebastian Falbesoner)
a6d9b415aa3afcfe463887d0fde00c3d2d32672a wallet: refactor: introduce `CMasterKey::DEFAULT_DERIVE_ITERATIONS` constant (Sebastian Falbesoner)
62c209f50d9c33fde5062ebca317b9a4233aff62 wallet: doc: remove mentions of unavailable scrypt derivation method (Sebastian Falbesoner)
Pull request description:
This PR contains various cleanups around the wallet's master key encryption logic. The default/minimum key derivation rounds magic number of 25000 is hoisted into a constant (member of `CMasterKey`) and two new functions `EncryptMasterKey`/`DecryptMasterKey` are introduced in order to deduplicate code for the derivation round determination and master key en/decryption. Also, mentions of the never-implemented derivation method `scrypt` are removed from the wallet crypter header and both plain and encrypted master key instances are renamed to adapt to moderning coding style (hopefully improving readability).
ACKs for top commit:
davidgumberg:
ACK a8333fc9ff
achow101:
ACK a8333fc9ff9adaa97a1f9024f5783cc071777150
Tree-SHA512: 5a66d3b26f481347d0b5b4f742dd237803a35aad6e3480ed15fd38b7fa3700650bd5f67f4c30ed88f5fad45d6cd4c893fe4f1657e36e563b4294fd3596187724
524f981bb87319fdd6ff2ab4a932c4b4e31a7398 Bugfix: Miner: Don't reuse block_reserved_weight for "block is full enough to give up" weight delta (Luke Dashjr)
Pull request description:
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.
ACKs for top commit:
achow101:
ACK 524f981bb87319fdd6ff2ab4a932c4b4e31a7398
darosior:
utACK 524f981bb87319fdd6ff2ab4a932c4b4e31a7398
ismaelsadeeq:
ACK 524f981bb87319fdd6ff2ab4a932c4b4e31a7398
Tree-SHA512: c066debc34a021380424bd21b40444071b736325e41779a41590c2c8a6822ceeaf910fe067817c1dba108210b24c574977b0350b29520502e7af79d3b405928b
7e8ef959d0637ca5543ed33d3919937e0d053e70 refactor: Fix Sonar rule `cpp:S4998` - avoid unique_ptr const& as parameter (Lőrinc)
e400ac53524d143467740e2f59698a7c94644c21 refactor: simplify repeated comparisons in `FindChallenges` (Lőrinc)
f670836112c01feb3cb71618192e9c0c2e55767f test: remove old recursive `FindChallenges_recursive` implementation (Lőrinc)
b80d0bdee4603aa8ab69587d0c311aad1a9b3c7a test: avoid stack overflow in `FindChallenges` via manual iteration (Lőrinc)
Pull request description:
`FindChallenges` explores the `Miniscript` node tree by going deep into the first child's subtree, then the second, and so on - effectively performing a pre-order Traversal (Depth-First Search) recursively, using the call stack which can result in stack overflows on Windows debug builds.
This change replaces the recursive implementation with an iterative version using an explicit stack. The new implementation also performs a pre-order depth-first traversal, though it processes children in right-to-left order (rather than left-to-right) due to the LIFO nature of the stack. Since both versions store results in a `std::set`, which automatically sorts and deduplicates elements, the exact traversal order doesn't affect the final result.
It is an alternative to increasing the Windows stack size, as proposed in #32349, and addresses the issue raised in #32341 by avoiding deep recursion altogether.
The change is done in two commits:
* add a new iterative `FindChallenges` method and rename the old method to `*_recursive` (to simplify the next commit where we remove it), asserting that its result matches the original;
* remove the original recursive implementation.
This approach avoids ignoring the `misc-no-recursion` warning as well.
I tried modifying the new method to store results in a vector instead, but it demonstrated that the deduplication provided by `std::set` was necessary. One example showing the need for deduplication:
Recursive (using set):
```
(6, 9070746)
(6, 19532513)
(6, 3343376967)
```
Iterative (using vector attempt):
```
(6, 19532513)
(6, 9070746)
(6, 3343376967)
(6, 9070746) // Duplicate entry
```
The performance of the test is the same as before, with the recursive method.
Fixes https://github.com/bitcoin/bitcoin/issues/32341
ACKs for top commit:
achow101:
ACK 7e8ef959d0637ca5543ed33d3919937e0d053e70
sipa:
utACK 7e8ef959d0637ca5543ed33d3919937e0d053e70
hodlinator:
re-ACK 7e8ef959d0637ca5543ed33d3919937e0d053e70
Tree-SHA512: 9e52eff82a7d76f5d37e3b74c508f08e5fced5386dad504bed111b27ed2b529008a6dd12a5116f009609a94c7ee7ebe3e80a759dda55dd1cb3ae52078f65ec71
b9d4d5f66a5a35c47e7abc9ec6ef5ab242b3f1e1 net: Use GetAdaptersAddresses to get local addresses on Windows (laanwj)
Pull request description:
Instead of a `gethostname` hack, which is not guaranteed to return all addresses, use the official way of calling `GetAdaptersAddresses` to get local network addresses on Windows.
Do the same checks as the UNIX path: interface is up, interface is not loopback.
Suggested by Ava Chow.
Addiional changes:
- Cleanup: move out `FromSockAddr` in `netif.cpp` from MacOS and use it everywhere appropriate. This avoids code duplication.
ACKs for top commit:
davidgumberg:
utreACK b9d4d5f66a
achow101:
ACK b9d4d5f66a5a35c47e7abc9ec6ef5ab242b3f1e1
Tree-SHA512: e9f0a7ec0c46f21c0377d5174e054a6569f858630727f94dac00c0cb7c241c56892d0b902706d6dd53880cc3b5ae1f2dba9caa1fec40e64cd4cf0d34493a49c1
abe43dfadd6325f80975a76aea57a549c3162191 doc: release note for #27826 (Sjors Provoost)
f9fa28788e63e2bd059a21ec0e76ae6903b2a6be Use LogBlockHeader for compact blocks (Sjors Provoost)
bad7c914793134abe2f64d96c367d5e9b07e60fd Log which peer sent us a header (Sjors Provoost)
9d3e39c29c31775fd82af319d1d4dfbbd3e21bfa Log block header in net_processing (Sjors Provoost)
Pull request description:
Fixes#27744
Since #27278 we log received headers. For compact blocks we also log which peer sent it (e5ce8576349d404c466b2f4cab1ca7bf920904b2), but not for regular headers. That required an additional refactor, which this PR provides.
Move the logging from validation to net_processing.
This also reduces the number of log entries (under default configuration) per compact block header from 3 to 2: one for the header and one for the connected tip.
The PR introduces a new helper method `LogBlockHeader`.
When receiving a _compact block_ we call `LogBlockHeader` from the exact same place as where we previously logged. So that log message doesn't change. What does change is that we no longer _also_ log from `AcceptBlockHeader`.
When receiving a regular header(s) message, _we only log the last one_. This is a change in behaviour because it was simpler to implement, but it's probably better anyway. It does mean that if a peer sends of a bunch of headers of which _any_ is invalid, we won't log it (here).
Lastly I expanded the code comment explaining why we log this. It initially only covered selfish mining, but we also care about peers sending us headers but not following up (see e.g. #27626).
Example log:
```
2023-06-05T13:12:21Z Saw new header hash=000000000000000000045910263ef84b575ae3af151865238f1e5c619e69c330 height=792964 peer=0
2023-06-05T13:12:23Z UpdateTip: new best=000000000000000000045910263ef84b575ae3af151865238f1e5c619e69c330 height=792964 version=0x20000000 log2_work=94.223098 tx=848176824 date='2023-06-05T13:11:49Z' progress=1.000000 cache=6.4MiB(54615txo)
2023-06-05T13:14:05Z Saw new cmpctblock header hash=00000000000000000003c6fd4ef2e1246a3f9e1fffab7247344f94cadb9de979 height=792965 peer=0
2023-06-05T13:14:05Z UpdateTip: new best=00000000000000000003c6fd4ef2e1246a3f9e1fffab7247344f94cadb9de979 height=792965 version=0x20000000 log2_work=94.223112 tx=848179461 date='2023-06-05T13:13:58Z' progress=1.000000 cache=7.2MiB(61275txo)
2023-06-05T13:14:41Z Saw new header hash=000000000000000000048e6d69c8399992782d08cb57f5d6cbc81a9f996c3f43 height=792966 peer=8
2023-06-05T13:14:42Z UpdateTip: new best=000000000000000000048e6d69c8399992782d08cb57f5d6cbc81a9f996c3f43 height=792966 version=0x2db3c000 log2_work=94.223126 tx=848182944 date='2023-06-05T13:14:35Z' progress=1.000000 cache=8.0MiB(69837txo)
```
ACKs for top commit:
danielabrozzoni:
tACK abe43dfadd6325f80975a76aea57a549c3162191
achow101:
ACK abe43dfadd6325f80975a76aea57a549c3162191
vasild:
ACK abe43dfadd6325f80975a76aea57a549c3162191
Tree-SHA512: 081e0de62cbd8a0b35cf54daaa09e3e6991d0cc9f706ef3eb50908752fe7815de69b367f7313381c90cd8d5de0ae5f532d1cd54948c5c1133b1832f266d9c232
f1b142856a4ecd0a0d90bc3d73ef5137997b14ff test: Same addr, diff port is already connected (David Gumberg)
94e85a82a753a0aa5ad688fc46330e83c9a697fe net: remove unnecessary check from AlreadyConnectedToAddress() (Vasil Dimov)
Pull request description:
`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.
---
In other words: let `A` be "CNetAddr equals" and `B` be "addr:port string matches", then:
* If `A` (is `true`), then `B` is irrelevant, so the condition `A || B` is equivalent to `A` is `true`.
* Observation in this PR: if `!A` (`A` is `false`), then `!B` for sure, thus the condition `A || B` is equivalent to `A` is `false`.
So, simplify `A || B` to `A`.
https://en.wikipedia.org/wiki/Modus_tollens `!A => !B` is equivalent to `B => A`. So the added fuzz test asserts that if `B` is `true`, then `A` is `true`.
ACKs for top commit:
davidgumberg:
crACK f1b142856a4ecd0a0d90bc3d
achow101:
ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff
theuni:
utACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff
mzumsande:
Code Review ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff
Tree-SHA512: d744b60e9bace121faa3a746463f6b6e0e6ef08eac0e7879326cbd5f4721e47e6e10f6203dfd3870a2057c4ddd1860692c070ef048a76d773b84e6c2f840cc86
e3014017bacff42d8d69f3061ce1ee621aaa450a test: add IsActiveAfter tests for versionbits (Anthony Towns)
60950f77c35e54e2884cfc14ab67623f3e325099 versionbits: docstrings for BIP9Info (Anthony Towns)
7565563bc7a5bb98ebf03a7d6881912a74d3f302 tests: refactor versionbits fuzz test (Anthony Towns)
2e4e9b9608c722aaf767638e9dba498d8dc3e772 tests: refactor versionbits unit test (Anthony Towns)
525c00f91bb27d0f2a1b2e5532aebec7fac97d3a versionbits: Expose VersionBitsConditionChecker via impl header (Anthony Towns)
e74a7049b477d1853191ded75fdf25024a6e233f versionbits: Expose StateName function (Anthony Towns)
d00d1ed52c8ee95eeed665d68d6715a694bd4c1f versionbits: Split out internal details into impl header (Anthony Towns)
37b9b67a39554465104c9cf1a74690f40019dbad versionbits: Simplify VersionBitsCache API (Anthony Towns)
1198e7d2fd665bf2bc49fd26773d4fd5fbc2b716 versionbits: Move BIP9 status logic for getblocktemplate to versionbits (Anthony Towns)
b1e967c3ec92738affb22d3b58483ebcdd8dfea2 versionbits: Move getdeploymentinfo logic to versionbits (Anthony Towns)
3bd32c20550e69688a4ff02409fb34b9a637b9c4 versionbits: Move WarningBits logic from validation to versionbits (Anthony Towns)
5da119e5d0e61f0b583f0fe21b9a00ee815a3e46 versionbits: Change BIP9Stats to uint32_t types (Anthony Towns)
a679040ec19ef17f3f03988a52207f1c03af701e consensus/params: Move version bits period/threshold to bip9 param (Anthony Towns)
e9d617095d4ce9525a4337d33624cac9d6b4abe6 versionbits: Remove params from AbstractThresholdConditionChecker (Anthony Towns)
9bc41f1b48b2e0cc6abf9714e860a29989d7809c versionbits: Use std::array instead of C-style arrays (Anthony Towns)
Pull request description:
Increases the encapsulation/modularity of the versionbits code, moving more of the logic into the versionbits module rather than having it scattered across validation and rpc code. Updates unit/fuzz tests to test the actual code used rather than just a close approximation of it.
ACKs for top commit:
achow101:
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
TheCharlatan:
Re-ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
darosior:
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
Tree-SHA512: 2978db5038354b56fa1dd6aafd511099e9c16504d6a88daeac2ff2702c87bcf3e55a32e2f0a7697e3de76963b68b9d5ede7976ee007e45862fa306911194496d
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>
71656bdfaa6bfe08ce9651246a3ef606f923351b 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 71656bdfaa6bfe08ce9651246a3ef606f923351b
pablomartin4btc:
re-ACK 71656bdfaa6bfe08ce9651246a3ef606f923351b
hebasto:
ACK 71656bdfaa6bfe08ce9651246a3ef606f923351b, 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.
edd46566bd66cea7d7f4116429fe1c11d2187ba2 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 edd46566bd66cea7d7f4116429fe1c11d2187ba2, I have reviewed the code and it looks OK.
Tree-SHA512: 32944e00dae0c62bb23e3d7abd486b63e445702483ca03c74c3057ef942f06e771d4d3d3a58fd728582889d6b638fae11ecc536a25febfd89a28522b7d6d08ba
`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.
3dbd50a576be55941cb4b5034dc2171c03afb07c 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 3dbd50a576be55941cb4b5034dc2171c03afb07c
achow101:
ACK 3dbd50a576be55941cb4b5034dc2171c03afb07c
laanwj:
re-ACK 3dbd50a576be55941cb4b5034dc2171c03afb07c
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>
3c3548a70eedb8dcf6a4a8d605a4a12e814c7cac validation: clarify final |= BLOCK_FAILED_VALID in InvalidateBlock (Matt Corallo)
aac5488909f72f8c5a91ca7f12398069b7cd9ce4 validation: correctly update BlockStatus for invalid block descendants (stratospher)
9e29653b424b324c753526fbbf9fdbc92e88244f test: check BlockStatus when InvalidateBlock is used (stratospher)
c99667583dd9b57612edf4c04611cd4857250600 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 3c3548a70eedb8dcf6a4a8d605a4a12e814c7cac
TheCharlatan:
Re-ACK 3c3548a70eedb8dcf6a4a8d605a4a12e814c7cac
mzumsande:
re-ACK 3c3548a70eedb8dcf6a4a8d605a4a12e814c7cac
Tree-SHA512: 83e0d29dea95b97519d4868135c965b86f6f43be50b15c0bd8f998b3476388fc7cc22b49c0c54ec532ae8222e57dfc436438f0c8e98f54757b384f220488b6a6
55b931934a34bab11446e8eed7bdaef92bb056de 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 55b931934a34bab11446e8eed7bdaef92bb056de
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.
faca46b0421b568e7e5fefe593420e773d0ec9af 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 faca46b0421b568e7e5fefe593420e773d0ec9af
BrandonOdiwuor:
Code Review ACK faca46b0421b568e7e5fefe593420e773d0ec9af
Tree-SHA512: 866f1ccff0313017dd313d5a218d7ee088b823601a129b9ed4c5819b0d57fd808d78e3ea28ca00714ae6b209df5312b7b9dea091b2b028821ff46b8ba263c48a
3669ecd4ccd8e7a1e2b1a9dcbe708c51c78e4d6c doc: Document fuzz build options (Anthony Towns)
c1d01f59acc2067ecbf8a8b42ba0d8e596694439 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 3669ecd4ccd8e7a1e2b1a9dcbe708c51c78e4d6c 🏉
marcofleon:
re ACK 3669ecd4ccd8e7a1e2b1a9dcbe708c51c78e4d6c
ryanofsky:
Code review ACK 3669ecd4ccd8e7a1e2b1a9dcbe708c51c78e4d6c with just variable renamed and documentation added since last review
Tree-SHA512: 5da5736462f98437d0aa1bd01aeacb9d46a9cc446a748080291067f7a27854c89f560f3a6481b760b9a0ea15a8d3ad90cd329ee2a008e5e347a101ed2516449e
The migration benchmark crashes if run more than once, because of `std::move(wallet)` and leaves subsequent iterations in an undefined state - avoiding `UndefinedBehaviorSanitizer` null‑dereference error.
`MigrateLegacyToDescriptor` returns both a spendable descriptor wallet and a watch‑only wallet.
If these remain attached, their files stay open and on Windows this can hang CI when removing the test directory.
By constructing them via `MakeWalletLoader` (which owns the `WalletContext`), both wallets are automatically unloaded when the loader is destroyed at the end.
This ensures no lingering handles or resource leaks when running the benchmark on CI with `-sanity-check`.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
