💬 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?
🤔 dergoegge reviewed a pull request: "refactor: Introduce EvictionManager and use it for the inbound eviction logic"
(https://github.com/bitcoin/bitcoin/pull/25572#pullrequestreview-1383046924)
@ajtowns Thank you for the comments!
(https://github.com/bitcoin/bitcoin/pull/25572#pullrequestreview-1383046924)
@ajtowns Thank you for the comments!
💬 dergoegge commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165346413)
Yea something like that would make sense. Also similar to John's [comment](https://github.com/bitcoin/bitcoin/pull/25572#discussion_r919919355).
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165346413)
Yea something like that would make sense. Also similar to John's [comment](https://github.com/bitcoin/bitcoin/pull/25572#discussion_r919919355).
💬 dergoegge commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165335739)
I think it makes sense to not have the eviction manager be owned by another component, in the context of also moving the outbound eviction logic to the eviction manager. I see it as more of a standalone component like the addrman or banman.
This might also give us flexibility in mocking it if we wanted to for testing purposes (e.g. to test that net processing and net are updating the stats correctly, or have a `FuzzedEvictionManager` to let the fuzzer choose which peers to evict).
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165335739)
I think it makes sense to not have the eviction manager be owned by another component, in the context of also moving the outbound eviction logic to the eviction manager. I see it as more of a standalone component like the addrman or banman.
This might also give us flexibility in mocking it if we wanted to for testing purposes (e.g. to test that net processing and net are updating the stats correctly, or have a `FuzzedEvictionManager` to let the fuzzer choose which peers to evict).
💬 dergoegge commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165282299)
> Repeatedly looking up the eviction manager map and locking and unlocking access to it
Two things:
* There are very few full outbound or block relay only connections, so this happens at most maybe 3 or 4 times for the block relay only loop and 11 or 12 times for the full outbound one.
* The outbound eviction logic is also moved to the eviction manager in a follow up (see #25268), which removes this repeated locking.
> and introducing the potential for maps to be out of sync and having t
...
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165282299)
> Repeatedly looking up the eviction manager map and locking and unlocking access to it
Two things:
* There are very few full outbound or block relay only connections, so this happens at most maybe 3 or 4 times for the block relay only loop and 11 or 12 times for the full outbound one.
* The outbound eviction logic is also moved to the eviction manager in a follow up (see #25268), which removes this repeated locking.
> and introducing the potential for maps to be out of sync and having t
...
📝 hebasto opened a pull request: "qt: Register `wallet::AddressPurpose` type"
(https://github.com/bitcoin-core/gui/pull/726)
This PR is a follow up of bitcoin/bitcoin#27217.
Fixes #725.
(https://github.com/bitcoin-core/gui/pull/726)
This PR is a follow up of bitcoin/bitcoin#27217.
Fixes #725.
💬 hebasto commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1165367412)
Fixed in https://github.com/bitcoin-core/gui/pull/726.
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1165367412)
Fixed in https://github.com/bitcoin-core/gui/pull/726.
💬 fanquake commented on pull request "ci: explicitly install libclang-rt-dev in valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27459#issuecomment-1506784122)
> Maybe https://github.com/bitcoin/bitcoin/pull/27436#issuecomment-1506443972 is related?
I'm pretty certain this is https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1005341, which isn't related to this change, but seems to have been broken in Debian for libc++ & fuzzing for LLVM versions 12 through 16. I'll file a new issue with the libc++ 16 package, and debug futher.
(https://github.com/bitcoin/bitcoin/pull/27459#issuecomment-1506784122)
> Maybe https://github.com/bitcoin/bitcoin/pull/27436#issuecomment-1506443972 is related?
I'm pretty certain this is https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1005341, which isn't related to this change, but seems to have been broken in Debian for libc++ & fuzzing for LLVM versions 12 through 16. I'll file a new issue with the libc++ 16 package, and debug futher.
🚀 fanquake merged a pull request: "ci: explicitly install libclang-rt-dev in valgrind jobs"
(https://github.com/bitcoin/bitcoin/pull/27459)
(https://github.com/bitcoin/bitcoin/pull/27459)
📝 MarcoFalke opened a pull request: "rpc: Add importmempool RPC "
(https://github.com/bitcoin/bitcoin/pull/27460)
Currently it is possible to import a mempool by placing it in the datadir and starting the node. However this has many issues:
* Users aren't expected to fiddle with the datadir, possibly corrupting it
* An existing mempool file in the datadir may be overwritten
* The node needs to be restarted
* Importing an untrusted file this way is dangerous, because it can corrupt the mempool
Fix all issues by adding a new RPC.
(https://github.com/bitcoin/bitcoin/pull/27460)
Currently it is possible to import a mempool by placing it in the datadir and starting the node. However this has many issues:
* Users aren't expected to fiddle with the datadir, possibly corrupting it
* An existing mempool file in the datadir may be overwritten
* The node needs to be restarted
* Importing an untrusted file this way is dangerous, because it can corrupt the mempool
Fix all issues by adding a new RPC.
💬 glozow commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1506796551)
> > Adding a loadmempool RPC seems to be exactly what we want here?
>
> concept ACK on this, if that's the only use-case. We should probably offer a similar service over RPC first, in addition to information gathering that it's not actually being used in meaningful amounts in production. I've never come across usage myself in my production work.
#27460
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1506796551)
> > Adding a loadmempool RPC seems to be exactly what we want here?
>
> concept ACK on this, if that's the only use-case. We should probably offer a similar service over RPC first, in addition to information gathering that it's not actually being used in meaningful amounts in production. I've never come across usage myself in my production work.
#27460