💬 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
...
💬 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-1544171098)
> The title "avoid serving non-announced txs as a result of a MEMPOOL message" is confusing - it gives the impression that this PR will change the response to `MEMPOOL` messages, but it does not do that. Before and after the PR we would send the full mempool in a reply to `mempool` (correct me if I got it wrong).
Maybe it should read "after receiving a MEMPOOL message" instead of "as a result of a MEMPOOL message".
What this PR is trying to achieve is to prevent us from responding to getda
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544171098)
> The title "avoid serving non-announced txs as a result of a MEMPOOL message" is confusing - it gives the impression that this PR will change the response to `MEMPOOL` messages, but it does not do that. Before and after the PR we would send the full mempool in a reply to `mempool` (correct me if I got it wrong).
Maybe it should read "after receiving a MEMPOOL message" instead of "as a result of a MEMPOOL message".
What this PR is trying to achieve is to prevent us from responding to getda
...
💬 MarcoFalke commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255)
Ok, good point. I was about to suggest that in that case it seems sufficient to only put `BLOCK` (maybe also `CONFLICTED`/`REORG`) transactions into `mapRelay` (keeping their mempool timestamp, to avoid adding ones that are older than 15 minutes). However, I think your point also applies to `REPLACED` transactions, because we'd still want to relay them, so that they are rejected by the peer and end up in their `AddToCompactExtraTransactions`? I wonder if it makes sense to drop a line or two of d
...
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255)
Ok, good point. I was about to suggest that in that case it seems sufficient to only put `BLOCK` (maybe also `CONFLICTED`/`REORG`) transactions into `mapRelay` (keeping their mempool timestamp, to avoid adding ones that are older than 15 minutes). However, I think your point also applies to `REPLACED` transactions, because we'd still want to relay them, so that they are rejected by the peer and end up in their `AddToCompactExtraTransactions`? I wonder if it makes sense to drop a line or two of d
...
💬 glozow commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1544174133)
> without https://github.com/bitcoin/bitcoin/pull/26711 we are still letting things into mempool that are possibly problematic, no?
It's more that we might reject things that were good - namely, parents that depend on each other and are good but the child isn't. However that is addressed easily by submitting those parents by themselves. I'm less concerned about rejecting too much and more concerned about accepting too much.
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1544174133)
> without https://github.com/bitcoin/bitcoin/pull/26711 we are still letting things into mempool that are possibly problematic, no?
It's more that we might reject things that were good - namely, parents that depend on each other and are good but the child isn't. However that is addressed easily by submitting those parents by themselves. I'm less concerned about rejecting too much and more concerned about accepting too much.
👍 jarolrod approved a pull request: "[24.x] qt: 24.1rc3 translations update"
(https://github.com/bitcoin/bitcoin/pull/27627#pullrequestreview-1422863844)
ACK a86b45cafaabd8eb4957b84a0d70484a597eacc7
Fetched translations on 24.x and have a zero-diff with this branch after removing the dutch translation which looks like someone reset the strings to English
(https://github.com/bitcoin/bitcoin/pull/27627#pullrequestreview-1422863844)
ACK a86b45cafaabd8eb4957b84a0d70484a597eacc7
Fetched translations on 24.x and have a zero-diff with this branch after removing the dutch translation which looks like someone reset the strings to English
🚀 fanquake merged a pull request: "[24.x] qt: 24.1rc3 translations update"
(https://github.com/bitcoin/bitcoin/pull/27627)
(https://github.com/bitcoin/bitcoin/pull/27627)
💬 achow101 commented on pull request "[24.x] qt: 24.1rc3 translations update":
(https://github.com/bitcoin/bitcoin/pull/27627#issuecomment-1544190749)
post merge ACK a86b45cafaabd8eb4957b84a0d70484a597eacc7
Diff is the same.
(https://github.com/bitcoin/bitcoin/pull/27627#issuecomment-1544190749)
post merge ACK a86b45cafaabd8eb4957b84a0d70484a597eacc7
Diff is the same.
💬 theuni commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1544190999)
@fanquake Do you happen to know what the earliest working clang+lld combo would be? Obviously 15 and 16 work, did 13 or 14 have the required options? Just curious because it'll be good to know what the minimum supported vanilla combo (outside of guix/depends) is as that'll become our new requirement.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1544190999)
@fanquake Do you happen to know what the earliest working clang+lld combo would be? Obviously 15 and 16 work, did 13 or 14 have the required options? Just curious because it'll be good to know what the minimum supported vanilla combo (outside of guix/depends) is as that'll become our new requirement.
💬 Sjors commented on pull request "[23.2] Backports for rc1":
(https://github.com/bitcoin/bitcoin/pull/27624#issuecomment-1544203102)
utACK ce8f812b0ac0905c26edd826c57886a08079b4a7
(https://github.com/bitcoin/bitcoin/pull/27624#issuecomment-1544203102)
utACK ce8f812b0ac0905c26edd826c57886a08079b4a7
💬 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-1544205452)
> 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
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544205452)
> 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
...
👍 MarcoFalke approved a pull request: "Fee estimation: avoid serving stale fee estimate"
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1422877741)
lgtm. Left some nits
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1422877741)
lgtm. Left some nits
💬 MarcoFalke commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191350003)
nit (feel free to ignore): An alternative to adding a function here would be to call the already existing `Ticks<std::chrono::hours>(a)` over `count_hours(a)`, but either seems fine.
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191350003)
nit (feel free to ignore): An alternative to adding a function here would be to call the already existing `Ticks<std::chrono::hours>(a)` over `count_hours(a)`, but either seems fine.
💬 MarcoFalke commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191352778)
nit:
```suggestion
fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"
```
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191352778)
nit:
```suggestion
fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"
```
💬 MarcoFalke commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191357213)
nit: I am not a fan of uninterruptible sleep in tests. `assert_debug_log` can be used to wait, if you supply a timeout argument (or rely on the default timeout)
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191357213)
nit: I am not a fan of uninterruptible sleep in tests. `assert_debug_log` can be used to wait, if you supply a timeout argument (or rely on the default timeout)