💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544047433)
I think the removed-for-block case is the main one we care about -- in a situation where a peer has requested a transaction where we have received a block containing the transaction in the time between announcement and relay, it seems likely that the peer is about to receive the block as well, and with the compact block protocol we may be helping our peer quite a bit (potentially saving a round trip) to deliver the transaction before the block arrives to them.
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544047433)
I think the removed-for-block case is the main one we care about -- in a situation where a peer has requested a transaction where we have received a block containing the transaction in the time between announcement and relay, it seems likely that the peer is about to receive the block as well, and with the compact block protocol we may be helping our peer quite a bit (potentially saving a round trip) to deliver the transaction before the block arrives to them.
📝 instagibbs opened a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626)
This is an attempt at mitigating https://github.com/bitcoin/bitcoin/issues/25258 , which is a revival of https://github.com/bitcoin/bitcoin/pull/10984, which is a revival of https://github.com/bitcoin/bitcoin/pull/9447.
This PR attempts to mitigate a single case, where high bandwidth peers can bail us out of a flakey
peer not completing blocks for us. We allow up to 2 additional getblocktxns requests per unique block.
This would hopefully allow the chance for an honest high bandwidth peer t
...
(https://github.com/bitcoin/bitcoin/pull/27626)
This is an attempt at mitigating https://github.com/bitcoin/bitcoin/issues/25258 , which is a revival of https://github.com/bitcoin/bitcoin/pull/10984, which is a revival of https://github.com/bitcoin/bitcoin/pull/9447.
This PR attempts to mitigate a single case, where high bandwidth peers can bail us out of a flakey
peer not completing blocks for us. We allow up to 2 additional getblocktxns requests per unique block.
This would hopefully allow the chance for an honest high bandwidth peer t
...
💬 hebasto commented on pull request "kernel: Remove args, chainparams, chainparamsbase from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1544060017)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1544060017)
Concept ACK.
💬 ajtowns commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191235815)
The `CompareTxMemPoolEntryByScore` comparator returns `false` on equality (ie, it's "less than" not "less than or equal"), so this preserves that behaviour.
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191235815)
The `CompareTxMemPoolEntryByScore` comparator returns `false` on equality (ie, it's "less than" not "less than or equal"), so this preserves that behaviour.
📝 MarcoFalke converted_to_draft 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.
...
🤔 Sjors reviewed a pull request: "p2p: Avoid prematurely clearing download state for other peers"
(https://github.com/bitcoin/bitcoin/pull/27608#pullrequestreview-1422180036)
utACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4
(https://github.com/bitcoin/bitcoin/pull/27608#pullrequestreview-1422180036)
utACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4
💬 Sjors commented on pull request "p2p: Avoid prematurely clearing download state for other peers":
(https://github.com/bitcoin/bitcoin/pull/27608#discussion_r1190909435)
This comment could be improved:
```cpp
// When processing block related messages from our peers,
// we treat requested blocks different from unsolicited ones.
// When making this distinction we only keep track of the
// last peer we requested from.
```
(https://github.com/bitcoin/bitcoin/pull/27608#discussion_r1190909435)
This comment could be improved:
```cpp
// When processing block related messages from our peers,
// we treat requested blocks different from unsolicited ones.
// When making this distinction we only keep track of the
// last peer we requested from.
```
💬 Sjors commented on pull request "p2p: Avoid prematurely clearing download state for other peers":
(https://github.com/bitcoin/bitcoin/pull/27608#discussion_r1191267648)
Can't we have a race condition where the block is not `new`? In any case this function is extremely fast if called twice.
(https://github.com/bitcoin/bitcoin/pull/27608#discussion_r1191267648)
Can't we have a race condition where the block is not `new`? In any case this function is extremely fast if called twice.
🤔 glozow reviewed a pull request: "[24.1] Backports for rc3"
(https://github.com/bitcoin/bitcoin/pull/27614#pullrequestreview-1422778545)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19
(https://github.com/bitcoin/bitcoin/pull/27614#pullrequestreview-1422778545)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19
💬 dergoegge commented on pull request "[24.1] Backports for rc3":
(https://github.com/bitcoin/bitcoin/pull/27614#issuecomment-1544115933)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19
(https://github.com/bitcoin/bitcoin/pull/27614#issuecomment-1544115933)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19
💬 dergoegge commented on pull request "[23.2] Backports for rc1":
(https://github.com/bitcoin/bitcoin/pull/27624#issuecomment-1544118582)
ACK ce8f812b0ac0905c26edd826c57886a08079b4a7
Are you going to add https://github.com/bitcoin/bitcoin/pull/27610 to this?
(https://github.com/bitcoin/bitcoin/pull/27624#issuecomment-1544118582)
ACK ce8f812b0ac0905c26edd826c57886a08079b4a7
Are you going to add https://github.com/bitcoin/bitcoin/pull/27610 to this?
👍 brunoerg approved a pull request: "[24.1] Backports for rc3"
(https://github.com/bitcoin/bitcoin/pull/27614#pullrequestreview-1422791729)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19
Just ran the tests locally, no problem. Release notes ok.
(https://github.com/bitcoin/bitcoin/pull/27614#pullrequestreview-1422791729)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19
Just ran the tests locally, no problem. Release notes ok.
📝 hebasto opened a pull request: "[24.x] qt: 24.1rc3 translations update"
(https://github.com/bitcoin/bitcoin/pull/27627)
This PR pulls the recent translations from the [Transifex.com](https://www.transifex.com/bitcoin/bitcoin) using the [`bitcoin-maintainer-tools/update-translations.py`](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/update-translations.py) tool.
According to our [Release Process docs](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-every-release-candidate), it is supposed to be merged before `v24.1rc3` tagging.
Will keep this PR updated regul
...
(https://github.com/bitcoin/bitcoin/pull/27627)
This PR pulls the recent translations from the [Transifex.com](https://www.transifex.com/bitcoin/bitcoin) using the [`bitcoin-maintainer-tools/update-translations.py`](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/update-translations.py) tool.
According to our [Release Process docs](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-every-release-candidate), it is supposed to be merged before `v24.1rc3` tagging.
Will keep this PR updated regul
...
💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191301314)
I think it's a bit strange for policy/fees to have a dependency on scheduler. Maybe register `FlushFeeEstimates()` with the scheduler within `AppInitMain()` instead?
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191301314)
I think it's a bit strange for policy/fees to have a dependency on scheduler. Maybe register `FlushFeeEstimates()` with the scheduler within `AppInitMain()` instead?
👍 hebasto approved a pull request: "[24.1] Backports for rc3"
(https://github.com/bitcoin/bitcoin/pull/27614#pullrequestreview-1422809452)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19, commits were backported locally, got zero diff.
(https://github.com/bitcoin/bitcoin/pull/27614#pullrequestreview-1422809452)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19, commits were backported locally, got zero diff.
💬 achow101 commented on pull request "[24.1] Backports for rc3":
(https://github.com/bitcoin/bitcoin/pull/27614#issuecomment-1544146197)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19
(https://github.com/bitcoin/bitcoin/pull/27614#issuecomment-1544146197)
ACK 97f5e28830dfa5786cdc5df62e245e03fa393e19
🚀 achow101 merged a pull request: "[24.1] Backports for rc3"
(https://github.com/bitcoin/bitcoin/pull/27614)
(https://github.com/bitcoin/bitcoin/pull/27614)
💬 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_r1191317284)
> > This is sometimes good for fingerprinting
> fingerprinting seems bad, so another reason for removal?
Sorry I meant that it's sometimes good for _preventing_ fingerprinting. In the replacement scenario which @ajtowns is describing. Timing of entry of replacement == notfound of replaced tx. With `mapRelay`, since you have the replaced tx for 15 minutes, you'll still serve it instead of immediately saying notfound. But `mapRelay` is useless against this if the spy node just waits 15 min
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191317284)
> > This is sometimes good for fingerprinting
> fingerprinting seems bad, so another reason for removal?
Sorry I meant that it's sometimes good for _preventing_ fingerprinting. In the replacement scenario which @ajtowns is describing. Timing of entry of replacement == notfound of replaced tx. With `mapRelay`, since you have the replaced tx for 15 minutes, you'll still serve it instead of immediately saying notfound. But `mapRelay` is useless against this if the spy node just waits 15 min
...
💬 vasild 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-1544163035)
Is it the case that this PR will change the behavior in the following two cases (from the PR title and description I got the impression that it is wider scope):
### Case 1
* the node receives `MEMPOOL` request and replies to it
* in the same second a new transaction enters the mempool (after the `MEMPOOL` message)
* the node receives `GETDATA` for that transaction
In `master` it would send the transaction even though it never advertised it with `INV` due to the `txinfo.m_time <= mempool
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544163035)
Is it the case that this PR will change the behavior in the following two cases (from the PR title and description I got the impression that it is wider scope):
### Case 1
* the node receives `MEMPOOL` request and replies to it
* in the same second a new transaction enters the mempool (after the `MEMPOOL` message)
* the node receives `GETDATA` for that transaction
In `master` it would send the transaction even though it never advertised it with `INV` due to the `txinfo.m_time <= mempool
...
💬 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-1544165800)
> I'm only a casual lurker on the net_processing side but i was under the impression we were thinking of the BIP35 as permissioned anyways. If we are already trusting the peer that sends use MEMPOOL message, why not just include everything we've got? A practical counter-argument to this could be - yeah but at this time [around 20% to 30% of reachable nodes](https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529678174) set `peerbloomfilters` so it's still worth closing an obvious fingerp
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544165800)
> I'm only a casual lurker on the net_processing side but i was under the impression we were thinking of the BIP35 as permissioned anyways. If we are already trusting the peer that sends use MEMPOOL message, why not just include everything we've got? A practical counter-argument to this could be - yeah but at this time [around 20% to 30% of reachable nodes](https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529678174) set `peerbloomfilters` so it's still worth closing an obvious fingerp
...