📝 laanwj opened a pull request: "common: Close non-std fds before exec in RunCommandJSON"
(https://github.com/bitcoin/bitcoin/pull/32343)
Picks up stale #30756, while addressing my fallback comment (https://github.com/bitcoin/bitcoin/pull/30756#discussion_r2030844440).

> Currently, RunCommandParseJSON runs its target with whatever fds happen to be open inherited on POSIX platforms. I don't think there's any practical scenario where this is a problem right now, but there's a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it's hanging - the listening port(s) won't get released and star
...

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2059063160)
Done (except for the edit part, it's better documentation this way I think)

💬 laanwj commented on pull request "run_command: Close non-std fds when execing slave processes":
(https://github.com/bitcoin/bitcoin/pull/30756#issuecomment-2828599460)
Continued in #32343.

✅ laanwj closed a pull request: "run_command: Close non-std fds when execing slave processes"
(https://github.com/bitcoin/bitcoin/pull/30756)

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2059063505)
Extended the comment

💬 laanwj commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2828606810)
Okay, that's really awful (doubt this is the only such case), but yes, better to revert to previous solution then.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2828608135)
Thanks for the reviews, addressed most of your concerns - except for the vector constructor for `Obfuscation` - but if other reviewers also think it's better that way, I'll do it of course.
Also extended the `BOOST_AUTO_TEST_CASE(dbwrapper)` test case with asserting that the obfuscation key can be read back by an unobfuscated instance as well.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2059069279)
No problem, glad it's sorted. Extended the comment to make it even clearer.

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