💬 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`.

💬 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_r1785015902)
It logs the actual Qt version in the output:
```
-- Found Qt: /usr/lib/x86_64-linux-gnu/cmake/Qt5 (found suitable version "5.15.13", minimum required is "5.11.3")
```
Otherwise, the output will be less informative:
```
-- Found Qt: /usr/lib/x86_64-linux-gnu/cmake/Qt5 (Required is at least version "5.11.3")
```

💬 ryanofsky commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2389355374)
Will not nack this pr since I don't think consequences are great, but I would prefer not to see this PR merged.

I don't think putting "error while formatting log message" in a std::string and then returning that std::string is a good way for a C++ API to return errors, especially errors that are almost certainly bugs in the code. Throwing an exception seems far more appropriate, and more likely to result in bugs being detected and fixed. (Also nit: the wording of that message doesn't make sen
...

💬 l0rinc commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module / variable names":
(https://github.com/bitcoin/bitcoin/pull/31010#issuecomment-2389361019)
utACK deacf3c7cd68dd4ee973526740e45c7015b6433a

💬 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_r1785022444)
Wow, so muc magic

💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2389519810)
My [Concept ACK](https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2339765974) accepted the current concept, while stating that I see it as one of the *least* worst alternatives (implying I don't see the error handling part as blocking). I'm happy to get feedback on that but my intent was not to start a debate.

@ryanofsky how would you feel about adding an `Assume()` statement that could fail Debug-builds when formatting fails?

🤔 hebasto reviewed a pull request: "depends: For mingw cross compile use -gcc-posix to prevent library conflict"
(https://github.com/bitcoin/bitcoin/pull/31013#pullrequestreview-2343918309)
It looks reasonable to be explicit about every aspect of a toolchain.

However, I'm wondering why the bug appears on Debian Bookworm, but not on Ubuntu 24.04? I've cross-checked CMake versions and can confirm they are not the cause.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1785137610)
Looking at it more, I don't think it's needed and won't be hit, so removed that line for now. (Thanks!)

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1785137960)
done