💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2058555357)
nit: Would label as out-of-chain (or non-active-chain for consistency)
```suggestion
// Mark out-of-chain descendants of the invalidated block as invalid
```

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2059128406)
nit: it's consistent with how `CBlockIndexWorkComparator` is used most of the time in our codebase, but I don't understand why we don't write this without the negation (swapping the arguments instead)?

```cpp
CBlockIndexWorkComparator()(m_chainman.m_best_header, candidate_it->second))
```

_(I might open a separate pull to add a `CBlockIndex::HasMoreWorkThan(const CBlockIndex* other)` convenience fn, given how frequently this comparator is used and confuses me - and maybe
...

💬 w0xlt commented on pull request "refactor: Update `XOnlyPubKey::GetKeyIDs()` to return a pair of pubkeys":
(https://github.com/bitcoin/bitcoin/pull/32332#discussion_r2059187808)
Done. Thanks.

💬 w0xlt commented on pull request "refactor: Update `XOnlyPubKey::GetKeyIDs()` to return a pair of pubkeys":
(https://github.com/bitcoin/bitcoin/pull/32332#discussion_r2059187883)
Done. Thanks.

💬 mzumsande commented on issue "test: bip324_tests & net_tests failure with `-O3 -flto`":
(https://github.com/bitcoin/bitcoin/issues/32337#issuecomment-2828811235)
I tried this with:
```
x86_64
gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0
GNU ld (GNU Binutils for Ubuntu) 2.42
```

I don't get any of the failures reported above, but `util_test_runner` fails for me:
```
bitcoin-util: ./kernel/chainparams.cpp:143: CMainParams::CMainParams(): Assertion `consensus.hashGenesisBlock == uint256{"000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"}' failed.
```
(`tool_signet_miner.py --descriptors` and` tool_wallet.py --descriptors` fail with the same e
...

💬 sipa commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2828814182)
See also #32337.

📝 ryanofsky opened a pull request: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345)
This PR fixes various problems when IPC connections are broken or hang which were reported in https://github.com/bitcoin-core/libmultiprocess/issues/123. The different fixes are described in commit messages.

---

This PR is a draft because it depends on https://github.com/bitcoin-core/libmultiprocess/pull/160. The non-base commits are:

- [`168144fcb974` ipc: Use EventLoopRef instead of addClient/removeClient](https://github.com/bitcoin/bitcoin/pull/32342/commits/168144fcb9744122c07418864
...

💬 hebasto commented on issue "ctest -C Debug fails on vs2022 (Exit code 0xc0000135)":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2828857830)
FWIW, [here](https://github.com/hebasto/bitcoin/commits/250424-msvc-debug-DEMO/) is the demo branch where the native Windows CI job has been switched to the "Debug" configuration.

CI logs: https://github.com/hebasto/bitcoin/actions/runs/14650416249.

🤔 mzumsande reviewed a pull request: "net: remove unnecessary check from AlreadyConnectedToAddress()"
(https://github.com/bitcoin/bitcoin/pull/32338#pullrequestreview-2792554528)
This being correct relies on the assumption that if `ToStringAddrPort` is equal for two addresses, these addresses must also be equal on the level of CNetAddr - I'm pretty sure that this is the case today, but could we add a fuzz test asserting this to `fuzz/netaddress.cpp`?

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2059298130)
I think it's fine to leave as is, it's under the "Legacy subdirectories and Files" heading. This document discusses the files that Bitcoin Core creates and uses, and legacy wallets are neither created nor really used after this PR.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2059298259)
Done.

💬 achow101 commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#issuecomment-2828964023)
ACK 3dbd50a576be55941cb4b5034dc2171c03afb07c

🚀 achow101 merged a pull request: "Fix failing util_time_GetTime test on Windows"
(https://github.com/bitcoin/bitcoin/pull/32318)

💬 pablomartin4btc commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2828981936)
> I tried to restore a legacy wallet with this PR and it doesn't do it, nor are there any issues...

Ok, I thought the restore would allow to do it and then the load_on_startup would fail but the restore fail on the wallet file verification, so all good.

Tested `restorewallet` creating a legacy on `master`, backed it up there and trying to restore it on this PR's branch.
```
./build_31250/bin/bitcoin-cli -regtest -datadir=/tmp/btc-wallet restorewallet "restored_legacy_master" /tmp/btc-wal
...

💬 pablomartin4btc commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2828988031)
> Do you mean there is a possibility here if the interface is used incorrectly internally?

I've tested it with this branch's QT and same failure on wallet file verifitcation while trying to restore a legacy backup using the GUI. All good.

💬 w0xlt commented on pull request "refactor: Update `XOnlyPubKey::GetKeyIDs()` to return a pair of pubkeys":
(https://github.com/bitcoin/bitcoin/pull/32332#issuecomment-2828996310)
@hodlinato yes, this decreases heap usage, but I don't think there are any relevant performance gains.

🤔 w0xlt reviewed a pull request: "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor"
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2792671179)
Concept ACK.

Functional tests might be necessary.

💬 davidgumberg commented on pull request "net: remove unnecessary check from AlreadyConnectedToAddress()":
(https://github.com/bitcoin/bitcoin/pull/32338#issuecomment-2829070906)
It would also be good (maybe sufficient) to add a unit test which enforces/documents the behavior that a peer with the same address as an existing peer and a different peer will be seen as already connected to. e.g.:

```diff
diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp
index e60ce8b99d..4441a8dde4 100644
--- a/src/test/net_peer_connection_tests.cpp
+++ b/src/test/net_peer_connection_tests.cpp
@@ -155,6 +155,13 @@ BOOST_FIXTURE_TEST_CASE(tes
...

💬 furszy commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2829077127)
> I believe we've kept it merely as a sanity check because it's the only test that will survive the annihilation of the legacy wallet code.

Best bench-test ever. Just caught a real bug in #31423.

💬 achow101 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2829171034)
It looks like this is happening early enough in init that the warning never actually makes it to the GUI, so GUI users (probably a majority of MacOS users) won't actually see the warning.