💬 furszy commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1186063927)
> Afaict this won't work here, because the thread might not finish and then the pool would just get clogged (see comment about abandoning the thread).
k, `getaddrinfo` doesn't have a timeout and `getaddrinfo_a` seems to have a segfault. Cool..
> Performance should also not really matter here since this doesn't happen that often and startup time for a thread vs. time of a DNS lookup is probably negligible.
I was thinking more about context switching. Isn't this used for `OpenNetworkConne
...
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1186063927)
> Afaict this won't work here, because the thread might not finish and then the pool would just get clogged (see comment about abandoning the thread).
k, `getaddrinfo` doesn't have a timeout and `getaddrinfo_a` seems to have a segfault. Cool..
> Performance should also not really matter here since this doesn't happen that often and startup time for a thread vs. time of a DNS lookup is probably negligible.
I was thinking more about context switching. Isn't this used for `OpenNetworkConne
...
💬 sipa commented on issue "[Linux] Add wayland support":
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536232647)
Is it the case that our `bitcion-qt` release binaries don't work on Wayland at all? If so, that should be fixed, I think. Many distributions are increasingly migrating away from X.
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536232647)
Is it the case that our `bitcion-qt` release binaries don't work on Wayland at all? If so, that should be fixed, I think. Many distributions are increasingly migrating away from X.
👍 stickies-v approved a pull request: "test: Treat `bitcoin-wallet` binary in the same way as others"
(https://github.com/bitcoin/bitcoin/pull/27554#pullrequestreview-1414754734)
re-ACK f6d7636be4eb0b19878428906dd5e394df7d07a2
(https://github.com/bitcoin/bitcoin/pull/27554#pullrequestreview-1414754734)
re-ACK f6d7636be4eb0b19878428906dd5e394df7d07a2
💬 hebasto commented on issue "[Linux] Add wayland support":
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536240176)
> Is it the case that our `bitcion-qt` release binaries don't work on Wayland at all? If so, that should be fixed, I think. Many distributions are increasingly migrating away from X.
Unfortunately, https://github.com/bitcoin/bitcoin/pull/22708 did not find enough support. The main concerns [were](https://github.com/bitcoin/bitcoin/pull/22708#issuecomment-1100861599):
> this change introduces a second Linux graphics backend to support, along with more packages in depends, new runtime dependen
...
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536240176)
> Is it the case that our `bitcion-qt` release binaries don't work on Wayland at all? If so, that should be fixed, I think. Many distributions are increasingly migrating away from X.
Unfortunately, https://github.com/bitcoin/bitcoin/pull/22708 did not find enough support. The main concerns [were](https://github.com/bitcoin/bitcoin/pull/22708#issuecomment-1100861599):
> this change introduces a second Linux graphics backend to support, along with more packages in depends, new runtime dependen
...
💬 dergoegge commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1186077225)
> k, getaddrinfo doesn't have a timeout and getaddrinfo_a seems to have a segfault. Cool..
Yea one would think that making DNS requests in c++ would be a solved problem.
Anyway, I'm also not sure about the approach here. The original issue was that we would block indefinitely while joining the DNS thread on shutdown (only happens if the user's system is broken I think?). I suggested to detach the dns thread if it doesn't join in a reasonable amount of time (would be fine, the OS will clean
...
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1186077225)
> k, getaddrinfo doesn't have a timeout and getaddrinfo_a seems to have a segfault. Cool..
Yea one would think that making DNS requests in c++ would be a solved problem.
Anyway, I'm also not sure about the approach here. The original issue was that we would block indefinitely while joining the DNS thread on shutdown (only happens if the user's system is broken I think?). I suggested to detach the dns thread if it doesn't join in a reasonable amount of time (would be fine, the OS will clean
...
💬 RandyMcMillan commented on pull request "util: Use steady clock instead of system clock to measure durations":
(https://github.com/bitcoin/bitcoin/pull/27405#issuecomment-1536244850)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27405#issuecomment-1536244850)
Concept ACK
💬 fanquake commented on issue "TSAN: lock-order-inversion (potential deadlock) in ZapSelectTx test":
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1536245439)
> Is this reproducible?
Has happened 2/2 runs so far.
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1536245439)
> Is this reproducible?
Has happened 2/2 runs so far.
🚀 fanquake merged a pull request: "test, init: perturb file to ensure failure instead of only deleting them"
(https://github.com/bitcoin/bitcoin/pull/26653)
(https://github.com/bitcoin/bitcoin/pull/26653)
💬 dergoegge commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186081315)
I think this belongs in `net.cpp` and shouldn't use the block height.
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186081315)
I think this belongs in `net.cpp` and shouldn't use the block height.
💬 dergoegge commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186082093)
(ignore if you were going to change this once out of draft anyway)
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186082093)
(ignore if you were going to change this once out of draft anyway)
👍 stickies-v approved a pull request: "util: improve FindByte() performance"
(https://github.com/bitcoin/bitcoin/pull/19690#pullrequestreview-1414773712)
re-ACK 72efc26439da9a1344a19569fb0cab01f82ae7d1
Verified that the only difference is to include `<util/fs.h>` instead of `<fs.h>` (from https://github.com/bitcoin/bitcoin/pull/27254/commits/00e9b97f37e0bdf4c647236838c10b68b7ad5be3)
```range-diff
% git range-diff HEAD~2 0fe832c4a4b2049fdf967bca375468d5ac285563 HEAD
1: 5842d92c8 ! 1: 604df63f6 [bench] add streams findbyte
@@ src/bench/streams_findbyte.cpp (new)
+
+#include <bench/bench.h>
+
-+#include <fs.h>
...
(https://github.com/bitcoin/bitcoin/pull/19690#pullrequestreview-1414773712)
re-ACK 72efc26439da9a1344a19569fb0cab01f82ae7d1
Verified that the only difference is to include `<util/fs.h>` instead of `<fs.h>` (from https://github.com/bitcoin/bitcoin/pull/27254/commits/00e9b97f37e0bdf4c647236838c10b68b7ad5be3)
```range-diff
% git range-diff HEAD~2 0fe832c4a4b2049fdf967bca375468d5ac285563 HEAD
1: 5842d92c8 ! 1: 604df63f6 [bench] add streams findbyte
@@ src/bench/streams_findbyte.cpp (new)
+
+#include <bench/bench.h>
+
-+#include <fs.h>
...
⚠️ Sjors opened an issue: "psbt: set global_xpubs (at least for multisig descriptors)"
(https://github.com/bitcoin/bitcoin/issues/27583)
### Please describe the feature you'd like to see added.
The `walletcreatefundedpsbt`, `walletprocesspsbt` and `send`* RPC, as well as the send dialog* in the GUI should populate the `PSBT_GLOBAL_XPUB` field (defined in [BIP 174](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki)).
At least when used in a multisig context, e.g. when spending from a `multi()` descriptor.
The Ledger Bitcoin app enforces this as of version 2.1.1., see https://github.com/bitcoin-core/HWI/issues/6
...
(https://github.com/bitcoin/bitcoin/issues/27583)
### Please describe the feature you'd like to see added.
The `walletcreatefundedpsbt`, `walletprocesspsbt` and `send`* RPC, as well as the send dialog* in the GUI should populate the `PSBT_GLOBAL_XPUB` field (defined in [BIP 174](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki)).
At least when used in a multisig context, e.g. when spending from a `multi()` descriptor.
The Ledger Bitcoin app enforces this as of version 2.1.1., see https://github.com/bitcoin-core/HWI/issues/6
...
💬 MarcoFalke commented on issue "TSAN: lock-order-inversion (potential deadlock) in ZapSelectTx test":
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1536249866)
Ok, then it would be nice to try outside the CI env, starting with the same configure flags
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1536249866)
Ok, then it would be nice to try outside the CI env, starting with the same configure flags
💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186087445)
I wasn't happy with the location myself but I needed more time to think about where it would fit in best, so happy to take suggestions. I just tried to put it somewhere where it requires the least amount of new dependencies and shipped it to get conceptual feedback.
> and shouldn't use the block height.
What do you suggest to use instead of block height?
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186087445)
I wasn't happy with the location myself but I needed more time to think about where it would fit in best, so happy to take suggestions. I just tried to put it somewhere where it requires the least amount of new dependencies and shipped it to get conceptual feedback.
> and shouldn't use the block height.
What do you suggest to use instead of block height?
💬 Sjors commented on issue "psbt: set global_xpubs (at least for multisig descriptors)":
(https://github.com/bitcoin/bitcoin/issues/27583#issuecomment-1536254243)
See also this discussion: https://github.com/cryptoadvance/specter-desktop/issues/1886#issuecomment-1246963927
(https://github.com/bitcoin/bitcoin/issues/27583#issuecomment-1536254243)
See also this discussion: https://github.com/cryptoadvance/specter-desktop/issues/1886#issuecomment-1246963927
💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1186086311)
re: https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1185099728
In general it's ok to pass pointers and references. These are both really useful for output parameters so the IPC framework does support them. But the type needs to be serializable, and it isn't really possible to write serialization code for the `CBlockIndex` type because of the pprev pointers it contains. It compile error in #10102 to try to pass types that aren't serializable.
For this PR would suggest not adding a
...
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1186086311)
re: https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1185099728
In general it's ok to pass pointers and references. These are both really useful for output parameters so the IPC framework does support them. But the type needs to be serializable, and it isn't really possible to write serialization code for the `CBlockIndex` type because of the pprev pointers it contains. It compile error in #10102 to try to pass types that aren't serializable.
For this PR would suggest not adding a
...
💬 MarcoFalke commented on issue "[Linux] Add wayland support":
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536259204)
I don't really buy the concern. Even if this is adding additional depends in a gui/qt-only context, what is the downside? If this allows people to get a feature they want and there are people who maintain it? If someone doesn't use gui/qt, they should be unaffected by this.
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536259204)
I don't really buy the concern. Even if this is adding additional depends in a gui/qt-only context, what is the downside? If this allows people to get a feature they want and there are people who maintain it? If someone doesn't use gui/qt, they should be unaffected by this.
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1186093566)
Thanks for taking a look. It's also not used by an actual client of the interface, so I think I'll just drop the commit then. No need to needlessly pollute the interface.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1186093566)
Thanks for taking a look. It's also not used by an actual client of the interface, so I think I'll just drop the commit then. No need to needlessly pollute the interface.
💬 hebasto commented on issue "[Linux] Add wayland support":
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536263017)
OK. Reopened this issue first :)
(https://github.com/bitcoin/bitcoin/issues/19950#issuecomment-1536263017)
OK. Reopened this issue first :)
⚠️ hebasto reopened an issue: "[Linux] Add wayland support"
(https://github.com/bitcoin/bitcoin/issues/19950)
**Is your feature request related to a problem? Please describe.**
- Wayland provides extra security as apps can't have access to windows from other apps
- X apps looks blurry using fractional scaling on GNOME.
- X is mostly unmaintained, in favour of Wayland
**Describe the solution you'd like**
Add Wayland support to precompiled binaries.
**Additional context**
As far as I understand wayland is supported on the KDE 5.15 runtime. Currently running with `QT_QPA_PLATFORM="wayland"` res
...
(https://github.com/bitcoin/bitcoin/issues/19950)
**Is your feature request related to a problem? Please describe.**
- Wayland provides extra security as apps can't have access to windows from other apps
- X apps looks blurry using fractional scaling on GNOME.
- X is mostly unmaintained, in favour of Wayland
**Describe the solution you'd like**
Add Wayland support to precompiled binaries.
**Additional context**
As far as I understand wayland is supported on the KDE 5.15 runtime. Currently running with `QT_QPA_PLATFORM="wayland"` res
...
💬 dergoegge commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186101443)
> happy to take suggestions.
I think it would make sense for the `opencon` thread (`ThreadOpenConnections`) to handle this.
> What do you suggest to use instead of block height?
```c++
if (m_last_asmap_health_check + 24h < NodeClock::now()) {
// Do health check
}
```
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186101443)
> happy to take suggestions.
I think it would make sense for the `opencon` thread (`ThreadOpenConnections`) to handle this.
> What do you suggest to use instead of block height?
```c++
if (m_last_asmap_health_check + 24h < NodeClock::now()) {
// Do health check
}
```