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

💬 maflcko commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556799506)
lgtm ACK d38ade7bc409207bf425e05ee10b633b0aef4c36

In theory, the shebang could be removed from the mocks files, to better enforce this, but this could make one of the linters sad, so could be done in a follow-up.

💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820716)
I want to keep the changes minimal. Happy to review a follow-up doing this, but I'll leave this as-is for now.

💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820757)
> we're getting `Functional-style cast is used instead of a C++ cast` warning now, what if we used character literals instead:

Where? Functional-style casts are recommended by the dev notes, so if you want to change that, it would be good to change the dev notes first.

💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820831)
Happy to review a follow-up doing this, but I'll leave this as-is for now.

💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820913)
> does this sorting (here and in unitester.cpp) serve some purpose of could we use natural ordering?

Yes, the purpose is to follow the clang-format sorting. If it was changed, clang-format will undo the change the next time.

> maybe even adding a 0 prefix for <10

Happy to review a follow-up doing this, but I'll leave this as-is for now.

💬 hebasto commented on pull request "guix: latest 2.31 glibc":
(https://github.com/bitcoin/bitcoin/pull/31529#issuecomment-2556813270)
My Guix build:
```
aarch64
8f50480a91024a77c45233e33b8151e213a08834c77493446191a6bc78d82a1f guix-build-b8710201fbd0/output/aarch64-linux-gnu/SHA256SUMS.part
6fd527ebfa0842cd3f3c743dc616d75cdcd0aea637d04e4e047f29e87c43d383 guix-build-b8710201fbd0/output/aarch64-linux-gnu/bitcoin-b8710201fbd0-aarch64-linux-gnu-debug.tar.gz
a40ec71782cc8018ffb9b8b73d99c55aece77c5779e752f1b4209894efd3c97b guix-build-b8710201fbd0/output/aarch64-linux-gnu/bitcoin-b8710201fbd0-aarch64-linux-gnu.tar.gz
74fb98d9
...