💬 optout21 commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2405916981)
I think what is meant here is that equal-height blocks have equal-height skip _values_, the pointer can be different (if on different forks). I suggest changing to "height skip values"
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2405916981)
I think what is meant here is that equal-height blocks have equal-height skip _values_, the pointer can be different (if on different forks). I suggest changing to "height skip values"
💬 janb84 commented on pull request "doc: add coverage instrumentation hint to libFuzzer quickstart":
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3371303641)
> > NACK [Developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-fuzz-coverage) already has an extensive section on generating coverage. Maintaining the same documentation in 2 places is not ideal.
>
> ok . Would you atleast like to see a reference to the developer notes in this section(assume for someone who lands here first)?
That would be better, imho (although the correct way would be to first read the contributing guide as a developer), bu
...
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3371303641)
> > NACK [Developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-fuzz-coverage) already has an extensive section on generating coverage. Maintaining the same documentation in 2 places is not ideal.
>
> ok . Would you atleast like to see a reference to the developer notes in this section(assume for someone who lands here first)?
That would be better, imho (although the correct way would be to first read the contributing guide as a developer), bu
...
💬 ryanofsky commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371304309)
What is the bug exactly?
The current description is confusing because it says "constructing `std::ifstream` or `std::ofstream` from an `fs::path` object triggers its implicit conversion to `std::string`". But there aren't any unsafe impilcit conversion from paths to strings, there is only a [safe implicit conversion](https://en.cppreference.com/w/cpp/filesystem/path/native.html) to the native string type.
So what is the bug? Are there compile errors on windows? Is this the concern? Would be go
...
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371304309)
What is the bug exactly?
The current description is confusing because it says "constructing `std::ifstream` or `std::ofstream` from an `fs::path` object triggers its implicit conversion to `std::string`". But there aren't any unsafe impilcit conversion from paths to strings, there is only a [safe implicit conversion](https://en.cppreference.com/w/cpp/filesystem/path/native.html) to the native string type.
So what is the bug? Are there compile errors on windows? Is this the concern? Would be go
...
💬 hebasto commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371355059)
> 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
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371355059)
> 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
💬 maflcko commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371355716)
> So what is the bug? Are there compile errors on windows? Is this the concern? Would be good to say what the specific bug or expected behavior is.
Yes, my understanding is there'd be compile errors, but I had difficulty as well figuring this out. My understanding is that there are no unsafe conversions or other bugs. So my suggestion was to just add an explicit cast. Your `std_path` suggestion seems fine as well.
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371355716)
> So what is the bug? Are there compile errors on windows? Is this the concern? Would be good to say what the specific bug or expected behavior is.
Yes, my understanding is there'd be compile errors, but I had difficulty as well figuring this out. My understanding is that there are no unsafe conversions or other bugs. So my suggestion was to just add an explicit cast. Your `std_path` suggestion seems fine as well.
🤔 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.
(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
...
(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.
(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
...
(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.
(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.
(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)
(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
...
(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
...
(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).
(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
...
(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.
(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
...
(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.
(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/#
...
(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/#
...