💬 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.
(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.
(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.
(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.
(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.
(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.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417954576)
Done, in a new comment-fixing commit.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417955778)
Done.
It's simply that in a singleton cluster, the ancestors of a transaction, the descendants of a transaction, and in fact the entire cluster are always just the same single element.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417955778)
Done.
It's simply that in a singleton cluster, the ancestors of a transaction, the descendants of a transaction, and in fact the entire cluster are always just the same single element.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417958188)
Nice. I've replaced it with an `Assume` that the adding always happens at the end.
This made me realize that there is probably a more efficient `GenericClusterImpl::Merge` implementation possible (which just translates the indices in the old cluster to the new one by appending an offset to all). It's pretty nontrivial to do, so I'm leaving that for a potential follow-up if anyone cares.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417958188)
Nice. I've replaced it with an `Assume` that the adding always happens at the end.
This made me realize that there is probably a more efficient `GenericClusterImpl::Merge` implementation possible (which just translates the indices in the old cluster to the new one by appending an offset to all). It's pretty nontrivial to do, so I'm leaving that for a potential follow-up if anyone cares.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417959306)
This is now dealt with more consistently in a new commit that introduces a range of valid transaction counts for each `Cluster` implementation.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417959306)
This is now dealt with more consistently in a new commit that introduces a range of valid transaction counts for each `Cluster` implementation.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417961019)
Why not both?
They're independent notions: one is a policy-configurable per-TxGraph maximum cluster count, and the other is a Cluster-implementation dependent limit.
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417961019)
Why not both?
They're independent notions: one is a policy-configurable per-TxGraph maximum cluster count, and the other is a Cluster-implementation dependent limit.
Done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417961661)
Right.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417961661)
Right.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417961927)
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417961927)
Done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417962125)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417962125)
Fixed.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417962445)
Good idea, done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417962445)
Good idea, done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417963146)
Fair point, done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417963146)
Fair point, done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417964329)
Indeed. Changed into an `assert(false);`, plus a comment explaining why, and a `SingletonClusterImpl::SanityCheck()` check to verify that claim.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417964329)
Indeed. Changed into an `assert(false);`, plus a comment explaining why, and a `SingletonClusterImpl::SanityCheck()` check to verify that claim.
📝 amishhaa opened a pull request: "Contrib: Updated macdeployqtplus to remove deprecated --deep signing"
(https://github.com/bitcoin/bitcoin/pull/33592)
**Description**
Removed the deprecated --deep flag from codesign in macdeployqtplus and replaced it with an explicit recursive signing process for all binaries, frameworks, and plugins.
Fixes #32486
<img width="624" height="370" alt="Screenshot 2025-10-10 at 2 36 16 AM" src="https://github.com/user-attachments/assets/6329b67e-e400-4d8d-947c-25bceab4cb55" />
**Steps To Reproduce**
N/A since not a bug only deprecating an arg
**Steps To Test**
- Set up the app normally.
- Signing w
...
(https://github.com/bitcoin/bitcoin/pull/33592)
**Description**
Removed the deprecated --deep flag from codesign in macdeployqtplus and replaced it with an explicit recursive signing process for all binaries, frameworks, and plugins.
Fixes #32486
<img width="624" height="370" alt="Screenshot 2025-10-10 at 2 36 16 AM" src="https://github.com/user-attachments/assets/6329b67e-e400-4d8d-947c-25bceab4cb55" />
**Steps To Reproduce**
N/A since not a bug only deprecating an arg
**Steps To Test**
- Set up the app normally.
- Signing w
...
📝 hebasto opened a pull request: "guix: Use UCRT runtime for Windows release binaries"
(https://github.com/bitcoin/bitcoin/pull/33593)
This PR switches Windows release binaries to UCRT and resolves one item from https://github.com/bitcoin/bitcoin/issues/30210:
- [x] A symbol-check that the new runtime is indeed being used.
Guix builds on both `x86_64` and `aarch64` platforms:
```
b8df1d6c3bf2a4342aa19385444b221503eec2faed8656645e656fc60c1563a0 guix-build-f380e6251021/output/dist-archive/bitcoin-f380e6251021.tar.gz
3c57789c22a0670b1395f25bab2b5951d58b3cef57d33d48c93b1f51b0ca21d9 guix-build-f380e6251021/output/x86_64-w64
...
(https://github.com/bitcoin/bitcoin/pull/33593)
This PR switches Windows release binaries to UCRT and resolves one item from https://github.com/bitcoin/bitcoin/issues/30210:
- [x] A symbol-check that the new runtime is indeed being used.
Guix builds on both `x86_64` and `aarch64` platforms:
```
b8df1d6c3bf2a4342aa19385444b221503eec2faed8656645e656fc60c1563a0 guix-build-f380e6251021/output/dist-archive/bitcoin-f380e6251021.tar.gz
3c57789c22a0670b1395f25bab2b5951d58b3cef57d33d48c93b1f51b0ca21d9 guix-build-f380e6251021/output/x86_64-w64
...
💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2418065022)
I like the suggested approach, it is alos much than a multi-step approach, so I changed to it.
> If performance of LastCommonAncestor is a concern
I don't think so, in particular because it replaces the long-existing `LastCommonAncestor(state->pindexLastCommonBlock, state->pindexBestKnownBlock);` call which should be of very similar duration (during IBD, the old `pindexLastCommonBlock` is close to the current tip, while `pindexBestKnownBlock` is at the best known header typically.
> I g
...
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2418065022)
I like the suggested approach, it is alos much than a multi-step approach, so I changed to it.
> If performance of LastCommonAncestor is a concern
I don't think so, in particular because it replaces the long-existing `LastCommonAncestor(state->pindexLastCommonBlock, state->pindexBestKnownBlock);` call which should be of very similar duration (during IBD, the old `pindexLastCommonBlock` is close to the current tip, while `pindexBestKnownBlock` is at the best known header typically.
> I g
...
💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3387634625)
[b2c6cb7](https://github.com/bitcoin/bitcoin/commit/b2c6cb7f026ff704c227e632f8c5c2f09e6f058a) to [01cc20f](https://github.com/bitcoin/bitcoin/commit/01cc20f3307c532f402cdf5dc17f2f14920b725b): changed the approach of the main commit to the suggestion by @sipa. I also added a comment and rewrote the commit message.
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3387634625)
[b2c6cb7](https://github.com/bitcoin/bitcoin/commit/b2c6cb7f026ff704c227e632f8c5c2f09e6f058a) to [01cc20f](https://github.com/bitcoin/bitcoin/commit/01cc20f3307c532f402cdf5dc17f2f14920b725b): changed the approach of the main commit to the suggestion by @sipa. I also added a comment and rewrote the commit message.