👍 ryanofsky approved a pull request: "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner"
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2826281937)
Code review ACK c8a3fabe4090fa2b9a0e8cef73ed5365e8221ad6

Thanks for the cleanup! This change should make the node interfaces.cpp more managable and organize code better. I left some suggestions for renames and API tweaks but they are not very important.

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

Would be better to return `std::unique_ptr<CBlockTemplate>` instead of `std::optional<std::unique_ptr<CBlockTemplate>>` here because unique_ptr is already nullable.

Also `node::WaitNext` is kind of a vague name. Maybe a name like `WaitAndCreateNewBlock` could be more descriptive. Or maybe just `CreateNextBlock`. Could be other ideas.

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

I think name `ChainTipChanged` could be a little misleading since this returns a bool and you might think it returns true when the tip has changed, false otherwise. But in reality it returns true whether or not this tip has changed (after the timeout), and the only case it returns false is when the node is shutting down.

To be less confusing I think it would be good to
...

💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2080455367)
Would using `ConsumeTransaction` make sense and just pushing that transaction into the block?

💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2080456412)
I used `ConsumeTransaction` in [44298f0](https://github.com/bitcoin/bitcoin/pull/32243/commits/44298f0cb9c3de42b1c0f464c0d7b2779aa4c3b7)

let me know if that looks good or if you would prefer to use `ConsumeScript`

🚀 ryanofsky merged a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660)

👍 ryanofsky approved a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2826389039)
Code review ACK a04f17a1882407db09b0a07338e12877ac1d9e92

Thanks for digging up the history! At least it's clear how the check get here, though not exactly what it was trying to do.

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2080480454)
In commit "doc: warn that CheckBlock() underestimates sigops" (a04f17a1882407db09b0a07338e12877ac1d9e92)

This comment is good to have but would be nice to clarify:

1. Why it is not a problem to underestimate?
2. What this check accomplishes if isn't accurate?
3. If it might be better to remove the check or update it?

Might suggest expanding comment to something like "This check underestimates the number of sigops because it does not count witness and p2sh sigops. This is not a problem
...

🤔 w0xlt reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2826415285)
reACK https://github.com/bitcoin/bitcoin/pull/27286/commits/51d4846f648e79e3e098f1f1bea7d5e823ebdea8

🤔 w0xlt reviewed a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436#pullrequestreview-2826428063)
ACK https://github.com/bitcoin/bitcoin/pull/32436/commits/1ee698fde2e8652d8eb083749edb8029c003b8db

💬 w0xlt commented on pull request "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`":
(https://github.com/bitcoin/bitcoin/pull/32432#issuecomment-2864385970)
Thanks for the review and suggestion @musaHaruna . Added that text to the PR's description.

🤔 pablomartin4btc reviewed a pull request: "contrib: remove bdb exception from FORTIFY check"
(https://github.com/bitcoin/bitcoin/pull/32448#pullrequestreview-2826445260)
cr ACK f9dfe8d5e0d3f628659702ab781b7919505c829f

FWIW: Tried to run the check on Ubuntu and it seems some lief symbols have [changed](https://github.com/lief-project/LIEF/blob/main/doc/sphinx/changelog.rst) previously to the latest released version (< 0.16.5 - eg `lief.EXE_FORMATS.ELF` => `lief.ELF.Binary`), corrected it locally and ran succesfully, then saw that the CI has 0.13.2, this works well too.

💬 murchandamus commented on pull request "coinselection: Optimize BnB exploration":
(https://github.com/bitcoin/bitcoin/pull/32150#issuecomment-2864494772)
The preceding PR https://github.com/bitcoin/bitcoin/pull/29532 got merged, so this is now ready for review.

👍 hodlinator approved a pull request: "cmake: Add application manifests when cross-compiling for Windows"
(https://github.com/bitcoin/bitcoin/pull/32396#pullrequestreview-2826464805)
ACK 477b69ad19d7fe37de9ca23926bcf007b309876d

### Concept

Paves the way for more intentional Windows manifest usage. [Linked documentation](https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests) confirms that it can be used to control codepage and specify supported Windows versions.

### Approach

Not something I could have easily written from scratch but makes logical sense in how the new CMake function `add_windows_application_manifest` generates a manifest file
...

📝 achow101 opened a pull request: "test: Remove legacy wallet RPC overloads"
(https://github.com/bitcoin/bitcoin/pull/32452)
`RPCOverloadWrapper` implemented overloads for legacy wallet only RPCs so that the same function call could be used within tests for both legacy wallets and descriptor wallets. With legacy wallets now removed, there is no need to continue to have these overloads.

For `importaddress`, `addmultisigaddress`, and `importpubkey`, the uses of these are converted to `importdescriptors`.

For `importprivkey`, a new helper function `wallet_imporprivkey` is introduced that does what the overload did.
...

📝 JeremyRubin opened a pull request: "[Policy] Discourage Unsigned Annexes"
(https://github.com/bitcoin/bitcoin/pull/32453)
This patch adds a "redundant" rule with the annex discouragement, to discourage Annexes that are present but do not get signed.

This PR does not include tests, as the code paths are entirely redundant (and small!) with existing annex discouragement. If desired, tests could be made.

This new policy would come into play if someone somewhere for some reason decided that they were going to modify standard policy on their node to allow annexes **even though it is abundantly clear that annex pol
...

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2080598493)
I could not figure out the CI failure, so I've reverted this change.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2864638615)
> Simplest fix would probably be to make the CWallet object a static variable in the function like the TestingSetup variable is, instead of a global variable.

It can't be static because the `FUZZ_TARGET` below needs to access it. However, the wallet could be created in there instead of in the setup, as the other wallet fuzz tests do.

But, with moving `WriteBestBlock` back to `RemoveWallet`, this is not necessary either.

👍 theStack approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2826561409)
Concept and code-review ACK 8c360d78b13fa7136db8d6e1e0af5d05f5141a64

💬 theStack commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2080597980)
Seeing this constant also tricked me into thinking that the script size limit might play a role here. Agree that a comment would be nice, or maybe just bump it up to something much higher like e.g. 50000.