💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2305675757)
Is the the check for the presence of private keys in a wallet where `WALLET_FLAG_DISABLE_PRIVATE_KEYS` is set also unnecessary?

💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2305694612)
Fixed the suggestion as I understood it, thanks for catching this.

💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2305695062)
Thanks for catching this, fixed.

💬 ajtowns commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#issuecomment-3230638657)
Would it be better to return an object, eg:

```json
{
"background": {
"blocks": 100000,
"bestblockhash": "000000000003ba27aa200b1cecaad478d2b00432346c3f1f3986da1afd33e506",
"mediantime": 1293622620,
"chainwork": "0000000000000000000000000000000000000000000000000644cb7f5234089e"
"verificationprogress": 0.001,
}
...
}
```

📝 kevkevinpal opened a pull request: "threading: reduce the scope of lock in getblocktemplate"
(https://github.com/bitcoin/bitcoin/pull/33264)
This change was motivated by https://github.com/bitcoin/bitcoin/pull/32592#discussion_r2294770722

It does exactly what is said in the comment. Reducing the scope of the lock by a bit before it is needed

💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#issuecomment-3231403389)
@ajtowns sgtm, done.
Not sure about the order thought... I'm open to suggestions.

💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2305989410)
Sorry, I don't quite understand why specifically external outputs are necessary.
In getblockstats, there are four UTXO-related statistics: utxo_increase, utxo_size_inc, utxo_increase_actual, and utxo_size_inc_actual.
As far as I can see, none of these depend on whether a UTXO is sent to an external or internal address.

That said, using send_self_transfer would significantly change the generated transactions compared to the original test, so I agree that it's better to try to keep the genera
...

💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2306149303)
Thank you!

It appears that we are no longer testing the case of `send 10 BTC with fee subtracted`, nor the case of `send 1 BTC with a higher fee rate`, since we are not specifying a fee in send_to().

💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3231877768)
Concept ACK

💬 ajtowns commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306188488)
Could make it `the` instead of `The` for consistency with other options. Could consider doing the same for the main mediantime help text that this was copied from too.

As far as ordering goes, probably copy the ordering of the fields in `getblockchaininfo`?

💬 ajtowns commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306180631)
Can just drop this field, and use the presence/absence of the object to indicate where background validation is occurring?

If not, "estimate of" isn't accurate here -- the node knows whether it's doing background validation or not.

💬 ajtowns commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306222771)
Should be able to test the field-exists case via feature_assumeutxo.py probably just prior to the invocation of `n1.submitblock(snapshot_block)`

💬 ajtowns commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306215222)
This is telling you how far the background has progressed if it were going to go all the way to the current tip; but it will actually stop when it hits the snapshot block. So I think this should be calling a new method `chainman.GetBackgroundVerificationProgress(btip)`, something like:

```c++
double ChainstateManager::GetBackgroundVerificationProgress(const CBlockIndex* tip)
{
if (tip == nullptr) return 0.0;
auto base = GetSnapshotBaseBlock();
if (base == nullptr || base->m_
...

💬 maflcko commented on pull request "threading: reduce the scope of lock in getblocktemplate":
(https://github.com/bitcoin/bitcoin/pull/33264#issuecomment-3232037034)
lgtm ACK 06ec4809045f5cbe51d54e8eda08db03ca4d5ca1

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3232064557)
Are you still working on this?

💬 maflcko commented on pull request "ci: use LLVM 21":
(https://github.com/bitcoin/bitcoin/pull/33258#issuecomment-3232102351)
lgtm ACK 4cf0ae474ba03830c86653f1abae4ab4d38c94e4

👍 l0rinc approved a pull request: "ci: use LLVM 21"
(https://github.com/bitcoin/bitcoin/pull/33258#pullrequestreview-3163409751)
utACK 4cf0ae474ba03830c86653f1abae4ab4d38c94e4

https://github.com/llvm/llvm-project/releases/tag/llvmorg-21.1.0 was just released 2 days ago

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3232177376)
Yep. But been on holiday this week and last.

🤔 BrandonOdiwuor reviewed a pull request: "Update `minisketch` subtree and switch to its build script"
(https://github.com/bitcoin/bitcoin/pull/32856#pullrequestreview-3163505949)
ACK a6b48f009f7bf88bc1f15ed9490276baa898884e

Verified migration to `minisketch` upstream CMake build script. Successfully built `minisketch` and `Bitcoin Core` on macOS 15.6 (Clang 17, CMake 4.0.2) and Ubuntu 24.04 (GCC 13, CMake 3.28.3)

**macOS**
```
$ cmake --build build -j9
```
<img width="1488" height="224" alt="Screenshot 2025-08-28 at 09 58 53" src="https://github.com/user-attachments/assets/538d6bc0-d55c-444d-9a81-4194678379dc" />

**Ubuntu**
```
$ cmake --build build -j9
`
...

💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2306467573)
[17:54:00.577] = help: Remove unused import: `decimal.Decimal`