π¬ ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894707254)
I think this snippet of code could be documented in the following fashion
```
// If we receive many `SENDTXRCNCL` from the same peer, before to receive a
// single `VERACK`, re-register the peer id state with potentially different local
// and remote salts.
```
As far as I can see, at `SENDTXRCNCL` reception we do not check for duplicate `SENDTXRCNCL` message (we check if we have txreconciliation enabled, then that itβs happening before verack reception and then that transaction-relay
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894707254)
I think this snippet of code could be documented in the following fashion
```
// If we receive many `SENDTXRCNCL` from the same peer, before to receive a
// single `VERACK`, re-register the peer id state with potentially different local
// and remote salts.
```
As far as I can see, at `SENDTXRCNCL` reception we do not check for duplicate `SENDTXRCNCL` message (we check if we have txreconciliation enabled, then that itβs happening before verack reception and then that transaction-relay
...
π¬ ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894707917)
Logging that a given peerβs local reconciliation set max size has been reached can be useuful info for future debug.
```
diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp
index d4d6bdf1ea..b99bb41681 100644
--- a/src/node/txreconciliation.cpp
+++ b/src/node/txreconciliation.cpp
@@ -150,7 +150,11 @@ public:
if (!peer_state) return false;
// Transactions which don't make it to the set due to the limit are announced via fan-out.
- if (
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894707917)
Logging that a given peerβs local reconciliation set max size has been reached can be useuful info for future debug.
```
diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp
index d4d6bdf1ea..b99bb41681 100644
--- a/src/node/txreconciliation.cpp
+++ b/src/node/txreconciliation.cpp
@@ -150,7 +150,11 @@ public:
if (!peer_state) return false;
// Transactions which don't make it to the set due to the limit are announced via fan-out.
- if (
...
π¬ sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894728411)
I simulated this to see if there was any difference between doing the decision-making before or after trickle.
I didn't do it for dependant transactions, just for a single one, trying to answer whether choosing fanout peers at scheduling time or at relay time made any difference. The answer is no, given the number of peers that know about the transaction at both times is almost the same.
The approach, simulation and results can be found here: https://srgi.notion.site/Select-fanout-candidates-
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894728411)
I simulated this to see if there was any difference between doing the decision-making before or after trickle.
I didn't do it for dependant transactions, just for a single one, trying to answer whether choosing fanout peers at scheduling time or at relay time made any difference. The answer is no, given the number of peers that know about the transaction at both times is almost the same.
The approach, simulation and results can be found here: https://srgi.notion.site/Select-fanout-candidates-
...
π¬ 1440000bytes commented on pull request "coins: warn on shutdown for big UTXO set flushes":
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894742730)
```suggestion
if (coins_mem_usage >= WARN_FLUSH_COINS_SIZE) LogWarning("Flushing %d GiB UTXO set to disk, it may take several minutes.", coins_mem_usage >> 30);
```
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894742730)
```suggestion
if (coins_mem_usage >= WARN_FLUSH_COINS_SIZE) LogWarning("Flushing %d GiB UTXO set to disk, it may take several minutes.", coins_mem_usage >> 30);
```
π¬ l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2558426619)
The PR has been split into 3 to simplify review, please check those out first:
* refactor: cache block[undo] serialized size for consecutive calls - https://github.com/bitcoin/bitcoin/pull/31490
* optimization: bulk reads(27%)/writes(290%) in [undo]block [de]serialization, 6% faster IBD - https://github.com/bitcoin/bitcoin/pull/31551
The 3 changes together achieve a 12.3% speedup in raw IBD (real nodes, multiple runs, 870k blocks, 1k dbcache, ssd)
<details>
<summary>Details</summary>
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2558426619)
The PR has been split into 3 to simplify review, please check those out first:
* refactor: cache block[undo] serialized size for consecutive calls - https://github.com/bitcoin/bitcoin/pull/31490
* optimization: bulk reads(27%)/writes(290%) in [undo]block [de]serialization, 6% faster IBD - https://github.com/bitcoin/bitcoin/pull/31551
The 3 changes together achieve a 12.3% speedup in raw IBD (real nodes, multiple runs, 870k blocks, 1k dbcache, ssd)
<details>
<summary>Details</summary>
...
π¬ l0rinc commented on pull request "coins: warn on shutdown for big UTXO set flushes":
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894920990)
The *large* part is important, we're not always displaying this, only when flushing is expected to take a long time
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894920990)
The *large* part is important, we're not always displaying this, only when flushing is expected to take a long time
π¬ 1440000bytes commented on pull request "coins: warn on shutdown for big UTXO set flushes":
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894921918)
It is grammatically incorrect to write "large N GiB".
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894921918)
It is grammatically incorrect to write "large N GiB".
π¬ l0rinc commented on pull request "coins: warn on shutdown for big UTXO set flushes":
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894926926)
K, rephrased it to `Flushing large (1 GiB) UTXO set to disk, it may take several minutes`
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894926926)
K, rephrased it to `Flushing large (1 GiB) UTXO set to disk, it may take several minutes`
π tdb3 approved a pull request: "coins: warn on shutdown for big UTXO set flushes"
(https://github.com/bitcoin/bitcoin/pull/31534#pullrequestreview-2519642270)
re ACK 5709718b830161b7c2ba0db545ef0cfa98423597
(https://github.com/bitcoin/bitcoin/pull/31534#pullrequestreview-2519642270)
re ACK 5709718b830161b7c2ba0db545ef0cfa98423597
π 1440000bytes approved a pull request: "coins: warn on shutdown for big UTXO set flushes"
(https://github.com/bitcoin/bitcoin/pull/31534#pullrequestreview-2519647341)
ACK https://github.com/bitcoin/bitcoin/pull/31534/commits/5709718b830161b7c2ba0db545ef0cfa98423597
(https://github.com/bitcoin/bitcoin/pull/31534#pullrequestreview-2519647341)
ACK https://github.com/bitcoin/bitcoin/pull/31534/commits/5709718b830161b7c2ba0db545ef0cfa98423597
π€ hebasto reviewed a pull request: "guix: disable gcov in base-linux-gcc"
(https://github.com/bitcoin/bitcoin/pull/31450#pullrequestreview-2519657730)
Post-merge ACK f6496a838828aa6d5a2a40380c2338fda2e0d812.
(https://github.com/bitcoin/bitcoin/pull/31450#pullrequestreview-2519657730)
Post-merge ACK f6496a838828aa6d5a2a40380c2338fda2e0d812.
π tdb3 approved a pull request: "qa: Limit `-maxconnections` in tests"
(https://github.com/bitcoin/bitcoin/pull/31537#pullrequestreview-2519666841)
tested ACK d9d5bc2e7466033d989432f53a112325fa3d6d4a
Tested using NetBSD 10 (master fails with `Warning: Reducing -maxconnections from 125 to 96, because of system limitations`, while the PR branch does not).
Also simulated the fd constraint on Debian 12.8 with `ulimit -n256`. Master saw the warning (and test failure). The PR branch did not.
(https://github.com/bitcoin/bitcoin/pull/31537#pullrequestreview-2519666841)
tested ACK d9d5bc2e7466033d989432f53a112325fa3d6d4a
Tested using NetBSD 10 (master fails with `Warning: Reducing -maxconnections from 125 to 96, because of system limitations`, while the PR branch does not).
Also simulated the fd constraint on Debian 12.8 with `ulimit -n256`. Master saw the warning (and test failure). The PR branch did not.
π¬ sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2558527144)
I have pushed a substantial change to the `TxGraphImpl::GroupClusters` function. In a benchmark with 64000 transactions being combined into 1000 clusters, this makes it go from ~140ms to ~16ms. I will add the benchmark to this PR when I clean it up.
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2558527144)
I have pushed a substantial change to the `TxGraphImpl::GroupClusters` function. In a benchmark with 64000 transactions being combined into 1000 clusters, this makes it go from ~140ms to ~16ms. I will add the benchmark to this PR when I clean it up.
π sipa opened a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553)
Part of cluster mempool (#30289). Builds on top of #31444.
During reorganisations, it is possible that dependencies get add which result in clusters that violate policy limits (cluster count, cluster weight), when linking the new from-block transactions to the old from-mempool transactions. Unlike RBF scenarios, we cannot simply reject these policy violations when they are due to received blocks. To accommodate this, add a `TxGraph::Trim()`, which removes some subset of transactions (includin
...
(https://github.com/bitcoin/bitcoin/pull/31553)
Part of cluster mempool (#30289). Builds on top of #31444.
During reorganisations, it is possible that dependencies get add which result in clusters that violate policy limits (cluster count, cluster weight), when linking the new from-block transactions to the old from-mempool transactions. Unlike RBF scenarios, we cannot simply reject these policy violations when they are due to received blocks. To accommodate this, add a `TxGraph::Trim()`, which removes some subset of transactions (includin
...
π¬ theStack commented on issue "RFC: support for writing UTXO set dump (`dumptxoutset` RPC) to a named pipe":
(https://github.com/bitcoin/bitcoin/issues/31373#issuecomment-2558601261)
> i'll review/test your PR when ready.
>
> i think 1 ,3 and 4 could/should be optional as it also can be handled by the process reading the pipe
Yes, I agree that named pipe creation (1) / deletion (4) and the call of the tool (3) shouldn't be done by the RPC itself. My plan was to base the change on #27432 and then provide an additional mode where the user has to provide the path to the bitcoin-cli binary rather than an input file, which would roughly do the following:
```
fifoname=/tmp
...
(https://github.com/bitcoin/bitcoin/issues/31373#issuecomment-2558601261)
> i'll review/test your PR when ready.
>
> i think 1 ,3 and 4 could/should be optional as it also can be handled by the process reading the pipe
Yes, I agree that named pipe creation (1) / deletion (4) and the call of the tool (3) shouldn't be done by the RPC itself. My plan was to base the change on #27432 and then provide an additional mode where the user has to provide the path to the bitcoin-cli binary rather than an input file, which would roughly do the following:
```
fifoname=/tmp
...
π¬ fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2558616951)
@alfonsoromanz While https://github.com/bitcoin/bitcoin/pull/30909 is stalling a bit it seems like it is the way we'll move forward when there is some more review since there was no push back on concept/approach. So I would suggest you proceed here assuming that #30909 is acceptable. Ideally you would change this PR to have the second test standalone, if that's not possible you can base it on #30909.
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2558616951)
@alfonsoromanz While https://github.com/bitcoin/bitcoin/pull/30909 is stalling a bit it seems like it is the way we'll move forward when there is some more review since there was no push back on concept/approach. So I would suggest you proceed here assuming that #30909 is acceptable. Ideally you would change this PR to have the second test standalone, if that's not possible you can base it on #30909.
π¬ TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2558635023)
Thank you for the suggestions @hodlinator, I appreciate the time you took for this. I was originally intent on tackling the 32bit case in a follow-up, mainly because I was thinking whether we should trap the max amount of cache at `numeric_limits<size_t>::max()`. That might be a bit friendlier than just running into an assert, some kind of allocation failure, or much less cache being allocated than requested. With your suggestions, I agree that this should be handled in this PR now.
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2558635023)
Thank you for the suggestions @hodlinator, I appreciate the time you took for this. I was originally intent on tackling the 32bit case in a follow-up, mainly because I was thinking whether we should trap the max amount of cache at `numeric_limits<size_t>::max()`. That might be a bit friendlier than just running into an assert, some kind of allocation failure, or much less cache being allocated than requested. With your suggestions, I agree that this should be handled in this PR now.
π€ PlatinumWrist5055 reviewed a pull request: "[28.x] 28.1rc2 backports"
(https://github.com/bitcoin/bitcoin/pull/31469#pullrequestreview-2519727585)
https://github.com/bitcoin/bitcoin/issues/31418
(https://github.com/bitcoin/bitcoin/pull/31469#pullrequestreview-2519727585)
https://github.com/bitcoin/bitcoin/issues/31418
π¬ PlatinumWrist5055 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/0725a374941355349bb4bc8a79dad1affb27d3b9#commitcomment-150656825)
0725a374941355349bb4bc8a79dad1affb27d3b9
(https://github.com/bitcoin/bitcoin/commit/0725a374941355349bb4bc8a79dad1affb27d3b9#commitcomment-150656825)
0725a374941355349bb4bc8a79dad1affb27d3b9
π¬ PlatinumWrist5055 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/5be34bacf6d07fb73a0cedfb63a384968d538f3e#commitcomment-150657416)
src/qt/rpcconsole.cpp
(https://github.com/bitcoin/bitcoin/commit/5be34bacf6d07fb73a0cedfb63a384968d538f3e#commitcomment-150657416)
src/qt/rpcconsole.cpp
π¬ pythcoiner commented on issue "RFC: support for writing UTXO set dump (`dumptxoutset` RPC) to a named pipe":
(https://github.com/bitcoin/bitcoin/issues/31373#issuecomment-2558813248)
i'll take a look at 27432 then
(https://github.com/bitcoin/bitcoin/issues/31373#issuecomment-2558813248)
i'll take a look at 27432 then