💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690051167)
Ok, done in both places

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690055634)
Reordered, because I changed the comment anyway in another push.

👍 theStack approved a pull request: "m_tx_download_mutex followups"
(https://github.com/bitcoin/bitcoin/pull/30507#pullrequestreview-2197161263)
lgtm ACK 1b732dbc3bd9c59303a774cf2095c2d6639dbe1d
(happy to re-ACK if the [`Assert` suggestion](https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689880324) for `pindexNewTip` is taken)

🤔 ismaelsadeeq reviewed a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2197175891)
Concept ACK

clean refactor IMO.

💬 stickies-v commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2248405674)
re-ACK fac0c3d4bfc97b94f0594f7606650921feef2c8a - only doc and test updates to address review comments, thanks!

👍 TheCharlatan approved a pull request: "cleanse: switch to SecureZeroMemory for Windows cross-compile"
(https://github.com/bitcoin/bitcoin/pull/26950#pullrequestreview-2197209786)
ACK c399c80a09a393d38368a44ef04753e9f62350f0

💬 ryanofsky commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1690107471)
re: https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1672501712

> nit: Maybe make a reference to a PR consistent with the [previous one](https://github.com/bitcoin/bitcoin/blob/9adebe145557ef410964dd48a02f3d239f488cd0/contrib/devtools/check-deps.sh#L55)?

Thanks, updated

🤔 ryanofsky reviewed a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415#pullrequestreview-2197231263)
Updated 5a8b9432cde11f6140855717af195d8b7e798d75 -> 114b2a406e604747bd856f566aa8c7ad84dd8f15 ([`pr/weakcheck.1`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.1) -> [`pr/weakcheck.2`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/weakcheck.1..pr/weakcheck.2)) making comment more consistent

💬 glozow commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1690114799)
I could add `Assume` to the `if` condition?

🤔 glozow reviewed a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2197271593)
ACK 25bf86a225b0df3f48ade1016b47f5ee1636b988

💬 luisschwab commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690137311)
while we're at it, should I also remove this cast?
```c++
unspent.pushKV("vout", (int32_t)outpoint.n);
````
`outpoint.n` is an `uint32_t`

🤔 mzumsande reviewed a pull request: "index: Fix coinstats overflow and introduce index versioning"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-2197320864)
Since rebuilding the coinstatsindex takes a long time and it's not corrupted (yet) on mainnet I thought that auto-migrating to the new format instead of making the user reindex might be more user-friendly:
I tried that out in https://github.com/mzumsande/bitcoin/commits/202407_coinstats_v1_automigrate/ (just an untested POC - also doesn't process any blocks saved by hash instead of height yet, but that should be possible to add.)
What do you think of that option?

🤔 sdaftuar reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2167121806)
Code review ACK cad318fa843f411e52c6761a4882bfaf0ad21812. Left some non-blocking nits/suggestions.

I've run the fuzz tests in earlier versions of this PR for a while, but I haven't done so with the latest branch -- will do that and also post some benchmarks in a bit.

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1671201334)
Perhaps worth documenting that if an existing linearization is passed in, it must be topologically valid, or else the output of `Linearize` may not be topologically valid.

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1671088434)
nit: s/the the/the/

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1676094213)
Given that there's more than one valid serialization that will deserialize to the same graph, perhaps it's worth commenting something to that effect here? (I guess we could also consider dropping this check and instead just checking that if we deserialize `hexenc` we get the same depgraph, but perhaps it's beneficial to check that what we think of as a canonical encoding is correct?)

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690139736)
Maybe a different variable here would be better, since `i` is already used in the outer loop?

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688552973)
We could consider checking that the entries in `linearization` are all in the expected range [0, depGraph.TxCount()), to avoid a crash here if a garbage linearization were passed in?

👍 hodlinator approved a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2196468742)
ACK fac0c3d4bfc97b94f0594f7606650921feef2c8a

`make check` passed and `test_runner.py` passed.

Happy you went with `FromHex()`!

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689620422)
Man, I was worried for a moment that this modification of switching to the `uint8_t` was setting the other end of the `base_blob` array, but it's the same. Will be glad to see the fragile function go. :+1: