💬 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)

🤔 ismaelsadeeq reviewed a pull request: "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict"
(https://github.com/bitcoin/bitcoin/pull/29680#pullrequestreview-2559684532)
Concept ACK

Left some initial comments

> Previously, I captured the replacement_tx when a tx was being kicked out of the Mempool, but with the addition of packages, any of the tx in the package could be the reason for the conflict.
I've changed the replacement_tx to be the last child tx in the package so the wallet doesn't have to watch the entire replacement package.

Can you elaborate on how that work, and the wallet detects that, what if the conflict is not from the child, it's from
...

💬 ismaelsadeeq commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1920580408)
In 4c64dd5a3cc4a48e5470fff26c99dde20f81c7e0 "Change MemPoolRemovalReason to variant type"
nit: Commit message should follow the guidelines in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

The explanation is a long should break it.

💬 ismaelsadeeq commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1920581830)
In 4c64dd5a3cc4a48e5470fff26c99dde20f81c7e0 "Change MemPoolRemovalReason to variant type"
nit: this line is too long, please break.