85368aafa0d1772626d5f615e272b1c1a580b42f test: Run simple tests at various feerates (Murch)
d610951c154663053a0e39a850dffd96f808581b test: Recreate BnB iteration exhaustion test (Murch)
2a1b2754f14a2686e78547b2e8a51a4fb35e3816 test: Remove redundant repeated test (Murch)
4781f5c8be55e41fb8144cdec6ec1989a5db72ae test: Recreate simple BnB failure tests (Murch)
a94030ae985bb6c9be98cf8258394dd50d34323a test: Recreate BnB clone skipping test (Murch)
7db6f012c08c796645c38b87d35e89cf6c18ed3b test: Move BnB feerate sensitivity tests (Murch)
2bafc462610cf5e6fd901706bbaedaf3cf35b2fd test: Recreate simple BnB success tests (Murch)
Pull request description:
This PR is splitting off some of the improvements made in #28985 and starts addressing the issues raised in #27754.
I aim to completely replace `coinselector_tests` with `coinselection_tests`. The goal is to generally use coins created per a nominal _effective value_ so we can get away from testing with `CoinSelectionParams` that are non-representative and effectuate counterintuitive behavior such as `feerate = 0` or `cost_of_change = 0`
ACKs for top commit:
achow101:
ACK 85368aafa0d1772626d5f615e272b1c1a580b42f
monlovesmango:
ACK 85368aafa0d1772626d5f615e272b1c1a580b42f
w0xlt:
ACK 85368aafa0
Tree-SHA512: 1a984837b4efddc0d8abe11668898fb207fb539e784bf911d4038211274b82e0fe1f8fffe7e5a19e0e013ccb7dc40e3f62d853a2a729980d0d935e66f12b9156
Add missing error check for fcntl(fd, F_GETFD, 0) in set_clo_on_exec.
Raise OSError on failure to align with existing FD_SETFD behavior.
This improves robustness in subprocess setup and error visibility.
Github-Pull: arun11299/cpp-subprocess#117
Rebased-From: 9974ff69cdd5fc1a2722cb63f006df9308628b35
This commit makes sure:
1. WaitForSingleObject returns with expected
code before proceeding.
2. Process handle is properly closed.
Github-Pull: arun11299/cpp-subprocess#116
Rebased-From: 625a8775791e62736f20f3fa3e6cc4f1b24aa89a
* refactor: Guard `util::quote_argument()` with `#ifdef __USING_WINDOWS__`
The `util::quote_argument()` function is specific to Windows and is used
in code already guarded by `#ifdef __USING_WINDOWS__`.
* Do not escape double quotes for command line arguments on Windows
This change fixes the handling of double quotes and aligns the behavior
with Python's `Popen` class. For example:
```
>py -3
>>> import subprocess
>>> p = subprocess.Popen("cmd.exe /c dir \"C:\\Program Files\"", stdout=subprocess.PIPE, text=True)
>>> print(f"Captured stdout:\n{stdout}")
```
Currently, the same command line processed by the `quote_argument()`
function looks like `cmd.exe /c dir "\"C:\Program" "Files\""`, which is
broken.
With this change, it looks correct: `cmd.exe /c dir "C:\Program Files"`.
Github-Pull: arun11299/cpp-subprocess#113
Rebased-From: ed313971c04ac10dc006104aba07d016ffc6542a
This suppresses the following warning caused by clang-20.
```
error: definition of implicit copy constructor for 'Streams' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
```
Copy constructor or move constructor is called when std::vector re-allocates
memory. In this case, move constructor should be called, because copying
Streams instances breaks file-descriptor management.
Communication class is modified as well, since it's instance is a member of
Streams class.
Github-Pull: arun11299/cpp-subprocess#107
Rebased-From: 38d98d9d20be50c7187b98ac9bc9a6e66920f6ef
The commit a32c0f3df4b6bcd1d2e93f19e8f380bb890cd507 introduced code to
silence MSVC's "warning C4996: The POSIX name for this item is
deprecated."
However, it exhibits several issues:
1. The aliases may leak into code outside the `subprocess.hpp` header.
2. They are unnecessarily applied when using the MinGW-w64 toolchain.
3. The fix is incomplete: downstream projects still see C4996 warnings.
4. The implementation lacks documentation.
This change addresses all of the above shortcomings.
Github-Pull: arun11299/cpp-subprocess#112
Rebased-From: 778543b2f2ca7f5d1c4f0241b635bbb265d750dd
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
Currently, wait() returns 0 on windows regardless
of the actual return code of processes.
Github-Pull: arun11299/cpp-subprocess#109
Rebased-From: 04b015a8e52ead4d8bb5f0eb486419c77e418a17
When passing in a rvalue reference, compiler
considers it ambiguous between std::string and
std::string&&. Making one of them take a lvalue
reference makes compilers correctly pick the right
one depending on whether the passed in value binds
to a rvalue or lvalue reference.
Github-Pull: arun11299/cpp-subprocess#110
Rebased-From: 2d8a8eebb03e509840e2c3c755d1abf32d930f33
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
We do not need to repeat the same test multiple times because BnB is
deterministic and will therefore always have the same outcome.
Additionally, this test was redundant because it repeats the "Smallest
combination too big" test.
Originally these tests verified that at a SelectCoins level that a
solution with fewer inputs gets preferred at high feerates, and a
solution with more inputs gets preferred at low feerates. This outcome
relies on the behavior of BnB, so we move these tests under the umbrella
of BnB tests.
Originally these tests relied on SFFO to work.
Recreates the tests in a new test suite coinselection_tests.cpp that is
based on UTXOs being created per their effective values rather than
nominal values and uses transactions with non-zero feerates.
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.
