💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2213681709)
> Has this been upstreamed...

Not yet.

> why is it needed?

Otherwise, IWYU will suggest platform-specific headers instead of the standard one provided by the C library.

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2213683784)
I'm skeptical if this is a possible scenario with today's index base code (which is much better with reorgs / unexpected restart scenarios than it was back then), but it's definitely more complicated than I thought, so this PR is not appropriate for this - I dropped the commit and will resolve this discussion.

💬 maflcko commented on issue "Avoid plural forms in non-GUI translatable strings (lacks `%n` support)":
(https://github.com/bitcoin/bitcoin/issues/31890#issuecomment-3084545041)
It seems a bit too much hassle to:

* Split any format string into multiple, so that transifex can handle the plural forms
* Implement runtime string-translation on plural strings (would need a manually placed macro/function call)

Seems fine to just let users figure out the plural form and then "correct the typo" while reading?

💬 hebasto commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3084555435)
Rebased to resolve conflict with merged https://github.com/bitcoin/bitcoin/pull/32881.

✅ hebasto closed an issue: "Avoid plural forms in non-GUI translatable strings (lacks `%n` support)"
(https://github.com/bitcoin/bitcoin/issues/31890)

💬 maflcko commented on pull request "ci: Run unit test parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33000#issuecomment-3084561583)
Looks like this gives a 20% faster Msan, Tsan, and previous releases task with N=1, comparing with the fastest master run in the last week:

* https://cirrus-ci.com/build/4989581210157056
* https://cirrus-ci.com/build/5602428818554880

💬 hebasto commented on pull request "ci: Enable more shellcheck":
(https://github.com/bitcoin/bitcoin/pull/32970#discussion_r2213730157)
Does `cmake -S ...` still need to be wrapped in `bash -c "..."`?

💬 maflcko commented on pull request "test: Do not pass tests on unhandled exceptions":
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2213737305)
> more informative without special case handling for `CalledProcessError`, with less code.

I don't think this is true. It is missing the output. You can try this for any command that captures the output:

`subprocess.run(['bash', '-c', 'echo aaaaa && false'], check=True, capture_output=True)`

There could be an argument about stderr, but this seems better as a separate commit or pull.



> KeyboardInterrupt

It currently uses `self.log.warning`, which is different in verbosity. I do
...

💬 maflcko commented on pull request "ci: Enable more shellcheck":
(https://github.com/bitcoin/bitcoin/pull/32970#discussion_r2213741192)
It is not trivial to replace. Maybe the `eval` hack from below can be used:

```sh
# parses TEST_RUNNER_EXTRA as an array which allows for multiple arguments such as TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6"'

eval "TEST_RUNNER_EXTRA=($TEST_RUNNER_EXTRA)"
```

however, I haven't tried this yet.

💬 hebasto commented on pull request "ci: Enable more shellcheck":
(https://github.com/bitcoin/bitcoin/pull/32970#discussion_r2213757515)
Maybe
```suggestion
# shellcheck disable=SC2086
cmake -S "$BASE_ROOT_DIR" -B "$BASE_BUILD_DIR" $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || (
```
?

💬 maflcko commented on pull request "ci: Enable more shellcheck":
(https://github.com/bitcoin/bitcoin/pull/32970#discussion_r2213780607)
No this won't work, because some elements contain spaces. `SC2086` is about this, to warn about word splitting.

For example. `DCMAKE_CXX_FLAGS` will be split:


```
[13:10:44.210] + cmake -S /ci_container_base -B /ci_container_base/ci/scratch/build-arm-linux-gnueabihf -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DCMAKE_TOOLCHAIN_FILE=/ci_container_base/depends/arm-linux-gnueabihf/toolchain.cmake -DWERROR=ON -DCMAKE_INSTALL_PREFIX=/ci_container_base/ci/scratch/out -Werror=dev -DREDUCE_EXPORTS=
...

💬 hebasto commented on pull request "init: [gui] Avoid UB/crash in InitAndLoadChainstate":
(https://github.com/bitcoin/bitcoin/pull/32987#issuecomment-3084665767)
Concept ACK.

👍 hebasto approved a pull request: "ci: Enable more shellcheck"
(https://github.com/bitcoin/bitcoin/pull/32970#pullrequestreview-3030212877)
ACK fa1fd074685ca96b9bd3855e9e6fe730a4f6462c.

💬 sfsegreto commented on issue "ARM Windows build and release":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3084688654)
Thanks @maflcko , I understand this application can run over emulation, but can you help clarify a bit more what is blocking from having Windows Arm builds of this project? Is it because QT /guix building on a Linux system can't make (or distribute) a WinArm binary? Having a fully Arm build of this project running on Windows Arm would be faster and more efficient than running over the emulation.

💬 maflcko commented on pull request "ci: Enable more shellcheck":
(https://github.com/bitcoin/bitcoin/pull/32970#discussion_r2213801696)
Yeah, the eval hack should work:

```
$ export T="-DREDUCE_EXPORTS=ON -DCMAKE_CXX_FLAGS='-Wno-psabi -Wno-error=maybe-uninitialized'"; eval "T=($T)"; for i in "${T[@]}"; do echo "_${i}_" ; done
_-DREDUCE_EXPORTS=ON_
_-DCMAKE_CXX_FLAGS=-Wno-psabi -Wno-error=maybe-uninitialized_

✅ josibake closed an issue: "ci: improve "test each commit" job to handle more complex scenarios"
(https://github.com/bitcoin/bitcoin/issues/32991)

💬 josibake commented on issue "ci: improve "test each commit" job to handle more complex scenarios":
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3084692489)
Following up, this was my mistake! AFAICT, this happened because I had two squashes into the `refresh-secp256k1` branch. I attempted to reproduce this on my fork and couldn't get a failure, and the only difference I can see is that the original failure had two squashes followed by a merge commit.

Thanks for taking a look @maflcko and for the help in reproducing.

💬 hebasto commented on issue "ARM Windows build and release":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3084699194)
> ... can you help clarify a bit more what is blocking from having Windows Arm builds of this project?

From https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-2854518383:

> ... the required toolchain isn’t even provided by major distributions.

💬 fanquake commented on pull request "init: [gui] Avoid UB/crash in InitAndLoadChainstate":
(https://github.com/bitcoin/bitcoin/pull/32987#issuecomment-3084727391)
Is there any way to write an actual GUI test for this? Having to add an obscure, single-use option to bitcoind, to simulate crashing behaviour in the GUI, seems like a last resort?

💬 HowHsu commented on pull request "checkqueue: implement a new scriptcheck worker pool with atomic variables":
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3084736836)
> Are the first two commits related to the PR? Seem REST-related?
>

No, they appears here after I rebase against the origin/master and then push -f.

> Benchmark results suggest a 12-thread crossover to become faster than master, any chance that I'm reading this wrong?

the master behaves a little bit better when the number of threads is small, just like the table I put in the PR description. The crossover point is machine/hardware related, and it is usually around 12~15 threads. After
...