💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784831993)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776017230

It's interesting to see what that looks like but I see few disadvantages to that approach:

- Superficially, the resulting code is longer and the diff is bigger. Current PR is trying to minimally change this function.
- It seems to be duplicating some code like "Cannot set -%s with no value" error, increasing risk that differences between the two copies of code will be introduced accidentally, and introducing unintend
...

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784899977)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776028495

Thanks, applied suggestion

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784804173)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775907489

> nit: this is only used once, maybe we could have a local `TryParseInt64` next to `InterpretBool` - details below

Thanks, adopted suggested approach.

Note: other comment with details is: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775907489

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784852608)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776023938

Again, it's interesting to see what this looks like. so thanks for spelling it out. I don't love the `flags & ALLOW` syntax, so it's nice that helper functions hide it, but I feel the helper functions add a level of indirection that make it harder to plainly see what the code is doing.

Specifically they make this checking code less clear because of instead of checking the flags and then mentioning the flags just check
...

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784833826)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775907489

> we could group the two bools and use an optional for the in parsing (assuming `std::optional` `value` parameter):
> TryParseInt64

Thanks, switched to this approach.

:lock: fanquake locked an issue: "Bitcoin 💵"
(https://github.com/bitcoin/bitcoin/issues/31020)

🤔 l0rinc reviewed a pull request: "cmake: Avoid hardcoding Qt's major version in Find module / variable names"
(https://github.com/bitcoin/bitcoin/pull/31010#pullrequestreview-2343622187)
Concept ACK

💬 l0rinc commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module / variable names":
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784944693)
Do we need to keep the versions here?

💬 l0rinc commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module / variable names":
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784945226)
nit: is there a reason for keeping the `t` lowercase? Looks weird this way...

⚠️ nulladoses opened an issue: "bitcoin.service issue for a noob like me"
(https://github.com/bitcoin/bitcoin/issues/31021)
### Motivation

Hi i don't know if i'm in the right section but i have a problem installing contrib/init/bitcoind.service. i'm a noob so please if someone can help me. I downloaded bitcoin core full node and Tor. Now i installed the service to start bitcoin core on boot of my machine that is Linux Mint on an old macbook pro.
When i make sudo systemctl start bitcoind show like this
![Schermata del 2024-10-02 19-15-59](https://github.com/user-attachments/assets/b781e7c5-9a29-440e-9492-03cf329e35
...

👍 theuni approved a pull request: "depends: For mingw cross compile use -gcc-posix to prevent library conflict"
(https://github.com/bitcoin/bitcoin/pull/31013#pullrequestreview-2343640805)
utACK ae56b3230b287eef5a5657d3089abebffde51484.

Annoying upstream bug aside, this seems reasonable enough to me.

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784969353)
Can we at least make absolutely sure we're only catching allowed exceptions (either by type or message)?

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784970051)
I mean that you've already extracted a `IsTypedArg(flags)` method for `ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING`, right?

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784970693)
I see you went the other way - I'm fine with that as well, it's more verbose, but at least consistent

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784970850)
* We could still use the`IsTypedArg(flags)` on this line
* Eventually we will hopefully migrate away from flags, I would personally prefer we encapsulate them early

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784970987)
Agree, it was an alternative, both are fine

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784971122)
Your criticism is reasonable, do you see a clearer way of breaking up this method?
Currently I need to know too much about the possible states on a single level, would prefer excluding some states while I'm reviewing.

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784971346)
Thanks for considering

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784971572)
The change seems small, since we're already converting an optional variable into a pointer parameter (seems simpler to just pass it along) - but of course will leave it up to you, just want to make sure we're on the same page here.

💬 hebasto commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module / variable names":
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1785004767)
Variables such as `<PackageName>_FIND_VERSION`, `<PackageName>_FIND_VERSION_MAJOR`, and `<PackageName>_FIND_COMPONENTS`, are part of the `Find<PackageName>` [interface](https://cmake.org/cmake/help/latest/command/find_package.html#package-file-interface-variables). In our case, `<PackageName>` is replaced with `Qt`.