Bitcoin Core Github
44 subscribers
121K links
Download Telegram
📝 sdaftuar opened a pull request: "Cluster mempool followups"
(https://github.com/bitcoin/bitcoin/pull/33591)
As suggested in the main cluster mempool PR (https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-3177119367), I've pulled out some of the non-essential optimizations and cleanups into this separate PR.

Will continue to add more commits here to address non-blocking suggestions/improvements as they come up.
💬 sdaftuar commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3387174668)
At @glozow's suggestion I've removed some optimization/cleanup commits from this PR and opened #33591 for non-blocking cleanups/followups.
💬 glozow commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3387203032)
fwiw @achow101 has already done so
💬 purpleKarrot commented on pull request "cmake: Use builtin support for .manifest files":
(https://github.com/bitcoin/bitcoin/pull/33585#issuecomment-3387213880)
> This doesn't Guix build:

I added a workaround for upstream issue [23244](https://gitlab.kitware.com/cmake/cmake/-/issues/23244). It should pass now.
🚀 glozow merged a pull request: "Revert "depends: Update URL for `qrencode` package source tarball""
(https://github.com/bitcoin/bitcoin/pull/33577)
💬 plebhash commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3387283609)
hmm thinking about it deeper, I guess I could simply save the `waitNext` future into a `mut` variable so it's still available across the unified loop iterations, and then replace it whenever it either returns or is cancelled (via `interruptWait`) due to `CoinbaseOutputConstraints`

so I wouldn't say your original gut feeling is totally wrong!

although I still appreciate the benefits of separating concerns into different loops... I guess I'll experiment with both approaches whenever `interruptWa
...
🚀 glozow merged a pull request: "[30.x] Finalise v30.0"
(https://github.com/bitcoin/bitcoin/pull/33559)
💬 trevarj commented on pull request "[WIP] doc: update Guix install instructions":
(https://github.com/bitcoin/bitcoin/pull/33574#issuecomment-3387394913)
While using Guix's `install.sh` is my preferred method of installation on a "foreign distro", I will acknowledge that it isn't always a smooth experience. For example, when a user isn't using bash as their shell and doesn't know how Guix works they could miss the necessary env vars.

**A crazy idea** could be to offer a `.deb` package that is created using `guix pack -f deb -m manifest.scm`, where the manifest contains `guix` itself and the packages within `contrib/guix/manifest.scm`. Users on
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417943637)
I've moved the implementation out of the class definition, so that it doesn't need to move when the abstraction is added.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417947258)
Good idea, I've done something like this (with the repetition made optional).



> And given that `GetMainMemoryUsage` has some surprising side-effects (`SplitAll` & `ApplyDependencies`), we could also check that it's not changing counts or staging presence.

There is no need for that; comparing with the simulation will catch that as those are well-defined properties we check (either when invoking inspector methods in the simulation, or in the full comparison at the end).
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417948078)
I have removed some of them through use of `GetTxCount()` as suggested elsewhere.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417948453)
Nice catch. Removed.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417948776)
Moved it.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417949550)
Added a commit that fixes some outdated comments.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417950481)
Fixed in a commit that fixes some outdated comments.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417951078)
Indeed, fixed.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417952778)
Nice catch.

Even better: the fact that this was possible indicated that the function was never called on empty clusters (because `TotalMemoryUsage()` is not 0, even for empty clusters). I've thus changed it to an `Assume()` for non-emptiness.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417953257)
This was a leftover, gone.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417954153)
Addressed by introducing a separate `FindCluster` and `FindClusterAndLevel` function.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417954576)
Done, in a new comment-fixing commit.