📝 hebasto converted_to_draft a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176)
This PR enables on the CI tests of cross-compiled Windows binaries on Windows.
It is important to have such tests in CI because the release binaries for Windows are also cross-compiled.
Two functional tests, `wallet_migration.py` and `wallet_multiwallet.py`, are temporarily disabled. They require fixes, such as https://github.com/bitcoin/bitcoin/pull/31410, and adjustments for error message handling. Re-enabling these tests will be addressed in follow-up PRs.
Resolves https://github.com
...
(https://github.com/bitcoin/bitcoin/pull/31176)
This PR enables on the CI tests of cross-compiled Windows binaries on Windows.
It is important to have such tests in CI because the release binaries for Windows are also cross-compiled.
Two functional tests, `wallet_migration.py` and `wallet_multiwallet.py`, are temporarily disabled. They require fixes, such as https://github.com/bitcoin/bitcoin/pull/31410, and adjustments for error message handling. Re-enabling these tests will be addressed in follow-up PRs.
Resolves https://github.com
...
💬 hebasto commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556851076)
Friendly ping Python connoisseur @stickies-v :)
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556851076)
Friendly ping Python connoisseur @stickies-v :)
💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2556857324)
Friendly ping a Python connoisseur @stickies-v :)
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2556857324)
Friendly ping a Python connoisseur @stickies-v :)
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893852851)
Right. That's why I suggest to move `BASE_BUILD_DIR=${BASE_BUILD_DIR:-$BASE_SCRATCH_DIR/build-$HOST}` before.
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893852851)
Right. That's why I suggest to move `BASE_BUILD_DIR=${BASE_BUILD_DIR:-$BASE_SCRATCH_DIR/build-$HOST}` before.
💬 fanquake commented on issue "depends: `capnp` package fails to build on NetBSD 10.0":
(https://github.com/bitcoin/bitcoin/issues/31499#issuecomment-2556860449)
Nothing for us to do here, so could be reopened/linked from a PR once a proper issue is filed upstream.
(https://github.com/bitcoin/bitcoin/issues/31499#issuecomment-2556860449)
Nothing for us to do here, so could be reopened/linked from a PR once a proper issue is filed upstream.
✅ fanquake closed an issue: "depends: `capnp` package fails to build on NetBSD 10.0"
(https://github.com/bitcoin/bitcoin/issues/31499)
(https://github.com/bitcoin/bitcoin/issues/31499)
💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893856069)
Options that start with `DANGER_` require in-depth knowledge of the inner workings of the CI system. Trying to make the existing code harder to understand for others seems suboptimal to solve a problem that doesn't really exist in practise. If there is a strong use-case, I am happy to reconsider, but absent that I don't see the value in making the code more complex.
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893856069)
Options that start with `DANGER_` require in-depth knowledge of the inner workings of the CI system. Trying to make the existing code harder to understand for others seems suboptimal to solve a problem that doesn't really exist in practise. If there is a strong use-case, I am happy to reconsider, but absent that I don't see the value in making the code more complex.
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893858493)
Let's consider this resolved.
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893858493)
Let's consider this resolved.
💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#issuecomment-2556874826)
force pushed with some minor style-only fixups
(https://github.com/bitcoin/bitcoin/pull/31428#issuecomment-2556874826)
force pushed with some minor style-only fixups
💬 vasild commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893862352)
No, wait, it was the tsan task, not the multiprocess task! Edited the above comment of mine. Anyway, it does not matter that much - it is an ABI incompatibility, so the conclusion is the same: don't enable `_LIBCPP_ABI_BOUNDED*` on users when `ENABLE_HARDENING=ON`.
> Why would the other tasks not break the ABI?
I am not sure exactly, but it has probably something to do with the way libc++ is compiled or `_LIBCPP_HARDENING_MODE_FAST` not playing well with `_LIBCPP_ABI_BOUNDED*`. Here is the
...
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893862352)
No, wait, it was the tsan task, not the multiprocess task! Edited the above comment of mine. Anyway, it does not matter that much - it is an ABI incompatibility, so the conclusion is the same: don't enable `_LIBCPP_ABI_BOUNDED*` on users when `ENABLE_HARDENING=ON`.
> Why would the other tasks not break the ABI?
I am not sure exactly, but it has probably something to do with the way libc++ is compiled or `_LIBCPP_HARDENING_MODE_FAST` not playing well with `_LIBCPP_ABI_BOUNDED*`. Here is the
...
👍 hebasto approved a pull request: "ci: Allow build dir on CI host"
(https://github.com/bitcoin/bitcoin/pull/31428#pullrequestreview-2517422562)
ACK fac0ab3286888c2eab674f6814ba8006e3a850a8.
(https://github.com/bitcoin/bitcoin/pull/31428#pullrequestreview-2517422562)
ACK fac0ab3286888c2eab674f6814ba8006e3a850a8.
💬 maflcko commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893876303)
> This error just does not happen on the other tasks.
Why would it not happen? I am happy to review and test this myself, but I'd appreciate if you could explain, or at least test the changes yourself. The link error will absolutely happen on the tasks, if you tested it.
The only reason why the CI is passing is that the ABI checks haven't been implemented in clang-16, so if you tested your change, you could see that the addition to the CI task is a no-op (as in: It can't find any more bugs
...
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893876303)
> This error just does not happen on the other tasks.
Why would it not happen? I am happy to review and test this myself, but I'd appreciate if you could explain, or at least test the changes yourself. The link error will absolutely happen on the tasks, if you tested it.
The only reason why the CI is passing is that the ABI checks haven't been implemented in clang-16, so if you tested your change, you could see that the addition to the CI task is a no-op (as in: It can't find any more bugs
...
🤔 hodlinator reviewed a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2517170254)
Code reviewed 80608ba5d282ae8713ea0136ea9a0208b254c082
`git range-diff master 54ad580 80608ba`
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2517170254)
Code reviewed 80608ba5d282ae8713ea0136ea9a0208b254c082
`git range-diff master 54ad580 80608ba`
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893895081)
Typo:
```
`-norpcwallet`, and `-no have
```
Also not sure this should be included in this sentence assuming it is `-asmap` since it was not used as a list arg?
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893895081)
Typo:
```
`-norpcwallet`, and `-no have
```
Also not sure this should be included in this sentence assuming it is `-asmap` since it was not used as a list arg?
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893738529)
> Reason for doing this is to fit in with existing tests in this file, and to fail right away instead of timing out when the expected log prints are missing. Tests that time out instead of failing are unpleasant to work with.
The rest of the tests are not using the new `info` functionality, so my suggestion is arguably a better fit, and your version would still time out if `addcon_thread_started` was not found (although that timeout would happen in earlier checks). Not sure we should be optim
...
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893738529)
> Reason for doing this is to fit in with existing tests in this file, and to fail right away instead of timing out when the expected log prints are missing. Tests that time out instead of failing are unpleasant to work with.
The rest of the tests are not using the new `info` functionality, so my suggestion is arguably a better fit, and your version would still time out if `addcon_thread_started` was not found (although that timeout would happen in earlier checks). Not sure we should be optim
...
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893728563)
Should people really be running functional tests with those flags?
```
~/bitcoin/test/functional
₿ git grep -E "\bassert\b" |wc -l
1499
~/bitcoin/test/functional
₿ git grep assert |wc -l
7228
```
My bet is that more than a handful of those 20.7% cases are not for internal assumptions.
If we really want to only use `assert` for internal assumptions maybe we shouldn't be using `AssertionError` for other cases and should not be calling our custom functions `assert_*`?
Tried running
...
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893728563)
Should people really be running functional tests with those flags?
```
~/bitcoin/test/functional
₿ git grep -E "\bassert\b" |wc -l
1499
~/bitcoin/test/functional
₿ git grep assert |wc -l
7228
```
My bet is that more than a handful of those 20.7% cases are not for internal assumptions.
If we really want to only use `assert` for internal assumptions maybe we shouldn't be using `AssertionError` for other cases and should not be calling our custom functions `assert_*`?
Tried running
...
💬 hodlinator commented on pull request "fuzz: Faster leak check when generating corpus":
(https://github.com/bitcoin/bitcoin/pull/31481#issuecomment-2556961836)
> Just for completeness, the performance increase should be mild overall, assuming that the leak detection is turned off automatically anyway for long runs, but I haven't confirmed this.
>
> If [#31481 (comment)](https://github.com/bitcoin/bitcoin/pull/31481#discussion_r1882498320) is too controversial, an alternative could be to just disable it when `empty_min_time` is active, or just close this pull.
Yeah, if libfuzzer automatically stops leak detection after a while, I can see a case fo
...
(https://github.com/bitcoin/bitcoin/pull/31481#issuecomment-2556961836)
> Just for completeness, the performance increase should be mild overall, assuming that the leak detection is turned off automatically anyway for long runs, but I haven't confirmed this.
>
> If [#31481 (comment)](https://github.com/bitcoin/bitcoin/pull/31481#discussion_r1882498320) is too controversial, an alternative could be to just disable it when `empty_min_time` is active, or just close this pull.
Yeah, if libfuzzer automatically stops leak detection after a while, I can see a case fo
...
👍 hebasto approved a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517520234)
re-ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b.
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517520234)
re-ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b.
🤔 maflcko reviewed a pull request: "build: enable libc++ and libstdc++ hardening"
(https://github.com/bitcoin/bitcoin/pull/31424#pullrequestreview-2517521376)
concept ack, but it would be good to get the (CI) changes right
(https://github.com/bitcoin/bitcoin/pull/31424#pullrequestreview-2517521376)
concept ack, but it would be good to get the (CI) changes right
💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2556981982)
(assigned milestone, because this is a blocker to run the tests on native windows, which would be nice to *finally* get running)
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2556981982)
(assigned milestone, because this is a blocker to run the tests on native windows, which would be nice to *finally* get running)