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

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112726648)
In commit "feature: Add TxOrphanageImpl"

Many of the includes here seem unused:

```diff
--- a/src/node/txorphanage_impl.h
+++ b/src/node/txorphanage_impl.h
@@ -5,33 +5,20 @@
#ifndef BITCOIN_NODE_TXORPHANAGE_IMPL_H
#define BITCOIN_NODE_TXORPHANAGE_IMPL_H

-#include <coins.h>
-#include <consensus/amount.h>
-#include <indirectmap.h>
#include <logging.h>
-#include <net.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
-#include <sync.h>
-#include <util/epoc
...

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112750662)
In commit "feature: Add TxOrphanageImpl"

This has an unstated assumption that `it` is the iterator the first entry in the `ByWtxid` index for a given wtxid.

Given that there is only one call site (`CountWtxid` below), maybe it is better to inline this function there?

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112773426)
In commit "feature: Add TxOrphanageImpl"

Nit: maybe `auto& peer_info = reconstructed_peer_info[it->m_announcer];` ?

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112744241)
In commit "feature: Add TxOrphanageImpl"

`Assume(peer_it != m_peer_orphanage_info.end());` ?

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112731246)
In commit "feature: Add TxOrphanageImpl"

Would it be possible to use `std::map<COutPoint, std::set<Iter>>` here as type? That would be faster (avoiding a lookup to resolve `Wtxid` -> `Iter`) and use less memory. `boost::multi_index` iterator objects are stable (remain valid as long as the object they point to exists), unlike `std::unordered_map`.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112783352)
In commit "feature: Add TxOrphanageImpl"

This could be avoided by checking the `ret.first` result of `auto ret = m_orphans.get<ByWtxid>().emplace(tx, peer, m_current_sequence);` below. The `ByWtxid` index is unique, so emplacement will fail if the same the wtxid/peer combination already exist, I think.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112772043)
In commit "feature: Add TxOrphanageImpl"

Nit: ` ` after `int64_t`.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2112777005)
In commit "feature: Add TxOrphanageImpl"

Style: `MIN_PEER`? It wasn't immediately clear when reading the code that this referred to a constant.

💬 achow101 commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2112803011)
In 6d9bf36b2c061088abb0d839fee90683ff1525e1 "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet"

Instead of having this output parameter, perhaps we could check if `local_wallet` has any SPKMs after `DoMigration`e.g. `if (local_wallet->GetAllScriptPubKeyMans().empty())`. These should be equivalent checks.