💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1913911212)
This is done to align the method with PSBT. It could be removed without breaking the change. But it also is just clearing data which should take minimal time. I think it's worth keeping to align with PSBT format.

🤔 mzumsande reviewed a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2548206643)
Code Review ACK 66dbfaca22ba55aea3a5e7ab73974b42134ba8f4

💬 mzumsande commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#discussion_r1913933802)
could use `UNREACHABLE_PROXY_ARG` here as well

👍 tdb3 approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2548335954)
code review re ACK b945668216abbc978ce351b8a89910e651c6e595

Changes were to address the latest comments (removing "0x" and adjusting a comment).

```diff
$ git range-diff 228aba2c4d9ac0b2ca3edd3c2cdf0a92e55f669b..58b828f7f7ad216c7d7bf2e2ff431a66591e9d5c e8ea483fec05d824d2f35182b1a3bd68416b2df3~..b945668216abbc978ce351b8a89910e651c6e595
1: 6accee1844 = 1: e8ea483fec consensus: add DeriveTarget() to pow.h
2: 3e976d4171 = 2: 6a72497f50 build: move pow and chain to bitcoin_common
3
...

🤔 tdb3 reviewed a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2548351524)
Concept ACK

Nice. Better consistency/privacy as well as a test runtime reduction. Will circle back to take a deeper look.

💬 darosior commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#issuecomment-2588847257)
I was playing with a couple routers and figured i'd test this. Could successfully map ports against an OPNSense box and a Spectrum "[WIFI 6E router](https://www.spectrum.com/internet/wifi-service/spectrum-advanced-wifi)".

💬 luke-jr commented on pull request "wallet: allow lable for external descriptor & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2588907624)
It would be nice to have a test that checks all 4 success/failure cases.

💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914319868)
I've moved the new user to the strange_user list, renamed user3 to strangedude6 for consistency, and replaced "" with None as suggested here https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1904497634

💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914324487)
https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914319868

💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914326733)
Done https://github.com/bitcoin/bitcoin/pull/29858/commits/cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b

💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#issuecomment-2589165785)
> Code review ACK [fcf0ead](https://github.com/bitcoin/bitcoin/commit/fcf0ead6cd358fd35909e1102bdae2c176b0ace6). I left suggestions below, but none are important and they can be ignored. I think it is better to have this test coverage for this than not to have it, and no need to spend too much time iterating on the PR just to improve test code quality which is already decent.

Thanks for the review, I have pushed an update and resolved all the suggestions, https://github.com/bitcoin/bitcoin/
...

💬 vasild commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2589289729)
`66dbfaca22...2ed161c5ce`: do https://github.com/bitcoin/bitcoin/pull/31646#discussion_r1913933802

💬 vasild commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#discussion_r1914418707)
Indeed! Done, thanks!

💬 hodlinator commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2589299296)
(I did a second run on 433412fd8478923dfdb20044f74c5d1e19fa8dd8, it took 3h45m53s, 9.5 minutes / 4.4% faster. So there is some variance).

💬 l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2589302015)
These are the fastest IBDs I've seen so far

💬 Sjors commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2589341922)
It seems useful to illustrate the change here by regenerating the alternative mainnet blocks introduced in #31583, switching their coinbase outputs to taproot addresses. Before this PR the last block, which spends a coinbase output, would fail with `unexpected-witness`.

Marking as draft when that lands.

📝 Sjors converted_to_draft a pull request: "miner: always treat SegWit as active"
(https://github.com/bitcoin/bitcoin/pull/31625)
The `getblocktemplate` RPC would check if SegWit has activated yet at the tip. This has been the case for more than seven years.

💬 hebasto commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2589347934)
> > The root cause of the issue is that the -Wl,-z,separate-code linker option is incompatible with NetBSD's dynamic linker.
>
> That's pretty surprising, because `-z,separate-code` has been enabled by default, in the NetBSD linker, for the upcoming 11.0 release. https://www.netbsd.org/changes/changes-11.0.html#x86:
>
> > Enable the -z separate-code security feature by default in [ld(1)](https://man.netbsd.org/ld.1).
>
> It seems kinda unlikely they'd do that if the option was incompati
...

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914459164)
dec5f8ba8ff579e3e708079dde8a7ff732884479: @maflcko I'm puzzled by the need to `deepcopy` here, but if I just do `block.vtx = txs` then in the next call to `mine`, without passing any transactions in, `txs` will have the coinbase of the previous block in it.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914465891)
Well the linter forced me to change the `txs` default to `None`, which "solved" the issue.