💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3387039466)
Addressed many comments, and made some bigger changes:
* Restructured the commits to first make all abstractions, and then do the abstract class `Cluster` change cleanly.
* Use `GetTxCount()` in more places, instead of `m_graph_index != GraphIndex(-1)`
* Made `Compact()` and memory accounting work at the `TxGraphImpl::Merge` level rather than `Cluster::Merge`: the old code could be compacting O(n) times when merging n clusters together simultaneously.
* Made the dealing with which implementa
...
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3387039466)
Addressed many comments, and made some bigger changes:
* Restructured the commits to first make all abstractions, and then do the abstract class `Cluster` change cleanly.
* Use `GetTxCount()` in more places, instead of `m_graph_index != GraphIndex(-1)`
* Made `Compact()` and memory accounting work at the `TxGraphImpl::Merge` level rather than `Cluster::Merge`: the old code could be compacting O(n) times when merging n clusters together simultaneously.
* Made the dealing with which implementa
...
💬 andrewtoth commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417636066)
> The next send will only include the highest feerate transactions in the queue
Ah, right, but then can we say it is scheduled for the next periodic send which will include it as one of the highest feerate transactions?
> If we keep receiving higher feerate transactions, it won't be inv'd for some time
Sure, but this is I believe an implementation detail. The tx has been scheduled when we call this method, and newly received txs can alter the broadcast schedule after the fact. The inten
...
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417636066)
> The next send will only include the highest feerate transactions in the queue
Ah, right, but then can we say it is scheduled for the next periodic send which will include it as one of the highest feerate transactions?
> If we keep receiving higher feerate transactions, it won't be inv'd for some time
Sure, but this is I believe an implementation detail. The tx has been scheduled when we call this method, and newly received txs can alter the broadcast schedule after the fact. The inten
...
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3387099398)
Updated with a fuzz test case from @TheCharlatan. Thanks!
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3387099398)
Updated with a fuzz test case from @TheCharlatan. Thanks!
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417685776)
There are certainly possibilities, like storing the clusters in two vectors (one for each implementation), and using positions within that vector + some marker to identify the implementation as "identifier" for the cluster. The difficulty is that you now need to deal with holes in those vectors, or swapping to compact them, meaning you need to go find and update all places where those cluster identifiers are used.
Absent something like that, I think the current approach is close to ideal. It in
...
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417685776)
There are certainly possibilities, like storing the clusters in two vectors (one for each implementation), and using positions within that vector + some marker to identify the implementation as "identifier" for the cluster. The difficulty is that you now need to deal with holes in those vectors, or swapping to compact them, meaning you need to go find and update all places where those cluster identifiers are used.
Absent something like that, I think the current approach is close to ideal. It in
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417686261)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417686261)
Fixed.
💬 maflcko commented on pull request "build: Bump clang minimum supported version to 17":
(https://github.com/bitcoin/bitcoin/pull/33555#issuecomment-3387122253)
> Do we still need the build specialization in
Seems unrelated, because the check is to be disabled for clang-15 and less. Not sure why it is there in the first place, but if you want to remove it, it could be removed already today, unrelated to this pull.
> Do we need a build release notes for this?
Probably not, but I've added one. Thx.
> Can we simplify these tests now
Nice. Reverted that commit.
(https://github.com/bitcoin/bitcoin/pull/33555#issuecomment-3387122253)
> Do we still need the build specialization in
Seems unrelated, because the check is to be disabled for clang-15 and less. Not sure why it is there in the first place, but if you want to remove it, it could be removed already today, unrelated to this pull.
> Do we need a build release notes for this?
Probably not, but I've added one. Thx.
> Can we simplify these tests now
Nice. Reverted that commit.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417689081)
Addressed by creating separate FindCluster and FindClusterAndLevel functions. In most call sites, FindCluster can remain unchanged.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417689081)
Addressed by creating separate FindCluster and FindClusterAndLevel functions. In most call sites, FindCluster can remain unchanged.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417690581)
I have added a few `/*level=*/0` etc. as arguments. Does that help?
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417690581)
I have added a few `/*level=*/0` etc. as arguments. Does that help?
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417692771)
I have incorporated this, and added you as co-author.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417692771)
I have incorporated this, and added you as co-author.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417694634)
I have reordered the statements.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417694634)
I have reordered the statements.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417695849)
I have reordered these statements and added comments.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417695849)
I have reordered these statements and added comments.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417696402)
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417696402)
Done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417696760)
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417696760)
Done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417697174)
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417697174)
Done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417699157)
Fixed by doing the introduction of abstraction functions first, before making Cluster an abstract class.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417699157)
Fixed by doing the introduction of abstraction functions first, before making Cluster an abstract class.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417700024)
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417700024)
Done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417700578)
Indeed, fixed.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417700578)
Indeed, fixed.
💬 maflcko commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3387143955)
If someone wants to locally restore it manually, drahtbot happens to have the old version as well:
```
$ curl -LOf 'https://drahtbot.space/depends_download_fallback/qrencode-4.1.1.tar.gz'
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 530k 100 530k 0 0 363k 0 0:00:01 0:00:01 --:--:-- 362k
$ sha256sum ./qrencode-4.1.1.tar.gz
da448ed4f52aba6bcb0cd48cac0
...
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3387143955)
If someone wants to locally restore it manually, drahtbot happens to have the old version as well:
```
$ curl -LOf 'https://drahtbot.space/depends_download_fallback/qrencode-4.1.1.tar.gz'
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 530k 100 530k 0 0 363k 0 0:00:01 0:00:01 --:--:-- 362k
$ sha256sum ./qrencode-4.1.1.tar.gz
da448ed4f52aba6bcb0cd48cac0
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417702683)
Instead of adding an `IsEmpty()`, I ended up using the added `GetTxCount()` virtual function in many places.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417702683)
Instead of adding an `IsEmpty()`, I ended up using the added `GetTxCount()` virtual function in many places.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417703204)
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417703204)
Done.