💬 jonatack commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#issuecomment-1543922356)
Rebased ⚡

💬 jonatack commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1543923414)
Rebased 🧸

💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1543937304)
> With #27468 merged, I don't really see the need for [8ef940d](https://github.com/bitcoin/bitcoin/commit/8ef940d39bfa7bc4ba867b70495b68c507297694) anymore? Also the PR title and description need updating, this is not a bugfix PR anymore.

Updated them, thanks!

🤔 glozow reviewed a pull request: "Improve performance of p2p inv to send queues"
(https://github.com/bitcoin/bitcoin/pull/27610#pullrequestreview-1422536495)
code review ACK 5b3406094f2679dfb3763de4414257268565b943

👍 dergoegge approved a pull request: "Improve performance of p2p inv to send queues"
(https://github.com/bitcoin/bitcoin/pull/27610#pullrequestreview-1422347722)
utACK 5b3406094f2679dfb3763de4414257268565b943

The code and approach looks good to me.

(my comments can be addressed in a follow-up)

💬 dergoegge commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191017253)
* `INVENTORY_BROADCAST_MAX` should be renamed or have its comment amended
* clang-format?
* Maybe add the commit description as a comment here

💬 dergoegge commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191018175)
Explaining that in a comment would be nice

💬 dergoegge commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191140520)
```suggestion
size_t broadcast_max{INVENTORY_BROADCAST_MAX + (tx_relay->m_tx_inventory_to_send.size() / 1000) * count_seconds(INBOUND_INVENTORY_BROADCAST_INTERVAL)};
```

💬 martinus commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#discussion_r1191134489)
Hm, I don't really like that all of that SHA256 specific code is becoming part of the benchmark framework. Wouldn't it be sufficient to just adapt the SHA benchmark in `crypto_hash.cpp` so that there is a separate test for each of the implementations? There could be a separate benchmark for each value of the `enum UseImplementation`.

💬 martinus commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#discussion_r1191135383)
This always prints a SHA log even when no SHA related benchmark is run

💬 glozow commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1543955685)
> What's the proportion of announced vs non-announced transactions? How much would be left out with this patch?

The PR (as it is right now) still announces everything in response to mempool. The client just cannot request extremely recent things, which will be announced normally every ~5 seconds. This is "I haven't even announced this to you yet, why are you requesting it from me? seems suspicious. Just wait 5 seconds and I'll announce it to you."

A normal client, who presumably does not
...

💬 glozow commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191157352)
> I wonder if we can remove mapRelay and as a result can get more efficient data structures to remember (w)txids per Peer, knowing that only transaction in the mempool can be in them.

`mapRelay` is mostly what's in the mempool, but can also contain entries that were announced less than 15min ago + fell out of the mempool since then. This is sometimes good for fingerprinting :shrug: but also seems like largely redundant information to me.

💬 hebasto commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile, check for usage":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-1543981739)
Concept ACK.

💬 instagibbs commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191169420)
static asserts or something would be even better

💬 ajtowns commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191170092)
I don't think we can remove `mapRelay` -- we need it so we can continue to relay recently announced txs that we've removed from the mempool?

How about this approach: when we get a MEMPOOL message, at the next `fSendTrickle` time, for each tx that matches the filter, if it's older than 2 minutes, announce it immediately, otherwise just put it into `m_tx_inventory_to_send` and let it be sent out at the usual rate. Only use `m_recently_announced_invs` for txs relayed via `m_tx_inventory_to_send`
...

👋 fanquake's pull request is ready for review: "depends: no-longer nuke libc++abi.so* in native_clang package"
(https://github.com/bitcoin/bitcoin/pull/27493)

💬 fanquake commented on pull request "depends: no-longer nuke libc++abi.so* in native_clang package":
(https://github.com/bitcoin/bitcoin/pull/27493#issuecomment-1543996017)
Guix Build:
```bash
f05e6ab973d999e7fd8445c580bf919a6c7ffcdabbfac30cefaa8a81c1c8d804 guix-build-9ae854da193f/output/aarch64-linux-gnu/SHA256SUMS.part
4294c97d12f0c29ad9d3211fb455ac2a2b9933c13efa061b1753917a11dbbaf5 guix-build-9ae854da193f/output/aarch64-linux-gnu/bitcoin-9ae854da193f-aarch64-linux-gnu-debug.tar.gz
cf80ca7545331b69ca2c1496eab26122fcf7499e6674e9cabf168b3cfe3d10d8 guix-build-9ae854da193f/output/aarch64-linux-gnu/bitcoin-9ae854da193f-aarch64-linux-gnu.tar.gz
816d7fc4bbc6deba
...

🚀 fanquake merged a pull request: "Improve performance of p2p inv to send queues"
(https://github.com/bitcoin/bitcoin/pull/27610)

👍 brunoerg approved a pull request: "Improve performance of p2p inv to send queues"
(https://github.com/bitcoin/bitcoin/pull/27610#pullrequestreview-1422604159)
crACK 5b3406094f2679dfb3763de4414257268565b943

💬 brunoerg commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191178712)
Could we have these cases covered by a unit test?