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

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953057497)
```suggestion
%1$s bench [ARGS] Run bench command, equivalent to running 'bench_bitcoin [ARGS]'"
%1$s test [ARGS] Run unit tests, equivalent to running 'test_bitcoin [ARGS]'"
%1$s test-gui [ARGS] Run GUI unit tests, equivalent to running 'test_bitcoin-qt [ARGS]'"
```

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953127351)
I think the following two commits should be squashed into one commit:

bde036879d `multiprocess: Add bitcoin wrapper executable`
b7213977ed `multiprocess: Add bitcoin wrapper windows support`

because otherwise there is an intermediate state where the code is broken on Windows (would try to use `execvp()` which does not exist, I guess it will not compile on Windows).

📝 fanquake opened a pull request: "ci: switch MSAN to use prebuilt Clang binaries"
(https://github.com/bitcoin/bitcoin/pull/31850)
Use the packages from https://apt.llvm.org/.

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953137357)
How so? This is basically my main concern: why are we storing blocks during IBD at all, when pruned? Or if they're needed for some reason, do we really need to store 288 blocks throughout the IBD as well?

👍 theuni approved a pull request: "cmake: Improve compatibility with Python version managers"
(https://github.com/bitcoin/bitcoin/pull/31421#pullrequestreview-2612764527)
No opinion on the python selection changes themselves, but code-review ACK dead9086543671b07e6ce041821e4d2a6627075b