💬 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
}
```
👍 dergoegge approved a pull request: "fuzz: BIP 42, BIP 30, CVE-2018-17144"
(https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1414856320)
Code review ACK fa2d8b61f9343d350b67357a12f39b613c8ee8ad
(https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1414856320)
Code review ACK fa2d8b61f9343d350b67357a12f39b613c8ee8ad
👍 pablomartin4btc approved a pull request: "Wallet : Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722#pullrequestreview-1414865992)
re-ACK 78660e72001a2561c7ad3026367a69f65414dbd9. cc @jarolrod.
I see[`wallet_create_tx.py --descriptors`](https://cirrus-ci.com/task/6164029874372608?logs=functional_tests#L2752)test is failing and you are re-running it, I don't think it's related with your change but let's see.
(https://github.com/bitcoin-core/gui/pull/722#pullrequestreview-1414865992)
re-ACK 78660e72001a2561c7ad3026367a69f65414dbd9. cc @jarolrod.
I see[`wallet_create_tx.py --descriptors`](https://cirrus-ci.com/task/6164029874372608?logs=functional_tests#L2752)test is failing and you are re-running it, I don't think it's related with your change but let's see.
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1536319288)
Thank you for the review @MarcoFalke and @ryanofsky
Updated 9033dc6827cc623f1f7176fde120229f36ff5e74 -> e056d6f758382d3418682095ab02f8d487aa641f ([removeBlockstorageArgs_19](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_19) -> [removeBlockstorageArgs_20](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_20), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_19..removeBlockstorageArgs_20))
* Dropped the commit changing t
...
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1536319288)
Thank you for the review @MarcoFalke and @ryanofsky
Updated 9033dc6827cc623f1f7176fde120229f36ff5e74 -> e056d6f758382d3418682095ab02f8d487aa641f ([removeBlockstorageArgs_19](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_19) -> [removeBlockstorageArgs_20](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_20), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_19..removeBlockstorageArgs_20))
* Dropped the commit changing t
...
💬 RandyMcMillan commented on issue "deadlock with wrong system date/time and non-empty wallet":
(https://github.com/bitcoin-core/gui/issues/643#issuecomment-1536322254)
I vaguely remember finding a solution to this... I will try to find it - buried in a comment somewhere in bitcoin/bitcoin issues.
If I recall - the fix had to do with (re)casting the time back to int when passing it to the gui. The gui wasnt handling the chronos? variable correctly - it was likely a bug in the QT gui framework itself. I wasnt able to "prove" it because the crash didnt produce a stack error (for the gui) - but it worked. :)
I will try repost the fix with some tests to prov
...
(https://github.com/bitcoin-core/gui/issues/643#issuecomment-1536322254)
I vaguely remember finding a solution to this... I will try to find it - buried in a comment somewhere in bitcoin/bitcoin issues.
If I recall - the fix had to do with (re)casting the time back to int when passing it to the gui. The gui wasnt handling the chronos? variable correctly - it was likely a bug in the QT gui framework itself. I wasnt able to "prove" it because the crash didnt produce a stack error (for the gui) - but it worked. :)
I will try repost the fix with some tests to prov
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin-gui -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-1536369001)
Thanks Sjors, I'm trying to reproduce the shutdown error but didn't succeed yet. Will try again with a mainnet node. I didn't look into the external signer bug yet, but maybe that is more straightforward. In both cases running with -debug=ipc could make it clearer what's going on, especially in the shutdown case. Probably what is happening in that case is that some thread is trying make an IPC call after the socket is closed, so an ipc::Exception is raised which is uncaught. Fix might be synchro
...
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-1536369001)
Thanks Sjors, I'm trying to reproduce the shutdown error but didn't succeed yet. Will try again with a mainnet node. I didn't look into the external signer bug yet, but maybe that is more straightforward. In both cases running with -debug=ipc could make it clearer what's going on, especially in the shutdown case. Probably what is happening in that case is that some thread is trying make an IPC call after the socket is closed, so an ipc::Exception is raised which is uncaught. Fix might be synchro
...
💬 jamesob commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536375432)
It seems inevitable to me that well-coordinated "third party" mempools will emerge which eschew all policy and use out-of-band payment as the only means of DoS resistance. Seems likely these sources will be paid attention by large mining pools. I'm sympathetic to the motivations behind this change on those grounds, as well as with the general frustration around the sharp edges of policy (e.g. pinning).
The notion of having to lobby a council, however qualified, to "accept a usecase" is also u
...
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536375432)
It seems inevitable to me that well-coordinated "third party" mempools will emerge which eschew all policy and use out-of-band payment as the only means of DoS resistance. Seems likely these sources will be paid attention by large mining pools. I'm sympathetic to the motivations behind this change on those grounds, as well as with the general frustration around the sharp edges of policy (e.g. pinning).
The notion of having to lobby a council, however qualified, to "accept a usecase" is also u
...
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1186200624)
unrelated: The following diff compiles for me:
```diff
diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h
index 2395f60164..dd75985332 100644
--- a/src/kernel/chainstatemanager_opts.h
+++ b/src/kernel/chainstatemanager_opts.h
@@ -29,7 +29,7 @@ namespace kernel {
*/
struct ChainstateManagerOpts {
const CChainParams& chainparams;
- fs::path datadir;
+ const fs::path datadir;
const std::function<NodeClock::time_point()> adjusted_t
...
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1186200624)
unrelated: The following diff compiles for me:
```diff
diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h
index 2395f60164..dd75985332 100644
--- a/src/kernel/chainstatemanager_opts.h
+++ b/src/kernel/chainstatemanager_opts.h
@@ -29,7 +29,7 @@ namespace kernel {
*/
struct ChainstateManagerOpts {
const CChainParams& chainparams;
- fs::path datadir;
+ const fs::path datadir;
const std::function<NodeClock::time_point()> adjusted_t
...
🤔 pablomartin4btc reviewed a pull request: "net: Continuous ASMap health check"
(https://github.com/bitcoin/bitcoin/pull/27581#pullrequestreview-1414971325)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27581#pullrequestreview-1414971325)
Concept ACK.
💬 pablomartin4btc commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186205062)
I think you need to declare `addr` as `const` (`tidy`CI test is failing due to that).
```suggestion
for (const auto &addr : clearnet_addrs) {
```
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186205062)
I think you need to declare `addr` as `const` (`tidy`CI test is failing due to that).
```suggestion
for (const auto &addr : clearnet_addrs) {
```
👍 MarcoFalke approved a pull request: "doc: Add post branch-off note about fuzz input pruning"
(https://github.com/bitcoin/bitcoin/pull/27574#pullrequestreview-1414977410)
lgtm
(https://github.com/bitcoin/bitcoin/pull/27574#pullrequestreview-1414977410)
lgtm
🤔 stickies-v reviewed a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1414950382)
Would it be possible to provide instructions on replicating how to make this fail without`sys.path.append(tests_dir)`? Often times, messing with `system.path` is treating the symptoms when actually the package structure needs to be improved. May not be true in this case, though.
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1414950382)
Would it be possible to provide instructions on replicating how to make this fail without`sys.path.append(tests_dir)`? Often times, messing with `system.path` is treating the symptoms when actually the package structure needs to be improved. May not be true in this case, though.
💬 stickies-v commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1186191720)
Found two more instances in test_runner.py:
```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 0c9a7173c..d495615fc 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -412,7 +412,7 @@ def main():
# Read config generated by configure.
config = configparser.ConfigParser()
- configfile = os.path.abspath(os.path.dirname(__file__)) + "/../config.ini"
+ configfile = os.path.abspath(os.path.join(os.p
...
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1186191720)
Found two more instances in test_runner.py:
```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 0c9a7173c..d495615fc 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -412,7 +412,7 @@ def main():
# Read config generated by configure.
config = configparser.ConfigParser()
- configfile = os.path.abspath(os.path.dirname(__file__)) + "/../config.ini"
+ configfile = os.path.abspath(os.path.join(os.p
...