💬 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):
`
...
💬 theStack commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2103562671)
That makes sense, thanks for clarifying (missed that the one-parameter variant was not `noexcept`). Might be worth it to add a small comment about this in a follow-up PR.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2103562671)
That makes sense, thanks for clarifying (missed that the one-parameter variant was not `noexcept`). Might be worth it to add a small comment about this in a follow-up PR.
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103574633)
Expanded the comment to say that.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103574633)
Expanded the comment to say that.
🤔 1440000bytes reviewed a pull request: "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs"
(https://github.com/bitcoin/bitcoin/pull/32596#pullrequestreview-2862873626)
ACK https://github.com/bitcoin/bitcoin/pull/32596/commits/e5cbea416b2f63e5d99819052f3e69a6383336d6
<details>
<summary>Signature</summary>
<pre>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK e5cbea416b2f63e5d99819052f3e69a6383336d6
-----BEGIN PGP SIGNATURE-----
iHUEARYKAB0WIQSSUwYqT5LWNFkIXKYtIwUgISpZAQUCaC++7QAKCRAtIwUgISpZ
AZIaAP9bxiLFrXtgDk7+yoGgHAUO2xP2AmG5qRcYtZDSxuqkmAEAotL4R3L9iDoX
NZeq+w5EEr+4SO7zLYo+Cnb7krWYpQg=
=pUtX
-----END PGP SIGNATURE-----
</pre>
</detai
...
(https://github.com/bitcoin/bitcoin/pull/32596#pullrequestreview-2862873626)
ACK https://github.com/bitcoin/bitcoin/pull/32596/commits/e5cbea416b2f63e5d99819052f3e69a6383336d6
<details>
<summary>Signature</summary>
<pre>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK e5cbea416b2f63e5d99819052f3e69a6383336d6
-----BEGIN PGP SIGNATURE-----
iHUEARYKAB0WIQSSUwYqT5LWNFkIXKYtIwUgISpZAQUCaC++7QAKCRAtIwUgISpZ
AZIaAP9bxiLFrXtgDk7+yoGgHAUO2xP2AmG5qRcYtZDSxuqkmAEAotL4R3L9iDoX
NZeq+w5EEr+4SO7zLYo+Cnb7krWYpQg=
=pUtX
-----END PGP SIGNATURE-----
</pre>
</detai
...
📝 achow101 opened a pull request: "wallet: Always set descriptor cache upgraded flag for new wallets"
(https://github.com/bitcoin/bitcoin/pull/32597)
Newly created wallets will always have an upgraded descriptor cache, so set those.
Also, to verify this behavior, add a new `flags` field to `getwalletinfo` and check that in the functional tests.
Split from #32489
(https://github.com/bitcoin/bitcoin/pull/32597)
Newly created wallets will always have an upgraded descriptor cache, so set those.
Also, to verify this behavior, add a new `flags` field to `getwalletinfo` and check that in the functional tests.
Split from #32489
📝 achow101 opened a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598)
Many exceptions thrown for corruption are `std::runtime_error`; we should catch those and log the message to help with debugging.
Split from #32489
(https://github.com/bitcoin/bitcoin/pull/32598)
Many exceptions thrown for corruption are `std::runtime_error`; we should catch those and log the message to help with debugging.
Split from #32489
💬 1440000bytes commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2903002368)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2903002368)
Concept ACK
💬 davidgumberg commented on pull request "wallet: Fix logging of wallet version":
(https://github.com/bitcoin/bitcoin/pull/32553#issuecomment-2903003935)
post-merge ACK https://github.com/bitcoin/bitcoin/pull/32553/commits/4b2cd0b41ff4800c8801f2c44883eaec60a035fa
(https://github.com/bitcoin/bitcoin/pull/32553#issuecomment-2903003935)
post-merge ACK https://github.com/bitcoin/bitcoin/pull/32553/commits/4b2cd0b41ff4800c8801f2c44883eaec60a035fa
💬 hebasto commented on issue "`system_tests/run_command` test fails on illumos OS":
(https://github.com/bitcoin/bitcoin/issues/32574#issuecomment-2903353573)
The underlying issue is not specific to illumos.
With the following patch:
```diff
--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(run_command)
const std::string expected{"err"};
BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
const std::string what(e.what());
- BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned
...
(https://github.com/bitcoin/bitcoin/issues/32574#issuecomment-2903353573)
The underlying issue is not specific to illumos.
With the following patch:
```diff
--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(run_command)
const std::string expected{"err"};
BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
const std::string what(e.what());
- BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned
...
💬 maflcko commented on pull request "rev_32343_wip_nomerge_ci":
(https://github.com/bitcoin/bitcoin/pull/32535#issuecomment-2903371304)
The failure remains, when triggered, so it would be good to investigate this. Let's continue discussion in https://github.com/bitcoin/bitcoin/issues/32524
(https://github.com/bitcoin/bitcoin/pull/32535#issuecomment-2903371304)
The failure remains, when triggered, so it would be good to investigate this. Let's continue discussion in https://github.com/bitcoin/bitcoin/issues/32524
✅ maflcko closed a pull request: "rev_32343_wip_nomerge_ci"
(https://github.com/bitcoin/bitcoin/pull/32535)
(https://github.com/bitcoin/bitcoin/pull/32535)
💬 maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2903373050)
This is closed, but the failure remains, when triggered, so it would be good to investigate this long-term at some point. Discussion can continue here, or in a new issue.
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2903373050)
This is closed, but the failure remains, when triggered, so it would be good to investigate this long-term at some point. Discussion can continue here, or in a new issue.