💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2059071228)
Done, thanks

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2828620136)
> If a user tries to restore a legacy wallet (using RPC or QT) setting "load_on_startup" (can't be done on QT but it's being set in the wallet interface code), next time the node or QT starts it will be closed with the error _"... Failed to open database path ... The wallet appears to be a Legacy wallet, please use the wallet migration tool... "_. That case shouldn't be handled here? We shouldn't allow `load_on_startup` on legacy...
>
> (Currently this situation is not happening as a side eff
...

💬 theuni commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2828630396)
Ok, I'm back after a week of paging all of this back in and hacking around. This is actually something I wanted to get cleaned up a long time ago, so I'm happy it has eyes on it again. Sorry in advance for the novel :)

---

I think the discussions about `shared_ptr`/`unique_ptr`/refcounting have distracted from what really needs to be discussed here: the lifetime and concurrency requirements/guarantees of the various subsystems.

The refcounters are (ab)used as a generic means of controll
...

💬 laanwj 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-2828641166)
LGTM now, raising OSError appears to be how other errors are handled in subprocess as well, so it's at least consistent. Needs to be tested, though, i don't really know the effect of this up the callchain. It doesn't look like we actually catch OSError in `RunCommandParseJSON`.

Please squash your commits and use a more informative commit message than "Update subprocess.h" .

📝 Eunovo opened a pull request: "Bug/Wallet: Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor "
(https://github.com/bitcoin/bitcoin/pull/32344)
Closes https://github.com/bitcoin/bitcoin/issues/31728

This PR updates the `DescriptorScriptPubKeyMan` to skip range checks for non-ranged descriptors, which previously caused errors when updating a non-ranged descriptor with the range [0,0]

#### Testing
A unit test was added to test the new behaviour

💬 Eunovo commented on issue "Bug: Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in `AddWalletDescriptor`":
(https://github.com/bitcoin/bitcoin/issues/31728#issuecomment-2828647076)
> > Hey [@asklokesh](https://github.com/asklokesh), are you still working on this or can I chime in to help as well?
>
> You can tackle it. asklokesh comment is surely from chatGPT and makes no sense in our code. We should delete it.

Opened PR https://github.com/bitcoin/bitcoin/pull/32344 to close this since it's been a while since @Chand-ra indicated interest

💬 furszy commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2828678405)
Updated. Back into the `clientModel` nullptr check approach.

> (doubt this is the only such case)

For sure. We could create a global connection function that checks if the app is tearing down before processing the event. Still, I think I'd rather rework the shutdown process than go that route. Yet, not something for this PR.

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