💬 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
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415131623)
They are unobservable side-effects.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415131623)
They are unobservable side-effects.
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415133326)
yes, with current versions it would use the max size, it's why I wrote "we would likely need some tricks to save memory". I don't have a concrete better idea yet but will try to come up with something better.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415133326)
yes, with current versions it would use the max size, it's why I wrote "we would likely need some tricks to save memory". I don't have a concrete better idea yet but will try to come up with something better.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415139924)
Yes, or when `BlockBuilder` objects exist. All functions in the `TxGraph` interface declare when they're allowed to be called.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415139924)
Yes, or when `BlockBuilder` objects exist. All functions in the `TxGraph` interface declare when they're allowed to be called.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415143034)
We're using the name `SetType` in many places in the cluster linearization code.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415143034)
We're using the name `SetType` in many places in the cluster linearization code.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415145512)
The code already uses `final` on all of the `TxGraphImpl` functions. Will leave for a follow-up.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415145512)
The code already uses `final` on all of the `TxGraphImpl` functions. Will leave for a follow-up.
💬 achow101 commented on pull request "doc: how to update a subtree":
(https://github.com/bitcoin/bitcoin/pull/33568#discussion_r2415146156)
It's actually not necessary to add it as a remote. You can give the repo url directly and it will fetch as needed. This avoids adding a bunch of unnecessary refs to your repo.
(https://github.com/bitcoin/bitcoin/pull/33568#discussion_r2415146156)
It's actually not necessary to add it as a remote. You can give the repo url directly and it will fetch as needed. This avoids adding a bunch of unnecessary refs to your repo.
💬 earonesty commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3383559491)
>
> If JPEG-degens turn out to be the highest paying user segment, maybe
> humanity has failed Bitcoin?
>
Currently, fee-bearing transactions are not occurring at a high rate for
financial transactions because there is a lower-demand for large-value,
slow-settlement transactions than there is for instant transactions.
While lightning resolves much of this, so many implementations are
custodial (reasonable for a low values) and rarely clear to the chain.
Focusing on improving layer-2 accessibi
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3383559491)
>
> If JPEG-degens turn out to be the highest paying user segment, maybe
> humanity has failed Bitcoin?
>
Currently, fee-bearing transactions are not occurring at a high rate for
financial transactions because there is a lower-demand for large-value,
slow-settlement transactions than there is for instant transactions.
While lightning resolves much of this, so many implementations are
custodial (reasonable for a low values) and rarely clear to the chain.
Focusing on improving layer-2 accessibi
...
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415235592)
The question was referring to the documentation of `NEEDS_SPLIT`
```
/** This cluster may have multiple disconnected components, which are all NEEDS_RELINEARIZE. */
NEEDS_SPLIT,
``
and was asking if this still holds in every case (even for small clusters)
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415235592)
The question was referring to the documentation of `NEEDS_SPLIT`
```
/** This cluster may have multiple disconnected components, which are all NEEDS_RELINEARIZE. */
NEEDS_SPLIT,
``
and was asking if this still holds in every case (even for small clusters)
💬 Dorex45 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3383602356)
@earonesty
I don’t believe humanity has “failed” Bitcoin just because certain segments like JPEG-degens are experimenting and paying high fees. Bitcoin was never about dictating use cases; it was about open access, permissionless innovation, and monetary sovereignty. What we’re seeing is simply "market discovery on-chain.""
But here’s the real issue , Bitcoin’s base layer was never meant to host high-volume or high-frequency applications. It’s a settlement layer, and anything that tries to pus
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3383602356)
@earonesty
I don’t believe humanity has “failed” Bitcoin just because certain segments like JPEG-degens are experimenting and paying high fees. Bitcoin was never about dictating use cases; it was about open access, permissionless innovation, and monetary sovereignty. What we’re seeing is simply "market discovery on-chain.""
But here’s the real issue , Bitcoin’s base layer was never meant to host high-volume or high-frequency applications. It’s a settlement layer, and anything that tries to pus
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415494808)
Maybe it could say "which will all be made NEEDS_RELINEARIZE". It's always possible (even for non-tiny clusters) that nothing really needs to be done with it anymore.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2415494808)
Maybe it could say "which will all be made NEEDS_RELINEARIZE". It's always possible (even for non-tiny clusters) that nothing really needs to be done with it anymore.
🤔 kannapoix reviewed a pull request: "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py"
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3317125427)
code review ACK: f45b94d302a
tests pass locally. I left a nit/question about mocktime.
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3317125427)
code review ACK: f45b94d302a
tests pass locally. I left a nit/question about mocktime.
💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2415489353)
nit: Do we still need this mocktime? The tests seem to pass without it.
If it is necessary, could we use a more understandable value? Previously it was chosen to minimize diffs in generated test data, but that constraint no longer applies here: https://github.com/bitcoin/bitcoin/pull/14802#discussion_r279476692
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2415489353)
nit: Do we still need this mocktime? The tests seem to pass without it.
If it is necessary, could we use a more understandable value? Previously it was chosen to minimize diffs in generated test data, but that constraint no longer applies here: https://github.com/bitcoin/bitcoin/pull/14802#discussion_r279476692
💬 maflcko commented on pull request "ci: Properly include $FILE_ENV in DEPENDS_HASH":
(https://github.com/bitcoin/bitcoin/pull/33581#issuecomment-3384368809)
No idea what the unrelated intermittent CI failure is about: https://github.com/bitcoin/bitcoin/actions/runs/18358988770/job/52298824086?pr=33581#step:12:287 It just seems to stop during the very first init of bitcoind.exe.
(https://github.com/bitcoin/bitcoin/pull/33581#issuecomment-3384368809)
No idea what the unrelated intermittent CI failure is about: https://github.com/bitcoin/bitcoin/actions/runs/18358988770/job/52298824086?pr=33581#step:12:287 It just seems to stop during the very first init of bitcoind.exe.