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

💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816718706)
> I'm just going with our clang-format settings as defined in src/.clang-format

It's not a hard requirement to retouch existing code with that tool, and I'd prefer avoiding the churn.

🚀 fanquake merged a pull request: "optimization: Preallocate addresses in GetAddr based on nNodes"
(https://github.com/bitcoin/bitcoin/pull/29608)