💬 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

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1785146799)
Also note that `BOOST_CHECK()` is deprecated for unclear reasons: https://github.com/boostorg/test/blob/develop/include/boost/test/tools/interface.hpp#L273-L276 ([Commit](https://github.com/boostorg/test/commit/bae8de14b4b54481c5bd77e19befe9ebec2b9936#diff-fcba8b2de1aa5e32c7d0b3556513209f9791d2a149f546d7b8822a79b02562a4))

👍 hebasto approved a pull request: "depends: For mingw cross compile use -gcc-posix to prevent library conflict"
(https://github.com/bitcoin/bitcoin/pull/31013#pullrequestreview-2343972453)
ACK ae56b3230b287eef5a5657d3089abebffde51484. I've tested on both Debian Bookworm and Ubuntu 24.04 with the `g++-mingw-w64-x86-64` package installed. The resulting CMake internal configuration appears more accurate. For instance, on Ubuntu 24.04, for the `bitcoin-tx` target, the diff in `build/src/CMakeFiles/bitcoin-tx.dir/linkLibs.rsp` looks as follows:
```diff
- -L"/usr/lib/gcc/x86_64-w64-mingw32/13-win32" libbitcoin_common.a util/libbitcoin_util.a univalue/libunivalue.a -liphlpapi libbitc
...

👍 instagibbs approved a pull request: "fuzz: fix bug in p2p_headers_presync harness"
(https://github.com/bitcoin/bitcoin/pull/30980#pullrequestreview-2343982525)
ACK a7498cc7e26b4b3de976e111de2bd2d79b056b31

💬 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-2389625672)
Thanks, I didn't read the previous discussion history, so my feedback is only based on the implementation of this PR. not on earlier comments.

I think tinyformat's original decision to throw exceptions when invalid format strings were specified was flexible and sound, and the additional consteval checking we were able to add on top of that was a nice improvement that should be extended more places.

I just don't see a need to rip out tinyformat's simple handling mechanism and replace it wit
...

👍 hodlinator approved a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2344029932)
ACK b6368fc285bf00b3033061dcd4e29298b227c6df

Reviewed `git range-diff master f83a4f6 b6368fc` + "add tx_in_orphanage" commit + GitHub Files Changed.

Thanks for addressing the latest feedback!

`build/test/functional/rpc_getorphantxs.py` passed locally.
Ran Doxygen and inspected `ParseVerbosity` docs to confirm `[in]`-parameters were handled well etc.

💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1785202459)
Also have a slight preference against default args in new code in general.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2389650513)
Updated to take @tdb3 review feedback (thanks!) and few adjustments.

<details><summary><code>git diff 51206c6 683b558</code></summary><p>

```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 3207a5d9c97..000f7695de6 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -391,10 +391,10 @@ private:
return UNKNOWN_NETWORK;
}
uint8_t m_details_level{0}; //!< Optional user-supplied arg to set dashboard details level
- bool OutboundOnlySelec
...

💬 jonatack commented on pull request "rpc: add address_type field in getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/30727#issuecomment-2389661303)
Yes, will update.

💬 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-2389669785)
> 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.

(This part my previous comment was more for maflcko & stickies-v. I think they were interpreting me as being more critical of the error-handling than I intended).

Th
...