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

💬 hebasto commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2589365958)
Concept ACK.

💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914467060)
I guess you figured it out, given the CI failure https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2589364296 ?

💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1914473845)
thanks, I was wondering if there should be a blank line. for me it shows the table correctly (firefox, gh mobile) so I thought when in doubt just keep it compact.

weird that it displays correctly for me but not for you though (also tested with `gh-markdown-preview` locally).

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1914478423)
> Your suggested implementation LGTM but I think keeping std::numeric_limits<T>::digits instead of sizeof(T) * CHAR_BIT is preferable since it takes into account the sign bit?

As far as I understand it is legal to shift e.g. `int32_t{1} << 31` to get the minimum `int32_t` value. But I think in this case we should prohibit this quirk of the underlying bit representation and use `std::numeric_limits<T>::digits`.