💬 mzumsande commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3229982021)
Thanks for the reviews and suggestions - I was busy the last couple of weeks, but I'll look at them in detail / update the PR next week!

🤔 luke-jr reviewed a pull request: "rpc, logging: add backgroundvalidation to getblockchaininfo"
(https://github.com/bitcoin/bitcoin/pull/33259#pullrequestreview-3162020387)
Concept ACK. Would be nice to test the active state.

💬 lucifermmmenriquejr commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#issuecomment-3230000566)
Thanks for asking me to reply lol 😘

EnriqueRamirez

On Wed, 27 Aug 2025, 4:50 pm Luke Dashjr, ***@***.***> wrote:

> ***@***.**** commented on this pull request.
>
> Concept ACK. Would be nice to test the active state.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/33259#pullrequestreview-3162020387>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BSKJHMLB5H7PZXGL3ULONU33PYY4FAVCNFSM6AAAAACE4PAMZ2VHI2DSMVQ
...

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305521667)
Tried the same on Linux:
```bash
# build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -bind=127.0.0.1:11012 -bind=127.0.0.1:11012
2025-08-27T23:09:10Z Bitcoin Core version v29.99.0-28fb7f74c4ae (release build)
2025-08-27T23:09:10Z parameter interaction: -bind set -> setting -listen=1
...

💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#issuecomment-3230049818)
> Would be nice to test the active state.

@luke-jr what do you mean by active state?

💬 achow101 commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2305515236)
In 4c6890cdae77ca0d230227530b000f4b3c6fd77a "wallet: Create separate function for wallet load"

When loading, `wallet_creation_flags` is unnecessary. Flags cannot be updated when loading a wallet, and having a specific warning for `WALLET_FLAG_DISABLE_PRIVATE_KEYS` seems entirely unnecessary.

💬 achow101 commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2305511879)
In 4c6890cdae77ca0d230227530b000f4b3c6fd77a "wallet: Create separate function for wallet load"

I think this can be simplified:

```suggestion
auto nLoadWalletRet = walletInstance->PopulateWalletFromDB(error, warnings);
bool rescan_required = nLoadWalletRet == DBErrors::NEED_RESCAN;
if (nLoadWalletRet != DBErrors::LOAD_OK && nLoadWalletRet != DBErrors::NONCRITICAL_ERROR && !rescan_required) {
return nullptr;
}
```

💬 w0xlt commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305558166)
If you try another condition that also triggers `InitError()`, for example:
```
build/bin/bitcoind -i2psam=invalid_addr
```
what happens on your side?

💬 luke-jr commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#issuecomment-3230150529)
When there is a background sync, not just `false`

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305592114)
it seems to stop for empty blockchain, but it will just continue if it something already exists
```
# build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -i2psam=invalid_addr
...
025-08-27T23:49:17Z Discover: 2a01:4f9:2a:a61::2
2025-08-27T23:49:17Z [error] Invalid -i2psam address or hostname: 'invalid_addr'
Error: Invalid -i2psam address or hostname: 'invalid_addr'
2025-08-27T23:49:17Z tor: Thread interrupt
2025-08-27T23:49:17Z Shutdown in progress...
2025-08-27T23:49:17Z torcontrol
...

💬 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`?