💬 maflcko commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893644508)
I don't expect the downgrade will ever affect users. So instead of "Ubuntu 123.45 LTS" I can only imagine a test-only build of a distro that has debug hardening enabled in the system toolchain. This way, they can simply compile all system (and third-party) packages and run the tests with maximum hardening to spot any mistakes before they hit real users.

This was possible before your changes, but won't be possible after your changes.

Maybe it is expected that they need to specifically sele
...

✅ 0xB10C closed a pull request: "ci: allow passing extra args to docker build with CI_IMAGE_BUILD_EXTRA_ARGS"
(https://github.com/bitcoin/bitcoin/pull/31377)

💬 0xB10C commented on pull request "ci: allow passing extra args to docker build with CI_IMAGE_BUILD_EXTRA_ARGS":
(https://github.com/bitcoin/bitcoin/pull/31377#issuecomment-2556550827)
closing in favor of https://github.com/bitcoin/bitcoin/pull/31545

💬 maflcko commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1893664120)
Is this required? It seems like a no-op, apart from the docs.

Though, docker is already caching by default, and this is normally not needed. So I think it is fine to omit the docs, or inline them in 02_run_container.

I am even thinking this should have the `DANGER_` prefix, as it calls `rm -rf` on the host for the folder.

👍 hebasto approved a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517189261)
ACK faf23a8d0328f288d64dd697de4efedbc6970531.

💬 A-Manning commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1893731134)
Both helper functions are internal to `run_test` in ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e.
`check_getmininginfo` makes specific assertions on values for fields such as `blocks`, which may change during a test. It is best to keep it as localized as possible, since it is unlikely to be broadly useful.

💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2556700234)
Thx, fixed typo (good catch)! Also, pushed again for `inline constexpr` :sweat_smile:

👍 TheCharlatan approved a pull request: "cmake: Remove unused `BUILD_TESTING` variable from "dev-mode" preset"
(https://github.com/bitcoin/bitcoin/pull/31544#pullrequestreview-2517259051)
ACK e196190a284fc6ffd21a39cfae957fdd8b4657b6

💬 0xB10C commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1893769621)
Good point. Will move docs to 02_run_container and prefix with `DANGER_`.

💬 0xB10C commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#issuecomment-2556731379)
A consideration might also to only specify `--cache-to` when ` CIRRUS_BRANCH=master`. This makes PR runs slightly faster as the cache isn't overwritten. Also, most PRs don't introduce changes that cause a cache-miss, and if they do we want to not cache it until it's merged. Obviously, someone can fake this by overwriting `CIRRUS_BRANCH`.

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2556774572)
Rebased on https://github.com/bitcoin/bitcoin/pull/31428 with cherry-picked commits from https://github.com/bitcoin/bitcoin/pull/31542.

👍 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()) {
```