97eaadc3bf 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 97eaadc3bf
davidgumberg:
untested crACK 97eaadc3bf
achow101:
ACK 97eaadc3bf
laanwj:
Code review ACK 97eaadc3bf
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.
a8333fc9ff scripted-diff: wallet: rename plain and encrypted master key variables (Sebastian Falbesoner)
5a92077fd5 wallet: refactor: dedup master key decryption (Sebastian Falbesoner)
846545947c wallet: refactor: dedup master key encryption / derivation rounds setting (Sebastian Falbesoner)
a6d9b415aa wallet: refactor: introduce `CMasterKey::DEFAULT_DERIVE_ITERATIONS` constant (Sebastian Falbesoner)
62c209f50d 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 a8333fc9ff
Tree-SHA512: 5a66d3b26f481347d0b5b4f742dd237803a35aad6e3480ed15fd38b7fa3700650bd5f67f4c30ed88f5fad45d6cd4c893fe4f1657e36e563b4294fd3596187724
524f981bb8 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 524f981bb8
darosior:
utACK 524f981bb8
ismaelsadeeq:
ACK 524f981bb8
Tree-SHA512: c066debc34a021380424bd21b40444071b736325e41779a41590c2c8a6822ceeaf910fe067817c1dba108210b24c574977b0350b29520502e7af79d3b405928b
7e8ef959d0 refactor: Fix Sonar rule `cpp:S4998` - avoid unique_ptr const& as parameter (Lőrinc)
e400ac5352 refactor: simplify repeated comparisons in `FindChallenges` (Lőrinc)
f670836112 test: remove old recursive `FindChallenges_recursive` implementation (Lőrinc)
b80d0bdee4 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 7e8ef959d0
sipa:
utACK 7e8ef959d0
hodlinator:
re-ACK 7e8ef959d0
Tree-SHA512: 9e52eff82a7d76f5d37e3b74c508f08e5fced5386dad504bed111b27ed2b529008a6dd12a5116f009609a94c7ee7ebe3e80a759dda55dd1cb3ae52078f65ec71
b9d4d5f66a 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 b9d4d5f66a
Tree-SHA512: e9f0a7ec0c46f21c0377d5174e054a6569f858630727f94dac00c0cb7c241c56892d0b902706d6dd53880cc3b5ae1f2dba9caa1fec40e64cd4cf0d34493a49c1
abe43dfadd doc: release note for #27826 (Sjors Provoost)
f9fa28788e Use LogBlockHeader for compact blocks (Sjors Provoost)
bad7c91479 Log which peer sent us a header (Sjors Provoost)
9d3e39c29c 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 (e5ce857634), 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 abe43dfadd
achow101:
ACK abe43dfadd
vasild:
ACK abe43dfadd
Tree-SHA512: 081e0de62cbd8a0b35cf54daaa09e3e6991d0cc9f706ef3eb50908752fe7815de69b367f7313381c90cd8d5de0ae5f532d1cd54948c5c1133b1832f266d9c232
f1b142856a test: Same addr, diff port is already connected (David Gumberg)
94e85a82a7 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 f1b142856a
achow101:
ACK f1b142856a
theuni:
utACK f1b142856a
mzumsande:
Code Review ACK f1b142856a
Tree-SHA512: d744b60e9bace121faa3a746463f6b6e0e6ef08eac0e7879326cbd5f4721e47e6e10f6203dfd3870a2057c4ddd1860692c070ef048a76d773b84e6c2f840cc86
e3014017ba test: add IsActiveAfter tests for versionbits (Anthony Towns)
60950f77c3 versionbits: docstrings for BIP9Info (Anthony Towns)
7565563bc7 tests: refactor versionbits fuzz test (Anthony Towns)
2e4e9b9608 tests: refactor versionbits unit test (Anthony Towns)
525c00f91b versionbits: Expose VersionBitsConditionChecker via impl header (Anthony Towns)
e74a7049b4 versionbits: Expose StateName function (Anthony Towns)
d00d1ed52c versionbits: Split out internal details into impl header (Anthony Towns)
37b9b67a39 versionbits: Simplify VersionBitsCache API (Anthony Towns)
1198e7d2fd versionbits: Move BIP9 status logic for getblocktemplate to versionbits (Anthony Towns)
b1e967c3ec versionbits: Move getdeploymentinfo logic to versionbits (Anthony Towns)
3bd32c2055 versionbits: Move WarningBits logic from validation to versionbits (Anthony Towns)
5da119e5d0 versionbits: Change BIP9Stats to uint32_t types (Anthony Towns)
a679040ec1 consensus/params: Move version bits period/threshold to bip9 param (Anthony Towns)
e9d617095d versionbits: Remove params from AbstractThresholdConditionChecker (Anthony Towns)
9bc41f1b48 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 e3014017ba
TheCharlatan:
Re-ACK e3014017ba
darosior:
ACK e3014017ba
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>
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.
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
`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>
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.
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
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>
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.
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
acee5c59e6 descriptors: Have GetPrivKey fill keys directly (Ava Chow)
4b0303197e descriptors: Move FlatSigningProvider pubkey filling to GetPubKey (Ava Chow)
25a3b9b0f5 descriptors: Have GetPubKey fill origins directly (Ava Chow)
6268bde0af descriptor: Remove unused parent_info from BIP32PUbKeyProvider::GetPubKey (Ava Chow)
0ff072caa1 wallet, rpc: Only allow keypool import from single key descriptors (Ava Chow)
Pull request description:
Instead of having `MakeScripts` infer what pubkeys need to go into the output `FlatSigningProvider`, have each of the `PubkeyProviders` that have `GetPubKey` and `GetPrivKey` called fill it directly with relevant keys and origins.
This allows for keys and origins to be added that won't directly appear in the output, which is necessary for `musig()` descriptors.
Split from #29675
ACKs for top commit:
fjahr:
Code review ACK acee5c59e6
theStack:
re-ACK acee5c59e6
rkrux:
ACK acee5c5
Tree-SHA512: c1841359bcb08cdd433122deef96579236928660785f3357a3eb584e47d290cd1c60ebe8f7fba50f178ba45c9a90773124e0f509e36c5a0df97c1a4890e03e5c
e3d7533ac9 test: improves tapscript unit tests (Ethan Heilman)
3e167085ba test: Ensures test fails if witness is not hex (Ethan Heilman)
Pull request description:
This commit creates new test utilities for future Taproot script tests within script_tests.json. The key features of this commit are the addition of three new tags: `#SCRIPT#`, `#CONTROLBLOCK#`, and `#TAPROOTOUTPUT#`. These tags streamline the test creation process by eliminating the need to manually generate these components outside the test suite.
* `#SCRIPT#`: Parses Tapscript and outputs a byte string of opcodes.
* `#CONTROLBLOCK#`: Automatically generates the control block for a given Taproot output.
* `#TAPROOTOUTPUT#`: Generates the final Taproot scriptPubKey.
This code was originally part of the OP_CAT PR https://github.com/bitcoin/bitcoin/pull/29247 but was pulled out into a separate PR to reduce the rebase treadmill for the OP_CAT PR.
Additionally this PR adds a check to ensure that if the witness data can not be parsed as hex the test fails. Prior to this PR, the test code would fail silently and set the values it couldn't parse as empty stack elements. This fix was suggested by @instagibbs.
## Rationale
While writing JSON script tests (script_tests.json) for https://github.com/bitcoin/bitcoin/pull/29247 we ran into the following problem. The JSON script tests are simple and easy to write for pre-Tapscript scripts, but adding or changing a Tapscript test requires substantial work per test. Consider the following pre-tapscript test:
```
["'aa' 'bb'", "CAT 0x4c 0x02 0xaabb EQUAL", "P2SH,STRICTENC", "DISABLED_OPCODE", "CAT disabled"]
````
whereas a Tapscript test for the same script (annotated with comments for better readability) would look like:
```
[
[
"aa",
"bb",
"7e4c02aabb87", // output script
"c0d6889cb081036e0faefa3a35157ad71086b123b2b144b649798b494c300a961d", // control block
0.00000001
],
"",
"0x51 0x20 0x15048ed3a65748549c27b671936987093cf73a4c9cb18522a74fb9553060ca99", // Tapscript output
"P2SH,WITNESS,TAPROOT",
"OK",
"TAPSCRIPT CATs aa and bb together and checks if EQUAL to aabb"
]
```
Computing the Tapscript output, such as `0x51 0x20 0x15048ed3a65748549c27b671936987093cf73a4c9cb18522a74fb9553060ca99`, requires writing custom code and running it for each test. The same is true for the Tapscript control block, such as `c0d6889cb081036e0faefa3a35157ad71086b123b2b144b649798b494c300a961d`. If a test is changed or updated new outputs and control blocks must be computed. The complexity of doing this is likely the reason that no one has added any Tapscript tests to JSON script tests until this PR.
In this PR we address this issue by adding the following improvements to JSON script tests:
Adding simple macros ("#SCRIPT# and #CONTROLBLOCK#) that allow the script test parser to automatically generate and inject a valid Tapscript output and control block to be computed automatically from the JSON script.
Allowing Tapscript scripts to use the human readable strings like pre-script scripts by marking the location of the script in the witness stack using #SCRIPT#. This transforms the unreadable script 7e4c02aabb87 into #SCRIPT# CAT 0x4c 0x02 0xaabb EQUAL.
This results in the following JSON script test which is far easier to write and easier to read.
```
[
[
"aa",
"bb",
"#SCRIPT# CAT",
"#CONTROLBLOCK#",
0.00000001
],
"",
"0x51 0x20 #TAPROOTOUTPUT#",
"P2SH,WITNESS,TAPROOT,OP_CAT",
"OK",
"TAPSCRIPT Test of OP_CAT flag by calling CAT on two elements. TAPSCRIPT_OP_CAT flag is set so CAT is executed."
],
```
ACKs for top commit:
instagibbs:
reACK e3d7533ac9
sipa:
utACK e3d7533ac9
janb84:
Re ACK [e3d7533](e3d7533ac9)
Tree-SHA512: 948c3ec28a4b2b222c2d77e48918ed19d298b51d64662fc20959073edd9978fc796516a392da9755a7e173f556e3021816dc6ce8eb3ed16bbe0fa6ebc574fd48
This commit creates new test utilities for future Taproot script
tests within script_tests.json. The key features of this commit are the
addition of three new tags: `#SCRIPT#`, `#CONTROLBLOCK#`, and
`#TAPROOTOUTPUT#`. These tags streamline the test creation process by
eliminating the need to manually generate these components outside the
test suite.
* `#SCRIPT#`: Parses Tapscript and outputs a byte string of opcodes.
* `#CONTROLBLOCK#`: Automatically generates the control block for a given
Taproot output.
* `#TAPROOTOUTPUT#`: Generates the final Taproot scriptPubKey.
Update src/test/script_tests.cpp
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
5cb1241814 feefrac: avoid integer overflow in temporary (Pieter Wuille)
Pull request description:
In `FeeFrac::Div(__int128 n, int32_t d, bool round_down)` in src/util/feefrac.h, the following line computes the result:
```c++
return quot + (mod > 0) - (mod && round_down);
```
The function can only be called under conditions where the result is in range, and thus doesn't involve any integer overflow. However, the intermediary result computed by just `quot + (mod > 0)` may still overflow if it's going to be corrected by the `- (mod && round_down)` that follows.
Fix this by balancing the two correction steps with each other first:
```c++
return quot + ((mod > 0) - (mod && round_down));
```
Fixes#32294.
ACKs for top commit:
l0rinc:
Tested ACK 5cb1241814
maflcko:
lgtm ACK 5cb1241814
achow101:
ACK 5cb1241814
Tree-SHA512: 9daaccdf9acd7652d53b52cad2dc12872558265e863acdde2d6015f885cb87c0505f9bd5be5499fc0a0eded29bec719643f6af1fbc3604518143985094226c95
