💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953060817)
In commit "qa: Add feature_framework_startup_failures.py" (2226e1489101f363114ce474d7f5e56ec50c9c6a)

Maybe give these arguments a prefix like `--internal-` so it obvious they are internal, and to avoid risk of silent conflicts if real test arguments with the same names are added later.

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2654397674)
> > I've run a build, but don't yet see everything matching:
>
> It looks like the binaries match, but not the `SHA256SUMS.part` files.

Ah, I think it's probably that the ` bitcoin-096525e92cc2-codesignatures-<commit>.tar.gz` file is the one that's different in the `SHA256SUMS.part` files as I did not use the same codesignatures commit that @pinheadmz made. I was untarring and committing the signatures into my local repos.

👍 laanwj approved a pull request: "depends: add missing Darwin objcopy"
(https://github.com/bitcoin/bitcoin/pull/31840#pullrequestreview-2612710149)
Code review ACK 3edaf0b4286a771520b7e5b0b5064eca713ff0ad

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953116997)
It's clearer using `* 1024` IMO

👍 theuni approved a pull request: "depends: add missing Darwin objcopy"
(https://github.com/bitcoin/bitcoin/pull/31840#pullrequestreview-2612728849)
utACK 3edaf0b4286a771520b7e5b0b5064eca713ff0ad

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953122891)
It's to disable pruning during IBD.

I'm not seeing a use case to set this lower than `-prune` (we already prune extra during IBD when we prune), only higher (to avoid flushing).

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953123976)
Merge it into master for testing. Rebasing for no reason is a bad practice.

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953126552)
There's no such thing as orphan blocks. Stale blocks are implicitly included in the more general "blocks".

Anyway, this is just a move.

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953130763)
Outside the scope of this PR

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953132732)
It's a new PR, why base it on an old state? I ran the test locally and they're passing, but rebasing the PR assures that CI also runs against the latest changes.

👍 theuni approved a pull request: "cmake: Copy `cov_tool_wrapper.sh.in` to the build tree"
(https://github.com/bitcoin/bitcoin/pull/31722#pullrequestreview-2612746737)
utACK e3c015276962192355e16ca91391b8bc8e188291

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953134231)
idk, not sure why it matters

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953134261)
Looks like we should update MIN_DISK_SPACE_FOR_BLOCK_FILES in that case in a separate PR.

🤔 vasild reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2612338457)
Approach ACK 0e4ee158ca.

> I wonder if python test framework changes here are scaring reviewers from this PR?

Moving the python changes to another PR would probably speed the review of this one and probably delay the review of the other one (with the python changes).

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952926320)
Is only used in this file:
```suggestion
static void ExecCommand(const std::vector<const char*>& args, std::string_view wrapper_argv0)
```

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952885840)
On Windows the [current directory is also searched (before PATH)](https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/path) for executable location. So, if the current directory is `C:\bitcoin\src` (which is not in PATH) and the user types `bitcoin` (not `.\bitcoin`) and presses `<enter>` and if `C:\bitcoin\src\bitcoin` exists then it will be executed. This means that the variable `search_system_path` will be wrongly set to `true` in this case.

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953063825)
```suggestion
//! that will be located on the PATH or relative to wrapper_argv0.
```

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953105287)
The `dtrace` example would print all exec calls on the system, not just the ones by the given program.

```suggestion
//! dtrace -n 'proc:::exec-success /pid == $target/ { trace(curpsinfo->pr_psargs); }' -c ...
```

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952940524)
This is equivalent to:

```suggestion
try_exec(wrapper_dir / arg0.filename(), search_system_path) ||
// Otherwise just look on the system path.
try_exec(arg0.filename(), false);
```
because `try_exec()` would only return if `search_system_path` is `true` and if that is the case then `search_system_path && A` is equivalent to just `A`.

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952943101)
I don't get it why `search_system_path` is used as `allow_notfound`. Those are two different things.