💬 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

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953144139)
I wonder if this can be made dumb stupid and reduce it to just call `execvp()`. That is, reduce `ExecCommand()` to just 1 - 5 lines of code and deal with the consequences of it...

🚀 fanquake merged a pull request: "depends: add missing Darwin objcopy"
(https://github.com/bitcoin/bitcoin/pull/31840)

💬 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-2654464619)
Rebuilt with @pinheadmz's committed sigs

```
$ find guix-build-096525e92cc2/output -wholename "*codesigned*" -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
207621cdc43868870f4136e9e6784a2a3e9ba89ec1edc6fa92b315cfa3c4432c guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/SHA256SUMS.part
111016205f0a2ac732feb934acb3e8a36d5251f119d8fa9215790310ba46c31d guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/bitcoin-096525e92cc2-arm64-apple-darwin.tar.gz
a1228d
...

👍 theuni approved a pull request: "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree"
(https://github.com/bitcoin/bitcoin/pull/31379#pullrequestreview-2612828482)
utACK c4c5cf174883cb53256e869f0d1673e29576a97c.

It makes sense to forward `APPEND_LDFLAGS` and `APPEND_CFLAGS` to their secp equivalents for convenience and consistency for tests.

💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1953186524)
@fanquake Are you ok if we break OSS-Fuzz here with a proper fix (to avoid future technical debt), and PR the one-liner to fix it there post-merge?

💬 andrewtoth commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953187368)
I think the scope of this PR is to reduce chainstate writes, which occur when we need to prune blocks. Pruning blocks means we can't reindex them and would have to redownload, so we write the chainstate then to be safe.

If we were to not store blocks, we would have to write the chainstate after indexing every block to avoid potentially redownloading them.

👍 theuni approved a pull request: "cmake: Add `CheckLinkerSupportsPIE` module"
(https://github.com/bitcoin/bitcoin/pull/31359#pullrequestreview-2612881018)
utACK 81c174e3186efae084829dcde314b081cad3d3cb.

Nice work upstreaming the correct fix. Makes sense for us to carry the workaround for a few years. No opinion on the second commit.

💬 theuni commented on pull request "depends: Use `CC_FOR_BUILD` for `config.guess `":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2654583997)
I don't understand this change. It seems setting CC_FOR_BUILD to CC is exactly the opposite of what we want to do?

@hebasto's original change makes more sense to me:
```
BUILD = $(shell env --unset CC ./config.guess)
```

That ignores what's set in CC when detecting the native platform, which seems like the correct behavior to me.

What was your intention with this patch, @fanquake ?

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953239521)
You are eaten by a grue.