💬 MarcoFalke commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1165110007)
Any reason to change the order? Unless there is a reason, this shouldn't be changed, because previously and commonly the first include in the cpp file is the corresponding header file to catch missing includes.
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1165110007)
Any reason to change the order? Unless there is a reason, this shouldn't be changed, because previously and commonly the first include in the cpp file is the corresponding header file to catch missing includes.
📝 hebasto opened a pull request: "build: Fix USDT detection on FreeBSD"
(https://github.com/bitcoin/bitcoin/pull/27458)
From https://github.com/hebasto/bitcoin/pull/13#pullrequestreview-1381000080:
> [On FreeBSD] Tracepoints (USDT) are not detected even though `sys/sdt.h` is available in `/usr/include`, this is the same as with autotools (unrelated to the cmake effort).
The suggested commit fixes USDT detection, but compiling fails because the `/usr/include/sys/sdt.h` header does not define `DTRACE_PROBE{6,7,8,9,10,11,12}` macros.
(https://github.com/bitcoin/bitcoin/pull/27458)
From https://github.com/hebasto/bitcoin/pull/13#pullrequestreview-1381000080:
> [On FreeBSD] Tracepoints (USDT) are not detected even though `sys/sdt.h` is available in `/usr/include`, this is the same as with autotools (unrelated to the cmake effort).
The suggested commit fixes USDT detection, but compiling fails because the `/usr/include/sys/sdt.h` header does not define `DTRACE_PROBE{6,7,8,9,10,11,12}` macros.
💬 hebasto commented on pull request "build: Fix USDT detection on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506586033)
cc @0xB10C @vasild
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506586033)
cc @0xB10C @vasild
💬 0xB10C commented on pull request "build: Fix USDT detection on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506629380)
Thanks for looking into this, however, I'm not sure if it's the right time for this. At the moment, we only really support the tracepoints on Linux.
> The suggested commit fixes USDT detection [..]
The suggested commit **enables** USDT detection on FreeBSD, something that (somewhat unintentionally) wasn't enabled before. Not sure if it's worth enabling them at the moment for FreeBSD. It would be awesome to have them on more OS's (e.g. macOS see https://github.com/bitcoin/bitcoin/pull/25541
...
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506629380)
Thanks for looking into this, however, I'm not sure if it's the right time for this. At the moment, we only really support the tracepoints on Linux.
> The suggested commit fixes USDT detection [..]
The suggested commit **enables** USDT detection on FreeBSD, something that (somewhat unintentionally) wasn't enabled before. Not sure if it's worth enabling them at the moment for FreeBSD. It would be awesome to have them on more OS's (e.g. macOS see https://github.com/bitcoin/bitcoin/pull/25541
...
⚠️ fanquake opened an issue: "Crash using getnewaddress in the console"
(https://github.com/bitcoin-core/gui/issues/725)
Calling `getnewaddress` in the GUI console currently crashes bitcoin-qt. Reported here: https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1165016111.
```bash
* thread #35, name = 'b-qt-rpcconsole', stop reason = hit program assert
frame #4: 0x00000001000e9150 bitcoin-qt`NotifyAddressBookChanged(walletmodel=0x0000600003d270c0, address=<unavailable>, label=<unavailable>, isMine=<unavailable>, purpose=<unavailable>, status=CT_NEW) at walletmodel.cpp:392:5 [opt]
389
...
(https://github.com/bitcoin-core/gui/issues/725)
Calling `getnewaddress` in the GUI console currently crashes bitcoin-qt. Reported here: https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1165016111.
```bash
* thread #35, name = 'b-qt-rpcconsole', stop reason = hit program assert
frame #4: 0x00000001000e9150 bitcoin-qt`NotifyAddressBookChanged(walletmodel=0x0000600003d270c0, address=<unavailable>, label=<unavailable>, isMine=<unavailable>, purpose=<unavailable>, status=CT_NEW) at walletmodel.cpp:392:5 [opt]
389
...
💬 fanquake commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1165251694)
Thanks, opened an issue here to follow up: https://github.com/bitcoin-core/gui/issues/725.
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1165251694)
Thanks, opened an issue here to follow up: https://github.com/bitcoin-core/gui/issues/725.
💬 fanquake commented on pull request "build: Fix USDT detection on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506637735)
I agree with @0xB10C here. Not sure about making this change, given the tracepoints still wont work anyways.
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506637735)
I agree with @0xB10C here. Not sure about making this change, given the tracepoints still wont work anyways.
🚀 fanquake merged a pull request: "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs"
(https://github.com/bitcoin/bitcoin/pull/27444)
(https://github.com/bitcoin/bitcoin/pull/27444)
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506644468)
Did you test this? CI is red, looking at https://cirrus-ci.com/task/5858910658101248
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506644468)
Did you test this? CI is red, looking at https://cirrus-ci.com/task/5858910658101248
💬 0xB10C commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1506649842)
Thanks, didn't know `rr`. Especially the chaos mode (https://robert.ocallahan.org/2016/02/introducing-rr-chaos-mode.html) seems to be helpful here. However, rr and bpf don't work well together yet. `rr` asserts when running the python USDT interface tests. I've tried to patch `rr` to make this work but haven't been successful yet.
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1506649842)
Thanks, didn't know `rr`. Especially the chaos mode (https://robert.ocallahan.org/2016/02/introducing-rr-chaos-mode.html) seems to be helpful here. However, rr and bpf don't work well together yet. `rr` asserts when running the python USDT interface tests. I've tried to patch `rr` to make this work but haven't been successful yet.
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506651144)
Yea, these work for me locally. Isn't that CI expected that to fail, before this was merged, because it'd be using the wrong image to run the job?
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506651144)
Yea, these work for me locally. Isn't that CI expected that to fail, before this was merged, because it'd be using the wrong image to run the job?
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506659825)
Locally it also fails for me:
```
FILE_ENV="./ci/test/00_setup_env_native_fuzz_with_valgrind.sh" ./ci/test_run_all.sh
```
```
...
sbindir='${exec_prefix}/sbin'
sharedstatedir='${prefix}/com'
subdirs=''
sysconfdir='${prefix}/etc'
target_alias=''
## ----------- ##
## confdefs.h. ##
## ----------- ##
/* confdefs.h */
#define PACKAGE_NAME "Bitcoin Core"
#define PACKAGE_TARNAME "bitcoin"
#define PACKAGE_VERSION "24.99.0"
#define PACKAGE_STRING "Bitcoin Core 24.99.0"
...
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506659825)
Locally it also fails for me:
```
FILE_ENV="./ci/test/00_setup_env_native_fuzz_with_valgrind.sh" ./ci/test_run_all.sh
```
```
...
sbindir='${exec_prefix}/sbin'
sharedstatedir='${prefix}/com'
subdirs=''
sysconfdir='${prefix}/etc'
target_alias=''
## ----------- ##
## confdefs.h. ##
## ----------- ##
/* confdefs.h */
#define PACKAGE_NAME "Bitcoin Core"
#define PACKAGE_TARNAME "bitcoin"
#define PACKAGE_VERSION "24.99.0"
#define PACKAGE_STRING "Bitcoin Core 24.99.0"
...
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506661656)
Can you provide the full config.log?
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506661656)
Can you provide the full config.log?
💬 hebasto commented on pull request "build: Fix USDT detection on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506670061)
Besides the FreeBSD case, shouldn't tokens be passed to the tested macro rather than string literals?
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506670061)
Besides the FreeBSD case, shouldn't tokens be passed to the tested macro rather than string literals?
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506671526)
> Isn't that CI expected that to fail, before this was merged, because it'd be using the wrong image to run the job?
No, you didn't change anything in this pull, other than the image tag. So if the CI passed before and after, there is no "wrong image" tag.
> Can you provide the full config.log?
Should be the same I linked to above.
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506671526)
> Isn't that CI expected that to fail, before this was merged, because it'd be using the wrong image to run the job?
No, you didn't change anything in this pull, other than the image tag. So if the CI passed before and after, there is no "wrong image" tag.
> Can you provide the full config.log?
Should be the same I linked to above.
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506682075)
> No, you didn't change anything in this pull, other than the image tag. So if the CI passed before and after, there is no "wrong image" tag.
The image tags in the qa-assets repo are now wrong right? They are pointing to the wrong OS, than what we are expecting to run the job on?
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506682075)
> No, you didn't change anything in this pull, other than the image tag. So if the CI passed before and after, there is no "wrong image" tag.
The image tags in the qa-assets repo are now wrong right? They are pointing to the wrong OS, than what we are expecting to run the job on?
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506688903)
I wouldn't call it "wrong", just "mismatch". I think absent any other reason the CI should support the largest range possible of image tags. See https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1164382055
This means a user/dev can easily force the tag to be a different one to quickly check different package versions.
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506688903)
I wouldn't call it "wrong", just "mismatch". I think absent any other reason the CI should support the largest range possible of image tags. See https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1164382055
This means a user/dev can easily force the tag to be a different one to quickly check different package versions.
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506693578)
> I think absent any other reason the CI should support the largest range possible of image tags.
> This means a user/dev can easily force the tag to be a different one to quickly check different package versions.
Yea I think this is fine, but obviously constrained to where the exact same apt invocations are going to work. i.e you can't necessarily drop in an `:experimental`, and have it work, because packages will have changed, and you'd need to adjust the apt call, to add something like li
...
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1506693578)
> I think absent any other reason the CI should support the largest range possible of image tags.
> This means a user/dev can easily force the tag to be a different one to quickly check different package versions.
Yea I think this is fine, but obviously constrained to where the exact same apt invocations are going to work. i.e you can't necessarily drop in an `:experimental`, and have it work, because packages will have changed, and you'd need to adjust the apt call, to add something like li
...
📝 fanquake opened a pull request: "ci: explicitly install libclang-rt-dev in valgrind jobs"
(https://github.com/bitcoin/bitcoin/pull/27459)
This fixes some cases, i.e under --no-install-recommends, where libclang-rt-dev wouldn't be installed, and configuring would then fail.
Followup to #27444.
(https://github.com/bitcoin/bitcoin/pull/27459)
This fixes some cases, i.e under --no-install-recommends, where libclang-rt-dev wouldn't be installed, and configuring would then fail.
Followup to #27444.
👍 MarcoFalke approved a pull request: "ci: explicitly install libclang-rt-dev in valgrind jobs"
(https://github.com/bitcoin/bitcoin/pull/27459#pullrequestreview-1383128219)
lgtm. It is good to fix bugs, but it would be better to fix all related bugs. Maybe https://github.com/bitcoin/bitcoin/pull/27436#issuecomment-1506443972 is related?
(https://github.com/bitcoin/bitcoin/pull/27459#pullrequestreview-1383128219)
lgtm. It is good to fix bugs, but it would be better to fix all related bugs. Maybe https://github.com/bitcoin/bitcoin/pull/27436#issuecomment-1506443972 is related?