💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909271584)
optional nit when their is need to retouch
In "txgraph: (feature) add initial version" 0c8dc2323eb1ec34357a807f0860cf0a08a63a75
you mean `QualityLevel::ACCEPTABLE` right we could just reference the enum like you were doing `txgraph.cpp`?
```suggestion
* The transactions will be returned in a topologically-valid order of acceptable quality (QualityLevel::ACCEPTABLE).
```
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909271584)
optional nit when their is need to retouch
In "txgraph: (feature) add initial version" 0c8dc2323eb1ec34357a807f0860cf0a08a63a75
you mean `QualityLevel::ACCEPTABLE` right we could just reference the enum like you were doing `txgraph.cpp`?
```suggestion
* The transactions will be returned in a topologically-valid order of acceptable quality (QualityLevel::ACCEPTABLE).
```
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909270566)
nit when retouching
In " txgraph: (feature) add staging support" 972df15af28ba6787f096162f776b2973342e674
```suggestion
/** Get the feerate of the chunk which transaction arg is in the main graph. Returns the
```
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909270566)
nit when retouching
In " txgraph: (feature) add staging support" 972df15af28ba6787f096162f776b2973342e674
```suggestion
/** Get the feerate of the chunk which transaction arg is in the main graph. Returns the
```
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909277721)
In "clusterlin: add FixLinearization function + fuzz test" 1c5f1c4601e0a895258cfe073125361f7a6ea012
Is it safe to get the `ClusterIndex` when j is 0, we will subtract 1 from `uint32_t` which will result to a large positive number?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909277721)
In "clusterlin: add FixLinearization function + fuzz test" 1c5f1c4601e0a895258cfe073125361f7a6ea012
Is it safe to get the `ClusterIndex` when j is 0, we will subtract 1 from `uint32_t` which will result to a large positive number?
📝 achow101 opened a pull request: "doc: Archive 28.1 release notes"
(https://github.com/bitcoin/bitcoin/pull/31630)
(https://github.com/bitcoin/bitcoin/pull/31630)
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r1893202920)
Could make sense to run this before the `tee /tmp/iwyu_ci.out` command, given that it errors? Or would it be missing (part of) the output? It would be good to see an example CI run. Maybe a `-- --keep-going` could be used for a slightly better output, if stuff is possibly missing?
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r1893202920)
Could make sense to run this before the `tee /tmp/iwyu_ci.out` command, given that it errors? Or would it be missing (part of) the output? It would be good to see an example CI run. Maybe a `-- --keep-going` could be used for a slightly better output, if stuff is possibly missing?
🤔 theuni reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2540686525)
Very quick and shallow pass through the initial impl commit. This PR is a lot to get through :)
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2540686525)
Very quick and shallow pass through the initial impl commit. This PR is a lot to get through :)
💬 theuni commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909286865)
I'm not sure how this is intended to be used, but storing a stack address seems like a problem? RVO may help but that seems brittle. I imagine the caller should be passing in their own `Ref` instead?
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909286865)
I'm not sure how this is intended to be used, but storing a stack address seems like a problem? RVO may help but that seems brittle. I imagine the caller should be passing in their own `Ref` instead?
💬 theuni commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909293407)
Why is this doing an effective swap? I would expect this to call `UnlinkRef` on the moved-from value and reset its `m_graph` and `m_index`. Otherwise it wouldn't be unlinked until the moved-from variable goes out of scope, no?
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909293407)
Why is this doing an effective swap? I would expect this to call `UnlinkRef` on the moved-from value and reset its `m_graph` and `m_index`. Otherwise it wouldn't be unlinked until the moved-from variable goes out of scope, no?
💬 theuni commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909321579)
Assume `!to_remove.empty()` or early return if it's allowed?
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909321579)
Assume `!to_remove.empty()` or early return if it's allowed?
💬 theuni commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909295799)
`m_ref{nullptr};`
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909295799)
`m_ref{nullptr};`
💬 theuni commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909307659)
Trying to convince myself this is guaranteed to terminate...
`do{} while (!chunk.transactions.None())` rather than the `break` for readability? Or just `while()` if we need to guard against an empty linearization (presumably not?)
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909307659)
Trying to convince myself this is guaranteed to terminate...
`do{} while (!chunk.transactions.None())` rather than the `break` for readability? Or just `while()` if we need to guard against an empty linearization (presumably not?)
💬 theuni commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909348870)
Looks like these 3 functions could `reserve()` for their `ret` vectors.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909348870)
Looks like these 3 functions could `reserve()` for their `ret` vectors.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909373642)
I consider the `QualityLevel` enum to be an implementation detail of the txgraph module that should not leak into its interface.
However, I guess it would be helpful to specify more clearly what acceptable mean. I'll think about that; it's hard to define.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909373642)
I consider the `QualityLevel` enum to be an implementation detail of the txgraph module that should not leak into its interface.
However, I guess it would be helpful to specify more clearly what acceptable mean. I'll think about that; it's hard to define.
👍 theuni approved a pull request: "depends: Fix spacing issue"
(https://github.com/bitcoin/bitcoin/pull/31627#pullrequestreview-2540839651)
Nice catch. utACK 8a46286da667d19414c30350df48ebf245589e32
(https://github.com/bitcoin/bitcoin/pull/31627#pullrequestreview-2540839651)
Nice catch. utACK 8a46286da667d19414c30350df48ebf245589e32
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909379046)
`j` cannot be 0. If it were, there is necessarily nothing `elem` needs to be moved before anymore, and `place_before` would be empty, and the loop would have terminated.
Will add an `Assume(j > 0);` on the next push.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909379046)
`j` cannot be 0. If it were, there is necessarily nothing `elem` needs to be moved before anymore, and `place_before` would be empty, and the loop would have terminated.
Will add an `Assume(j > 0);` on the next push.
👍 TheCharlatan approved a pull request: "Safegcd-based modular inverses in MuHash3072"
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2540842762)
Re-ACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
Range-diff'ed from the last push, changes are marking functions as `inline` and adding some constants. There are some minor formatting issues, which can be fixed by running the commits through `clang-format-diff`.
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2540842762)
Re-ACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
Range-diff'ed from the last push, changes are marking functions as `inline` and adding some constants. There are some minor formatting issues, which can be fixed by running the commits through `clang-format-diff`.
💬 theuni commented on pull request "depends: Use base system's `sha256sum` utility":
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1909384870)
`sha256sum` has never existed by default on macOS afaik. That's actually why this variable exists in the first place. perl or no, shasum is always there.
It's not present for me on Sonoma.
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1909384870)
`sha256sum` has never existed by default on macOS afaik. That's actually why this variable exists in the first place. perl or no, shasum is always there.
It's not present for me on Sonoma.
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909385059)
I believe it is safe, both with RVO and without (but since C++17 RVO is mandatory, so that suffices).
With RVO, `ret` is constructed directly in the caller's target destination, so this isn't a pointer to local stack space.
Without RVO, the `Ref(Ref&&)` move constructor is invoked, which will update the pointer to the caller's destination.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909385059)
I believe it is safe, both with RVO and without (but since C++17 RVO is mandatory, so that suffices).
With RVO, `ret` is constructed directly in the caller's target destination, so this isn't a pointer to local stack space.
Without RVO, the `Ref(Ref&&)` move constructor is invoked, which will update the pointer to the caller's destination.
👍 theuni approved a pull request: "doc: upgrade license to 2025."
(https://github.com/bitcoin/bitcoin/pull/31611#pullrequestreview-2540859694)
ACK b537a2c02a9921235d1ecf8c3c7dc1836ec68131
Just missed it for 28.1, but this needs a backport in case we cut a future v28.
(https://github.com/bitcoin/bitcoin/pull/31611#pullrequestreview-2540859694)
ACK b537a2c02a9921235d1ecf8c3c7dc1836ec68131
Just missed it for 28.1, but this needs a backport in case we cut a future v28.
💬 hebasto commented on pull request "depends: Use base system's `sha256sum` utility":
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1909390612)
Thanks! This change has been dropped.
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1909390612)
Thanks! This change has been dropped.