💬 davidgumberg commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103355444)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103355444)
Thanks, fixed.
👍 ismaelsadeeq approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2862510597)
fwiw my last review implied an ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Further improvement can come after this as mentioned in the description, and also since I could not and no one reproduced the freeze I encountered in the unclean shutdown it is not a blocker to this.
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2862510597)
fwiw my last review implied an ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Further improvement can come after this as mentioned in the description, and also since I could not and no one reproduced the freeze I encountered in the unclean shutdown it is not a blocker to this.
📝 achow101 opened a pull request: "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC"
(https://github.com/bitcoin/bitcoin/pull/32593)
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked c
...
(https://github.com/bitcoin/bitcoin/pull/32593)
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked c
...
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103368896)
> To be clear, Cluster::IsOversized() returns whether the Cluster is oversized in general.
huh, I missed thanks for the clarity.
I think you can add this comment as well or anything that would make this explicit and clearer will be appreciated.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103368896)
> To be clear, Cluster::IsOversized() returns whether the Cluster is oversized in general.
huh, I missed thanks for the clarity.
I think you can add this comment as well or anything that would make this explicit and clearer will be appreciated.
💬 fanquake commented on issue "seeds: seed.bitcoin.jonasschnelli.ch not returning results":
(https://github.com/bitcoin/bitcoin/issues/32590#issuecomment-2902578249)
Thanks mate.
(https://github.com/bitcoin/bitcoin/issues/32590#issuecomment-2902578249)
Thanks mate.
✅ fanquake closed an issue: "seeds: seed.bitcoin.jonasschnelli.ch not returning results"
(https://github.com/bitcoin/bitcoin/issues/32590)
(https://github.com/bitcoin/bitcoin/issues/32590)
💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2103406828)
this looks much better, I will review again soon.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2103406828)
this looks much better, I will review again soon.
💬 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