💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212242754)
maybe we could just join them via comma instead, something like:
```C++
const auto prefixes{util::Join(encoded_prefixes, ", ")};
```
or combining it with the above:
```C++
const auto prefixes{util::Join(Cat(std::vector(params.Base58EncodedPrefix(CChainParams::SCRIPT_ADDRESS)),
params.Base58EncodedPrefix(CChainParams::PUBKEY_ADDRESS)), ", ")};

```
(untested and we'd likely have to adjust the full texts accordingly)

-----

or combining it with the
...

💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212239517)
I know this was here before as well, but we have already checked `is_bech32` in the if condition, we could break up `if (!is_bech32 && DecodeBase58Check(str, data, MAX_BASE58_CHECK_CHARS)) {` into an ` if (!is_bech32) { if (DecodeBase58Check) ... } else { ... }` instead - preferably inverting it as well to `if (is_bech32) {} else if (DecodeBase58Check...)`. If you decide to accept these comments, please do it in multiple, focused commits, the changes should guide the reviewer and tell a story.

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212388363)
> Actually I feel a bit less convinced that dropping a `sudo ... ` command into the CI scripts is ideal

Yeah, they should be fine to run as a user normally. The only dependencies are Python, Bash, and a container engine.

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3082824189)
> This means a new pull request should hit a _pretty relevant_ cache.
> Old pull requests **which are not being rebased on master** may suffer from lower cache hit-rate.

I don't think this is true. A pull request that modifies a core header (like serialize.h) will now always start from a cold cache. The current persistent workers have a high ccache hit rate for pulls that are (force) pushed for minor fixups (https://0xb10c.github.io/bitcoin-core-ci-stats/graph/ccache/). Also, before CI runs,
...

🤔 naiyoma reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3028174620)
Concept ACK,

Wondering if the CI failure is related to this issue. ->https://github.com/bitcoin/bitcoin/issues/32855

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3082859342)
> > This means a new pull request should hit a _pretty relevant_ cache.
> > Old pull requests **which are not being rebased on master** may suffer from lower cache hit-rate.
>
> I don't think this is true. A pull request that modifies a core header (like serialize.h) will now always start from a cold cache. The current persistent workers have a high ccache hit rate for pulls that are (force) pushed for minor fixups ([0xb10c.github.io/bitcoin-core-ci-stats/graph/ccache](https://0xb10c.github.
...

💬 maflcko commented on pull request "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects":
(https://github.com/bitcoin/bitcoin/pull/32868#issuecomment-3082914368)
very nice, but it would be good to rebase and fix the ci failure.

review ACK 50b41bcfbfd93f103cf6e2af1999c43947bc3f40 🏋

<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_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+k
...

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