🤔 brunoerg reviewed a pull request: "rpc: Fixed signed integer overflow for large feerates"
(https://github.com/bitcoin/bitcoin/pull/29434#pullrequestreview-1882956966)
light crACK fa2a4fdef779b01e847def5985deafedc6dd3da8

💬 maflcko commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1491115366)
This is unrelated to this pull request, so I am happy to review another pull that adds this check.

💬 achow101 commented on issue "wallet: pre-HD HDD migratewallet ":
(https://github.com/bitcoin/bitcoin/issues/29438#issuecomment-1946227192)
> Does it not reproduce for you (or anyone else)? If not, then the problem may be entirely different.

My test wallet was not nearly as slow. I'm in the process of making a bigger one.

💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491131500)
Done.

💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491133503)
Done as a separate commit because it required changing `NetMsgType::*` constants to `constexpr` as well.

A +36 / -77 change, thanks!

💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491136346)
> Why the sort?

https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1491029750

You are right that comparing the pointers here is wrong. I added a custom sort function to compare the contents. Thanks!

💬 maflcko commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491140321)
nit: If `string_view` are stored, the compiler can derive this code and you won't have to change this line of code.

👍 BrandonOdiwuor approved a pull request: "rpc: Fixed signed integer overflow for large feerates"
(https://github.com/bitcoin/bitcoin/pull/29434#pullrequestreview-1883000447)
Code Review ACK fa2a4fdef779b01e847def5985deafedc6dd3da8

💬 maflcko commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491144268)
Now it may put all string literals in all translation units as well. Would be good to benchmark the effect, if any.

💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491148886)
Just tried that - if I store the constants as `string_view`, e.g. `constexpr std::string_view VERSION{"version"};`, then I get this greeting:

```
test/util/net.cpp:34:9: error: no matching function for call to 'Make'
34 | NetMsg::Make(NetMsgType::VERSION,
| ^~~~~~~~~~~~
./netmessagemaker.h:14:23: note: candidate function template not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'std::string' (aka 'basic_string<
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1946293497)
Bumping macOS to 14 on the CI does not help. I also can't reproduce this failure on my own Intel macOS machines, not on 13.6.4 and not on 14.2.1. A `Sock` mock is probably the most robust solution, but it'd be nice to find another workaround.

👍 ryanofsky approved a pull request: "log: Nuke error(...)"
(https://github.com/bitcoin/bitcoin/pull/29236#pullrequestreview-1883061202)
Code review ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96

This is a nice improvement, and it actually doesn't change many lines of code, so I don't think we should be worried about conflicts if we want to make more improvements later, like changing log levels, or adding categories, or switching to util::Result.

One thing I think we probably should revisit after this is the interaction with `-logsourcelocations`, since it seems like a lot (maybe most?) of the new LogError calls are manually
...

💬 maflcko commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1946313480)
```
test/sharedlock_tests.cpp:18:17: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
18 | workers.push_back(std::thread(task));
| ^~~~~~~~~~~~~~~~~~~~~~ ~
| emplace_back(

👍 ryanofsky approved a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1883074045)
Code review ACK f1684bb88a878eb3f54e945db39ea69b14256eef

"This RPC may take a long time to complete" seems like much more helpful and accurate documentation than "This call may not work as expected and may be changed in future releases" at this point.

💬 TheCharlatan commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1946371110)
Rebased c34dcea019ea3b639b9f387df18777bc9a2d1534 -> 7882748c1c260dd77c22ac3a85bad18768c08f0d ([setEntryRefs_4](https://github.com/TheCharlatan/bitcoin/tree/setEntryRefs_4) -> [setEntryRefs_5](https://github.com/TheCharlatan/bitcoin/tree/setEntryRefs_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/setEntryRefs_4..setEntryRefs_5))

* Fixed conflict with #29424

👍 willcl-ark approved a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1883175987)
ACK f1684bb88a878eb3f54e945db39ea69b14256eef

Agree that this should no longer be marked as experimental in the next release even though some migrations do appear to take a(n unexpectedly) long time.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1946417576)
Thank you for the review @vasild,

Updated c02fd0379c425f486f1b80a81962c1aa68b8a852 -> 4e270f3aa70f0bb4e7053d84d5cdd1a1fdcf85c2 ([noGlobalSignals_15](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_15) -> [noGlobalSignals_16](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_16), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_15..noGlobalSignals_16))

* Addressed @vasild's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1
...

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1491224522)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1484708844

> The next paragraph refers to this deleted comment ("using the nStatus flag mentioned above") and should be updated too.

Thanks, fixed

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1491225507)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1483671675

> comment can be removed, it's no longer faked.

Good catch, dropped this comment.

🤔 ryanofsky reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1883156605)
Thanks for the review!

Updated c9ca4ca4d970f9d42d0254775c04d114bd2152c3 -> 8ab4e07a1a54c9f4818a557fd1182bbb59e1cf9b ([`pr/nofake.7`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.7) -> [`pr/nofake.8`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nofake.7..pr/nofake.8)) with suggestions