💬 maflcko commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1490885359)
Maybe good to re-check this when/after the cmake migration is done?

💬 maflcko commented on issue "wallet: pre-HD HDD migratewallet ":
(https://github.com/bitcoin/bitcoin/issues/29438#issuecomment-1945940628)
Putting a perf context around the RPC call (`with n.profile_with_perf("migrate-wallet-perf"):`) allows to generate flame graphs. However, I am not sure how useful they are, as they may not capture IO?

SSD:



HDD:


💬 hebasto commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#discussion_r1490894282)
It seems there are no reasons to keep `libbitcoin_crypto_sse4` library anymore. The `crypto/sha256_sse4.cpp` source might be a part of the `libbitcoin_crypto_base`, no?

✅ maflcko closed an issue: "wallet: pre-HD HDD migratewallet "
(https://github.com/bitcoin/bitcoin/issues/29438)

💬 maflcko commented on issue "wallet: pre-HD HDD migratewallet ":
(https://github.com/bitcoin/bitcoin/issues/29438#issuecomment-1945949263)
Closing for now, but feel free to leave a comment if you think this should be re-opened. Happy to provide more details, if there are questions.

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490912711)
Yes, nice catch. I'll address it.

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490938595)
It doesn't work with vector I guess. But since I'll change it to `unordered_set`, it will work.

💬 GBKS commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-1946005139)
Definitely more consistent this way.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1490966954)
The pointers in the options and in `CTxMemPool` are supposed to be optional values (see the first commit message). Using a raw pointer for optional non-owned values seems like an acceptable pattern.

👍 ryanofsky approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1882699845)
Code review ACK c02fd0379c425f486f1b80a81962c1aa68b8a852. Just rebase and moving test_deterministic_coverage.sh change since last review

🤔 hebasto reviewed a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1882740742)
Approach ACK 7ef33b2bad71a230428b653d6d0297df49630d99.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1490999199)
re: https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489457095

@vasild, this seems like an argument to always use shared_ptr or unique_ptr everywhere and never use plain pointers or references. If you would not want a plain reference to be stored here, is there any case where you would want one to be stored?

To me, the lifetime of the signals object seems pretty easy to determine and reason about. In bitcoind and bitcoin-qt, there is only one signals object, it is created when th
...

💬 hebasto commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-1946087439)
> Not sure. All info displayed on the About page is fixed details about the software itself, not about the current (test)chain.

In that case, shouldn't the logo on the About page be black-toned?

🤔 furszy reviewed a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1882761405)
diff ACK c02fd037

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1491007792)
Empty container and nothing means the samething, all networks.

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1491008021)
Nice idea! I'll address it.

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1491008584)
Nice, I'll address it.

👍 ryanofsky approved a pull request: "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1882763867)
Code review ACK 0a3b5739ce8b9a2d219bcf3208069436c66fd5bb. Just rebase and some python test cleanups since last review

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491014229)
See the comments [here](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455548750) for the reasoning. Basically we want to guard in the shutdown (`Stop`) case and it is less important to do so in the rest, since we have tests heavily exercising those paths. Question is also on which de-reference we should be guarding and what to do about the existing unguarded lines that call `m_chain` and its members.

🤔 hebasto reviewed a pull request: "bitcoin-config.h includes cleanup"
(https://github.com/bitcoin/bitcoin/pull/29404#pullrequestreview-1882777594)
Concept ACK.

https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1933987554:
> So I guess we could add a little ... lint job for this instead?

Agree. Without a linter, the code might be drifted back in its current state.