💬 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")'`

🤔 pablomartin4btc reviewed a pull request: "restrict std::cerr to errors; use std::cout for warnings and info"
(https://github.com/bitcoin/bitcoin/pull/32538#pullrequestreview-2847419694)
Concept ACK

I think some tests using `stop_node` with the warning in the args would fail as in `is_node_stopped` checks for the warning in `stderr` (eg `wallet_multiwallet.py`), it needs to be fixed too.

💬 pablomartin4btc commented on pull request "restrict std::cerr to errors; use std::cout for warnings and info":
(https://github.com/bitcoin/bitcoin/pull/32538#issuecomment-2887476146)
> This change means that warnings may be missed in tests/CI or elsewhere (see the failing CI).

That's my concern with this approach.

💬 purpleKarrot commented on issue "cmake: Cannot find Qt 6 on SunOS / illumos (OpenIndiana Distribution)":
(https://github.com/bitcoin/bitcoin/issues/32536#issuecomment-2887477698)
It sure is annoying when system packages are installed to a location where they cannot be found by default. On the other hand, package maintainers may have their reasons for doing that. There are probably multiple packages which depend on Qt6 and are built with CMake. It is very likely that package maintainers have their build environment set up for that.

Trying to work around that "issue" creates a maintenance burden for us and potentially for package maintainers too, because our workaround ma
...

🤔 l0rinc reviewed a pull request: "node: cap `-maxmempool` and `-dbcache` values for 32-bit"
(https://github.com/bitcoin/bitcoin/pull/32530#pullrequestreview-2847428745)
Looking at the code, I see that I misunderstood the description, I though we're capping the values in every circumstance, but now I see that the architectures are handles independently.

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093554238)
Done, added to musig.h

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093554514)
Updated the comments.

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093554645)
Done

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093554764)
Done