💬 ryanofsky commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491078659)
In commit "wallet: Use scriptPubKey cache in GetScriptPubKeyMans" (38cfcf3435a5113606c8066833d49a4ab560aea9)

New code is no longer calling CanProvide. Would be good to use Assert or Assume to document that it should be true and verify integrity of the cache. (Same comment applies to GetSolvingProvider in next commit too)

💬 ryanofsky commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491016791)
In commit "wallet: Introduce a callback called after TopUp completes" (e06705140d09adb9baf83e24babc575ecdfc0f38)

Passing both WalletStorage callbacks and a separate topup_callback to DescriptorScriptPubKeyMan objects seems awkward. Is there a reason topup_callback is not just a WalletStorage method?

💬 ryanofsky commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491071095)
In commit "wallet: Use scriptPubKey cache in IsMine" (523010b7bded819fdc11396b307d9772ec8ffe33)

Would be helpful to say what cache lookups are an alternative to in this context. Maybe add comment "Search m_cached_spks cache to only call IsMine on relevant SPKMs instead of calling it on everything in m_spk_managers." Could add similar comments to GetScriptPubKeyMan and GetSolvingProvider in following commits too.

💬 maflcko commented on issue "wallet: pre-HD HDD migratewallet ":
(https://github.com/bitcoin/bitcoin/issues/29438#issuecomment-1946219576)
> Can you provide the raw perf data?

Does it not reproduce for you? If not, then the problem may be entirely different.

💬 brunoerg commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1491112250)
nit (fa2a4fdef779b01e847def5985deafedc6dd3da8): We could test negative values as well.

🤔 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