🤔 janb84 reviewed a pull request: "Clear out space on GHA jobs"
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3304256794)
re ACK c01214e72b9876c59924c98e96f43906f5df3353

Changes sinds last ACK:
- changed precondition test for action step.
- removed superfluous line that sets a (now unused) variable

I like the solution because the alternative would be to create an custom GHA Image (as [suggested](https://github.com/orgs/community/discussions/160692#discussioncomment-10281958) by gh) which would be a more maintenance burden than this script.

💬 hebasto commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371370716)
> > > I guess this means any fs::path would have to be explicitly cast to a std::fs::path, which seems fine to do.
> >
> >
> > This is one way of doing it.
>
> I don't understand this response. What are the other ways of doing it?

https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371016620:
> > > I guess this means any fs::path would have to be explicitly cast to a std::fs::path, which seems fine to do.
> >
> >
> > This is one way of doing it.
>
> Another approach would be to
...

💬 apogio commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3371377309)
Pipeline issues should be fixed now.

💬 ryanofsky commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371385637)
> > What is the bug exactly?
>
> The code [fails](https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3369255505) to compile using the [LLVM MinGW](https://github.com/mstorsjo/llvm-mingw) toolchain

Thanks this is very helpful. I think deleting `operator std::string` with comment like `// Disallow std::string conversion to ensure code is portable, because this operator is not defined on windows` would be a good solution to ensure the problem does not go undetected.

I wouldn't want `fs
...

💬 optout21 commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#issuecomment-3371390469)
ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b

With the new `chain_test` test, this optimization reduces the number of steps in `LastCommonAncestor` by at least 5-fold (e.g. from 5898713 to 1119703), an obvious optimization.
However, timing the tests (pre and post optimization) I could not measure a difference.
I've left two non-blocking nit comments.

💬 fanquake commented on pull request "Clear out space on GHA jobs":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3371390690)
> A run applying to all jobs when using GHA can be seen here: https://github.com/willcl-ark/bitcoin/actions/runs/18274913987

Looking there, it seems like the cleanup step can take up to 10 minutes to complete i.e. https://github.com/willcl-ark/bitcoin/actions/runs/18274913987/job/52024710440. Although I guess that is less of an issue if it's not running in this repo.

💬 maflcko commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371416000)
> would be a good solution to ensure the problem does not go undetected.

For reference, the MWE for this would be https://godbolt.org/z/KeKq7dxfW (Our fs namespace with `operator std::string` deleted and a minimal main function)

💬 jblachly commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-3371418908)
Still happening in 29.1

