💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817930536)
Fixed, thanks.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2439740158)
Rebased (to pick up the test introduced in #31152) and addressed latest feedback.

💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1817939448)
Ended up modifying a lot more in the latest push, so this line was formatted again

💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1817939675)
I've replaced all vectors to 64 bit ints, the new peproducer is: [godbolt.org/z/sc5xPTcW6](https://godbolt.org/z/sc5xPTcW6), which shows:
* _Clang_ (x86-64) - uses SSE (`movdqu`/`pshufd`/`pxor`) for SIMD operations, processing **32** bytes / iteration
* _GCC_ (x86-64) - uses simple 64-bit operations (`xor` with `QWORD PTR`), processes **16** bytes / iteration
* _RISC-V_ (32-bit) - uses basic 32-bit register operations (`lbu`, `xor`, `srli`, `sb`), processes **8** bytes / iteration

(please
...

📝 theStack opened a pull request: "scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp}"
(https://github.com/bitcoin/bitcoin/pull/31163)
The confusing "command" terminology for the 12-byte field in the (v1) p2p message header was replaced with the more proper term "message type" in other modules already years ago, see eg #18533, #18937, #24078, #24141. This PR does the same for the protocol.{h,cpp} module to complete the replacements. Note that "GetCommand" is a method name also used in the `ArgsManager` (there it makes much more sense), so the scripted-diff lists for this replacement the files explicitly, rather than using `$(gi
...

💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1817939890)
I've change the usages to avoid std::vector keys, this way GCC and Clang both agree that the new results are faster (even though clang manages to compile to 32 byte SIMD, while GCC only to 16 bytes per iteration, see https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1817939675)

💬 laanwj commented on pull request "util: Remove RandAddSeedPerfmon":
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2439914238)
> I haven't thought very seriously about what high quality entropy sources are and whether or not we have enough of them on Windows without this performance data

The high quality OS entropy source on windows is `CryptGenRandom`, and we use that.

Not opposed to collecting other ancillary data to mix in, though let's try to avoid something as crude as pulling data from all drivers and processes on the system.

📝 laanwj opened a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164)
Add a method `CNetMessage::GetMemoryUsage` and use this for accounting of the size of the process receive queue instead of the raw message size.

This ensures that allocation and deserialization overhead is taken into account.

💬 laanwj commented on pull request "scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp}":
(https://github.com/bitcoin/bitcoin/pull/31163#issuecomment-2439920615)
Concept ACK, always has been a pet peeve of mine.

💬 laanwj commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#issuecomment-2439923324)
i'll review this when #31130 is merged, i have some of my own ideas about portmap.cpp as well that i haven't fully worked out yet.

💬 davidgumberg commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2439923901)
> My reasoning was that there should be enough CI runs on master and all runs on PRs based on top of the merged fix.

Oh, didn't think of that, makes sense to me.

💬 laanwj commented on pull request "test: Don't enforce BIP94 on regtest unless specified by arg":
(https://github.com/bitcoin/bitcoin/pull/31156#discussion_r1818022053)
Might want to prefix this argument with `regtest` to make it doubly clear that it doesn't do anything on other networks.

👍 rkrux approved a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2397501918)
tACK 0ea84bc362f395fd247623c22942eb5ca3d1b874

👍 itornaza approved a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2397502622)
tACK 0ea84bc362f395fd247623c22942eb5ca3d1b874

Tested against all of the standard test and extended harness. Everything looks great!

👍 laanwj approved a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130#pullrequestreview-2397521287)
Code review ACK 40e5f26a3ff77e50df808f6f850c617aec2df203
i have also checked that:
- no mentions of miniupnp as a dependency remain
- `-natpmp` still works to forward ports on my router after this
- `-upnp`, if still present in the configuration file or command line, gives a custom message and exits

💬 sipa commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818083019)
I think it suffices to have a `bool m_have_changeset GUARDED_BY(cs){false};`, as it's only used for tracking and enforcing the "no two changesets" rule?

💬 sipa commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818083695)
Nit, if it's a member class of `CTxMempool` anyway, maybe a less clunky name like `ChangeSet` suffices?

💬 sipa commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818086982)
Style: use braces/newlines/indentation when the entire `if` statement doesn't fit on a single line.

📝 andrewtoth converted_to_draft a pull request: "validation: fetch block inputs on parallel threads ~17% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132)
When fetching inputs in ConnectBlock, each input is fetched from the cache in series. A cache miss means a round trip to the disk db to fetch the outpoint and insert it into the cache. Since the db is locked from being written during ConnectTip, we can fetch all block inputs missing from the cache on parallel threads before entering ConnectBlock. Using this strategy resulted in a ~17% faster IBD (or master was ~21% slower).

Doing IBD with 16 vcores from a local peer with default settings, sto
...

💬 sipa commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818101205)
I do find this function a bit confusing; it seems like `m_ancestors` is effectively a cache for the result of `m_pool->CalculateMempoolAncestors()`, but then I'd expect this function to also act like a cache?

How about:

```c++
util::Result<CTxMemPool::setEntries> CalculateMemPoolAncestors(TxHandle tx, const Limits& limits)
{
LOCK(m_pool->cs);
auto it = m_ancestors.find(tx);
util::Result<CTxMemPool::setEntries> ret;
if (it == m_ancestors.end()) {
ret = m_pool-
...