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

💬 fanquake commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437809265)
> 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.

Sure, that's fine, any more-invasive changes could be done later, but we shouldn't be leaving around dead code, or commentary that is incorrect.

💬 fanquake commented on pull request "optimization: Preallocate addresses in GetAddr based on nNodes":
(https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437813806)
> I don't think this is a particularly impactful change but it's very small and straightforward to review.

I agree, and I think we are probably overdue for a review of what is in `src/bench`, and cleaning out benchmarks that aren't (very) meaningful. IIRC `clang-tidy` has a check for exactly this kind of change, so I think if any more were going to be made, they could be done using that.

💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437814832)
> 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.

i agree. Let's make sense there are no clearly wrong comments, or dead code. But as we're sure that the only port mapping method we want to keep is PCP/NATPMP, a lot of the logic in this file can be removed and methods can be renamed. But might as well do that in a future follow-up to #30043, as some of those need larger restructures to the loop t
...

💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816714440)
Thanks for the example. As the primary author of this code, I think the run-time error would be more clear, though a compile-time error would catch it one CLI command earlier. I haven't found this kind of error to be an issue for me over the past half decade on this code.

Also, this code path is manually tested by myself, reviewers and users. Perhaps for that reason, and because it's non-critical code, a maintainer explicitly asked that no test coverage be written for it.

So either way is
...