💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184561400)
In 03f585d58ccf4d5c02d621c5b6046d45807b3201 _bitcoin wrapper: improve help output_: users might be confused by the distinction between no command, `--help` and `help`.

That's because if a user starts with "bitcoin" then "this help message" implies there's nothing additional to see. Hopefully they'll still be encouraged to try by the sentence at the end ("Run 'bitcoin help' to see additional commands").

But maybe a better phrasing here is: "List all available commands."

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184594801)
In 03f585d58ccf4d5c02d621c5b6046d45807b3201 _bitcoin wrapper: improve help output_: this is a nice improvement.

Before:

```
bitcoin --help
Usage: bitcoin [OPTIONS] COMMAND..

which bitcoin
/usr/local/bin/bitcoin

/usr/local/bin/bitcoin --help
Usage: /usr/local/bin/bitcoin [OPTIONS] COMMAND...
```

After:

```
/usr/local/bin/bitcoin --help
Usage: bitcoin [OPTIONS] COMMAND..
```

Only tested on macOS

👍 Sjors approved a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2985811000)
re-ACK 03f585d58ccf4d5c02d621c5b6046d45807b3201

Left a suggestion for slightly improved wording.

💬 Sjors commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin/bitcoin/pull/32871#issuecomment-3034796044)
TIL it's possible to make a PR with 0 commits in it. Did you mean to open an issue?

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3034814885)
Rebased after https://github.com/bitcoin/bitcoin/pull/29307

✅ maflcko closed a pull request: "New SVG, Icons, PNGs and X PixMaps"
(https://github.com/bitcoin/bitcoin/pull/32871)

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