💬 laanwj commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2828513340)
> Hmm, I should check, but I have the hunch that events already queued are still delivered after disconnection. Which, if that is the case, the previous clientModel check would work better.

The signals are queued, but if no handler is connected, they sould just be dropped.

✅ maflcko closed an issue: "ctest -C Debug fails on vs2022 (Exit code 0xc0000135)"
(https://github.com/bitcoin/bitcoin/issues/32341)

💬 maflcko commented on issue "ctest -C Debug fails on vs2022 (Exit code 0xc0000135)":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2828524157)
I guess I caused the issue myself. False alarm

💬 achow101 commented on issue "Moving this repo to bitcoin-core":
(https://github.com/bitcoin/bitcoin/issues/32340#issuecomment-2828537251)
> I would like to see a write-up of the different GitHub arrangements so that we can compare the plans by name.

I've written it in this gist: https://gist.github.com/achow101/e20dcc0818e2b346e699438b70ee8b8c

I think discussion of bips is a bit off topic for this repo as the actions taken by either repo are largely unrelated.

> If the main motivation is making moderation easier, moving out the bips repository and leaving bitcoin under the bitcoin org seems a less risky option.

I don't think t
...

💬 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-2828562780)
@laanwj Thanks for the review!
I've updated `set_clo_on_exec` to check the return value of `fcntl(fd, F_GETFD, 0)` and throw OSError on failure, similar to the existing check for `F_SETFD`

💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2828565105)
[96e6371](https://github.com/bitcoin/bitcoin/commit/96e6371328c7715ef814da5831d08b5d30285dd3) to [41860d9](https://github.com/bitcoin/bitcoin/commit/41860d98408f52c29e78f7708e8a0355ea5cc04c): rebased due to conflict with https://github.com/bitcoin/bitcoin/pull/31835

💬 maflcko commented on pull request "ci: Merge fuzz task for macOS":
(https://github.com/bitcoin/bitcoin/pull/32339#issuecomment-2828589191)
(dropped the windows changes)

💬 furszy commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2828595545)
> > Hmm, I should check, but I have the hunch that events already queued are still delivered after disconnection. Which, if that is the case, the previous clientModel check would work better.
>
> It's worth checking but imo it's unlikely it works that way. The signals are queued, but if no handler is connected, they sould just be dropped (next time the event loop runs).

It seems to be the case: https://stackoverflow.com/questions/2532341/problem-with-qtqueuedconnection-signal-delivered-aft
...

📝 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" .