💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909208594)
> This changes the meaning of `getmininginfo` fields unexpectedly, and forces us to add in the coinbase reserved size over and over. Seems like it would be better to include it here once.

> We could either adjust the value in the RPC code, or as @luke-jr suggests restore the original behavior here.

I've reverted and included it but clarify the `getmininginfo` help text.

> I agree we should not change the `weight`, `currentblockweight` and `sigops` fields in `getblocktemplate` and `getmi
...

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1909208803)
Done.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2580905232)
Thanks for reviewing @pinheadmz @luke-jr @Sjors

I've recently foreced pushed from 2fb833e69627a8f9b1c4b14b60d1df9903adf210 to 7ab1344eb13228998bcead7328d5cc0a905971d4 [diff](https://github.com/bitcoin/bitcoin/compare/2fb833e69627a8f9b1c4b14b60d1df9903adf210..7ab1344eb13228998bcead7328d5cc0a905971d4)

Changes were:

1. Reverting https://github.com/bitcoin/bitcoin/commit/76a95522b322c1a8ab1594ca8bf5ac99ab379c46 but improving the help text.
2. Use policy `DEFAULT_MAX_BLOCK_WEIGHT` instead
...

💬 theuni commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580925856)
> > And doing that all over would be a recipe for never-ending churn
>
> Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?

Not change for change's sake, no. Please consider this from the POV of reviewers and maintainers. A constant stream of changes like that is exhausting. It would distract from larger improvements.

I think @maflcko's comment above is a good d
...

📝 mzumsande opened a pull request: "wallet: fix rescanning inconsistency"
(https://github.com/bitcoin/bitcoin/pull/31629)
If the chain advances during a rescan, ScanForWalletTransactions would previously process the new blocks without adjusting `m_last_processed_block`, which would leave the wallet in an inconsistent state temporarily, and could lead to crashes in the GUI reported in #31474.
Fix this by not rescanning blocks beyond `m_last_processed_block` - for all blocks beyond that height, there will be pending BlockConnected notifications that will process them after the rescan is finished.

This means that
...

💬 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.

💬 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.

💬 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.

💬 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.

💬 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).
```

💬 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
```

💬 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?

📝 achow101 opened a pull request: "doc: Archive 28.1 release notes"
(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?

🤔 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 :)

💬 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?

💬 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?

💬 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?

💬 theuni commented on pull request "cluster mempool: add TxGraph reorg functionality":
(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?)