💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2103408818)
+1 `bitcoin-test-ipc` sounds good to me
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2103408818)
+1 `bitcoin-test-ipc` sounds good to me
💬 hebasto commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2902589733)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2902589733)
Concept ACK.
📝 achow101 opened a pull request: "Getaddrinfo normalized parent"
(https://github.com/bitcoin/bitcoin/pull/32594)
Instead of prividing the descriptor string as stored in the db, use the normalized descriptor as is done for getaddressinfo's parent_desc field.
Split from #32489
(https://github.com/bitcoin/bitcoin/pull/32594)
Instead of prividing the descriptor string as stored in the db, use the normalized descriptor as is done for getaddressinfo's parent_desc field.
Split from #32489
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2103415625)
I removed the `batch` argument is having the caller provide that is entirely unnecessary. It's been replaced with `bool persist` in `LockCoin`, and no extra argument is needed for `UnlockCoin`.
Also split into #32593
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2103415625)
I removed the `batch` argument is having the caller provide that is entirely unnecessary. It's been replaced with `bool persist` in `LockCoin`, and no extra argument is needed for `UnlockCoin`.
Also split into #32593
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2103415859)
Added a test.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2103415859)
Added a test.
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-2902602828)
Split the `LockCoin` commit into #3259, and the `parent_descs` to #32594
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-2902602828)
Split the `LockCoin` commit into #3259, and the `parent_descs` to #32594
📝 achow101 converted_to_draft a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489)
Currently, if a user wants to use an airgapped setup, they need to manually create the watchonly wallet that will live on the online node by importing the public descriptors. This PR introduces `exportwatchonlywallet` which will create a wallet file with the public descriptors to avoid exposing the specific internals to the user. Additionally, this RPC will copy any existing labels, transactions, and wallet flags. This ensures that the exported watchonly wallet is almost entirely a copy of the o
...
(https://github.com/bitcoin/bitcoin/pull/32489)
Currently, if a user wants to use an airgapped setup, they need to manually create the watchonly wallet that will live on the online node by importing the public descriptors. This PR introduces `exportwatchonlywallet` which will create a wallet file with the public descriptors to avoid exposing the specific internals to the user. Additionally, this RPC will copy any existing labels, transactions, and wallet flags. This ensures that the exported watchonly wallet is almost entirely a copy of the o
...
💬 davidgumberg commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2103417889)
Dumb question: Is there a reason why the order was changed here between `HasThreads()` and `fSCriptChecks`? Is it because `HasThreads()` is cheap and the line is more readible with the check next to the initializer?
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2103417889)
Dumb question: Is there a reason why the order was changed here between `HasThreads()` and `fSCriptChecks`? Is it because `HasThreads()` is cheap and the line is more readible with the check next to the initializer?
📝 willcl-ark opened a pull request: "build: add a depends dependency provider"
(https://github.com/bitcoin/bitcoin/pull/32595)
Fixes: #32428
This PR adds a [dependency provider](https://cmake.org/cmake/help/latest/guide/using-dependencies/index.html#dependency-providers) to depends builds.
A dependency provider allows overriding of `cmake`'s `find_package()` therefore giving total control over where dependencies come from in a build.
This achieves two things:
1. Provides stronger guarantees about where dependencies come from during a (depends) build; i.e. _only_ from depends.
2. Fixes issues like a non-sta
...
(https://github.com/bitcoin/bitcoin/pull/32595)
Fixes: #32428
This PR adds a [dependency provider](https://cmake.org/cmake/help/latest/guide/using-dependencies/index.html#dependency-providers) to depends builds.
A dependency provider allows overriding of `cmake`'s `find_package()` therefore giving total control over where dependencies come from in a build.
This achieves two things:
1. Provides stronger guarantees about where dependencies come from during a (depends) build; i.e. _only_ from depends.
2. Fixes issues like a non-sta
...
💬 theuni commented on pull request "refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#discussion_r2103422435)
I think this should go a little further. At this point, `CheckInputScripts` is essentially meant to be a mempool function and `PrepareInputScriptChecks` is for validation, except in the weird edge-case when we're single-threaded.
I think I'd rather just see this go all the way and make the split purposeful. Something like:
```patch
diff --git a/src/validation.cpp b/src/validation.cpp
index 1c81620889f..d9a447b460f 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2723,19 +27
...
(https://github.com/bitcoin/bitcoin/pull/32575#discussion_r2103422435)
I think this should go a little further. At this point, `CheckInputScripts` is essentially meant to be a mempool function and `PrepareInputScriptChecks` is for validation, except in the weird edge-case when we're single-threaded.
I think I'd rather just see this go all the way and make the split purposeful. Something like:
```patch
diff --git a/src/validation.cpp b/src/validation.cpp
index 1c81620889f..d9a447b460f 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2723,19 +27
...
💬 willcl-ark commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2902613460)
Opened in draft for feedback on approach.
There is also a bypass for the boost package (in _cmake/module/AddBoostIfNeeded.cmake_) which I'd like to get working with the dependency provider ideally.
Note that this also bumps the minimum cmake version to 3.24.
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2902613460)
Opened in draft for feedback on approach.
There is also a bypass for the boost package (in _cmake/module/AddBoostIfNeeded.cmake_) which I'd like to get working with the dependency provider ideally.
Note that this also bumps the minimum cmake version to 3.24.
💬 hebasto commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2902616029)
cc @purpleKarrot
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2902616029)
cc @purpleKarrot
💬 davidgumberg commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2902617708)
post-merge crACK https://github.com/bitcoin/bitcoin/commit/fd290730f530a8b76a9607392f49830697cdd7c5
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2902617708)
post-merge crACK https://github.com/bitcoin/bitcoin/commit/fd290730f530a8b76a9607392f49830697cdd7c5
💬 fanquake commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2902638382)
> Provides stronger guarantees about where dependencies come from during a (depends) build; i.e. only from depends.
Can you elaborate on this? Currently, CMake configured with a depends toolchain, should never look outside depends (doing so would be a bug). What are the stronger guarantees?
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2902638382)
> Provides stronger guarantees about where dependencies come from during a (depends) build; i.e. only from depends.
Can you elaborate on this? Currently, CMake configured with a depends toolchain, should never look outside depends (doing so would be a bug). What are the stronger guarantees?
💬 mzumsande commented on pull request "refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#discussion_r2103434017)
Just a rough idea, but did you consider the alternative approach of doing the `pvChecks` population not here (but in `ConnectBlock()` or a separate function), so that `CheckInputScripts` could also call `PrepareInputScriptChecks` and there would be no duplication of the coinbase check, cache check and `TxData` init?
(https://github.com/bitcoin/bitcoin/pull/32575#discussion_r2103434017)
Just a rough idea, but did you consider the alternative approach of doing the `pvChecks` population not here (but in `ConnectBlock()` or a separate function), so that `CheckInputScripts` could also call `PrepareInputScriptChecks` and there would be no duplication of the coinbase check, cache check and `TxData` init?
💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2103446971)
Yep, no reason other than I thought it was more readable that way.
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2103446971)
Yep, no reason other than I thought it was more readable that way.
💬 willcl-ark commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2902759379)
> > Provides stronger guarantees about where dependencies come from during a (depends) build; i.e. only from depends.
>
> Can you elaborate on this? Currently, CMake configured with a depends toolchain, should never look outside depends (doing so would be a bug). What are the stronger guarantees?
Right, perhaps I didn't use the best wording here.
When using the toolchain I _have not_ found us pulling in extraneous deps from outside of depends, but _have_ encountered us missing dependenc
...
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2902759379)
> > Provides stronger guarantees about where dependencies come from during a (depends) build; i.e. only from depends.
>
> Can you elaborate on this? Currently, CMake configured with a depends toolchain, should never look outside depends (doing so would be a bug). What are the stronger guarantees?
Right, perhaps I didn't use the best wording here.
When using the toolchain I _have not_ found us pulling in extraneous deps from outside of depends, but _have_ encountered us missing dependenc
...
📝 theStack opened a pull request: "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs"
(https://github.com/bitcoin/bitcoin/pull/32596)
This PR contains a few smaller wallet RPC cleanups based on that we only ever operate on descriptor wallets now:
* remove the now obsolete "keypoololdest" field from the `getwalletinfo` RPC and the corresponding CWallet/ScriptPubKeyMan methods
* in RPCs where potential fast wallet rescan is documented, remove the "descriptor wallet" mentions (back then introduced in commit ca48a4694f73e5be8f971ae482ebc2cce4caef44, PR #25957)
* for the `createwallet` RPC examples, remove the "descriptors" para
...
(https://github.com/bitcoin/bitcoin/pull/32596)
This PR contains a few smaller wallet RPC cleanups based on that we only ever operate on descriptor wallets now:
* remove the now obsolete "keypoololdest" field from the `getwalletinfo` RPC and the corresponding CWallet/ScriptPubKeyMan methods
* in RPCs where potential fast wallet rescan is documented, remove the "descriptor wallet" mentions (back then introduced in commit ca48a4694f73e5be8f971ae482ebc2cce4caef44, PR #25957)
* for the `createwallet` RPC examples, remove the "descriptors" para
...
💬 mzumsande commented on pull request "refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#discussion_r2103526405)
I like that idea.
I think it would be a small change in behaviour, because if I read [this line](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/validation.cpp#L2248) right it will add entries to the cache if
1)`pvChecks` is not set (single-threaded)
2) `cacheFullScriptStore` is set (i.e. if `ConnectBlock()` was called with `fJustCheck`, so currently only from `TestBlockValidity`).
I wonder if the current behavior is intended - it seems very strange
...
(https://github.com/bitcoin/bitcoin/pull/32575#discussion_r2103526405)
I like that idea.
I think it would be a small change in behaviour, because if I read [this line](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/validation.cpp#L2248) right it will add entries to the cache if
1)`pvChecks` is not set (single-threaded)
2) `cacheFullScriptStore` is set (i.e. if `ConnectBlock()` was called with `fJustCheck`, so currently only from `TestBlockValidity`).
I wonder if the current behavior is intended - it seems very strange
...
💬 davidgumberg commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#discussion_r2103549376)
In commit https://github.com/bitcoin/bitcoin/commit/832c57a53410870471a52647ce107454de3bc69c (_thread-safety: modernize thread safety macros_)
----
Feel free to disregard, I am just trying to understand the motivation of this commit:
> unlock_function actually maps to release_generic_capability, which does not
work properly when implementing a scoped unlocker.
Looking at the example code in clang's [documentation](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#mutex-h):
`
...
(https://github.com/bitcoin/bitcoin/pull/32465#discussion_r2103549376)
In commit https://github.com/bitcoin/bitcoin/commit/832c57a53410870471a52647ce107454de3bc69c (_thread-safety: modernize thread safety macros_)
----
Feel free to disregard, I am just trying to understand the motivation of this commit:
> unlock_function actually maps to release_generic_capability, which does not
work properly when implementing a scoped unlocker.
Looking at the example code in clang's [documentation](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#mutex-h):
`
...