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