💬 0xB10C commented on pull request "ci: Temporarily disable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/pull/29788#issuecomment-2034008362)
> GH actions 23.04 VM
GitHub currently only supports: `ubuntu-latest` (22.04), `ubuntu-22.04`, and `ubuntu-20.04`
https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories
(https://github.com/bitcoin/bitcoin/pull/29788#issuecomment-2034008362)
> GH actions 23.04 VM
GitHub currently only supports: `ubuntu-latest` (22.04), `ubuntu-22.04`, and `ubuntu-20.04`
https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories
👍 fanquake approved a pull request: "build, depends: Fix `libmultiprocess` cross-compilation"
(https://github.com/bitcoin/bitcoin/pull/29665#pullrequestreview-1976129823)
ACK 2de2ea2ff63b97eacb23234932c6e1f1f65e4494 - I checked that this fixes the macOS cross-compilation issue. I'm assuming these packages are also likely to change further in the (near) future, given the changes going in upstream: https://github.com/chaincodelabs/libmultiprocess/pulls?q=is%3Apr+is%3Aclosed.
> I don't actually understand why the fix is needed, when this was broken, or why mpgen linking should fail when it is cross compiled.
I agree with Russ that it'd be better to actually f
...
(https://github.com/bitcoin/bitcoin/pull/29665#pullrequestreview-1976129823)
ACK 2de2ea2ff63b97eacb23234932c6e1f1f65e4494 - I checked that this fixes the macOS cross-compilation issue. I'm assuming these packages are also likely to change further in the (near) future, given the changes going in upstream: https://github.com/chaincodelabs/libmultiprocess/pulls?q=is%3Apr+is%3Aclosed.
> I don't actually understand why the fix is needed, when this was broken, or why mpgen linking should fail when it is cross compiled.
I agree with Russ that it'd be better to actually f
...
💬 mzumsande commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2034027583)
> @mzumsande, now that unit tests are being run in parallel, we could treat them like any other test (i.e. no special treatment with `--skipunit`, so this option could now be removed). Do you see this aligning with the original goals of `skipunit` or is there something else to consider?
Yes, if they are treated like other tests, `-skipunit` is no longer necessary and should be removed. That's even better than the status quo because it removes the need to remember that `-skipunit` exists when
...
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2034027583)
> @mzumsande, now that unit tests are being run in parallel, we could treat them like any other test (i.e. no special treatment with `--skipunit`, so this option could now be removed). Do you see this aligning with the original goals of `skipunit` or is there something else to consider?
Yes, if they are treated like other tests, `-skipunit` is no longer necessary and should be removed. That's even better than the status quo because it removes the need to remember that `-skipunit` exists when
...
🚀 fanquake merged a pull request: "build, depends: Fix `libmultiprocess` cross-compilation"
(https://github.com/bitcoin/bitcoin/pull/29665)
(https://github.com/bitcoin/bitcoin/pull/29665)
👍 fanquake approved a pull request: "[25.x] Finalize 25.2"
(https://github.com/bitcoin/bitcoin/pull/29794#pullrequestreview-1976147858)
ACK e95c484f7de7fe30d5a2e87c0e7e2a5faf9c1a5f
(https://github.com/bitcoin/bitcoin/pull/29794#pullrequestreview-1976147858)
ACK e95c484f7de7fe30d5a2e87c0e7e2a5faf9c1a5f
🚀 fanquake merged a pull request: "[25.x] Finalize 25.2"
(https://github.com/bitcoin/bitcoin/pull/29794)
(https://github.com/bitcoin/bitcoin/pull/29794)
🚀 fanquake merged a pull request: "depends: add -g to DEBUG=1 flags"
(https://github.com/bitcoin/bitcoin/pull/29527)
(https://github.com/bitcoin/bitcoin/pull/29527)
💬 fanquake commented on pull request "depends: add -g to DEBUG=1 flags":
(https://github.com/bitcoin/bitcoin/pull/29527#issuecomment-2034083503)
Going to followup with an optimization flags alingment RFC.
(https://github.com/bitcoin/bitcoin/pull/29527#issuecomment-2034083503)
Going to followup with an optimization flags alingment RFC.
💬 glozow commented on pull request "doc: update release-process.md":
(https://github.com/bitcoin/bitcoin/pull/29645#issuecomment-2034094652)
(Yay v26.1) I have a couple more things for the last section, and then will take out of draft.
(https://github.com/bitcoin/bitcoin/pull/29645#issuecomment-2034094652)
(Yay v26.1) I have a couple more things for the last section, and then will take out of draft.
💬 fanquake commented on pull request "depends: add new LLVM debug macro":
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2034145974)
> IIRC usage was turned into a compile error (maybe only intermittently, will check again).
I think I was thinking of something else here. Have changed this to just adding the new macro.
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2034145974)
> IIRC usage was turned into a compile error (maybe only intermittently, will check again).
I think I was thinking of something else here. Have changed this to just adding the new macro.
👋 fanquake's pull request is ready for review: "depends: add new LLVM debug macro"
(https://github.com/bitcoin/bitcoin/pull/29781)
(https://github.com/bitcoin/bitcoin/pull/29781)
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-2034186584)
> Dealing with a read-only tmp file is probably outside the scope of this PR, but maybe we should reconsider if +rw might actually be the correct mode to use.
@luke-jr that seems like an unlikely situation (unless bitcoind is killed in the moment between writing the temporary cookie file and renaming it?), nonetheless, it could be addressed by changing where we apply the new more restrictive `400` permissions, from the temp file to the renamed `.cookie`.
I don't think the temporary cookie
...
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-2034186584)
> Dealing with a read-only tmp file is probably outside the scope of this PR, but maybe we should reconsider if +rw might actually be the correct mode to use.
@luke-jr that seems like an unlikely situation (unless bitcoind is killed in the moment between writing the temporary cookie file and renaming it?), nonetheless, it could be addressed by changing where we apply the new more restrictive `400` permissions, from the temp file to the renamed `.cookie`.
I don't think the temporary cookie
...
💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2034218395)
Rebased and pushed. `skipunit` now removed.
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2034218395)
Rebased and pushed. `skipunit` now removed.
💬 vostrnad commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034241283)
Tested self-built MSVC binaries for 26.0 and 27.0rc1, definitely no regression.
26.0:

27.0rc1:

Note that I made corrections to my previous graphs because of a bug that caused some runs to leak from one graph to the other. The regression is even more prominent now.
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034241283)
Tested self-built MSVC binaries for 26.0 and 27.0rc1, definitely no regression.
26.0:

27.0rc1:

Note that I made corrections to my previous graphs because of a bug that caused some runs to leak from one graph to the other. The regression is even more prominent now.
💬 maflcko commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034250821)
> Tested self-built MSVC binaries for 26.0 and 27.0rc1, definitely no regression.
I forgot to mention that optimization was enabled in commit 41e378a0a1f0729b423034f47c28a4e83287a1e8. So for a fair comparison, one would have to backport 41e378a0a1f0729b423034f47c28a4e83287a1e8 to 26.0.
I presume that benchmarking IBD is expensive. So maybe comparing the micro-benchmarks can provide a hint at which code is slower?
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034250821)
> Tested self-built MSVC binaries for 26.0 and 27.0rc1, definitely no regression.
I forgot to mention that optimization was enabled in commit 41e378a0a1f0729b423034f47c28a4e83287a1e8. So for a fair comparison, one would have to backport 41e378a0a1f0729b423034f47c28a4e83287a1e8 to 26.0.
I presume that benchmarking IBD is expensive. So maybe comparing the micro-benchmarks can provide a hint at which code is slower?
💬 fanquake commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034264315)
> Tested pre-built binaries on Debian 12 WSL on the same system, measured 45 runs of each. No performance regression,
> Tested self-built MSVC binaries for 26.0 and 27.0rc1, definitely no regression.
It's not clear to me if there is still an issue here or not. Or is there still a regression when comparing our 26.x and 27.x Windows release binaries?
If the only issue is in relation to self-compiled MSVC binaries, then this isn't a blocker for 27.x.
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034264315)
> Tested pre-built binaries on Debian 12 WSL on the same system, measured 45 runs of each. No performance regression,
> Tested self-built MSVC binaries for 26.0 and 27.0rc1, definitely no regression.
It's not clear to me if there is still an issue here or not. Or is there still a regression when comparing our 26.x and 27.x Windows release binaries?
If the only issue is in relation to self-compiled MSVC binaries, then this isn't a blocker for 27.x.
💬 hebasto commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034267106)
Speaking of MSVC builds, it's worth noting that they still don't have a hardware-accelerated SHA256 implementation (see https://github.com/bitcoin/bitcoin/pull/24773).
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034267106)
Speaking of MSVC builds, it's worth noting that they still don't have a hardware-accelerated SHA256 implementation (see https://github.com/bitcoin/bitcoin/pull/24773).
💬 naiyoma commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#issuecomment-2034278715)
CONCEPT ACK , Using `calculate_input_weight` is a better approach than manual estimations, also increases readability.
(https://github.com/bitcoin/bitcoin/pull/29777#issuecomment-2034278715)
CONCEPT ACK , Using `calculate_input_weight` is a better approach than manual estimations, also increases readability.
💬 maflcko commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#discussion_r1549504485)
Could also print the result on success? IIUC, stdout will be hidden by the test runner, and when running this test independently, having some feedback is better than no feedback?
(https://github.com/bitcoin/bitcoin/pull/29771#discussion_r1549504485)
Could also print the result on success? IIUC, stdout will be hidden by the test runner, and when running this test independently, having some feedback is better than no feedback?
💬 vostrnad commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034285249)
@maflcko
> I forgot to mention that optimization was enabled in commit https://github.com/bitcoin/bitcoin/commit/41e378a0a1f0729b423034f47c28a4e83287a1e8. So for a fair comparison, one would have to backport https://github.com/bitcoin/bitcoin/commit/41e378a0a1f0729b423034f47c28a4e83287a1e8 to 26.0.
The original regression is also in variance which doesn't appear with the MSVC binary, so I don't think it's necessary to test the backport.
@fanquake
> It's not clear to me if there is st
...
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034285249)
@maflcko
> I forgot to mention that optimization was enabled in commit https://github.com/bitcoin/bitcoin/commit/41e378a0a1f0729b423034f47c28a4e83287a1e8. So for a fair comparison, one would have to backport https://github.com/bitcoin/bitcoin/commit/41e378a0a1f0729b423034f47c28a4e83287a1e8 to 26.0.
The original regression is also in variance which doesn't appear with the MSVC binary, so I don't think it's necessary to test the backport.
@fanquake
> It's not clear to me if there is st
...