💬 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)};
```
(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`.
(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
(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
...
(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.
(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.
(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
(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`
...
(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)
(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
...
(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)
(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
(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?
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191178712)
Could we have these cases covered by a unit test?
💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1544006512)
without #26711 we are still letting things into mempool that are possibly problematic, no?
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1544006512)
without #26711 we are still letting things into mempool that are possibly problematic, no?
💬 willcl-ark 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_r1191189563)
```suggestion
self.log.info("Check all transactions are returned by the match-all filter")
```
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191189563)
```suggestion
self.log.info("Check all transactions are returned by the match-all filter")
```
💬 willcl-ark 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_r1191190564)
I think this log line could be removed as this seems part of the previous check?
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191190564)
I think this log line could be removed as this seems part of the previous check?
💬 brunoerg commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1544020183)
@hebasto I got an error when trying to access [25.0 Draft Release Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Notes-draft)
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1544020183)
@hebasto I got an error when trying to access [25.0 Draft Release Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Notes-draft)
💬 sr-gi 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-1544020927)
> > 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
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544020927)
> > 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
...
💬 josibake commented on pull request "[24.1] Backports for rc3":
(https://github.com/bitcoin/bitcoin/pull/27614#issuecomment-1544027466)
ACK https://github.com/bitcoin/bitcoin/pull/27614/commits/97f5e28830dfa5786cdc5df62e245e03fa393e19
Verified that the commits added are the correct ones from the PRs linked, compiled and ran the tests. Document changes look good. Looks like the CI failures are just github being crappy
(https://github.com/bitcoin/bitcoin/pull/27614#issuecomment-1544027466)
ACK https://github.com/bitcoin/bitcoin/pull/27614/commits/97f5e28830dfa5786cdc5df62e245e03fa393e19
Verified that the commits added are the correct ones from the PRs linked, compiled and ran the tests. Document changes look good. Looks like the CI failures are just github being crappy
🤔 hebasto reviewed a pull request: "depends: no-longer nuke libc++abi.so* in native_clang package"
(https://github.com/bitcoin/bitcoin/pull/27493#pullrequestreview-1422646806)
ACK 9ae854da193f3c4bda38a75e96f9b989b289baab
Actually, this PR introduces no changes to the `native_clang` package at all:
```
$ git fetch origin pull/27493/head
$ git checkout FETCH_HEAD
$ make -C depends clean-all
$ make -C depends native_clang HOST=x86_64-apple-darwin
$ cat depends/built/x86_64-apple-darwin/native_clang/native_clang-10.0.1-*.tar.gz.hash
300b13fa3db495f3ad33f729a2fa2688e84d928a17ef8f4eb1164e062ad924c6 native_clang-10.0.1-a7086a951bd.tar.gz
$ git checkout HEAD~1
$
...
(https://github.com/bitcoin/bitcoin/pull/27493#pullrequestreview-1422646806)
ACK 9ae854da193f3c4bda38a75e96f9b989b289baab
Actually, this PR introduces no changes to the `native_clang` package at all:
```
$ git fetch origin pull/27493/head
$ git checkout FETCH_HEAD
$ make -C depends clean-all
$ make -C depends native_clang HOST=x86_64-apple-darwin
$ cat depends/built/x86_64-apple-darwin/native_clang/native_clang-10.0.1-*.tar.gz.hash
300b13fa3db495f3ad33f729a2fa2688e84d928a17ef8f4eb1164e062ad924c6 native_clang-10.0.1-a7086a951bd.tar.gz
$ git checkout HEAD~1
$
...