💬 fanquake commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1690009227)
Thanks, had a look at this, and ran into issues with at least leveldb, which we'd first have to fix in the subtree, i.e:
```bash
In file included from leveldb/util/comparator.cc:14:
./leveldb/util/no_destructor.h:40:17: error: 'aligned_storage<8, 8>' is deprecated [-Werror,-Wdeprecated-declarations]
40 | typename std::aligned_storage<sizeof(InstanceType),
| ^
leveldb/util/comparator.cc:71:47: note: in instantiation of template class 'leveldb::NoDestructor<leveldb
...

💬 fanquake commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1690012798)
Thanks, added.

💬 fanquake commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1690022414)
There is an up-upstream issue / PR for this. https://github.com/google/leveldb/issues/1013 & https://github.com/google/leveldb/pull/896.

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