💬 1BitcoinBoWP1FZ4xwTNkq6XksKidmgYYw commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin/bitcoin/pull/32871#issuecomment-3034899121)
@Sjors @maflcko the pull (bot) destroyed my PR. I see some of my changes reverted here: https://github.com/bitcoin/bitcoin/compare/a7270b8fb8fd6c92d88de87de492cf88aef1eda7..e3f416dbf7633b2fb19c933e5508bd231cc7e9cf

I use a the pull github app to keep the master branch up to date with bitcoin/bitcoin/master maybe that caused the issue. I will turn it off and send a new PR.

🤔 stratospher reviewed a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-2946966884)
ACK 11a9d55e.

Agree that applying reindex-assumevalid combo can be useful in the special cases mentioned above to speedup reindexing when we don't have all blocks.

A behaviour change during assumevalid+reindex now would be:
- we'd skip script checks even when the assumevalid blockindex doesn't exist (ex:we put `assumevalid=somegarbage(or)blockhash-with-typo`)
- this was not the case on master where we skip script checks only when know that the assumevalid blockindex exists.

Don't thin
...

💬 stratospher commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2184580600)
any particular reason for having this high minimum chain work? (ex: for 32256 blocks instead of 3256 blocks). also curious how you computed it?

💬 stratospher commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2184701733)
the tests now take 2m34.878s on my system (was around 18 seconds before).
since the tests takes much more time now, maybe remove it from the "Tests less than 30s" section in `test_runner.py`?

💬 fanquake commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-3034989842)
> Same here, fwiw, running with --random=3450808900320758527 didn't fail for me on master (at commit https://github.com/bitcoin/bitcoin/commit/bf75c9964fb28a5e9b8a07097248ffa960738478).

This is the one that fails for me on 29.x. Backported to 29.x in #32863.

💬 m3dwards commented on pull request "ci: update pwsh to use custom shell that fails-fast":
(https://github.com/bitcoin/bitcoin/pull/32672#issuecomment-3035043670)
> 1. The output in the "Get tool information" step has changed:

I just added this as a nice to have but happy to remove.

> 2\. The exit code is not propagated to the action's status.

Unfortunately I think this is just how it works. When there is a failure it will always use code 1. Without having to write code that checks error codes and returns those codes each time we call a native executable I don't think it's possible to have the same error code.

💬 maflcko commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2184792071)
Yeah, seems fine to keep this at the end for users and devs that have it not set? Maybe just use a trivial one-line patch?

```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4da12e0f46..c3dea019b4 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -710,6 +710,7 @@ if(configure_warnings)
message(WARNING "${warning}")
endforeach()
message(" ******\n")
+ message(AUTHOR_WARNING "Warnings have been encountered!")
endif()

# We want all build properties to be encapsul
...

💬 fanquake commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#issuecomment-3035097049)
Probably close this now, if #32577 has been re-opened, and the approach here has been nacked for re-introducing bugs?

💬 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?