📝 maflcko opened a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303)
Currently, the `actions/checkout@v5` checks out pull requests merged against master, which is what we want.

However, sometimes, it checks out ancient/stale merge commits. For example:

* https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:9:914 compiles with IPC=ON, even though latest master is at ed2ff3c63d83e275b0785a695fa4db38870abf1a
* https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3133536724 (example explained in comment)

This is pr
...

💬 maflcko commented on pull request "ci: disable cirrus cache in 32bit arm job":
(https://github.com/bitcoin/bitcoin/pull/33302#discussion_r2321455669)
?

`any || true`, will just be `true`, no?

Also, the error looks the same, so this has no effect?

💬 maflcko commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#issuecomment-3252845186)
concept ack, but I don't have the slightest idea if this is even possible (or how) with GHA

🚀 fanquake merged a pull request: "net, pcp: handle multi-part responses and filter for default route while querying default gateway"
(https://github.com/bitcoin/bitcoin/pull/32159)

✅ maflcko closed a pull request: "test: Fix CLI_MAX_ARG_SIZE issues"
(https://github.com/bitcoin/bitcoin/pull/33243)

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#issuecomment-3252941074)
(+GHA ci)

📝 maflcko reopened a pull request: "test: Fix CLI_MAX_ARG_SIZE issues"
(https://github.com/bitcoin/bitcoin/pull/33243)
`CLI_MAX_ARG_SIZE` has many edge case issues:

* It seems to be lower on some systems, but it is unknown how to reproduce locally: https://github.com/bitcoin/bitcoin/pull/33079#issuecomment-3139957274
* `MAX_ARG_STRLEN` is a limit per arg, but we probably want "The maximum length of [all of] the arguments": See https://www.man7.org/linux/man-pages/man3/sysconf.3.html, section `ARG_MAX - _SC_ARG_MAX`.
* It doesn't account for the additional args added by the `bitcoin` command later on: https:
...

✅ maflcko closed an issue: "integer sanitizer warning, when running with -natpmp=1 enabled"
(https://github.com/bitcoin/bitcoin/issues/33245)

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2321612373)
Thanks! Fixed in 91408755df...961f94b293.

💬 hebasto commented on pull request "cmake: Inherit WERROR setting for secp256k1 build":
(https://github.com/bitcoin/bitcoin/pull/33297#issuecomment-3253238601)
> [30f6429](https://github.com/bitcoin/bitcoin/commit/30f642952cb5bf39479bdbe467b3950f0d09324a) was a mistake...

Here is some historical context.

When migrating the build system from Autotools to CMake, there was a unanimous expectation that, at least initially, we should aim for 100% build option compatibility. That commit introduced CMake’s counterpart to Autotools’ `--enable-werror`:https://github.com/bitcoin/bitcoin/blob/6e62b705328f4a52875a1dd3bfb63fbba8586a01/configure.ac#L284-L289

💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#issuecomment-3253250540)
@Sjors Thanks for checking. I'll leave it as a follow-up.

💬 hebasto commented on pull request "cmake: Inherit WERROR setting for secp256k1 build":
(https://github.com/bitcoin/bitcoin/pull/33297#issuecomment-3253260402)
> Yes, that requires CMake 3.24 and we allow 3.22 as the minimum. It does not matter. Just accept that the setting will have no effect on Ubuntu 22.04. 🤷‍♂️

That seems acceptable to me, as none of the CI jobs use CMake older that 3.24. And when someone sets `CMAKE_COMPILE_WARNING_AS_ERROR` with an older CMake, it issues a warning:
```
CMake Warning:
Manually-specified variables were not used by the project:

CMAKE_COMPILE_WARNING_AS_ERROR


```

💬 stickies-v commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253266431)
If I understand correctly - the expected behaviour for `actions/checkout@v5` for `pull_request` is to checkout a temporary merge commit onto the merge branch, which should be re-evaluated every time the workflow is reran (e.g. a manual trigger), which is what we want to avoid silent merge conflicts. But you've observed that in some cases, the merge branch (`GITHUB_REF`) is stale. Do you know why that is happening, and is this an upstream bug? I've browsed the documentation and https://github.com
...

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2321775620)
nit: Any reason to use `md` here? Most of the time 7/8 CPUs will be idle (to download the caches or to upload the artifact). Also, the similar mac-cross build is using `sm`.

Suggested diff:

```diff
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 6876b8328d..2efe17a04e 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -302,7 +302,7 @@ jobs:
windows-cross:
name: 'Linux->Windows cross, no tests'
needs: runners
- runs-on: ${
...

💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321725927)
Interesting - took me some time to find out.

I see that the transactions are marked dirty when the descriptor is imported that breaks the transaction caches. I confirmed this by commenting the following portion from the `MarkDirty` function after which the test fails:

https://github.com/bitcoin/bitcoin/pull/33268/commits/113a4228229baedda2a730e097f2d59ad58a4b0d#diff-d41d68c5a65d67956c91b33ca86da7df1981d84a0b15b4a186deea566563fed5R344

```cpp
m_cached_from_me = std::nullopt;
```

I
...

👍 rkrux approved a pull request: "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet"
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3184703865)
lgtm ACK 113a4228229baedda2a730e097f2d59ad58a4b0d

Generally, I find it easy to miss these `Cached*` functions because they lie in this `wallet/receive.cpp` file that I don't find intuitive; `wallet/transaction.cpp` could be more intuitive because most of these functions update the `CWallet` object somehow - this PR need not do anything about it though.

Ok with addition of `m_cached_from_me` within `WalletTx` in this PR but I find `Cached*` functions are too many to keep track of.

💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2321729980)
In https://github.com/bitcoin/bitcoin/commit/009b273df8ad8a7470276a31f3cb900e480aa99b "test: Test wallet 'from me' status change"

Nit if retouched:
```diff
- self.log.info("Test gettransaction after changing a transaction's 'from me' status")
+ self.log.info("Test gettransaction after changing a transaction's 'from me' status due to importing descriptor")
```

💬 janb84 commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253317106)
> If I understand correctly - the expected behaviour for `actions/checkout@v5` for `pull_request` is to checkout a temporary merge commit onto the merge branch, which should be re-evaluated every time the workflow is reran (e.g. a manual trigger), which is what we want to avoid silent merge conflicts. But you've observed that in some cases, the merge branch (`GITHUB_REF` [aka](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request) `refs/pul
...

💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253384950)
I am not familiar with the action written in typescript and I don't want to touch it upstream. However, I am happy to close this pull, if someone else does.

The issue is that the action does not use the ref, but the commit to fetch: https://github.com/actions/checkout/blob/ff7abcd0c3c05ccf6adc123a8cd1fd4fb30fb493/src/ref-helper.ts#L96

You can also see this here: https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:3:59:

```
/usr/bin/git -c protoco
...

💬 hebasto commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3253418623)
> Currently, the `actions/checkout@v5` checks out pull requests merged against master, which is what we want.
>
> However, sometimes, it checks out ancient/stale merge commits. For example:
>
> * https://github.com/bitcoin/bitcoin/actions/runs/17458152407/job/49579638898?pr=29641#step:9:914 compiles with IPC=ON, even though latest master is at [ed2ff3c](https://github.com/bitcoin/bitcoin/commit/ed2ff3c63d83e275b0785a695fa4db38870abf1a)
>
> * [ci: Migrate CI to hosted Cirrus Runner
...