🤔 danielabrozzoni reviewed a pull request: "config: allow setting -proxy per network"
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2840193686)
Code Review ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64

I have reviewed the code, and it looks good to me.
I run the tests locally and did some very limited manual testing, mimicking the functional test behavior (passing in `-proxy=...=network` and checking the `getnetworkinfo` output)

💬 l0rinc commented on pull request "refactor: Remove UB in prevector reverse iterators":
(https://github.com/bitcoin/bitcoin/pull/32490#issuecomment-2880252363)
tested ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685

Thanks for fixing it properly - this explains why I couldn't trigger an actual failure (local or CI) in https://github.com/bitcoin/bitcoin/pull/32296.

💬 polespinasa commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2088973429)
I agree, want it deleted too. Just added this comment because of the discussions some core members were having yesterday on twitter regarding what deprecation means bc people were getting crazy saying it means removal.

But it's just an observation, feel free to ignore.

💬 hodlinator commented on issue "qa: Failure in wallet_basic.py spendzeroconfchange test":
(https://github.com/bitcoin/bitcoin/issues/32456#issuecomment-2880297237)
@mzumsande thanks for fixing this! Sorry I didn't have the time to fully understand how the wallet syncs before the fix was merged.

💬 l0rinc commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088990681)
Split into 2 commits now, minimizing the same line being changed multiple times.
Also dropped the `prevector` related change which you've fixed properly in https://github.com/bitcoin/bitcoin/pull/32490

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2880338215)
All the points about drawbacks of addressing this issue with a comment ("I usually treat comments as admitting defeat") are well taken and substantive, but I think it is good to merge this PR anyway because it accurately points out a confusing behavior that could cause bugs. I don't think we need to admit defeat if we can clear the way for new approaches by not spending too much time debating this particular approach. So am planning to merge this shortly.

🚀 ryanofsky merged a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624)

📝 fanquake opened a pull request: "doc: remove Carls substitute server from Guix docs"
(https://github.com/bitcoin/bitcoin/pull/32498)
This no-longer exists. Use one of the other Guix servers in the example.

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2089083910)
I think you're right that this can't happen in the current codebase. `TestBlockValidity` is only called by `checkBlock()`. When this is called via the IPC interface, it will have a fresh `CBlock` each time (unless we start supporting shared memory between processes). Simiarly when called by the `generateblock` RPC or `getblocktemplate` in `proposal` mode it will have a fresh instance.

The code in `miner_tests.cpp` does reuse the same `CBlock` across multiple calls, though not in parallel.


...

👍 stickies-v approved a pull request: "refactor: Remove UB in prevector reverse iterators"
(https://github.com/bitcoin/bitcoin/pull/32490#pullrequestreview-2840449544)
ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685, nice find.

Verified the UB, and removing the dead code makes sense. I'm getting the same clang-format diff for `prevector_tests.cpp` (+ copyright year change).

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2089101819)
I'm confused about what you're suggesting here.

👍 maflcko approved a pull request: "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`"
(https://github.com/bitcoin/bitcoin/pull/32296#pullrequestreview-2840461109)
Makes sense to drop the file-wide (!) suppression in favour of just documenting the intentional casts explicitly in the code.

review ACK 516f0689b511c09153e4b6b4a58dfedd61c6cda7 🎈

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5n
...

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2089109680)
That would mean change from:

```cpp
bool IsConnectedToAddrPort(const std::string& addr_port);
bool IsConnectedToAddrPort(const CService& addr_port);
bool IsConnectedToAddr(const CNetAddr& addr);
```

to:

```cpp
bool AlreadyConnectedToAddressPort(const std::string& addr_port);
bool AlreadyConnectedToAddressPort(const CService& addr_port);
bool AlreadyConnectedToAddress(const CNetAddr& addr);
```

I am fine either way. @hodlinator, what do you think?

✅ maflcko closed an issue: "wallet: balance gone when tx broadcast failed"
(https://github.com/bitcoin/bitcoin/issues/20943)

💬 maflcko commented on issue "wallet: balance gone when tx broadcast failed":
(https://github.com/bitcoin/bitcoin/issues/20943#issuecomment-2880505282)
Closing as duplicate of https://github.com/bitcoin/bitcoin/issues/11887 for now

✅ maflcko closed an issue: "intermittent issue in wallet_backwards_compatibility.py: line 245, in run_test assert txs[3]["abandoned"] AssertionError"
(https://github.com/bitcoin/bitcoin/issues/29806)

💬 maflcko commented on issue "intermittent issue in wallet_backwards_compatibility.py: line 245, in run_test assert txs[3]["abandoned"] AssertionError":
(https://github.com/bitcoin/bitcoin/issues/29806#issuecomment-2880545543)
Closing again for now. I guess it is unlikely that a user runs into this (outside of testing). My preference would still be https://github.com/bitcoin/bitcoin/pull/18840, but I guess I don't mind too much.

📝 hebasto opened a pull request: "cmake: Restrict MSVC-specific workaround to affected versions"
(https://github.com/bitcoin/bitcoin/pull/32499)
The recent MSVC version 17.14 has fixed a [bug](https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-with-st/10830350), so we can now enable compilation of `fuzz/utxo_snapshot.cpp` with the updated compiler.

💬 hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2880564520)
> https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-with-st/10830350#T-N10903514 . So I guess this was fixed in Visual Studio 2022 v17.14?

Also see https://github.com/bitcoin/bitcoin/pull/32499.

💬 maflcko commented on pull request "cmake: Restrict MSVC-specific workaround to affected versions":
(https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2880571731)
No objection, but I guess it also seems fine to wait a few weeks until GitHub bumps their Windows image and then just bump the msvc minimum required version.