💬 achow101 commented on pull request "Register `wallet::AddressPurpose` type":
(https://github.com/bitcoin-core/gui/pull/726#issuecomment-1506961246)
ACK a45b54406dbce4fbf8a316a0e91615eb480da653
(https://github.com/bitcoin-core/gui/pull/726#issuecomment-1506961246)
ACK a45b54406dbce4fbf8a316a0e91615eb480da653
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165532774)
Looks like gcc works fine, see also https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1163983711. Though, this won't help with the fuzz task.
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165532774)
Looks like gcc works fine, see also https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1163983711. Though, this won't help with the fuzz task.
✅ hebasto closed an issue: "Crash using getnewaddress in the console"
(https://github.com/bitcoin-core/gui/issues/725)
(https://github.com/bitcoin-core/gui/issues/725)
🚀 hebasto merged a pull request: "Register `wallet::AddressPurpose` type"
(https://github.com/bitcoin-core/gui/pull/726)
(https://github.com/bitcoin-core/gui/pull/726)
💬 ajtowns commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1507067647)
> Right, the changes for package relay only (i.e. no v3, no package RBF) would be this, persist packages across restart, and allow any ancestor package instead of just child-with-parents.
Sorry, I more just mean "what's the minimal set of additional commits that need this that results in something that's useful on its own?"
Maybe the current overarching "useful thing" is just "package mempool acceptance works in regtest via rpc in a way that would be acceptable to expose on mainnet for pac
...
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1507067647)
> Right, the changes for package relay only (i.e. no v3, no package RBF) would be this, persist packages across restart, and allow any ancestor package instead of just child-with-parents.
Sorry, I more just mean "what's the minimal set of additional commits that need this that results in something that's useful on its own?"
Maybe the current overarching "useful thing" is just "package mempool acceptance works in regtest via rpc in a way that would be acceptable to expose on mainnet for pac
...
💬 MarcoFalke commented on pull request "ci: set docker run --ulimit to workaround Valgrind assertion":
(https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1507077524)
I tried to reproduce on fedora 37 on current master, and it passed with podman
(https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1507077524)
I tried to reproduce on fedora 37 on current master, and it passed with podman
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165620563)
The fuzz task seems to be passing, see https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1507077524, so no need to bump valgrind. You can simply use gcc:
```diff
diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
index 97b85755e..794a2dcca 100755
--- a/ci/test/00_setup_env_native_valgrind.sh
+++ b/ci/test/00_setup_env_native_valgrind.sh
@@ -8,10 +8,10 @@ export LC_ALL=C.UTF-8
export CI_IMAGE_NAME_TAG="debian:bookworm"
export CON
...
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165620563)
The fuzz task seems to be passing, see https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1507077524, so no need to bump valgrind. You can simply use gcc:
```diff
diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
index 97b85755e..794a2dcca 100755
--- a/ci/test/00_setup_env_native_valgrind.sh
+++ b/ci/test/00_setup_env_native_valgrind.sh
@@ -8,10 +8,10 @@ export LC_ALL=C.UTF-8
export CI_IMAGE_NAME_TAG="debian:bookworm"
export CON
...
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165623156)
I'll still open a PR for discussion, as I don't think our reponse to a tool being broken/having known issues, should be, change compiler, as opposed to just using a non-broken version of the tool?
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165623156)
I'll still open a PR for discussion, as I don't think our reponse to a tool being broken/having known issues, should be, change compiler, as opposed to just using a non-broken version of the tool?
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165625114)
I guess we'd also have to document that (some versions of?) Valgrind can't be used under Clang?
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165625114)
I guess we'd also have to document that (some versions of?) Valgrind can't be used under Clang?
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165632857)
> I guess we'd also have to document that (some versions of?) Valgrind can't be used under Clang?
Sure if you think it is useful, but that seems unrelated to the CI system.
> I'll still open a PR for discussion
I am not a fan of starting to build our own package manager, unless it is for a temporary workaround. And if something is a temporary workaround, one might as well pick the one that is the least effort to implement/revert/test/maintain.
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165632857)
> I guess we'd also have to document that (some versions of?) Valgrind can't be used under Clang?
Sure if you think it is useful, but that seems unrelated to the CI system.
> I'll still open a PR for discussion
I am not a fan of starting to build our own package manager, unless it is for a temporary workaround. And if something is a temporary workaround, one might as well pick the one that is the least effort to implement/revert/test/maintain.
💬 MarcoFalke commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1507099435)
> > Reproducible
>
> 100%. Running `time FILE_ENV="./ci/test/00_setup_env_native_tsan.sh" ./ci/test_run_all.sh` on a aarch64 machine running Fedora 37.
Idk. Locally this fails for me when building bdb:
```
Extracting bdb...
/root/bitcoin-core/depends/sources/db-4.8.30.NC.tar.gz: OK
Preprocessing bdb...
Configuring bdb...
patching file dbinc/atomic.h
patching file mp/mp_fget.c
patching file mp/mp_mvcc.c
patching file mp/mp_region.c
patching file mutex/mut_method.c
patching file
...
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1507099435)
> > Reproducible
>
> 100%. Running `time FILE_ENV="./ci/test/00_setup_env_native_tsan.sh" ./ci/test_run_all.sh` on a aarch64 machine running Fedora 37.
Idk. Locally this fails for me when building bdb:
```
Extracting bdb...
/root/bitcoin-core/depends/sources/db-4.8.30.NC.tar.gz: OK
Preprocessing bdb...
Configuring bdb...
patching file dbinc/atomic.h
patching file mp/mp_fget.c
patching file mp/mp_mvcc.c
patching file mp/mp_region.c
patching file mutex/mut_method.c
patching file
...
💬 fanquake commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1507100965)
> Idk. Locally this fails for me when building bdb:
Yea. You'll have to use `NO_BDB=1`, or fix BDB.
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1507100965)
> Idk. Locally this fails for me when building bdb:
Yea. You'll have to use `NO_BDB=1`, or fix BDB.
💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165654729)
> I am not a fan of starting to build our own package manager,
Fair enough. I agree that having to maintain too many things oursevles is not idea, but I think I'm starting to move the other way, where, I'd be happy to maintain the 3 lines of bash (in the CI system) required to compile and use a newer version of Valgrind, and keep compatibility with both of the compilers we use to build releases, as opposed to dropping the ability to test under both, for the sake of conviniently being able to
...
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165654729)
> I am not a fan of starting to build our own package manager,
Fair enough. I agree that having to maintain too many things oursevles is not idea, but I think I'm starting to move the other way, where, I'd be happy to maintain the 3 lines of bash (in the CI system) required to compile and use a newer version of Valgrind, and keep compatibility with both of the compilers we use to build releases, as opposed to dropping the ability to test under both, for the sake of conviniently being able to
...
👍 pinheadmz approved a pull request: "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs"
(https://github.com/bitcoin/bitcoin/pull/27231#pullrequestreview-1383617680)
ACK 5991eddb98ce59b883ae695752d4e90f32d43960
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 5991eddb98ce59b883ae695752d4e90f32d43960
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQ4FuQACgkQ5+KYS2KJ
yTqVKQ//TIOVSamur/OBGsHB1lgx9C3wyqQFSg+RzImVrDvquEhTDdwfjWI56DzL
sYWxaSDjHLP+4jYK7x3O4QoZJW1iJDlz56PhakxtDSzWMIr0WIfHGpTV6oH1p4Uh
Kaz57R6Y71G4ADMqeR48MqLK/B7vMuEKhzUUmGk14O/8bPUHk7HJE70b5F3zOTb9
EBu/NtFd
...
(https://github.com/bitcoin/bitcoin/pull/27231#pullrequestreview-1383617680)
ACK 5991eddb98ce59b883ae695752d4e90f32d43960
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 5991eddb98ce59b883ae695752d4e90f32d43960
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQ4FuQACgkQ5+KYS2KJ
yTqVKQ//TIOVSamur/OBGsHB1lgx9C3wyqQFSg+RzImVrDvquEhTDdwfjWI56DzL
sYWxaSDjHLP+4jYK7x3O4QoZJW1iJDlz56PhakxtDSzWMIr0WIfHGpTV6oH1p4Uh
Kaz57R6Y71G4ADMqeR48MqLK/B7vMuEKhzUUmGk14O/8bPUHk7HJE70b5F3zOTb9
EBu/NtFd
...
💬 hebasto commented on pull request "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings":
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1507142439)
> > Maybe add a check to configure.ac whether ignoring of -Wgnu-zero-variadic-macro-arguments is [#26916 (comment)](https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1400156218) required?
>
> Sounds good, but I have no idea how to do this with `configure.ac`...
Something like in this [branch](https://github.com/hebasto/bitcoin/commits/230413-variadic):
```diff
--- a/configure.ac
+++ b/configure.ac
@@ -470,6 +470,19 @@ if test "$CXXFLAGS_overridden" = "no"; then
if test "
...
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1507142439)
> > Maybe add a check to configure.ac whether ignoring of -Wgnu-zero-variadic-macro-arguments is [#26916 (comment)](https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1400156218) required?
>
> Sounds good, but I have no idea how to do this with `configure.ac`...
Something like in this [branch](https://github.com/hebasto/bitcoin/commits/230413-variadic):
```diff
--- a/configure.ac
+++ b/configure.ac
@@ -470,6 +470,19 @@ if test "$CXXFLAGS_overridden" = "no"; then
if test "
...
💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1507152541)
rebase to: 58fba35e25c2b9dcdd5b3dd0aa8f412801a98146
- added `nodiscard`
- cleaned up help message in `getaddressinfo`
- added release note file
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1507152541)
rebase to: 58fba35e25c2b9dcdd5b3dd0aa8f412801a98146
- added `nodiscard`
- cleaned up help message in `getaddressinfo`
- added release note file
🤔 vasild reviewed a pull request: "net: support unix domain sockets for -proxy and -onion"
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1382786000)
Concept ACK, this would be nice to have.
I do not like that the current approach expands `enum Network` and `CNetAddr` with the UNIX type, making it first-class-citizen network that has to be handled all over the networking code, whereas it is only needed to connect to the proxy. It is not like this PR adds support for connecting to other Bitcoin nodes over UNIX sockets (using the P2P protocol). This creates meaningless situations like having a `CService` on a UNIX socket (it is meaningless b
...
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1382786000)
Concept ACK, this would be nice to have.
I do not like that the current approach expands `enum Network` and `CNetAddr` with the UNIX type, making it first-class-citizen network that has to be handled all over the networking code, whereas it is only needed to connect to the proxy. It is not like this PR adds support for connecting to other Bitcoin nodes over UNIX sockets (using the P2P protocol). This creates meaningless situations like having a `CService` on a UNIX socket (it is meaningless b
...
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165198178)
This looks wrong as `paddrun` is a pointer (`sizeof(paddrun)` will be always 8 bytes on 64 bit computers). According to https://www.man7.org/linux/man-pages/man7/unix.7.html it should be:
```cpp
offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1
```
FreeBSD has this, defined in `sys/un.h`:
```cpp
#define SUN_LEN(su) \
(sizeof(*(su)) - sizeof((su)->sun_path) + strlen((su)->sun_path))
```
and Linux has:
```cpp
/* Evaluate to actual length of the `sockaddr_u
...
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165198178)
This looks wrong as `paddrun` is a pointer (`sizeof(paddrun)` will be always 8 bytes on 64 bit computers). According to https://www.man7.org/linux/man-pages/man7/unix.7.html it should be:
```cpp
offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1
```
FreeBSD has this, defined in `sys/un.h`:
```cpp
#define SUN_LEN(su) \
(sizeof(*(su)) - sizeof((su)->sun_path) + strlen((su)->sun_path))
```
and Linux has:
```cpp
/* Evaluate to actual length of the `sockaddr_u
...
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165186109)
This is dangerous. Better check the size before copying (untested):
```suggestion
const std::string path{fs::PathToString(m_path)};
if (sizeof(paddrun->sun_path) < path.length() + 1) {
return false;
}
memcpy(paddrun->sun_path, path.c_str(), path.length() + 1);
```
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165186109)
This is dangerous. Better check the size before copying (untested):
```suggestion
const std::string path{fs::PathToString(m_path)};
if (sizeof(paddrun->sun_path) < path.length() + 1) {
return false;
}
memcpy(paddrun->sun_path, path.c_str(), path.length() + 1);
```
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165266423)
It is a bit odd to support `unix:/path/to/socket` in `Lookup()` because there is nothing to lookup in that and it is not like the other inputs supported by `Lookup()` begin with the socket type, e.g. `ipv4://1.2.3.4`.
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165266423)
It is a bit odd to support `unix:/path/to/socket` in `Lookup()` because there is nothing to lookup in that and it is not like the other inputs supported by `Lookup()` begin with the socket type, e.g. `ipv4://1.2.3.4`.