💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817922747)
Reorgs are split: additions happen via the changesets (since we just use ATMP), while removals happen in their own special funky way.

I can update the comment.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817923233)
Additions never happen outside a changeset. RBF-related removals always go through a changeset as well.

Removals that happen because a block is found, or due to a reorg, or due to mempool expiry or mempool limiting do not go through the changeset.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1817924314)
Not sure I follow -- if somehow the PolicyScriptChecks() passed for something that failed ConsensusChecks(), then this logic would trigger and we would then remove the transactions from the mempool. It's slightly different than the old behavior of never allowing it to enter in the first place, but given that we're holding the mempool locks the whole time it is not really directly observable?

💬 theStack commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2439735659)
Concept ACK

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

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

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

💬 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