💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816643611)
> Clang-format can be helpful for new code, but I don't think it's necessary to re-indent code for it that isn't otherwise touched -- that's just noisy churn.

I'm just going with our `clang-format` settings as defined in `src/.clang-format` - if you feel like those settings are suboptimal opening a separate pull/issue for that is probably more appropriate?

> Please provide an example of how this code could crash currently without this change?

The PR states that there is no behaviour cha
...

💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816650254)
> Please provide an example of how this code could crash currently without this change?

If you're asking about how such an error could sneak in without this PR, a change as simple adding a single extra `*` would lead to a run-time exception if no extra parameter is added:

```cpp
result += tfm::format_raw("<-> type net v mping ping send recv txn blk hb %**s%*s%*s ",
```

👍 dergoegge approved a pull request: "fuzz: fuzz connman with non-empty addrman + ASMap"
(https://github.com/bitcoin/bitcoin/pull/29536#pullrequestreview-2395241979)
Code review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc

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

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only option":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1816659057)
Thanks! Done.

💬 fanquake commented on pull request "guix: use GCC 13 to build releases":
(https://github.com/bitcoin/bitcoin/pull/29881#issuecomment-2437744416)
Guix Build (x86_64 & aarch64):
```bash
c27a601dbc89ed3abccb6f742178f15f0eb256cd5086d8360dd6727c9c866cab guix-build-a4e17239ca54/output/aarch64-linux-gnu/SHA256SUMS.part
0fdb1ff5320398ec5cf6378f72c826c619909e1079a9e8ab871bac938db41c0e guix-build-a4e17239ca54/output/aarch64-linux-gnu/bitcoin-a4e17239ca54-aarch64-linux-gnu-debug.tar.gz
60f6d4f34f3d5af5aeb6cf678a6f2ca60035d52cf8c48b3dbea7a54a8ce89776 guix-build-a4e17239ca54/output/aarch64-linux-gnu/bitcoin-a4e17239ca54-aarch64-linux-gnu.tar.g
...

👍 brunoerg approved a pull request: "Introduce `g_fuzzing` global for fuzzing checks"
(https://github.com/bitcoin/bitcoin/pull/31093#pullrequestreview-2395260296)
crACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53

💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1816669777)
i think cleaning up a bit more makes sense here, though let's keep it easy to review refactor-wise

💬 instagibbs commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816679333)
tried rephrasing

💬 instagibbs commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816679411)
If someone opens a PR to persist I'll push for an RPC to set it explicitly :)

💬 instagibbs commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816679497)
changed in both places

💬 instagibbs commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816679569)
fixed

💬 instagibbs commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816679628)
deleted

💬 instagibbs commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816679967)
a little bit of test redundancy that makes it more readable imo? leaving as is

👍 theStack approved a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2395207875)
ACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1816633184)
```suggestion
// Constructor. We are passing and casting ints because they are more readable in a table (see expected_behaviors).
```

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1816676364)
is it intentional to do no backward jumps in time here? (in contrast to the `txdownloadman` fuzz test above)

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only option":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2437780560)
Updated the last commit to take @vasild's feedback (thanks!) per the following small diff.

<details><summary><code>git diff 17f8f03 87532fe</code></summary><p>

```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 6bda4cbe0c5..f63dd22e621 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -486,9 +486,9 @@ public:
if (std::string_view s{args.at(1)}; n && (s == "o" || s == "outonly")) {
m_outbound_only_selected = true;

...

💬 darosior commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1816693931)
My bad i did this one in a hurry. Fixed now, thanks for catching this.

💬 fanquake commented on pull request "cmake: Add `FindZeroMQ` module":
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2437789224)
Concept ACK - Tested this (rebased) branch on Alpine, which ships pkg-config files and CMake config files for ZeroMQ.

```bash
cmake -B build -DWITH_ZMQ=ON
-- Found ZeroMQ: /usr/lib/cmake/ZeroMQ (found suitable version "4.3.5", minimum required is "4.0.0")

# hide cmake config file
mv /usr/lib/cmake/ZeroMQ/ZeroMQConfig.cmake /usr/lib/cmake/ZeroMQ/ZeroMQConfig.cmake.bk

# cmake -B build -DWITH_ZMQ=ON
-- Found PkgConfig: /usr/bin/pkg-config (found version "2.2.0")
-- Found ZeroMQ: /usr/
...

💬 darosior commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437800785)
I know this can be cleaned up, but i wanted to keep it minimal and focused on dropping UPnP. I'm happy to do a few cleanups (such as the ones you suggest) but i don't want to start refactoring the whole file. It can always happen in a follow-up if someone is interested and more familiar with this code.