💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-1944409727)
cc: @vasild @naumenkogs @amitiuttarwar @mzumsande

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1489927269)
Except the macOS native CI... https://github.com/Sjors/bitcoin/actions/runs/7905485495/job/21578223892?pr=34#step:7:6060

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1489958435)
Working on a fix in https://github.com/Sjors/bitcoin/pull/34

💬 luke-jr commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1944469750)
It is intentional that compact blocks become less efficient the more miners deviate from the network norms. That's a reason for miners to comply with nodes, not nodes to comply with miners.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1489998271)
ASAN is tripping up somewhere around here. The last thing it logs is ` - Connect 2 transactions:`. It doesn't get to ` - Verify ... txins:`. I wonder if this is related to mock time, which I'm testing in https://github.com/Sjors/bitcoin/pull/34

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1490043524)
I don't understand your comment, sorry. They're listed in increasing feerate, with decreasing size as a tie breaker.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1490054499)
I should probably look into using `StaticContentsSock`

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1490088941)
You are right, it's not strictly needed in `add_outbound_p2p_connection`, (though it is in `add_p2p_connection`, where the corresponding wait depends on the `send_version` option which may be unset).
I think it makes sense conceptually because it kind of decouples the v2 logic from the message logic. If we'd introduce a similar parameter `send_version` also for `add_outbound_p2p_connection` (e.g. because we might want to craft a custom version message like `p2p_sendtxrcncl.py` does) we'd need
...

💬 pablomartin4btc commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1490109535)
I'll remove it entirely as it was also [pointed out](https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1484148120) by @hebasto.

💬 pablomartin4btc commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1490112584)
I've reconsidered @hebasto suggestion and will drop the `~/.transifexrc` section.

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