💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1946203298)
It would be interesting where this changes the code. I presume the places are:

* The `timingsafe_bcmp` impl in the code was previously always picked. Now it may or may not be picked.
* `LogQtInfo` will always print "dynamic" for plugin. Now it will print the correct thing.



Also, there is an iwyu false-positive in the CI output:

```
httpserver.cpp should remove these lines:
- #include <config/bitcoin-config.h> // lines 6-6

💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1946203944)
ACK 9d1dbbd4ceb8c04340927f5127195dc306adf3f 🚪

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 9d1dbbd4ceb8c04340927f51271
...

👍 ryanofsky approved a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1882776315)
Code review ACK b20d882fd53c21098eb7858b2dbca84eafd2b344. I don't have much insight into over performance impact of this change, but it seems like it should make migrated legacy wallets much faster (10 times faster loading in one case: https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1943469180) even if it does increase memory usage, and and it seems to be implemented correctly.

re: https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1876002633

> Apart from that, jus
...

💬 ryanofsky commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1491049788)
In commit "wallet: Cache scriptPubKeys for all DescriptorSPKMs" (75f0fcf2ef17e3844dac6445913f31fecc4f76b1)

It would be helpful to have a comment here explaining why this is necessary. I assume this is related to the "Use no-op topup_callback, cache is filled later in migration" comments in the earlier commit, but I don't understand why migration code needs this special flow instead of using topup_callback / CacheNewScriptPubKeys like other wallet code. So this would be a good place for a comm
...

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