📝 tomasandroil opened a pull request: "Fix missing error check in `set_clo_on_exec` for FD_CLOEXEC handling"
(https://github.com/bitcoin/bitcoin/pull/32342)
#### Changes:
- Replaces unchecked `fcntl(fd, F_SETFD, flags);` with a version that checks the return value.
- Throws `OSError` on failure with an appropriate error message.
#### Motivation:
Proper error handling is critical in system-level code to avoid silent failures, especially when setting file descriptor flags like `FD_CLOEXEC`. This fix ensures robustness and helps catch configuration issues early during process setup.
### Test coverage
No functional change beyond improved err
...
(https://github.com/bitcoin/bitcoin/pull/32342)
#### Changes:
- Replaces unchecked `fcntl(fd, F_SETFD, flags);` with a version that checks the return value.
- Throws `OSError` on failure with an appropriate error message.
#### Motivation:
Proper error handling is critical in system-level code to avoid silent failures, especially when setting file descriptor flags like `FD_CLOEXEC`. This fix ensures robustness and helps catch configuration issues early during process setup.
### Test coverage
No functional change beyond improved err
...
💬 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#discussion_r2058986286)
This `fcntl` can return `-1` on error too.
(https://github.com/bitcoin/bitcoin/pull/32342#discussion_r2058986286)
This `fcntl` can return `-1` on error too.
💬 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.