💬 ryanofsky commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2248316407)
Mentioning the history of this change in the PR description is helpful, but it would be clearer if it said what the change is and what effects it has.

From code comments in this PR and https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2188840483, it seems like the main intended effect of this change is to prevent github tasks from running twice in PRs and causing the following output:

> <img width="1038" alt="Scherm­afbeelding 2024-06-25 om 14 48 34" src="https://github.com/bitcoi
...

💬 fanquake commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2248331845)
Guix Build (aarch64):
```bash
b2617f05ed438479493daac5d6faa637d9bc8368186f41db270392ae8bcc2ff1 guix-build-c399c80a09a3/output/aarch64-linux-gnu/SHA256SUMS.part
255827ffdae24ce4099885caa4517a0d05e883adc49f92bf36226afa910d1b6b guix-build-c399c80a09a3/output/aarch64-linux-gnu/bitcoin-c399c80a09a3-aarch64-linux-gnu-debug.tar.gz
299e1401c19e901c30096b870b34c342318d08c89bc3b6ac02a82e718ddc41ad guix-build-c399c80a09a3/output/aarch64-linux-gnu/bitcoin-c399c80a09a3-aarch64-linux-gnu.tar.gz
dea5a7
...

💬 maflcko commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#issuecomment-2248337297)
re-ACK e3edaccd9deb2da50be70d2d8768eca8821785c7

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

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

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690049630)
Good catch. Fixed the typo!

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