👍 hodlinator approved a pull request: "fuzz: Faster leak check"
(https://github.com/bitcoin/bitcoin/pull/31481#pullrequestreview-2517306266)
ACK eeee163513918540f483925f36b3399430de1c57

### Performance increase

Changed `-max_total_time=60` beneath the line this PR is modifying and ran with & without `-detect_leaks=0`. The performance improvement is VERY stark. The number of fuzz inputs generated where either around 67 (2 tests) without, and with, it was a staggering 1937 (average from 2 runs). **Around 28x faster.**

### What it actually does

Without this PR, fuzzing stops early after 3 leaking runs (using suggested diff).
...

💬 hodlinator commented on pull request "fuzz: Faster leak check":
(https://github.com/bitcoin/bitcoin/pull/31481#discussion_r1893795721)
(Comment about `job`-function further up)

Suggested logging improvement in commit 3dd6ef5c174ce36119660cd2ccc82ab09d82300b, here tested with early leak-detection still enabled.

#### Before
```
Generating corpus to foo
Running '['/home/hodlinator/bitcoin/build_fuzz/src/test/fuzz/fuzz', '-rss_limit_mb=8000', '-max_total_time=10', '-reload=0', '-use_value_profile=0', PosixPath('foo/addition_overflow')]'
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/build_fuzz/test/f
...

💬 fanquake commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556782705)
> Tracked in hebasto/bitcoin-core-nightly#6.

Unless there's a good reason not too it'd be good to track issues in our issue tracker, rather than some other repository. Otherwise they are not included in stats tracking, the metadata archives, Core Check dashboards, overall less discoverable etc

💬 maflcko commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1893808813)
This will be left dangling and cause out-of-storage, if the build fails?

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893655528)
typo:
```suggestion
-M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)\n"
```

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

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893685756)
Since everything inserted into `args` is a constant literal, it can be `std::vector<std::string_view>` and then also `ExecCommand()` can be:

```diff
- void ExecCommand(const std::vector<std::string>& args, ...
+ void ExecCommand(const std::vector<std::string_view>& args, ...
```

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893661643)
Somehow I feel that `test` does not belong here - it is not for users to execute after the installation. `test_bitcoin` may not even be installed. The tests are normally run either by developers or by users after compile and before install, to verify the result of the compilation is correct.

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893680784)
```suggestion
} else if (!cmd.command.empty()) {
```

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893670004)
So, the user must have a knowledge whether his Bitcoin Core was compiled with or without multiprocess and start either `bitcoin -M gui` or `bitcoin -m gui`. Could we spare them this? The `bitcoin` executable has that knowledge.

Then the user can run just `bitcoin gui` and under the hood either `bitcoin-qt` or `bitcoin-gui` will be executed, depending on how it was compiled? Or maybe we can probe at run time whether the executables `bitcoin-qt` and `bitcoin-gui` exist and run whichever is pres
...

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893770067)
I think this is the same check as above: `argv0.find('/') == std::string_view::npos`, right? Would be good to use the same expression in both locations.

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893702291)
If you switch this to doxygen compatible comment it will be attached to the doxygen generated docs about `ExecCommand()`. Would be nice to format the arguments' descriptions with `@param[in] ...`.

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893696352)
This adds a trailing newline, but the above 2 `tfm::format()` calls don't. I think they should.

I think the `tfm::format(..., FormatParagraph(LicenseInfo()))` call adds a trailing `\n` because `LicenseInfo()` has it, but I did not check.

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893709992)
I find the names confusing because I would assume this will execute `argv0 args[0] args[1] args[2] ...`. Maybe rename `argv0` to `parent` or `wrapper_prog`.

💬 maflcko commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1893810274)
If this is fine, you could just drop the line (and leave it to the host to clean up). If not, I am not sure how to fix it.

👍 TheCharlatan approved a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517335576)
Nice improvements, Re-ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b

🤔 hebasto reviewed a pull request: "ci: Allow build dir on CI host"
(https://github.com/bitcoin/bitcoin/pull/31428#pullrequestreview-2517330988)
Approach ACK faf15ef3a78f6ee5ef5fb9468e9de04b5fca6b21, tested with https://github.com/bitcoin/bitcoin/pull/31176 built on top of this PR.

💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893810387)
An extra brace:
```suggestion
echo "BASE_BUILD_DIR=${RUNNER_TEMP}/build-asan" >> "$GITHUB_ENV"
```
?

💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893812448)
Should the definition of `BASE_BUILD_DIR` from https://github.com/bitcoin/bitcoin/blob/faf15ef3a78f6ee5ef5fb9468e9de04b5fca6b21/ci/test/03_test_script.sh#L118-L119

be moved before this first usage?

💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893810563)
nit: I slightly prefer `${{ runner.temp }}` over `${RUNNER_TEMP}` because it expands in the log, which feels convenient.