💬 mzumsande commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2580943009)
See #31629, I took your version, just added some doc changes.
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2580943009)
See #31629, I took your version, just added some doc changes.
💬 l0rinc commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580961020)
This PR was a follow-up from the comments on Marco's change.
The main changes are Russ's template simplification and my suggestion to const the writes to obviate which we're modifying.
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580961020)
This PR was a follow-up from the comments on Marco's change.
The main changes are Russ's template simplification and my suggestion to const the writes to obviate which we're modifying.
💬 sipa commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2580988597)
ACK b7f3b6fd96ca2d5d1f9538affef682f778bd07e8
In the log message `[bench] FlushStateToDisk: write coins cache to disk (598202 coins, 86527KiB) started`, the number of coins and the size is the total cache size now, including non-dirty entries. I guess it should be changed to just account for the dirty ones, as well as in the "Flushing large UTXO set to disk" warning message.
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2580988597)
ACK b7f3b6fd96ca2d5d1f9538affef682f778bd07e8
In the log message `[bench] FlushStateToDisk: write coins cache to disk (598202 coins, 86527KiB) started`, the number of coins and the size is the total cache size now, including non-dirty entries. I guess it should be changed to just account for the dirty ones, as well as in the "Flushing large UTXO set to disk" warning message.
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2581015366)
> it should be changed to just account for the dirty ones
That would be a follow-up from #28233.
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2581015366)
> it should be changed to just account for the dirty ones
That would be a follow-up from #28233.
💬 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`.