💬 maflcko commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2212508710)
the typo wasn't fixed in the recent force pushes, fwiw

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-3083004175)
> will look into fixing the CI job

Just add a dummy commit so the subtree merge is more than 6 deep :-)

📝 maflcko opened a pull request: "ci: Use APT_LLVM_V in msan task"
(https://github.com/bitcoin/bitcoin/pull/32999)
This skips compilation of clang by using the apt.

💬 Sjors commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3083204938)
I tried to use this to boost the silent payment indexer, but I don't know what I'm doing :-) https://github.com/Sjors/bitcoin/pull/96

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212634650)
nit in https://github.com/bitcoin/bitcoin/commit/2d0ac71899eadcbff8252b1704d2affb73f0160d: Why not just `git rev-parse HEAD:depends`, like it was done for cirrus? (See `git log -S 'git rev-parse HEAD:depends'`)

👍 maflcko approved a pull request: "Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3028393275)
I guess you want review here and then address it, as it comes in? Once review is finished, the app will be installed and reviewers can also look at a "real" run in this repo?

looked at c0ad2b6aa8e8c31c9f9c9ea2b35ca86f7985c490~23 🎬

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_t
...

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212678980)
nit in the same commit: Could use `github.event.repository.default_branch` instead of `master`, so that caches can be saved for other repos, such as inquisition?

(same below)

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212606651)
nit in 2d0ac71899eadcbff8252b1704d2affb73f0160d: Should be named job-id, not the name?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212658179)
nit in https://github.com/bitcoin/bitcoin/commit/2d0ac71899eadcbff8252b1704d2affb73f0160d: why hardcode `master`. This makes it impossible to cache in pull requests, if this is ever done in the future. Also, only `master` caches are created, so this doesn't guard against anything.

Also, the others don't have it.

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212642449)
nit in https://github.com/bitcoin/bitcoin/commit/2d0ac71899eadcbff8252b1704d2affb73f0160d: Comment seems wrong? It is a cache for each job-id + ref_name + run_id. Could just remove the comment?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212693443)
same commit: Unused?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212685421)
same commit: This seems broken? A job-id with NO_QT=1 populating this would corrupt the cache for other job-id's that build qt?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212715537)
nit in b1a4adc814369aff21b8c12775270cfe648db486: Could add a comment that this is only used to extract CONTAINER_NAME.

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212722450)
9002faff0d7518ebafe05d8d43d6558d238c2659: adjust for https://quay.io/repository/bitcoincore/bitcoin-ci ?

💬 fanquake commented on pull request "ci: Use APT_LLVM_V in msan task":
(https://github.com/bitcoin/bitcoin/pull/32999#discussion_r2212750361)
`.8` while touching?

💬 fanquake commented on pull request "ci: switch MSAN to use prebuilt Clang binaries":
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-3083228052)
PIcked up in #32999.

💬 fanquake commented on pull request "ci: Use APT_LLVM_V in msan task":
(https://github.com/bitcoin/bitcoin/pull/32999#discussion_r2212772055)
Actually, I'm wondering why these can't both be 20.1.8? If the checkout needs to match the apt version, then its going to break whenever the apt is updated but this isn't bumped.

💬 maflcko commented on pull request "ci: Use APT_LLVM_V in msan task":
(https://github.com/bitcoin/bitcoin/pull/32999#discussion_r2212817198)
There shouldn't be any breaking changes in minor versions, so anything is probably fine here. However, if msan fuzz wants to use a different major version, it seems better to keep it separate?

Maybe a follow-up can investigate if msan fuzz can bootstrap `compiler-rt` (libfuzzer) directly, and remove all of this code here.

however, my guess is that you still need to compile at least libcxx(abi) in this this if-branch.

👋 maflcko's pull request is ready for review: "ci: Use APT_LLVM_V in msan task"
(https://github.com/bitcoin/bitcoin/pull/32999)

💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2212838453)
Yes, the return false would prevent potentially writing false stats because the member variables are not up-to-date with what the prev block should be. I don't remember what the scenarios were but I think there was also fear of some edge cases with reorgs but from today's perspective I am not sure anymore how that would happen. Maybe the refactorings in the index have made this scenario impossible but I'll have to think about it some more.