💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165045822)
You may expand commit b3261389eadf050c0de590c2d9cd69ad9f62acff `net, refactor: pass reference for peer address in GetReachabilityFrom` to remove `NET_UNKNOWN` since it is unused after that commit.
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165045822)
You may expand commit b3261389eadf050c0de590c2d9cd69ad9f62acff `net, refactor: pass reference for peer address in GetReachabilityFrom` to remove `NET_UNKNOWN` since it is unused after that commit.
💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165072581)
nit: might as well ditch `addr_empty` and use
```suggestion
BOOST_CHECK(!GetLocalAddress(*peer_onion).IsValid());
```
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165072581)
nit: might as well ditch `addr_empty` and use
```suggestion
BOOST_CHECK(!GetLocalAddress(*peer_onion).IsValid());
```
💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165073055)
```suggestion
// local addresses from all networks
```
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165073055)
```suggestion
// local addresses from all networks
```
🤔 MarcoFalke reviewed a pull request: "refactor: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1382795595)
any reason there are two commits? Seems odd to first add unused includes and then remove them in the second commit.
It might be better to not add the unused includes in the first place
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1382795595)
any reason there are two commits? Seems odd to first add unused includes and then remove them in the second commit.
It might be better to not add the unused includes in the first place
💬 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?