💬 fanquake commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164128824)
> What version of CMake are you using?

Happens with `3.28.3` & `4.0.3`.

💬 fanquake commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164147912)
> I think the guix build has set -e, so it should be fine and not proceed here.

Looks like this is the case, and tested build does fail. So I guess it will just (potentially) silently ignore errors everywhere else.

👋 fanquake's pull request is ready for review: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33074)

💬 Crypt-iQ commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2260386539)
> It does, as far as i can tell.

Oh ok, that was not obvious to me. I think the OP / last commit message needs to be modified: "The existing implementation would treat it as witness stripped if the P2WPKH input appears first, and consensus invalid if it appears second." is not true.

I tested this in master and both orders of inputs give consensus invalid, this PR gives witness-stripped in both orders. If the P2WPKH input appears first, the first `CheckInputScripts` will fail with a consen
...

🚀 fanquake merged a pull request: "cmake: Install internal binaries to <prefix>/libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679)

💬 TheCharlatan commented on pull request "cmake: Install internal binaries to <prefix>/libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3164325393)
Post-merge ACK f49840dd902c

💬 darosior commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2260436877)
> I tested this in master and both orders of inputs give consensus invalid

Ah, yes, right! Thank you for testing, will modify the commit message / OP.

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260443448)
Correct, I should add a comment perhaps, but choosing only a single cache type (blob storage or http endpoint) was done becuase I think we are just otherwise doubling the space the ccache storage will take per job:

1 x storage local blob
1x storage http endpoint

Both will store the same cached compilation, and both will use cache quota.

As the restore policy of the blob storage is currently backwards (IMO), use only http endpoint for ccache, and let ccache handle LRU evictions itself.

👍 pinheadmz approved a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-3097356180)
re-ACK 37cd2c076434e7acbdbb20996cf87afb2cb5bc84

Changes since last ACK:
`git range-diff ad01440~6..ad01440 37cd2c0764~9..37cd2c0764`
Address PR feedback adding a line to interface docs, add mining interface to the GUI for testing, and refactors the test commits.

I rebased this PR at this commit on to #32345 and pummeled the IPC-RPC interface for an hour with no crash, so I think it's important that those libmutliprocess changes get merged as well, and *probably* should be merged first?

...

💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3164396555)
guix build for verification

`
aarch64
daa954407e65e2f3baba9ed349d46f321786db7822db988f522577a1eb7647c1 guix-build-0560c98ca110/output/dist-archive/bitcoin-0560c98ca110.tar.gz
f7bcb25a8f555b22b07c713d13823f3e07cfb9f4e2e1da1ac197d019fb89da60 guix-build-0560c98ca110/output/riscv64-linux-gnu/SHA256SUMS.part
3275ce7634c4b93b5733985aa6b761ed2b87d7984e44bb79f375a3e25dbd6511 guix-build-0560c98ca110/output/riscv64-linux-gnu/bitcoin-0560c98ca110-riscv64-linux-gnu.tar.gz
7b5988013467c397fd2f4c23
...

💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3164401518)
guix build for verification

```
daa954407e65e2f3baba9ed349d46f321786db7822db988f522577a1eb7647c1 guix-build-0560c98ca110/output/dist-archive/bitcoin-0560c98ca110.tar.gz
f7bcb25a8f555b22b07c713d13823f3e07cfb9f4e2e1da1ac197d019fb89da60 guix-build-0560c98ca110/output/riscv64-linux-gnu/SHA256SUMS.part
3275ce7634c4b93b5733985aa6b761ed2b87d7984e44bb79f375a3e25dbd6511 guix-build-0560c98ca110/output/riscv64-linux-gnu/bitcoin-0560c98ca110-riscv64-linux-gnu.tar.gz
7b5988013467c397fd2f4c23105fc90
...

💬 m3dwards commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#issuecomment-3164429197)
> Other stuff to check would be that this works at all and doesn't just re-run an ancient commit, like [#32989 (comment)](https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3133536724)

Just ran a test to specifically test for this by updating the default (master) branch and adding and removing the label and it correctly picked an updated merge commit based on the PR merged into the updated master.

💬 m3dwards commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260511885)
Can you elaborate a little more on what this would look like? Would this be different labels for different jobs? As in a label that runs the Previous Releases job and a different label that would run a different job?

💬 maflcko commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260543464)
I don't know. Not sure if we want to see pull requests spammed with "added and removed" label events (https://github.com/testing-cirrus-runners/bitcoin/pull/19#event-19014793971).

GitHub doesn't make this easy and I wonder if we may want to just spin our own CI to detect silent merge conflicts.

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260557261)
how do evictions happen, if `export CCACHE_MAXSIZE=${CCACHE_MAXSIZE:-500M}` is set to 500M in total for all tasks?

💬 maflcko commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2260562862)
```suggestion
LogInfo("Failed to detect filesystem type for: %s", util::Join(error_paths, ", "));
```

nit: no need for newline in new code

💬 maflcko commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260589218)
To give some more context: We are now trying to add 200+ lines of code to this repo which have to be maintained, but they cause label spam, and likely aren't sufficient to achieve the goal, because the ci-failed notification will go to the one adding the label and triggering the workflow?

If so, we'd have to create a comment (or other notification) anyway. So if additional glue code is needed, we might as well handle all code externally.

🤔 Crypt-iQ reviewed a pull request: "validation: detect witness stripping without re-running Script checks"
(https://github.com/bitcoin/bitcoin/pull/33105#pullrequestreview-3097524181)
ACK 370770fb361fdb745cfa7bb9abc7ee4a7833b83b modulo commit message change and test comment fix

💬 Crypt-iQ commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2260577684)
should be "P2SH-wrapped P2WPKH"

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260635134)
Good question; remote backends are not governed by client-side CCACHE_MAXSIZE settings, see https://ccache.dev/manual/4.11.3.html#_http_storage_backend for a bit more info.

Basically the onus is on the server to periodically run `ccache --trim-dir` to keep it under control (we have asked for more details on this process cc @m3dwards )