💬 maflcko commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1943469180)
Tested `time loadwallet` with my normal regtest wallet (https://github.com/bitcoin/bitcoin/issues/28510#issuecomment-1727561717), didn't review or check for correctness.

Before:

```
real 0m35.182s
```

After:

```
real 0m3.622s

💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1943470403)
I guess `time loadwallet` will be improved by https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1943469180

💬 maflcko commented on pull request "test: add missing tests for Assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-1943485615)
@aureleoules Looks like https://corecheck.dev/bitcoin/bitcoin/pulls/29428 is dead?

💬 maflcko commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1489258713)
nit: Instead of c-style casts, you can use non-narrowing C++11 casts like `float{thing}`, or `float(thing)`, if narrowing is required.

👍 alfonsoromanz approved a pull request: "wallet: Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722#pullrequestreview-1880045594)
Re-Tested ACK


📝 Sjors opened a pull request: "Stratum v2 Template Provider (take 3)"
(https://github.com/bitcoin/bitcoin/pull/29432)
Based on on @Fi3's https://github.com/bitcoin/bitcoin/compare/master...Fi3:bitcoin:PatchTemplates which is based on @ccdle12's #27854. I rebased it and re-wrote the commit history. Compared to #28983 it introduces EllSwift in the handshake and fixes various bugs. I used that opportunity to change the branch name, which makes testing against [SRI](https://github.com/stratum-mining/stratum) slightly easier. There's no conceptual discussion on #28983 so it can be ignored by reviewers.

See [docs/
...

✅ Sjors closed a pull request: "Stratum v2 Template Provider (take 2)"
(https://github.com/bitcoin/bitcoin/pull/28983)

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1943589901)
This PR is superseeded by #29432.

👍 theStack approved a pull request: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358#pullrequestreview-1880066965)
Tested ACK bf5662c678455fb47c402f8520215726ddfe7a94

💬 theStack commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1489336354)
in commit 5fc9db504b9ac784019e7e8c215c31abfccb62b6, method `add_outbound_p2p_connection`:
Is this wait needed? IIUC, the wait in the line below should be sufficient, as it can only pass if a v2 handshake has already happened

👍 willcl-ark approved a pull request: "net: make the list of known message types a compile time constant"
(https://github.com/bitcoin/bitcoin/pull/29421#pullrequestreview-1880064231)
ACK 2e312ea909ae6c466007acf791ea0e3356e954cb

This is a clean tidy-up, and as mentioned useful for #29418 but stands on it's own (as a smalle improvement) too.

Left one req for comment update if touched again.

💬 willcl-ark commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1489333827)
There is a comment in src/protocol.h#56 which could be updated to reflect the new constant too:

```cpp
/**
* Bitcoin protocol message types. When adding new message types, don't forget
* to update allNetMessageTypes in protocol.cpp.
*/
```

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-1943613979)
Updated and simplified the description to account for the new parent PR. I'm leaving this in draft status pending two merges on the SRI side; that way the whole thing is easier to test.

💬 maflcko commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1489365242)
```suggestion
auto filename{PathFromString(strprintf("legacy_%d", wallet_num));
```

I don't like the c_str below. Also, it doesn't compile on Windows.

Using `PathFromString` should be fine to "convert" ascii to a path segment, IIUC

Same above and https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1420361508

💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1489378056)
```suggestion

self.log.info('Testing abortrescan when no rescan is in progress')
```

No need to type the same thing twice

🤔 furszy reviewed a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1880133512)
> Not sure. I am using the spinning storage only to test migratewallet.

I'm asking because if other processes are equally slow, then this could be "ok" (and we could declare the hardware unsupported for core entirely...). But if only this process takes forever, then we are sure that there is an issue.

> It finished after 2 hours.

We are definitely not expecting this process to take that long. The wallet you shared is really small.

> Another problem is that (I presume) large single-ke
...

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1489381387)
fixed

👍 BrandonOdiwuor approved a pull request: "wallet: Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722#pullrequestreview-1880142626)
re-Tested ACK cccddc03f0c625daeac7158eb20c1508aea5df39

📝 bstin opened a pull request: "Update rpcauth.py"
(https://github.com/bitcoin/bitcoin/pull/29433)
This is a simple change to rpcauth.py utility in order to output as json instead raw text.

This is beneficial because integrating json output is simpler with multiple different forms of automation and tooling

💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1489408285)
> I don't like the c_str below. Also, it doesn't compile on Windows.

yeah, me neither.. I just did this 5 months ago. Fixing..