📝 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

💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089312337)
In commit "interfaces: refactor: move `waitTipChanged` implementation to miner" (62fc42d475df4f23bd93313f95ee7b7eb0d4683f)


Not sure, but I think it might be a good idea to drop the 4th commit c39ca9d4f7bc9ca155692ac949be2e61c0598a97 and instead change the 5th commit 62fc42d475df4f23bd93313f95ee7b7eb0d4683f to use the `kernel_notifications.TipBlock()` consistently instead switching to `chainman.ActiveChain().Tip()` at the end:

<details><summary>diff</summary>
<p>

```diff
--- a/src/no
...

💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089303383)
In commit "interfaces: refactor: move `waitNext` implementation to miner" (720f201e652885b9e0aec8e62a1bf9590052b320)

Would be good to change `const std::unique_ptr<CBlockTemplate>& block_template` to just `const CBlockTemplate& block_template` since it doesn't look like it's allowed to be null.

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

Friendly ping @davidgumberg @hodlinator @sipsorcery :)

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2880958368)
Rebased to include the new comment added in #31624.

The tests I added don't cover the difference in sigops counting. Suggestions are welcome, it could look for "CheckBlock failed" vs "ConnectBlock failed".

💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2089388056)
I rebased #31981 after this was merged and suggested one way we could add test coverage for it.

💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089392199)
As long as getTip() is in the interface it seems consistent with the rest of this PR to move the implementation, so no need to drop c39ca9d4f7bc9ca155692ac949be2e61c0598a97?

Meanwhile you suggestion can be still be used in the 5th commit.