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

💬 hebasto commented on pull request "cmake: Restrict MSVC-specific workaround to affected versions":
(https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2880581947)
> 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.

Do we really need to force Windows developers to update their toolchains in this case?

📝 fanquake opened a pull request: "init: drop `-upnp`"
(https://github.com/bitcoin/bitcoin/pull/32500)
This was slated for removal in `30.0`, so remove it.

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2880617674)
I took @ryanofsky's patch to keep BlockValidationState in the interface _implementation_ and validation code.

Also split the test into multiple functions.

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2089185404)
Done (though I didn't put too much effort into making them actually independent).

💬 maflcko commented on pull request "init: drop `-upnp`":
(https://github.com/bitcoin/bitcoin/pull/32500#issuecomment-2880634484)
No objection, but if this reintroduces the `settings.json` error, I can also see waiting one more release (or so), because the migration code looks minimal and harmless.

💬 fanquake commented on pull request "init: drop `-upnp`":
(https://github.com/bitcoin/bitcoin/pull/32500#issuecomment-2880642968)
Yea I don't mind. I guess we'll have to touch this in either case, as we shouldn't have a note saying to remove things in `30.x`, if they aren't being removed in `30.x`.

💬 maflcko commented on pull request "cmake: Restrict MSVC-specific workaround to affected versions":
(https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2880645437)
> Do we really need to force Windows developers to update their toolchains in this case?

I expect the number of Windows devs is still small enough to just ping and ask them, when the time has come.

💬 Kixunil commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2089248938)
Good that finally someone commented. I think that this should go in line with the wishes of limit-promoters so if adding zero amount is undesirable it should be changed.

> Don't forget the scriptPubKey lengths.

IIUC these are accounted for by the `size` method.

💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2089279588)
@Kixunil the serialized size isn't included, but it was also not included in master. You could say it's a bug in the current code as well, but my intention here is to do no worse from payload perspective.

👍 ryanofsky approved a pull request: "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner"
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2840779063)
Code review ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f. Lots of suggest suggest changes made since last review, altering function names and signatures and also adding new commit to drop negative fee handling. I like the idea of making the wait function return a BlockRef, that is clearer than what I suggested. Left some comments below but they are not important and this looks good as-is

💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089293653)
In commit "interfaces: remove redundant coinbase fee check in `waitNext`" (02d4bc776bbe002ee624ec2c09d7c3f981be1b17)

Might be help to mention #31897 in the commit description to clarify what "now does not include" refers to