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

💬 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_r1490651364)
Why not constexpr?

Also, this may put a copy of this array in all translation units, but I guess this is fine, as there is no other way to achieve it.

💬 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_r1490653235)
Why the sort? Also, are you sure that sorting pointers is comparing the content it points to, as opposed to the memory address value?

💬 Hitechmaroh commented on issue ".":
(https://github.com/bitcoin/bitcoin/issues/29437#issuecomment-1945673196)
0xA69babEF1cA67A37Ffaf7a485DfFF3382056e78C

💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945683530)
> How many keys does that wallet have (it'll be in the debug log, look for "Legacy Wallet Keys")? This one I just made is 2333 keys.

There are more keys than transactions, and they are encrypted, the debug log says:

```
Wallet file version = 10500, last client version = 269900
Legacy Wallet Keys: 0 plaintext, 4998 encrypted, 4998 w/ metadata, 4998 total.
Descriptors: 0, Descriptor Keys: 0 plaintext, 0 encrypted, 0 total.
Wallet completed loading in 229ms
setKeyPool.size()
...

👍 vasild approved a pull request: "rpc: Fixed signed integer overflow for large feerates"
(https://github.com/bitcoin/bitcoin/pull/29434#pullrequestreview-1882124132)
ACK fa19cf33f9dcfd3ccaa8012d09875fa939c02365

💬 vasild commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1490639830)
That is a bit confusing - the input is in BTC/kvB and the output is in sat/kvB. Maybe elaborate more:

```suggestion
/**
* Parse a json number or string into a fee rate. The input is interpreted as a number
* in BTC/kvB while the output is in sat/kvB.
* For example ParseFeeRate("1").GetFeePerK() == 100'000'000
* Reject negative values or rates larger than 1BTC/kvB.
*/
CFeeRate ParseFeeRate(const UniValue& json);
```