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

👍 vasild approved a pull request: "netinfo: add peer services column and outbound-only option"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2395186250)
ACK 17f8f03ec69b6dd3d61137a39b8f88201ec500dc

💬 vasild commented on pull request "netinfo: add peer services column and outbound-only option":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1816619674)
nit: since `args.at(1)` is repeated also in the `throw` statements, maybe:

```cpp
std::string_view arg1{args.at(1)};
if (n && (arg1 == "o" || arg1 == "outonly")) {
} else {
throw ... arg1 ...
```

💬 vasild commented on pull request "netinfo: add peer services column and outbound-only option":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1816633325)
nit: `level|outonly|help` reads as "exactly one of `level`, `outonly` or `help`". I guess it should be

```
-netinfo (level [outonly]) | help
```
or two lines:

```
"-netinfo \"help\"\n"
"-netinfo level [outonly]\n\n"
```

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