💬 fanquake commented on issue "GUI (?): Copying output from console causes large mem usage/OOM":
(https://github.com/bitcoin/bitcoin/issues/33285#issuecomment-3248931861)
I guess if it's known and has just never been fixed, maybe we should add a warning that copying from the console (too much?) might crash your node.
(https://github.com/bitcoin/bitcoin/issues/33285#issuecomment-3248931861)
I guess if it's known and has just never been fixed, maybe we should add a warning that copying from the console (too much?) might crash your node.
💬 fanquake commented on pull request "macdeploy: avoid use of `Bitcoin Core` in Linux cross build":
(https://github.com/bitcoin/bitcoin/pull/33158#discussion_r2318743803)
Dropped.
(https://github.com/bitcoin/bitcoin/pull/33158#discussion_r2318743803)
Dropped.
👍 hodlinator approved a pull request: "macdeploy: avoid use of `Bitcoin Core` in Linux cross build"
(https://github.com/bitcoin/bitcoin/pull/33158#pullrequestreview-3180311911)
ACK 8e434a84999c473a7295772a346cbce27888d28e
(https://github.com/bitcoin/bitcoin/pull/33158#pullrequestreview-3180311911)
ACK 8e434a84999c473a7295772a346cbce27888d28e
💬 jmoik commented on issue "build: secp256k1 warnings not turned into errors in MSAN job":
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3248965317)
I will take a look at this
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3248965317)
I will take a look at this
💬 maflcko commented on pull request "Fix compatibility with `-debuglogfile` command-line option":
(https://github.com/bitcoin-core/gui/pull/884#issuecomment-3249007687)
tested ACK c0d28c8f5b150a03de75155a0961b3d9b2695ed6 💳
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: tested ACK c0d28c8f
...
(https://github.com/bitcoin-core/gui/pull/884#issuecomment-3249007687)
tested ACK c0d28c8f5b150a03de75155a0961b3d9b2695ed6 💳
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: tested ACK c0d28c8f
...
💬 maflcko commented on issue "build: secp256k1 warnings not turned into errors in MSAN job":
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249024368)
The reason is that Werror is not passed down, so I guess the question is: What flags should be passed down under what circumstances?
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249024368)
The reason is that Werror is not passed down, so I guess the question is: What flags should be passed down under what circumstances?
📝 Sjors opened a pull request: "ci: cd into BASE_BUILD_DIR for GetCMakeLogFiles"
(https://github.com/bitcoin/bitcoin/pull/33291)
When a bug is introduced in cmake, we render its logs, which was broken:
https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3248645770
(https://github.com/bitcoin/bitcoin/pull/33291)
When a bug is introduced in cmake, we render its logs, which was broken:
https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3248645770
💬 Sjors commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249033527)
@maflcko the log is very verbose, but in indeed useful, so I opened #33291.
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249033527)
@maflcko the log is very verbose, but in indeed useful, so I opened #33291.
📝 Sjors converted_to_draft a pull request: "build: suggest -DENABLE_IPC=OFF when missing capnp"
(https://github.com/bitcoin/bitcoin/pull/33290)
Since #31802, when existing users upgrade to a recent version of master, or the upcoming v30 release, they'll be treated by an error that CapnProto is missing. Unless they read the release notes :-)
This error is generated by `src/ipc/libmultiprocess/CMakeLists.txt` which is a git subtree and so it doesn't have context of our project, and doesn't know about the `-DENABLE_IPC` option.
This pull request adds a simple pre-check in own CMake file to see if Cap'n Proto is missing. For ease of m
...
(https://github.com/bitcoin/bitcoin/pull/33290)
Since #31802, when existing users upgrade to a recent version of master, or the upcoming v30 release, they'll be treated by an error that CapnProto is missing. Unless they read the release notes :-)
This error is generated by `src/ipc/libmultiprocess/CMakeLists.txt` which is a git subtree and so it doesn't have context of our project, and doesn't know about the `-DENABLE_IPC` option.
This pull request adds a simple pre-check in own CMake file to see if Cap'n Proto is missing. For ease of m
...
💬 fanquake commented on issue "build: secp256k1 warnings not turned into errors in MSAN job":
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249065390)
My expectation would be that the CI shouldn't produce any spurious output, and any unexpected output would be an error. That way, any output is either fixed, or documented/suppressed. Otherwise anyone looking at the CI can't know what is expected, what should be fixed, or what doesn't matter, and we avoid issues like this, #33256, etc.
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249065390)
My expectation would be that the CI shouldn't produce any spurious output, and any unexpected output would be an error. That way, any output is either fixed, or documented/suppressed. Otherwise anyone looking at the CI can't know what is expected, what should be fixed, or what doesn't matter, and we avoid issues like this, #33256, etc.
💬 maflcko commented on pull request "ci: cd into BASE_BUILD_DIR for GetCMakeLogFiles":
(https://github.com/bitcoin/bitcoin/pull/33291#issuecomment-3249067521)
I haven't checked, but it seems plausible that I introduced this bug in commit fad191ff48b15832a90c19d560a7c0525c146be3, which fixed two other bugs.
Regardless, it seems better to specify the dir explicitly, than to silently/implicitly depend on it.
lgtm ACK 9b76eef2d2b42703e2a30952d4c3474b533e360a 💽
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -
...
(https://github.com/bitcoin/bitcoin/pull/33291#issuecomment-3249067521)
I haven't checked, but it seems plausible that I introduced this bug in commit fad191ff48b15832a90c19d560a7c0525c146be3, which fixed two other bugs.
Regardless, it seems better to specify the dir explicitly, than to silently/implicitly depend on it.
lgtm ACK 9b76eef2d2b42703e2a30952d4c3474b533e360a 💽
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -
...
🤔 janb84 reviewed a pull request: "build: set ENABLE_IPC to OFF when fuzzing"
(https://github.com/bitcoin/bitcoin/pull/33235#pullrequestreview-3180445284)
ACK af4156ab75565acc5a71b68e70da5e2cf407df80
PR sets the option `ENABLE_IPC` to `OFF` when building for fuzzing. This seems reasonable for now, because we do not have any fuzz testing on IPC code. Setting ENABLE_IPC` to `OFF` will reduce a dependency (capnp).
- build and tested ✅
(https://github.com/bitcoin/bitcoin/pull/33235#pullrequestreview-3180445284)
ACK af4156ab75565acc5a71b68e70da5e2cf407df80
PR sets the option `ENABLE_IPC` to `OFF` when building for fuzzing. This seems reasonable for now, because we do not have any fuzz testing on IPC code. Setting ENABLE_IPC` to `OFF` will reduce a dependency (capnp).
- build and tested ✅
💬 jmoik commented on issue "build: secp256k1 warnings not turned into errors in MSAN job":
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249078560)
from CmakeLists.txt:
# The core_interface library aims to encapsulate common build flags.
# It is a usage requirement for all targets except for secp256k1, which
# gets its flags by other means.
We can simply add a section for WERROR to the cmake file just like for other flags like BUILD_TESTS, to inherit the WERROR setting.
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249078560)
from CmakeLists.txt:
# The core_interface library aims to encapsulate common build flags.
# It is a usage requirement for all targets except for secp256k1, which
# gets its flags by other means.
We can simply add a section for WERROR to the cmake file just like for other flags like BUILD_TESTS, to inherit the WERROR setting.
💬 jesterhodl commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3249083029)
> checkpoints
Don't see it yet, I suppose it needs a sync?
<img width="587" height="187" alt="image" src="https://github.com/user-attachments/assets/9efcb5a1-8b8c-4544-ab53-af574f2784c7" />
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3249083029)
> checkpoints
Don't see it yet, I suppose it needs a sync?
<img width="587" height="187" alt="image" src="https://github.com/user-attachments/assets/9efcb5a1-8b8c-4544-ab53-af574f2784c7" />
💬 maflcko commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249086372)
> @maflcko the log is very verbose, but in indeed useful, so I opened #33291.
for reference, this specific warning was also printed without the verbose log: https://cirrus-ci.com/task/4516741763563520?logs=ci#L894 :
```
...
[03:43:58.798] CMake Error at /usr/lib/x86_64-linux-gnu/cmake/CapnProto/CapnProtoTargets.cmake:213 (add_executable):
[03:43:58.798] add_executable cannot create imported target "CapnProto::capnpc_cpp"
[03:43:58.798] because another target with the same name alre
...
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249086372)
> @maflcko the log is very verbose, but in indeed useful, so I opened #33291.
for reference, this specific warning was also printed without the verbose log: https://cirrus-ci.com/task/4516741763563520?logs=ci#L894 :
```
...
[03:43:58.798] CMake Error at /usr/lib/x86_64-linux-gnu/cmake/CapnProto/CapnProtoTargets.cmake:213 (add_executable):
[03:43:58.798] add_executable cannot create imported target "CapnProto::capnpc_cpp"
[03:43:58.798] because another target with the same name alre
...
💬 theStack commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3249097001)
> > ```
> > Arguments: -8@x21 8@x24 8@[sp, 136] 8@[x0] 8@x1 8@x2
> > ```
>
> Are these the same if you build with `-DAPPEND_CXXFLAGS='-fno-inline'`?
They are quite different on a `-fno-inline` build:
```
Name: outbound_message
Location: 0x000000000012ed28, Base: 0x000000000111c6d8, Semaphore: 0x00000000016594d0
Arguments: -8@x23 8@x24 8@x25 8@x26 8@x27 8@x0
```
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3249097001)
> > ```
> > Arguments: -8@x21 8@x24 8@[sp, 136] 8@[x0] 8@x1 8@x2
> > ```
>
> Are these the same if you build with `-DAPPEND_CXXFLAGS='-fno-inline'`?
They are quite different on a `-fno-inline` build:
```
Name: outbound_message
Location: 0x000000000012ed28, Base: 0x000000000111c6d8, Semaphore: 0x00000000016594d0
Arguments: -8@x23 8@x24 8@x25 8@x26 8@x27 8@x0
```
💬 Sjors commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249117834)
Using `find_program` instead of `find_package` as suggested by an AI overlord, seems to do the trick.
Another issue was that the depends build also threw this warning, which I added a check for.
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249117834)
Using `find_program` instead of `find_package` as suggested by an AI overlord, seems to do the trick.
Another issue was that the depends build also threw this warning, which I added a check for.
👋 Sjors's pull request is ready for review: "build: suggest -DENABLE_IPC=OFF when missing capnp"
(https://github.com/bitcoin/bitcoin/pull/33290)
(https://github.com/bitcoin/bitcoin/pull/33290)
💬 jmoik commented on issue "build: secp256k1 warnings not turned into errors in MSAN job":
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249143680)
I actually cannot reproduce the exact warning of this macro, not on clang or g++. But adding an explicitly undefined variable produces a warning and is properly turned into an error with -DWERROR=ON after adding the inheritance.
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249143680)
I actually cannot reproduce the exact warning of this macro, not on clang or g++. But adding an explicitly undefined variable produces a warning and is properly turned into an error with -DWERROR=ON after adding the inheritance.
💬 stickies-v commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318903859)
> If they are independent, maybe give them independent default values?
Done, thanks. I used the default `BUILD_TESTS` value because I suspect in most cases if people don't want to build the unit tests, they also don't want to build the functional tests, but perhaps if we do want that logic it's best to have a `BUILD_TESTS` that acts like an umbrella setting, with `cmake_dependent_options` `BUILD_UNIT_TESTS` and `BUILD_FUNCTIONAL_TESTS`. That seems out of scope for this PR, so I've just change
...
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318903859)
> If they are independent, maybe give them independent default values?
Done, thanks. I used the default `BUILD_TESTS` value because I suspect in most cases if people don't want to build the unit tests, they also don't want to build the functional tests, but perhaps if we do want that logic it's best to have a `BUILD_TESTS` that acts like an umbrella setting, with `cmake_dependent_options` `BUILD_UNIT_TESTS` and `BUILD_FUNCTIONAL_TESTS`. That seems out of scope for this PR, so I've just change
...