💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415021453)
A variant has the same overhead as a vtable pointer (by adding a variant tag), but has the additional downside that the allocated memory will always have space for the larger of the member types. So in this case, that would boil down to always allocating as much memory as vtable-bearing `GenericClusterImpl`, even for singletons.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415021453)
A variant has the same overhead as a vtable pointer (by adding a variant tag), but has the additional downside that the allocated memory will always have space for the larger of the member types. So in this case, that would boil down to always allocating as much memory as vtable-bearing `GenericClusterImpl`, even for singletons.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415028019)
I think this should be fatal (as it will crash anyway later when the caller tries to access level -1).
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415028019)
I think this should be fatal (as it will crash anyway later when the caller tries to access level -1).
💬 achow101 commented on pull request "Revert "depends: Update URL for `qrencode` package source tarball"":
(https://github.com/bitcoin/bitcoin/pull/33577#issuecomment-3383227287)
> Additionally, I think we want to go ahead and change the source and filename in order to disambiguate any builders who are currently in limbo.
My understanding is that the revert needs to go in first to allow the CI caches to be fixed for the release branches. Then we can open a followup that does the URL and filename change, and in a way that won't break release branch CI now that we know it is a problem.
(https://github.com/bitcoin/bitcoin/pull/33577#issuecomment-3383227287)
> Additionally, I think we want to go ahead and change the source and filename in order to disambiguate any builders who are currently in limbo.
My understanding is that the revert needs to go in first to allow the CI caches to be fixed for the release branches. Then we can open a followup that does the URL and filename change, and in a way that won't break release branch CI now that we know it is a problem.
💬 hodlinator commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3383238791)
Concept ACK 04b943b63ca4c27aa857b03e5de82ed8cae7b17d
Good to see progress on this issue!
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3383238791)
Concept ACK 04b943b63ca4c27aa857b03e5de82ed8cae7b17d
Good to see progress on this issue!
💬 hodlinator commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2415036002)
(Inline comment in random place): Heads up that I've started working on a functional test for this at https://github.com/bitcoin/bitcoin/compare/04b943b63ca4c27aa857b03e5de82ed8cae7b17d...hodlinator:bitcoin:pr/33553_suggestions
It's super-slow (partially due to HeadersSyncState only being created when receiving the very hardcoded 2000 headers message), and still of questionable usefulness.
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2415036002)
(Inline comment in random place): Heads up that I've started working on a functional test for this at https://github.com/bitcoin/bitcoin/compare/04b943b63ca4c27aa857b03e5de82ed8cae7b17d...hodlinator:bitcoin:pr/33553_suggestions
It's super-slow (partially due to HeadersSyncState only being created when receiving the very hardcoded 2000 headers message), and still of questionable usefulness.
💬 theuni commented on pull request "Revert "depends: Update URL for `qrencode` package source tarball"":
(https://github.com/bitcoin/bitcoin/pull/33577#issuecomment-3383255459)
Ok, makes sense.
But after that, seems to me we should backport the bump (as well as #33580) to the release branches as well.
(https://github.com/bitcoin/bitcoin/pull/33577#issuecomment-3383255459)
Ok, makes sense.
But after that, seems to me we should backport the bump (as well as #33580) to the release branches as well.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415068153)
The code here is new, but the lay-out of `m_locator` predates this PR. Going to leave it for a follow-up if anyone cares.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415068153)
The code here is new, but the lay-out of `m_locator` predates this PR. Going to leave it for a follow-up if anyone cares.
💬 ryanofsky commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3383290115)
Good catch: [createNewBlock](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/ipc/capnp/mining.capnp#L20C5-L20C19) (and checkBlock below it) really should take an `mp.Context` parameter. Without it, it's impossible to specify a worker thread for the calls to run on, so the calls wiill block the event loop thread while they execute, potentially affecting other calls and clients. This looks like another thing that should be fixed. But fixing it is more-or-less i
...
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3383290115)
Good catch: [createNewBlock](https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/ipc/capnp/mining.capnp#L20C5-L20C19) (and checkBlock below it) really should take an `mp.Context` parameter. Without it, it's impossible to specify a worker thread for the calls to run on, so the calls wiill block the event loop thread while they execute, potentially affecting other calls and clients. This looks like another thing that should be fixed. But fixing it is more-or-less i
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415069320)
I'm not going to change that now.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415069320)
I'm not going to change that now.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415081345)
I don't understand the question. What change would affect the definition?
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415081345)
I don't understand the question. What change would affect the definition?
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415090220)
This entire `if` branch is an optimization which could be commented out without breaking anything. It just avoids creating a new cluster when the old one could be reused.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415090220)
This entire `if` branch is an optimization which could be commented out without breaking anything. It just avoids creating a new cluster when the old one could be reused.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415091446)
And extensibility, yes.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415091446)
And extensibility, yes.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415091933)
We always treat allocation errors as fatal.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415091933)
We always treat allocation errors as fatal.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415092866)
We could experiment with that in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415092866)
We could experiment with that in a follow-up.
📝 achow101 opened a pull request: "ci: Properly include $FILE_ENV in DEPENDS_HASH"
(https://github.com/bitcoin/bitcoin/pull/33581)
$FILE_ENV has a full relative path already, prepending with ci/test/ results in a non-existent path which means that DEPENDS_HASH was not actually committing to the test's environment file.
(https://github.com/bitcoin/bitcoin/pull/33581)
$FILE_ENV has a full relative path already, prepending with ci/test/ results in a non-existent path which means that DEPENDS_HASH was not actually committing to the test's environment file.
🤔 glozow reviewed a pull request: "Revert "depends: Update URL for `qrencode` package source tarball""
(https://github.com/bitcoin/bitcoin/pull/33577#pullrequestreview-3316637710)
ACK e4335a31920cd390d936cd51cc4478a234db1276
(https://github.com/bitcoin/bitcoin/pull/33577#pullrequestreview-3316637710)
ACK e4335a31920cd390d936cd51cc4478a234db1276
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415124751)
I do see fuzz failure if I remove the `Compact`; as for example it's possible that `m_linearization` is shrunk above, which will need to be accounted for.
Now, it is true that we *technically* don't really care about minimizing memory usage here, as we'll always have a `Split()` that follows, which will either create new Clusters, or update the existing one. The Compact() and corresponding updating of memory usage could be moved there, but I find that a bit harder to reason about.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415124751)
I do see fuzz failure if I remove the `Compact`; as for example it's possible that `m_linearization` is shrunk above, which will need to be accounted for.
Now, it is true that we *technically* don't really care about minimizing memory usage here, as we'll always have a `Split()` that follows, which will either create new Clusters, or update the existing one. The Compact() and corresponding updating of memory usage could be moved there, but I find that a bit harder to reason about.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415125290)
Seems overkill.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415125290)
Seems overkill.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415125528)
I will leave as-is.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415125528)
I will leave as-is.
💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3383395087)
Thank you for the quick review, I have restructured the change to tell a more intuitive story and added more commit message guidance for the context and rewrote the intro to make more sense (it helped that I wrote this a longer time ago and I also didn't remember the context).
The new structure starts with the simple refactorings and assertions and adding extra guards to document why and where the accounting is incorrect. Most of the accounting problems are test-only, so it's unlikely to fix
...
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3383395087)
Thank you for the quick review, I have restructured the change to tell a more intuitive story and added more commit message guidance for the context and rewrote the intro to make more sense (it helped that I wrote this a longer time ago and I also didn't remember the context).
The new structure starts with the simple refactorings and assertions and adding extra guards to document why and where the accounting is incorrect. Most of the accounting problems are test-only, so it's unlikely to fix
...