```
2025-10-06T12:27:14Z Pre-synchronizing blockheaders, height: 846996 (~92.38%)
2025-10-06T12:27:15Z Pre-synchronizing blockheaders, height: 852996 (~93.04%)
2025-10-06T12:27:15Z Pre-synchronizing blockheaders, height: 856996 (~93.46%)
2025-10-06T12:27:15Z Pre-synchronizing blockheaders, height: 860996 (~93.89%)
2025-10-06T12:27:15Z Pre-synchronizing blockheaders, height: 866996 (~94.54%)
2025-10-06T12:27:16Z Pre-synchronizing blockheaders, height: 874996 (~95.38%)
202
...

💬 willcl-ark commented on pull request "Clear out space on GHA jobs":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3371420237)
> > A run applying to all jobs when using GHA can be seen here: [willcl-ark/bitcoin/actions/runs/18274913987](https://github.com/willcl-ark/bitcoin/actions/runs/18274913987)
>
> Looking there, it seems like the cleanup step can take up to 10 minutes to complete i.e. [willcl-ark/bitcoin/actions/runs/18274913987/job/52024710440](https://github.com/willcl-ark/bitcoin/actions/runs/18274913987/job/52024710440). Although I guess that is less of an issue if it's not running in this repo.

This is
...

💬 optout21 commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#issuecomment-3371426440)
> However, timing the tests (pre and post optimization) I could not measure a difference.

Most of the CPU time in the test is taken by the `NaiveGetAncestor` and `NaiveLastCommonAncestor` naive implementations. Taken out calls to those, I have measured a 47% speedup in the execution of the test (user time).

💬 rkrux commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3371437153)
> Sidenote: currently it's unlikely you'll accidentally spend via an `older()` or `after()` script path. This is because you need to manually set the input `sequence` field in the `send` (etc.) RPC (and manually adjust the fee rate). Found out the hard way while testing 🤦 (there's currently no useful feedback when this happens).

**Scenario that I have not tested but expect to behave in this way:** 3 signers (A, B, C) form a MuSig (ABC) in the `keypath` and also have 3 non-Musig multi-sig scr
...

💬 rkrux commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3371447238)
If #32876 is not garnering reviews, can consider undrafting this PR itself because it contains 7 commits only.
Allowing skipping script path spending optionally seems reasonable to me and a ready-to-review PR is more inviting than a draft one.

💬 ryanofsky commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3371531256)
Haven't looked at code yet, so forgive if these are dumb questions, but could be nice if PR description addressed these:

- Is there a way to pass `bitcoin-cli` RPC arguments that begin with a `-` character? Can you escape these arguments with `--` for example?

- What's the PR's goal for backwards compatibility? Is the PR only allowing command lines that were disallowed before, and therefore fully backwards compatible? Or are there some command lines which used to be accepted which will now
...

💬 ryanofsky commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3371562956)
Concept ACK on the idea. I think it probably makes sense to allow options arguments to follow non-option arguments even this change is not strictly backwards compatible, as long as there is some way to escape non-option arguments beginning with `-`, such as with a `--` separator.

📝 maflcko opened a pull request: "ci: Use ARM instead of Intel in macOS cross task"
(https://github.com/bitcoin/bitcoin/pull/33549)
Cross compiling to Intel macOS seems fine, but it has to be switched at some point, see https://en.wikipedia.org/wiki/Mac_transition_to_Apple_silicon#Timeline.


Also, if the executable were to be run:

* Running the Intel executable in Rosetta 2 is slower
* It is harder to find native Intel macOS hardware (E.g. GitHub is in the process of dropping it: https://github.blog/changelog/2025-07-11-upcoming-changes-to-macos-hosted-runners-macos-latest-migration-and-xcode-support-policy-updates/#
...

💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3371932129)
ACK 0f1f02995b715a83f71b1b8fc92fa520625ec427

`git range-diff master a161f058089dfe5f3aa7d3b9635f0400f0147b5d 0f1f02995b715a83f71b1b8fc92fa520625ec427`

💬 ryanofsky commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3372043307)
Assuming we are ok with losing some backwards compatibility by starting to treating arguments that begin with `-` as options instead of non-options when they follow arguments that don't begin with `-`, there could be other interesting ways to extend this PR.

For example, `bitcoin-cli` could distinguish between options that begin with single and double dashes, and treat options after the RPC method name that begin with double dashes as RPC named parameters. In other words, accept:

```bash

...

✅ maflcko closed a pull request: "ci: Check macos-cross executables on macOS"
(https://github.com/bitcoin/bitcoin/pull/33509)

💬 maflcko commented on pull request "ci: Check macos-cross executables on macOS":
(https://github.com/bitcoin/bitcoin/pull/33509#issuecomment-3372189103)
> Since we don't have high hopes of this actually detecting any issues that the rest of the CI would not have found, seems to make more sense as a nightly job?

Thx, done in https://github.com/bitcoin/bitcoin/pull/33549 and https://github.com/maflcko/b-c-nightly/actions/runs/18282485376/job/52056370818

📝 ryanofsky opened a pull request: "Fix windows libc++ `fs::path` `fstream` compile errors"
(https://github.com/bitcoin/bitcoin/pull/33550)
As reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545, newer libc++ versions implementing https://wg21.link/lwg3430 will no longer implicitly convert `fs::path` objects to `std::filesystem::path` objects when constructing `std::ifstream` and `std::ofstream` types.

This is not a problem in Unix systems since `fs::path` objects use `std::string` as their native string type, but it causes compile errors on Windows which use `std::wstring` as their string type, since `fstream`
...