💬 theStack commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2103562671)
That makes sense, thanks for clarifying (missed that the one-parameter variant was not `noexcept`). Might be worth it to add a small comment about this in a follow-up PR.

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103574633)
Expanded the comment to say that.

🤔 1440000bytes reviewed a pull request: "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs"
(https://github.com/bitcoin/bitcoin/pull/32596#pullrequestreview-2862873626)
ACK https://github.com/bitcoin/bitcoin/pull/32596/commits/e5cbea416b2f63e5d99819052f3e69a6383336d6

<details>
<summary>Signature</summary>

<pre>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK e5cbea416b2f63e5d99819052f3e69a6383336d6
-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQSSUwYqT5LWNFkIXKYtIwUgISpZAQUCaC++7QAKCRAtIwUgISpZ
AZIaAP9bxiLFrXtgDk7+yoGgHAUO2xP2AmG5qRcYtZDSxuqkmAEAotL4R3L9iDoX
NZeq+w5EEr+4SO7zLYo+Cnb7krWYpQg=
=pUtX
-----END PGP SIGNATURE-----
</pre>
</detai
...

📝 achow101 opened a pull request: "wallet: Always set descriptor cache upgraded flag for new wallets"
(https://github.com/bitcoin/bitcoin/pull/32597)
Newly created wallets will always have an upgraded descriptor cache, so set those.

Also, to verify this behavior, add a new `flags` field to `getwalletinfo` and check that in the functional tests.

Split from #32489

📝 achow101 opened a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598)
Many exceptions thrown for corruption are `std::runtime_error`; we should catch those and log the message to help with debugging.

Split from #32489

💬 1440000bytes commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2903002368)
Concept ACK

💬 davidgumberg commented on pull request "wallet: Fix logging of wallet version":
(https://github.com/bitcoin/bitcoin/pull/32553#issuecomment-2903003935)
post-merge ACK https://github.com/bitcoin/bitcoin/pull/32553/commits/4b2cd0b41ff4800c8801f2c44883eaec60a035fa

💬 hebasto commented on issue "`system_tests/run_command` test fails on illumos OS":
(https://github.com/bitcoin/bitcoin/issues/32574#issuecomment-2903353573)
The underlying issue is not specific to illumos.

With the following patch:
```diff
--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(run_command)
const std::string expected{"err"};
BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
const std::string what(e.what());
- BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned
...

💬 maflcko commented on pull request "rev_32343_wip_nomerge_ci":
(https://github.com/bitcoin/bitcoin/pull/32535#issuecomment-2903371304)
The failure remains, when triggered, so it would be good to investigate this. Let's continue discussion in https://github.com/bitcoin/bitcoin/issues/32524

✅ maflcko closed a pull request: "rev_32343_wip_nomerge_ci"
(https://github.com/bitcoin/bitcoin/pull/32535)

💬 maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2903373050)
This is closed, but the failure remains, when triggered, so it would be good to investigate this long-term at some point. Discussion can continue here, or in a new issue.

💬 hebasto commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2903379032)
> * It's not clear to me what the problem in [#32574](https://github.com/bitcoin/bitcoin/issues/32574) on illumos or actually is or how this change fixes it.

I've updated https://github.com/bitcoin/bitcoin/issues/32574 with more details.

> On the c++11 issue, I think maybe just compiling the subprocess unit test in c++11 would be a good way to ensure compatibility. It seems cmake has a [CXX_STANDARD](https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html) per-target property. I
...

💬 hebasto commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2103886263)
A related discussion takes place at https://github.com/bitcoin/bitcoin/pull/32577.

⚠️ maflcko opened an issue: "intermittent issue in feature_init.py (bitcoind should have exited with expected error LevelDB error: Corruption)"
(https://github.com/bitcoin/bitcoin/issues/32600)
https://cirrus-ci.com/task/5185890844147712?logs=ci#L3553:

```
267/267 - [1mfeature_init.py [0m failed, Duration: 2420 s
[17:16:50.450]
[17:16:50.450] [1mstdout:
[17:16:50.450] [0m2025-05-21T20:36:30.563000Z TestFramework (INFO): PRNG seed is: 630547506121562679

...

[17:16:50.450] 2025-05-21T20:36:49.282000Z TestFramework (INFO): Perturbing file to ensure failure /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20250521_201845/feature_init_53/node0/regtest/indexes/coinstats/db/0
...

💬 vasild commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2903394731)
Concept ACK to improve this, but:

> what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion? ... one addnode peer the power to let our entire IBD come to a halt completely

Would it be too difficult to request a given block from more than one peer concurrently? That is, if a block is requested from a peer and not received within some time, then request it from other(s) as well, but don't cancel the original request. Then we take
...

💬 hebasto commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2103895670)
This is a first use of the `Popen(std::initializer_list<const char*> cmd_args, Args&& ...args)` ctor. Before this, it was a candidate for cleanup.

💬 maflcko commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#issuecomment-2903414680)
> `CHECK_THROW`

I don't think this solves issue (1). Instead of NONFATAL being inaccurately named in debug builds, it will now be THROW, because the check is neither nonfatal nor throwing in debug builds.

> Then, current `assert` uses could become `CHECK`, current `Assume` uses in hotspots could become `DCHECK`, majority of other `Assume` uses could become `CHECK_LOG`, and current `CHECK_NONFATAL` uses could become `CHECK_THROW`.

No objection, just mentioning that this would be a larger
...

💬 hebasto commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2903499043)
Tested 82aeba60c2b3a166302e10fbf6e3eaf0593fab4d.

The `-startupnotify="echo {\"success\": true}"` option prints `{success: true}`, which is the same as on the master branch.

But would it be appropriate to expect `{"success": true}` instead? If not, what is the correct way to output a quoted string, which may be required to represent a valid JSON object?

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2903559188)
utACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e

Since my last review this just adds `throw std::runtime_error("execvp returned unexpectedly")` and the copyright change.

💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104017923)
Just to clarify my initial comment: I can remove this loop _in the final version of this PR_ without breaking any test.