👍 hodlinator approved a pull request: "windows: Use predefined `RC_INVOKED` macro instead of custom one"
(https://github.com/bitcoin/bitcoin/pull/32633#pullrequestreview-2876232435)
crACK b97fc15df593d42296015218db197f377a71461c

The custom `WINDRES_PREPROC` #define was only read in clientversion.h. The built-in `RC_INVOKED` #define in the resource compiler is more straight forward to use to skip over sections of files that compiler does not support.

🤔 pablomartin4btc reviewed a pull request: "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet"
(https://github.com/bitcoin/bitcoin/pull/31423#pullrequestreview-2876222607)
code review ACK

Checked all pushes diffs. I like the latest/ current version with the refactoring & dedup re-loading wallets code. Since you touched that area, and there's no test for solvables wallet when there's no main/ local, perhaps you could add one?

I'll it test in a bit.

💬 pablomartin4btc commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2112616684)
nit: only if you to re-touch
```suggestion
# Always verify the backup path exists after migration
```

💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2917430246)
Rebased for silent merge conflict

🚀 achow101 merged a pull request: "test: fix sync function in rpc_psbt.py"
(https://github.com/bitcoin/bitcoin/pull/32630)

💬 achow101 commented on pull request "wallet: sqlite: there is no need to have exclusive locking when mocking":
(https://github.com/bitcoin/bitcoin/pull/32632#issuecomment-2917435899)
Does this materially change anything for performance? If not, I'd rather keep it as is for simplicity.

💬 kevkevinpal commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2917459536)
ACK [fe0432b](https://github.com/bitcoin/bitcoin/pull/32607/commits/fe0432b1c4a10b74844c2dedefccfe340c0d3b10)

🤔 Crypt-iQ reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-2876289798)
My main concern here is if this disables FIBRE in a future where it's used again; FIBRE apparently used unsolicited compact blocks (see: https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2905778748). If we are not concerned about that, then Concept ACK.

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2112660103)
I think this check is still permissive enough such that a `NetMsgType::CMPCTBLOCK` message can be sent to a node in -blocksonly mode and still get processed. This would require the -blocksonly node to request the block from the peer.

Maybe something like this instead?
```diff
+. if ((!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) || m_opts.ignore_incoming_txs) {
```

💬 davidgumberg commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2112704010)
Fixed, thanks.

💬 davidgumberg commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#issuecomment-2917542306)
Rebased to address @instagibbs feedback.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2917545576)
The PR title may need updating, as `-maxorphantxs` is gone now.

👍 hodlinator approved a pull request: "build: Add resource file and manifest to `bitcoin.exe`"
(https://github.com/bitcoin/bitcoin/pull/32634#pullrequestreview-2876260861)
ACK dbb2d4c3d54759f8c346882b6f98d26d5bb8e816

(Apologies for not catching this as I reviewed both of the conflicting PRs).

Ran into a similar issue yesterday when temporarily including bench_bitcoin.exe in a Windows Guix build to verify an optimization. See the collapsed "Patch" section under the "Guix + Windows" heading in https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2874254446.

Diffed new .rc file against bitcoind-res.rc to confirm only reasonable content differs.

...

💬 hodlinator commented on pull request "build: Add resource file and manifest to `bitcoin.exe`":
(https://github.com/bitcoin/bitcoin/pull/32634#discussion_r2112641918)
nit: Could update this for all existing RC files in commit prior to introducing this new file?
```suggestion
VALUE "CompanyName", "Bitcoin Core"
```

💬 davidgumberg commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#discussion_r2112707324)
I was pretty puzzled by the annotations I think in part because of the deprecated "lock/unlock" terminology instead of "acquire/release", but also because the meaning of `UNLOCK_FUNCTION`/`ACQUIRE` appears to shift from the constructor to the destructor.

A reasonable explanation of these semantics is provided by their author: https://bugs.llvm.org/show_bug.cgi?id=36162#c7
> ```cpp
> class SCOPED_CAPABILITY AutoUnlock {
> public:
> // Release mu, implicitly acquire *this and connect it
...

🤔 w0xlt reviewed a pull request: "log: Additional compact block logging"
(https://github.com/bitcoin/bitcoin/pull/32582#pullrequestreview-2876384059)
reACK https://github.com/bitcoin/bitcoin/pull/32582/commits/83df64d7491b8271f7dfa2aea30f055102e3ff39

💬 instagibbs commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#issuecomment-2917570611)
reACK https://github.com/bitcoin/bitcoin/pull/32582/commits/83df64d7491b8271f7dfa2aea30f055102e3ff39

💬 davidgumberg commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2917613764)
crACK 96a341d4064132292621af404de908f01fbe3e2f

`src/node/interfaces.cpp`'s `FillBlock()` is incorrectly annotated, and no warning is emitted, cherry-picking the commits that fix `reverse_lock`'s annotations, the thread safety analyzer complains as it should about `FillBlock()`:

```bash
$ git checkout --detach master && git cherry-pick 0065b9673db5da2994b0b07c1d50ebfb19af39d0^..96a341d4064132292621af404de908f01fbe3e2f
$ CC=clang CXX=clang++ cmake -B build && cmake --build build -j $(nproc
...

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112592805)
In commit "[prep/test] modify test to not access TxOrphanage internals"

It seems all invocations of `Random()` assume that the returned value won't be `nullptr` anyway, so I think they can just be replaced with:

```c++
CTransactionRef txPrev = orphans_added[m_rng.randrange(orphans_added.size())];
```

🤔 sipa reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2876188083)
Some comments already.