💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2598808316)
re: https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2556956641

> It seems better to either not install these binaries or to put them out of the way in libexec/ to avoid confusion. [...] Would like to see more discussion about this and find out where the disagreement is so binaries don't have to move twice, but this doesn't need to block the PR.

I opened #31679 to move internal binaries to libexec, since using libexec isn't directly related to what this PR is trying to do. W
...

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920217085)
It's `extern const` rather than `constexpr` because its defined in the cpp file, rather than the header -- for constexpr stuff you need to include its value in the header.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920262478)
This period is probably too low for testnet3 which would have ~20k periods analysed and cached rather than the ~1400 that it should have with the BIP9 recommended period size of 2016.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920266744)
Changed this, should be more obvious now.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920433539)
Ah, a later commit had added `using enum ThresholdState` to avoid the need for this in new code, guess I didn't move it quite early enough. Moved it earlier and changed the following line for consistency.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920447244)
They're implementation details that aren't required to use the interface, but also can't just go in the .cpp file (because of tests). `addrman_impl.h` is similar (though I see it's been included in rpc/net.h because `AddrInfo` has been added to addrman.h's interface without the class being moved into that file...) It's just one module, split into three files, so the "circular dependency" is just a false positive.

👍 ryanofsky approved a pull request: "Add multiprocess binaries to release build (except Windows, OpenBSD)"
(https://github.com/bitcoin/bitcoin/pull/30975#pullrequestreview-2559568739)
Code review ACK b924ee0b85f17f684c215ac164d685cfa747556c. Latest version looks good now, assuming CI passed. Includes host_os variable bugfix and some suggested simplifications since last review.

💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1920515277)
Changed it to set `period = chainparams.GetConsensus().DifficultyAdjustmentInterval();` which is much the same, but doesn't rely on the TESTDUMMY deployment.

💬 sipa commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920531697)
Any RPC **in** which one of the parameters...

💬 1440000bytes commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2598860948)
Concept ACK

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920535577)
Done

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920535704)
Done.

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2598867587)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920362724 and https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920531697

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1920546152)
Does this hard-coded thread name cause any issues if sockman is used more than once?

💬 furszy commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920553269)
This could also be a WIP formatted private key right?
Should use the general term "key" here. E.g.
```c++
strprintf(key '%s is invalid due to an extra whitespace at %s of the string', key_str, (IsSpace(str.front())?"beginning" : "end"));`
```

🤔 sipa reviewed a pull request: "net: Use GetAdaptersAddresses to get local addresses on Windows"
(https://github.com/bitcoin/bitcoin/pull/31014#pullrequestreview-2559638620)
Concept and code review ACK 219872fc75e552c87c20b5c1ce7e15fa887eec20, but I am not familiar with the Windows network API, and have not tested it.

💬 sipa commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#discussion_r1920557024)
The loops seems unnecessary to me.

```c++
out_buf.resize(std::min(std::max(out_buf_len, out_buf.size() * 2), MAX_ADAPTER_ADDR_SIZE));
```

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920583787)
Yes, it could be a WIF private key. I will change it to use "key".

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920591046)
Done.

💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2598956762)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1920553269 (thanks, @furszy)