🤔 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
(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
(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),
```
(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_
...
(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
...
(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.
(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.
(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.
(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.
(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.
(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
...
(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
...
💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893823673)
It requires the caller to define it, which is intentional. Should I add a comment explaining it?
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893823673)
It requires the caller to define it, which is intentional. Should I add a comment explaining it?
💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893825061)
Not sure what you mean, for me it is already expanded/evaluated, see https://github.com/bitcoin/bitcoin/pull/31428/checks#step:6:11
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893825061)
Not sure what you mean, for me it is already expanded/evaluated, see https://github.com/bitcoin/bitcoin/pull/31428/checks#step:6:11
👍 hebasto approved a pull request: "guix: latest 2.31 glibc"
(https://github.com/bitcoin/bitcoin/pull/31529#pullrequestreview-2517356688)
ACK b8710201fbd01bf2cba3c3b1fd312316b1ae22af.
(https://github.com/bitcoin/bitcoin/pull/31529#pullrequestreview-2517356688)
ACK b8710201fbd01bf2cba3c3b1fd312316b1ae22af.
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893827538)
The following works on FreeBSD. In case `dtrace` works on macOS:
```sh
dtrace -n 'proc:::exec-success { trace(curpsinfo->pr_psargs); }' -c 'bitcoin -m gui'
```
Maybe how to enable `dtrace` on macOS: https://github.com/suolapeikko/dtrace?tab=readme-ov-file#sample-dtrace-scripts-for-macos
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893827538)
The following works on FreeBSD. In case `dtrace` works on macOS:
```sh
dtrace -n 'proc:::exec-success { trace(curpsinfo->pr_psargs); }' -c 'bitcoin -m gui'
```
Maybe how to enable `dtrace` on macOS: https://github.com/suolapeikko/dtrace?tab=readme-ov-file#sample-dtrace-scripts-for-macos
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893834634)
I mean, expanded in a log for the current step, like [this](https://github.com/hebasto/bitcoin/actions/runs/12430191280/job/34705149833#step:3:2):
```
Run echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
echo "BASE_ROOT_DIR=/home/runner/work/_temp" >> "$GITHUB_ENV"
echo "DEPENDS_DIR=/home/runner/work/_temp/depends" >> "$GITHUB_ENV"
echo "BASE_BUILD_DIR=/home/runner/work/_temp/build" >> "$GITH
...
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893834634)
I mean, expanded in a log for the current step, like [this](https://github.com/hebasto/bitcoin/actions/runs/12430191280/job/34705149833#step:3:2):
```
Run echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
echo "BASE_ROOT_DIR=/home/runner/work/_temp" >> "$GITHUB_ENV"
echo "DEPENDS_DIR=/home/runner/work/_temp/depends" >> "$GITHUB_ENV"
echo "BASE_BUILD_DIR=/home/runner/work/_temp/build" >> "$GITH
...
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893837484)
> It requires the caller to define it, which is intentional.
Why? What happens if the default value will be used? (especially, if a CI job doesn't care about it)
> Should I add a comment explaining it?
If it helps :)
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893837484)
> It requires the caller to define it, which is intentional.
Why? What happens if the default value will be used? (especially, if a CI job doesn't care about it)
> Should I add a comment explaining it?
If it helps :)
💬 maflcko commented on pull request "fuzz: Faster leak check":
(https://github.com/bitcoin/bitcoin/pull/31481#discussion_r1893840379)
Also, the message is hidden anyway by default in the fuzz runner, unless you specified `-l DEBUG`.
(https://github.com/bitcoin/bitcoin/pull/31481#discussion_r1893840379)
Also, the message is hidden anyway by default in the fuzz runner, unless you specified `-l DEBUG`.
💬 maflcko commented on pull request "fuzz: Faster leak check":
(https://github.com/bitcoin/bitcoin/pull/31481#issuecomment-2556840485)
> ### 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.**
Just for completeness, the performance increase should be mild overall, assuming that the leak detection is turned off automatically anyway for long run
...
(https://github.com/bitcoin/bitcoin/pull/31481#issuecomment-2556840485)
> ### 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.**
Just for completeness, the performance increase should be mild overall, assuming that the leak detection is turned off automatically anyway for long run
...
💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893844905)
> > It requires the caller to define it, which is intentional.
>
> Why? What happens if the default value will be used? (especially, if a CI job doesn't care about it)
My understanding is that it would fail otherwise. I suspect the error message will be:
```
root@7b4d00222340:/# mkdir -p "${BASE_BUILD_DIR}"
mkdir: cannot create directory '': No such file or directory
root@7b4d00222340:/# mkdir -p ""
mkdir: cannot create directory '': No such file or directory
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893844905)
> > It requires the caller to define it, which is intentional.
>
> Why? What happens if the default value will be used? (especially, if a CI job doesn't care about it)
My understanding is that it would fail otherwise. I suspect the error message will be:
```
root@7b4d00222340:/# mkdir -p "${BASE_BUILD_DIR}"
mkdir: cannot create directory '': No such file or directory
root@7b4d00222340:/# mkdir -p ""
mkdir: cannot create directory '': No such file or directory