💬 achow101 commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2305442956)
In 0d9f0888542081b1792705594b570ae748a9f839 "[wallet]: add `maxfeerate` wallet startup option"

`-maxtxfee` adds a warning if the max fee is too high. I think it would make sense to have a similar warning for `-maxfeerate`.

💬 achow101 commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2305470201)
In 99b45844cab7896e206dd0b8e24355586bc1a84d "[wallet]: warn user if 1kvb tx with `-maxtxfee` base fee has fee rate less than `minrelaytxfee`"

This check does not make sense to me. This converts the max tx fee back into a feerate just for this check, even though `-maxfeerate` has taken over that check. I don't think we need to have this check here.

💬 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().