💬 maflcko commented on pull request "cmake: Improve Python robustness and test usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2184806592)
(Actually, while testing, a missing python is now a hard error already when `-DBUILD_GUI=ON`. So I don't think the wording "refactor" accurately represents the changes here.)

👍 dergoegge approved a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2986225449)
Code review ACK 6a7147358c9d6e3883dcdbbee9fb2c1cb4baf5ff

💬 maflcko commented on pull request "ci: update pwsh to use custom shell that fails-fast":
(https://github.com/bitcoin/bitcoin/pull/32672#issuecomment-3035127536)
> This is especially convenient for reproducing failures locally.

Just a general note on CI reproducibility: My recommendation would be to put as little code into vendor-specific locked-in yaml files and instead place CI code into vendor-agnostic scripts, so that they can be re-used outside of that vendor.

I understand that it is nice to have the CI log split into sections, but this should also be possible with a script that allows several entry points: `./ci.py install; ./ci.py build; ./c
...

💬 m3dwards commented on pull request "ci: update pwsh to use custom shell that fails-fast":
(https://github.com/bitcoin/bitcoin/pull/32672#issuecomment-3035182422)
After reading @maflcko's comment (which I agree with) and speaking with @hebasto offline I think it would be best that we change this to use powershell for all run steps on Windows jobs and look at condensing steps down into one or more scripts to aid in local reproducibility. As it stands, for someone to reproduce these steps locally they would have to jump between two shells. As I understand it, the change to use bash on windows was for the nicer fail fast behaviour but if we can get that in p
...

📝 m3dwards converted_to_draft a pull request: "ci: update pwsh to use custom shell that fails-fast"
(https://github.com/bitcoin/bitcoin/pull/32672)
Github by default sets [fail fast](https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference) behaviour on pswh shell which means that if any powershell cmdlet fails the script will stop and exit. The problem is that this behaviour doesn't apply when calling native executables, it only applies to powershell cmdlets.

I think the safest thing is to whenever we use pwsh to enable `$PSNativeCommandUseErrorActionPreference = $tru
...

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3035219100)
Signing the PSBT works fine while running locally, and removing non-witness UTXO from the PSBT.

📝 maflcko opened a pull request: "ci: Catch tests corrupting the source directory"
(https://github.com/bitcoin/bitcoin/pull/32874)
At best it is annoying when tests delete random files in my source dir, or when they leave around temp files. At worst, it is an attempt to inject a backdoor.

So try to catch them in CI.

For example, this should hopefully catch:

```
$ ( echo 'my file content' > streams_tmp ) && ls streams_tmp && ./bld-cmake/bin/bench_bitcoin --filter=FindByte && ls streams_tmp
streams_tmp
...
ls: cannot access 'streams_tmp': No such file or directory

📝 HowHsu opened a pull request: "index: Fix missing case in the comment in NextSyncBlock()"
(https://github.com/bitcoin/bitcoin/pull/32875)
The comment here overlooked a specific case where the block is the tip of m_chain, fix it.

💬 maflcko commented on pull request "ci: update pwsh to use custom shell that fails-fast":
(https://github.com/bitcoin/bitcoin/pull/32672#issuecomment-3035392582)
> As I understand it, the change to use bash on windows was for the nicer fail fast behaviour but if we can get that in powershell now we should probably use the more native windows shell.

bash is also easier for non-windows devs to read and modify, so an alternative would also be python or rust as the ci runner script, but no strong opinion, I'd say anything is fine here.

✅ hebasto closed a pull request: "test: Fix `system_tests/run_command` test"
(https://github.com/bitcoin/bitcoin/pull/32601)

💬 maflcko commented on pull request "ci: Catch tests corrupting the source directory":
(https://github.com/bitcoin/bitcoin/pull/32874#issuecomment-3035420524)
I am a bit confused about the ci failures. I guess it makes sense that chattr doesn't work on zfs, but is it also known to not work on overlayfs? If it doesn't work at all, I wonder how commit 5c2185b3b624ce87320ec16412f98ab591a5860c makes sense to enable the use of chattr in ci. Is there a single setup where it is known to work?

💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3035469911)
> Signing the PSBT works fine while running locally, and removing non-witness UTXO from the PSBT.

What command did you use to run locally?

💬 maflcko commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2185018245)
> An alternative would be to have a `AutoFile::CommitAndClose()`-function that always closes regardless of errors and clearly signals that calling code does not expect to continue using the `AutoFile`?

yes, that is what i was trying to suggest

💬 maflcko commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2185020751)
no write has happened, and an error is already logged. Seems redundant to log even more errors?

💬 maflcko commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2185030297)
same here. if truncation failed and is logged as error, i don't think it matters to log another error whether or not closing has failed as well.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3035641377)

>
> What command did you use to run locally?

I first checked by adding two log statements in `rpc_psbt.py` the first log to check the psbt before signing and the second log after `walletprocesspsbt` where CI is reporting an error. And then manually parsing the result with the help of `decodepsbt` to check whether the PSBT after`walletprocesspsbt` contains non-witness UTXO, but it doesn't in my case, so the test passed successfully.

I also ran all the functional test cases with `—exte
...

💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3035710020)
you'll have to use `--usecli` to run the tests, otherwise the cli is not used

👍 l0rinc approved a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2986225579)
Concept ACK, tested on Mac and Linux, it works as expected.

```bash
rm -rfd build install && \
cmake -B build -G Ninja \
-DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DCMAKE_INSTALL_PREFIX="$PWD/install" \
-DBUILD_TESTS=ON \
-DBUILD_BENCH=ON \
-DBUILD_FUZZ_BINARY=ON \
-DBUILD_UTIL_CHAINSTATE=ON \
-DBUILD_GUI=ON \
-DBUILD_GUI_TESTS=ON && \
ninja -C build install && \
tree -L 3 install/{bin,libexec}
```

Checked the install command on Mac and Linux, it works as expected.

```bash
rm -rfd b
...

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184813932)
nit: we might want to realign the comments after the change

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184948130)
Q: slightly unrelated, but if testing should be available for the installed version, how come fuzzing isn't? Seems it should be mentioned with a 🛠 at least...