💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2056489560)
My understanding is that `state.GetResult() != BlockValidationResult::BLOCK_MUTATED` is equivalent to `state.GetResult() == BlockValidationResult::BLOCK_CONSENSUS` here, because none of the other enum values are returned outside of block header validation, and at this point we've already passed `AcceptBlockHeader()`.

I tested this by adding an `Assume` and running the unit and functional test suite with it:

```cpp
...
Assume(state.GetResult() == BlockValidationResult::BLOCK_C
...

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2058645775)
nit: I find splitting this up in two statements to be much more readable:

```cpp
// BLOCK_FAILED_VALID should never be set if BLOCK_FAILED_CHILD is set because <...>
candidate->nStatus &= ~BLOCK_FAILED_VALID;
candidate->nStatus |= BLOCK_FAILED_CHILD;
```
Performance implications seems irrelevant here, and most likely optimized by the compiler anyway.

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2058657191)
nit: for grepping purposes: s/the best header/m_best_header

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2056509235)
I was surprised to see that `CheckBlockIndex()` is not a `const` method, which I think it should be. To help validate that this call is not having unexpected side-effects, I implemented the const-ness change and found no issues because of it: https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2b68b3f8343fe6582cc2ecbc474f489a50bdeb11

I think this change would be good to have on master, but it doesn't have to be in this PR either, so if you'd rather no include it in this PR
...

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2058150283)
I don't really understand why we need this extra check in every loop iteration. The commit message states that we need to check it again because `cs_main` was released, but it's not really obvious to me why that's a problem?

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2058660396)
nit: s/this block/pprev/ for reduced ambiguity

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