💬 sr-gi commented on issue "Moving this repo to bitcoin-core":
(https://github.com/bitcoin/bitcoin/issues/32340#issuecomment-2828495015)
I expressed my concerns about moving bitcoin out of the bitcoin org / leaving the bitcoin org empty during the meeting, but I think it is worth elaborating on them.
While I can see the logistical benefits of splitting the orgs and how it may make admin/moderator lives easier, I don't think that's enough motivation to overcome the potential risks of redirections being broken, github giving up the org due to inactivity, or a takeover of the bitcoin/bitcoin repo. I do agree that these threads are
...
(https://github.com/bitcoin/bitcoin/issues/32340#issuecomment-2828495015)
I expressed my concerns about moving bitcoin out of the bitcoin org / leaving the bitcoin org empty during the meeting, but I think it is worth elaborating on them.
While I can see the logistical benefits of splitting the orgs and how it may make admin/moderator lives easier, I don't think that's enough motivation to overcome the potential risks of redirections being broken, github giving up the org due to inactivity, or a takeover of the bitcoin/bitcoin repo. I do agree that these threads are
...
💬 furszy commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2828497800)
> That's really strange. Is there some thread race maybe?
> Does the old solution (to check clientModel in the callback) work better?
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.
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2828497800)
> That's really strange. Is there some thread race maybe?
> Does the old solution (to check clientModel in the callback) work better?
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.
💬 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.
(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)
(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
(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
...
(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`
(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
(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)
(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
...
(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
...
(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)
(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.
(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)
(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
(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.
(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.
(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.
(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
(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
...
(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
...