👍 stickies-v approved a pull request: "Remove legacy `Parse(U)Int*`"
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2847037118)
ACK, nice cleanup.

Introduces some slight behaviour change when passing integer arguments through startup options, the REST interface, as and through `bitcoin-cli` and `bitcoin-tx`. While some of these changes _may_ break existing user flows that were previously fine, they should hopefully be quite rare, and I think are worth the cleanup and more strict parsing.

💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093391212)
If we use `SAFE_CHARS_URI` here (seems appropriate since we're dealing with a uri) the `+` sign is returned in the error description:

<details>
<summary>git diff on fa24012c18</summary>

```diff
diff --git a/src/rest.cpp b/src/rest.cpp
index b340567bd1..464880224c 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -964,7 +964,7 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,

const auto blockheight{ToIntegral<int32_t>(height_str)};
if (!bl
...

💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093431546)
nit: looks safe because we're comparing with `tx.vin.size()`, but using `size_t` seems more appropriate for a container index? (+ for `MutateTxDelOutput`)

💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093425957)
nit: I think making this a `size_t` simplifies it a bit more?

<details>
<summary>git diff on fa24012c18</summary>

```diff
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 816f11045a..9e2c6d0902 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -229,13 +229,13 @@ static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)

static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInIdx)
{
- const auto idx{ToIntegral<
...

💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093439082)
It seems we now no longer have `ToIntegral` fuzz test coverage. It's a fairly simple function, so maybe that's fine, but so was `ParseIntegral`. Reckon it's worth keeping `ToIntegral` for the most common types in this harness?

💬 achow101 commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2093480971)
Yes

💬 achow101 commented on pull request "test: Remove legacy wallet RPC overloads":
(https://github.com/bitcoin/bitcoin/pull/32452#discussion_r2093483166)
With legacy wallets, this tested the `importprivkey` RPC. With descriptor wallets, it tests `importdescriptors` via the wrapper. But `importdescriptors` already has an explicit test on the next line, so this line is unnecessary.

💬 pablomartin4btc commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093487160)
If you do the `self.stop_node` with the warning [in it](https://github.com/bitcoin/bitcoin/pull/32449#pullrequestreview-2845981748) that would do (I've check this is also done in a few .py's - eg `feature_config_args.py`- and the buffer cleanup is done in `mining_mainnet`). Also because then you start the node without the settings, leaving it as before the test, could be cleaner, but up to you, it looks ok this way as well.

💬 maflcko commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093494320)
looks like dead code. `master` can never be less than 29

💬 maflcko commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093495170)
why the `.rpc.`?

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2093502405)
Ah, I see. I think that's unnecessary and the current test is sufficient to verify the behavior.

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093504031)
**sp**an?

💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#issuecomment-2887408696)
Addressed [feedback](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2089806374) removing unnecessary descriptor wallet checks.

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093515670)
"Aggregate" is used as an imperative verb here, so it is "(you,) aggregate (these) pubkeys"

📝 furszy opened a pull request: "restrict std::cerr to errors; use std::cout for warnings and info"
(https://github.com/bitcoin/bitcoin/pull/32538)
Warning messages were previously sent to `stderr`, which could cause them to
be misinterpreted as actual errors. This change redirects warning and informational
messages to `stdout` instead, making it explicit that they are not errors and
preventing unintended `stderr` handling.

To verify this behavior, added a test case triggering the simplest init warning I could find
(one that was actually not previously covered by tests): the `-bind` "bad-port" message.
So, running the test commit on
...

💬 furszy commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093528039)
Created #32538 as an alternative approach. Warning messages could be redirected to stdout instead of being sent through stderr. I'm not seeing a reason to treat them as errors.

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093530014)
It doesn't actually matter, the parameter does nothing as you can't do hardened derivation from the musig aggregate key.

💬 l0rinc commented on pull request "[IBD] precalculate SipHash constant salt calculations":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-2887450663)
@laanwj, @sipa, I've addressed the concerns you've raised, A re-review would be really appreciated here

💬 sipa commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-2887460978)
> Why would a 64 bit system care about the 32 bit limits?

It wouldn't. This PR only affects 32-bit systems.

💬 maflcko commented on pull request "restrict std::cerr to errors; use std::cout for warnings and info":
(https://github.com/bitcoin/bitcoin/pull/32538#issuecomment-2887470962)
> Warning messages were previously sent to `stderr`, which could cause them to
> be misinterpreted as actual errors.

Not sure.

* When something literally starts with `Warning: ...`, I fail to see how it can be misinterpreted.
* This change means that warnings may be missed in tests/CI or elsewhere (see the failing CI).
* Sending warnings to stderr is normal and expected. See for example `python -c 'print(b"\p")'`