💬 w0xlt commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305363666)
I’m also testing on a Mac right now.
On both Linux and macOS ARM (Sequoia 15.5), the node stops as expected.
It’s curious that on your machine `InitError()` prints and logs the error but doesn’t actually stop the node.

💬 davidgumberg commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3229951194)
Ideally, `p2p_leak_tx.py` would be rewritten so that it would deterministically exercise both branches, my impression is the that we only really care about the first case, that tx'es that haven't been inv'ed don't get leaked, but exercising the second case is a good validation of the assumptions of the test, and would have caught that the existing test was broken.

One solution is to set a mocktime as suggested by @enirox001 above so that no time passes and no inv's can be sent and the `getdat
...

💬 davidgumberg commented on pull request "ci: return to using dash in CentOS job":
(https://github.com/bitcoin/bitcoin/pull/33261#issuecomment-3229956996)
ACK https://github.com/bitcoin/bitcoin/commit/509ffea40abbc706ef8b8fc449b7de8677fc5096

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