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

💬 maflcko commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556797216)
I think in this case it is fine, because each issue has a pull request with an extended description, so it will end up in the metadata backup. But anything should be fine by me.

🤔 l0rinc requested changes to a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517230492)
Thanks for the cleanup, please consider a few simplification and unification recommendations

💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893758544)
it's a small change, please consider reformatting the whole file

💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893781576)
we're getting `Functional-style cast is used instead of a C++ cast` warning now, what if we used character literals instead:
```suggestion
string(REGEX REPLACE "[^\n][^\n]" "'\\\\x\\0'," formatted_bytes "${formatted_bytes}")
```
which would look like:
```C++
'\x5b','\x5c','\x6e','\x61','\x6b','\x65','\x64','\x5d',
```
instead of
```C++
char(0x5b),char(0x5c),char(0x6e),char(0x61),char(0x6b),char(0x65),char(0x64),char(0x5d),
```

💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893750818)
does this sorting (here and in unitester.cpp) serve some purpose of could we use natural ordering?
We can't see this way that e.g. `fail43` is missing and adding new lines would get confusing this way (maybe even adding a 0 prefix for <10):

<details>
<summary>Details</summary>

```C++
if(BUILD_TESTS)
include(GenerateHeaders)
generate_header_from_json(test/fail1.json)
generate_header_from_json(test/fail2.json)
generate_header_from_json(test/fail3.json)
generate_header_from_
...

💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893811557)
I understand why a macro is the smallest change to make `runtest` happy, but I think we can avoid that weird string parsing and introduce enums instead:
```C++
enum class TestType { Fail, Pass, RoundTrip };
```
and define the test as:
```C++
inline constexpr std::map<std::string_view, TestType> tests{
{json_tests::fail1, TestType::Fail},
{json_tests::fail2, TestType::Fail},
{json_tests::fail3, TestType::Fail},
...
```
which could simplify `runtest` to:
```C++
static voi
...