💬 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
$
...
📝 MarcoFalke opened a pull request: "p2p: Stop relaying non-mempool txs"
(https://github.com/bitcoin/bitcoin/pull/27625)
`mapRelay` (used to relay announced transactions that are no longer in the mempool) has issues:
* It doesn't have an absolute memory limit, only an implicit one based on the rate of transaction announcements
* It doesn't have a use-case, and can be used to spy (see below)
Fix all issues by removing `mapRelay`.
For more context, on why a transaction may have been removed from the mempool, see https://github.com/bitcoin/bitcoin/blob/c2f2abd0a4f4bd18bfca41b632d21d803479f3f4/src/txmempool.
...
(https://github.com/bitcoin/bitcoin/pull/27625)
`mapRelay` (used to relay announced transactions that are no longer in the mempool) has issues:
* It doesn't have an absolute memory limit, only an implicit one based on the rate of transaction announcements
* It doesn't have a use-case, and can be used to spy (see below)
Fix all issues by removing `mapRelay`.
For more context, on why a transaction may have been removed from the mempool, see https://github.com/bitcoin/bitcoin/blob/c2f2abd0a4f4bd18bfca41b632d21d803479f3f4/src/txmempool.
...
💬 glozow commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191209626)
I've written a unit test here: https://github.com/glozow/bitcoin/commits/2023-05-comparedepthandscore-test
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191209626)
I've written a unit test here: https://github.com/glozow/bitcoin/commits/2023-05-comparedepthandscore-test
🤔 jonatack reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1422614006)
Approach ACK, modulo comments and https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186378760 -- it might be better for that commit to be a pure refactoring. Thanks for addressing the previous feedback.
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1422614006)
Approach ACK, modulo comments and https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186378760 -- it might be better for that commit to be a pure refactoring. Thanks for addressing the previous feedback.
💬 jonatack commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1191201370)
Why this change?
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1191201370)
Why this change?