💬 tomasandroil commented on pull request "Fix missing error check in `set_clo_on_exec` for FD_CLOEXEC handling":
(https://github.com/bitcoin/bitcoin/pull/32342#issuecomment-2828695592)
@laanwj I've squashed the commits and updated the message as requested

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2828708948)
This causes the signer test to fail (timeout) in the CentOS run, interesting
```
[15:47:25.389]
wallet_signer.py --descriptors | ✖ Failed | 1205 s
```

```
[15:47:25.389]
test_framework.authproxy.JSONRPCException: 'walletdisplayaddress' RPC took longer than 1200.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```

🤔 stickies-v reviewed a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2788075175)
The implementation simplifications since my last review are great (and require a small update to the PR description which still mentions the addition of two caches).

I'm still not 100% comfortable that the changes don't introduce any unwanted side-effects, so I want to give it another full review first. Approach ACK.

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2058610946)
nit: I think it would make the code more readable to introduce a `CBlockIndex* candidate` here, to avoid all the `candidate_it->second` in an already verbose code block:

<details>
<summary>git diff on 5b92a30a20</summary>

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index fed9186553..6ceeaa6ab5 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3759,17 +3759,18 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
// A
...

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