💬 theStack commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1490120993)
Thanks for elaborating, that makes sense!

💬 pablomartin4btc commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#issuecomment-1944766074)
I've addressed feedback from reviewers, thanks to all!

💬 achow101 commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1944835540)
I made a non-HD legacy wallet with double the number of transactions, put it on an external hdd, and migrated it. Migration took 230 seconds, which, while slow, is certainly not multiple hours.

```
$ src/bitcoin-cli -regtest -rpcwallet=/run/media/ava/F5CD-B8E0/big_nonhd_wallet getwalletinfo
{
"walletname": "/run/media/ava/F5CD-B8E0/big_nonhd_wallet",
"walletversion": 60000,
"format": "bdb",
"balance": 251.59681094,
"unconfirmed_balance": 49.00000000,
"immature_balance": 0.
...

💬 willcl-ark commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1944841685)
I was able to reproduce it with wallet version 60000 and 1k keys/tx I also see 30 mins...

```fish
₿ /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/regtest-v60000 getwalletinfo
{
"walletname": "",
"walletversion": 60000,
"format": "bdb",
"balance": 14716.40625000,
"unconfirmed_balance": 0.00000000,
"immature_balance": 78.12500000,
"txcount": 1000,
"keypoololdest": 1707941916,
"keypoolsize": 1000,
"paytxfee": 0.00000000,
"private_ke
...

💬 furszy commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945080734)
q: the wallet to be migrated, is already loaded by the time you call `migratewallet`? Or you are migrating a non-loaded wallet?

Because, if the wallet wasn't loaded by the newer version yet, it will run the `UpgradeKeyMetadata` process during migration too. Which involves performing writes for 2 times the number of your key pool.

I just updated my branch batching that process too. In case someone want to test: https://github.com/furszy/bitcoin-core/tree/2024_wallet_batch_migration_multi_in
...

🤔 tdb3 reviewed a pull request: "test: add script compression coverage for not-on-curve P2PK outputs"
(https://github.com/bitcoin/bitcoin/pull/28193#pullrequestreview-1881593385)
Good work increasing unit test robustness. Looks good to me. Only a nit was observed. Author's choice for nit inclusion/exclusion.
ACK for 28287cfbe18b6c468f76389e9f66bdcf2d00f8d1.

Test actions taken:
Checkout the PR branch, built, ran all unit and functional tests (all passed).

💬 tdb3 commented on pull request "test: add script compression coverage for not-on-curve P2PK outputs":
(https://github.com/bitcoin/bitcoin/pull/28193#discussion_r1490257674)
nit: Looks like the approach employed is to pick a random X value until one is found that is not on the curve. What is the rationale for using randomness, rather than choosing a fixed X value that is not on the curve (e.g. `fd0473a380ddf239accbc31770fcc6e64096930eaa8bc57c10f5868f3596fe1e`)? Seems like the other tests in the test file use randomness as well (e.g. `GenerateRandomKey()`), so maybe I'm missing something. Selecting a known invalid X value would technically increase test repeatabil
...

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1945219371)
> They aren't really blocking but I'm still struggling with the memory consumption topic. Isn't `std::unordered_map` going to store all keys (entire scripts) in memory for duplicated so it can solve hash collisions?

Yes.

I've done some memory usage profiling with a large wallet, and various combinations of ordered and unordered maps and sets.

The baseline memory usage of `m_map_script_pub_keys`, which is the same for all of the following, is 1.2 MiB

* `std::unordered_map<CScript, std
...

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1490258337)
Done

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1490258989)
I've dropped this `CanProvide`.

💬 amitiuttarwar commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490381331)
instead of an optional of a vector, you could have the default value explicitly be all networks, or an empty vector be interpreted as all networks. probably the first one is clearer.

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-1945395946)
@sr-gi, i tried [this manually sending idea](https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1854149947) but i still think intermittent failures are possible there.
- we can't get rid of `can_data_be_received` variable because if we don't use this variable, test would succeed irrespective of whether we send 4 bytes network magic first or 4 bytes from ellswift bytes first and we don't want that.
- so since `data_received()` always happens in `Network thread` and send of ellswif
...

💬 achow101 commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945397211)
I'm not entirely convinced that migration is actually spending most of the time in disk I/O for these big wallets.

This is a flamegraph of the migration of my big nonhd wallet on master:

Ignoring the locking stuff since I didn't turn off the lock debugging, it seems like a big chunk of time is spent in `IsMine` after the descriptors have been created. This suggests to m
...

💬 achow101 commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945433701)
I'm any case, I didn't think the performance issues should be a blocker for this PR. It's clear that migration works, it may just take a while.

Missing this release _will_ push back the timeline another release cycle, if not more.

👍 stickies-v approved a pull request: "log: Nuke error(...)"
(https://github.com/bitcoin/bitcoin/pull/29236#pullrequestreview-1881976826)
ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96

💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945528839)
> I'm any case, I didn't think the performance issues should be a blocker for this PR. It's clear that migration works, it may just take a while.
>
> Missing this release _will_ push back the timeline another release cycle, if not more.

Yes, I agree. Sorry for not being clear.

Since it appears that at least one other person could reproduce a longer migrate time (30 minutes), why not mention that the user should take care to disable the client rpc timeout? Something along the lines of:
...

💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945535057)
Ok, I didn't see that you already did that. :sweat_smile:

lgtm ACK f1684bb88a878eb3f54e945db39ea69b14256eef

⚠️ Hitechmaroh opened an issue: "35c1089b1cf59c00a526ebd8b233494d500c71b75208bcd1bc469b665a36214e"
(https://github.com/bitcoin/bitcoin/issues/29437)
0x6Bb39276bA115B89F97433BF12307CcBA2d575Dc

✅ hebasto closed an issue: "35c1089b1cf59c00a526ebd8b233494d500c71b75208bcd1bc469b665a36214e"
(https://github.com/bitcoin/bitcoin/issues/29437)

💬 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_r1490642807)
```suggestion
for (const auto& msg : g_all_net_message_types) {
```

nit: Add missing `{` while touching?