💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305302493)
I tried it again:
```bash
git log -1 | head
commit 28fb7f74c4ae66dcb0f918774d644ac3c393f25b
Author: woltx <94266259+w0xlt@users.noreply.github.com>
Date: Mon Aug 25 20:22:33 2025 -0700
net: Prevent node from binding to the same CService
```
and did:
```bash
rm -rfd build && \
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && \
ninja -C build && \
build/bin/bitcoind -bind=127.0.0.1:11012 -bind=127.0.0.1:11012
```
which resulted in:
```
...
2025-08-27T21:07:33Z ini
...
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305302493)
I tried it again:
```bash
git log -1 | head
commit 28fb7f74c4ae66dcb0f918774d644ac3c393f25b
Author: woltx <94266259+w0xlt@users.noreply.github.com>
Date: Mon Aug 25 20:22:33 2025 -0700
net: Prevent node from binding to the same CService
```
and did:
```bash
rm -rfd build && \
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && \
ninja -C build && \
build/bin/bitcoind -bind=127.0.0.1:11012 -bind=127.0.0.1:11012
```
which resulted in:
```
...
2025-08-27T21:07:33Z ini
...
💬 l0rinc commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2305316780)
I would be fine with both as long as it's explicit. I still find the rationale that `struct` and `class` should be formatted similarly, it's harder for me to explain why `struct` is more like a for loop than a `class`...
> 85'849 lines
I have no idea what these numbers mean, I checked same-line opening braces with
```bash
% grep -E '^\s*\bstruct\b\s+[^;{]*\{' --include="*.h" --include="*.cpp" -r src/ | wc -l
393
```
and quickly vibe coded a script that does newlines:
<detai
...
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2305316780)
I would be fine with both as long as it's explicit. I still find the rationale that `struct` and `class` should be formatted similarly, it's harder for me to explain why `struct` is more like a for loop than a `class`...
> 85'849 lines
I have no idea what these numbers mean, I checked same-line opening braces with
```bash
% grep -E '^\s*\bstruct\b\s+[^;{]*\{' --include="*.h" --include="*.cpp" -r src/ | wc -l
393
```
and quickly vibe coded a script that does newlines:
<detai
...
💬 w0xlt commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305337219)
That's strange. On my machine, it's interrupted.
I tried running it by pointing the `-datadir` to an empty folder to see if that was the reason, but it also stopped (shown below).
Wouldn't the `feature_bind_extra` test fail if the node didn't stop?
Are you using a configuration file? Could some settings be affecting this?
```
2025-08-27T21:27:45Z Loaded best chain: hashBestChain=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f height=0 date=2009-01-03T18:15:05Z progres
...
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305337219)
That's strange. On my machine, it's interrupted.
I tried running it by pointing the `-datadir` to an empty folder to see if that was the reason, but it also stopped (shown below).
Wouldn't the `feature_bind_extra` test fail if the node didn't stop?
Are you using a configuration file? Could some settings be affecting this?
```
2025-08-27T21:27:45Z Loaded best chain: hashBestChain=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f height=0 date=2009-01-03T18:15:05Z progres
...
💬 murchandamus commented on pull request "coinselection: Tiebreak SRD eviction by weight":
(https://github.com/bitcoin/bitcoin/pull/33223#issuecomment-3229819476)
I want to use the opportunity to move the SRD tests to the coinselection_tests, but haven’t gotten around to it yet.
(https://github.com/bitcoin/bitcoin/pull/33223#issuecomment-3229819476)
I want to use the opportunity to move the SRD tests to the coinselection_tests, but haven’t gotten around to it yet.
💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305346479)
`feature_bind_extra` is skipped on a Mac
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2305346479)
`feature_bind_extra` is skipped on a Mac
💬 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.
(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
...
(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
(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`.
(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.
(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!
(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.
(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
...
(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
...
(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?
(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.
(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;
}
```
(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?
(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`
(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
...
(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
...