💬 sdaftuar 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_r1489790971)
> If `old`, `new`, and `tail` are CFeeFrac objects, this condition is exactly `!((new - old) << tail)`. If new is smaller than old, the `new - old` object has negative size.

@sipa With the various `Assume()` calls that check that if the size is 0, the fee must also be zero, doesn't that mean that we can't really write code like this example you gave? Unless we checked that new != old first -- otherwise you might create a FeeFrac with 0 size and non-zero fee.

⚠️ achow101 opened an issue: "wallet: Unrelated conflicted parent txs do not cause child txs to be marked as conflicted"
(https://github.com/bitcoin/bitcoin/issues/29435)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

If we receive a transaction which spends from an unconfirmed parent, and that parent is replaced, the child does not get marked as conflicted but rather only as inactive. If the child contains an input from our wallet, that input cannot be reused until the child is abandoned.

See also https://github.com/ACINQ/eclair/pull/2818

### Expected behaviour

We should try to detect if a such a
...

📝 brunoerg opened a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436)
This PR changes addrman's `Select` to support multiple networks and change `ThreadOpenConnections` to call it with reachable networks. It can avoid unnecessary `Select` calls and avoid exceeding the max number of tries (100), especially when turning a clearnet + Tor/I2P/CJDNS node to Tor/I2P/CJDNS. Compared to #29330, this approach is "less aggresive". It does not add a new init flag and does not impact address relay.

I did an experiment of calling `Select` without passing a network until it
...

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