👍 hebasto approved a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3185655446)
ACK fa8f944eaa1955e4e2c376ce36f1b1cbb1897769, tested in my personal repo.

At some point, it might be useful to use YAML anchor and aliases for repeated "Checkout" steps. For example: https://github.com/bitcoin-core/secp256k1/pull/1719/commits/50e73c6eee8be094ec75920b5b71cdc5984452de.

The new re-run behaviour will only be available for PRs that are rebased after this one is merged.

💬 hebasto commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2322407937)
1. The `github.ref` can be used directly:
```suggestion
ref: ${{ github.event.pull_request.number && github.ref || '' }}
```

2. When combined with suggestion 1, the check can be made more explicit:
```suggestion
ref: ${{ github.event_name == 'pull_request' && github.ref || '' }}
```

3. Since this PR changes the documented behavior when re-running a CI job, it seems reasonable to add a comment to document:
- the new behavior;
- a caution not to rely on `github.sh
...

💬 willcl-ark commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254061326)
> At some point, it might be useful to use YAML anchor and aliases for repeated "Checkout" steps. For example: [bitcoin-core/secp256k1@50e73c6](https://github.com/bitcoin-core/secp256k1/commit/50e73c6eee8be094ec75920b5b71cdc5984452de).

Wow, I'm not sure why but I had in my head that GitHub did not support anchors (annoyingly so). And their alternative was basically "make your own action for it".

Will look at using a few anchors now I know they work, thanks.

💬 glozow commented on issue "Please restrict Data Carrier/OP Return to < 80 bytes please before releasing 3":
(https://github.com/bitcoin/bitcoin/issues/33298#issuecomment-3254071981)
The concerns outlined in the issue have been addressed. The discussion is now about open source developers' responsibility for usage, which is off topic.

A discussion page is available for feedback on moderation around this issue, https://github.com/bitcoin/bitcoin/issues/33240, https://github.com/bitcoin/bitcoin/pull/32381, https://github.com/bitcoin/bitcoin/pull/32359, https://github.com/bitcoin/bitcoin/pull/32406 etc.

💬 hebasto commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254079267)
> > At some point, it might be useful to use YAML anchor and aliases for repeated "Checkout" steps. For example: [bitcoin-core/secp256k1@50e73c6](https://github.com/bitcoin-core/secp256k1/commit/50e73c6eee8be094ec75920b5b71cdc5984452de).
>
> Wow, I'm not sure why but I had in my head that GitHub did not support anchors (annoyingly so). And their alternative was basically "make your own action for it".

It was introduced fairly [recently](https://github.com/actions/runner/issues/1182#issueco
...

👍 dergoegge approved a pull request: "contrib: update fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/33283#pullrequestreview-3185709140)
ACK 939678940f6c3fdbc36d57a9c9ef6f8edf89d065

I ran the script myself and the diff of the result is quite large, which seems expected since none of this is deterministic.

💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254113224)
> Will look at using a few anchors now I know they work, thanks.

The qa-assets repo actually uses the GitHub actions shipped by Bitcoin Core, so I think keeping them is preferable than anchors in this instance. Ref:

https://github.com/bitcoin-core/qa-assets/blob/fc42e64f6e664e15549f608e2cc90d8af9a515b1/.github/workflows/ci.yml#L56-L75

💬 hebasto commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254136840)
> > Will look at using a few anchors now I know they work, thanks.
>
> The qa-assets repo actually uses the GitHub actions shipped by Bitcoin Core, so I think keeping them is preferable than anchors in this instance. Ref:
>
> https://github.com/bitcoin-core/qa-assets/blob/fc42e64f6e664e15549f608e2cc90d8af9a515b1/.github/workflows/ci.yml#L56-L75

Then perhaps encapsulate customized `chceckout` action as well?

💬 theuni commented on pull request "depends: disable builtin rules and suffixes.":
(https://github.com/bitcoin/bitcoin/pull/33045#issuecomment-3254157282)
Concept ACK and utACK 803a2f5e9f869502536560f5bbe5979a137a20e5.

I was going to suggest:
- Using `--no-builtin-rules` rather than `-r` to be a little more self-documenting
- Using `GNUMAKEFLAGS` rather than `MAKEFLAGS` for better compatibility

But...

BSD Make supports `-r`, so that pretty much moots both points. Also, I'm pretty sure we require GNU Make anyway.

Only nit: Does anything depend on the builtin variables, or could we use `-R` as well?

👍 hebasto approved a pull request: "ci: cd into BASE_BUILD_DIR for GetCMakeLogFiles"
(https://github.com/bitcoin/bitcoin/pull/33291#pullrequestreview-3185794856)
ACK 9b76eef2d2b42703e2a30952d4c3474b533e360a, [obviously](https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3248645770).

🚀 hebasto merged a pull request: "ci: cd into BASE_BUILD_DIR for GetCMakeLogFiles"
(https://github.com/bitcoin/bitcoin/pull/33291)

💬 fanquake commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2322519068)
Looks like there might actually be a new `2.1.0` release shortly: https://github.com/capnproto/pycapnp/commit/3a3adfb5f1a8d1b52c98e4984f38ceb5b89a94a6. In which case, I think we should pin the checkout to that version, rather than cloning master (which may break at any point).

💬 willcl-ark commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3254192254)
I moved variables as per https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2302222710 to try and fix this in here. I did not verify whether it worked, however.

💬 fanquake commented on pull request "depends: strip when installing qt binaries":
(https://github.com/bitcoin/bitcoin/pull/33304#issuecomment-3254195129)
Guix Build (aarch64):
```bash
c805e4ee4a80b3b03919d280a32f272426811470646aca1e3da0a60255347738 guix-build-c9d5f211c119/output/aarch64-linux-gnu/SHA256SUMS.part
58f02ff3f18cd39e9379d0896680c9f7b6a3544a02362bb98570b95131f9d8cf guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu-debug.tar.gz
5432eaaacb722bb998c2fe76e56e62ef728f60bc7fb27d50dd9ca08dc4648c09 guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu.tar.gz
997f4c
...

💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254202893)
> Then perhaps encapsulate customized `checkout` action as well?

I don't think it matters much for other repos, but I am happy to push a commit, if someone writes one, or close this pull in favour of one that refactors this one :)

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2322561383)
Yeah, I thought about setting `-natpmp` to off, but decided would be better to test with the default arguments as that more closely matches the real world. That argument seems weak.

> band-aid more than a cure

I agree. Changed to what you suggest above.

💬 fqlx commented on pull request "rpc: require integer verbosity; remove boolean 'verbose'":
(https://github.com/bitcoin/bitcoin/pull/33214#discussion_r2322561424)
Moved this back into `rpc` namespace `server.cpp`

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3254248014)
`a9ac49c8ac...778675ac71`: pick https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2322207556

💬 ryanofsky commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#discussion_r2322584553)
> NACK to adding any depends specific code. `CMAKE_TOOLCHAIN_FILE` also doesn't necessarily mean a depends build; anyone can bring/use a toolchain file.

Yeah I think it is clear this is not a good long term solution, but to be fair I don't think there is much actual harm done here if CMAKE_TOOLCHAIN_FILE is set, since this would just fail to print a warning that is likely to be irrelevant.

But I do think it could be nice to close this PR and extend @willcl-ark's approach as described in ht
...

🤔 mzumsande reviewed a pull request: "index: Force database compaction in coinstatsindex"
(https://github.com/bitcoin/bitcoin/pull/33306#pullrequestreview-3185973049)
Do you know if the current state with many small files and one large file has actual downsides, or does it just look weird?

I am not an expert on LevelDB, just mentioning that two AIs I consulted say that benefits of regular compaction would be only of cosmetic value and suggest to just let LevelDB do its thing.

💬 maflcko commented on pull request "ci: disable cirrus cache in 32bit arm job":
(https://github.com/bitcoin/bitcoin/pull/33302#discussion_r2322672396)
hmm, still wrong https://github.com/bitcoin/bitcoin/actions/runs/17469260337/job/49613109539?pr=33302#step:5:7 ?