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

📝 laanwj opened a pull request: "test: Add mockable steady clock, tests for PCP and NATPMP implementations "
(https://github.com/bitcoin/bitcoin/pull/31022)
Add a NodeSteadyClock, a steady_clock that can be mocked with millisecond precision. Use this in the PCP implementation.

Then add a mock for a simple scriptable UDP server,, which is used to test various code paths (including successful mappings, timeouts and errors) in the PCP and NATPMP implementations.

Includes "net: Add optional length checking to CService::SetSockAddr" from #31014 as a prerequisite.

💬 jarolrod commented on issue "Having qt(@6) breaks build for qt@5 on macOS 15.0 and 13.7":
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2389711696)
Confirming the same issue when building the current master with both qt@6 and qt@5 installed (on macOS 14.6). No issue (to the builder) when building the Qt6 PR branch. Interesting that we have this regression again, assuming related to the CMAKE migration. Related discussions:

- https://github.com/bitcoin/bitcoin/issues/21901
- https://github.com/bitcoin/bitcoin/issues/25947
- https://github.com/bitcoin/bitcoin/pull/25948

Previous version of this regression fixed by: https://github.com/
...

💬 promag commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2389755719)
Strong concept ACK 🚀 FWIW 6.7.3 was released recently.

🤔 glozow reviewed a pull request: "fuzz: fix bug in p2p_headers_presync harness"
(https://github.com/bitcoin/bitcoin/pull/30980#pullrequestreview-2344167597)
makes sense, utACK a7498cc7e26b4b3de976e111de2bd2d79b056b31

🤔 mzumsande reviewed a pull request: "fuzz: fix bug in p2p_headers_presync harness"
(https://github.com/bitcoin/bitcoin/pull/30980#pullrequestreview-2344178489)
ACK https://github.com/bitcoin/bitcoin/commit/a7498cc7e26b4b3de976e111de2bd2d79b056b31

💬 achow101 commented on pull request "refactor: move util/pcp and util/netif to common/":
(https://github.com/bitcoin/bitcoin/pull/31011#issuecomment-2389797994)
ACK fd38711217cafbd62e8abd22d2b43f85fede8cde

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1785316213)
Updated

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1785316256)
Good point. Updated.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1785316316)
Agreed. Updated

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1785316368)
Good point. Test added.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2389805371)
Updated to address comments from @glozow

Removed default argument from `ParseVerbosity()`, cleaned up comments, and updated the functional test to check negative verbosity.

```
git range-diff fc642c33ef28829eda0119a0fe39fd9bc4b84051..b6368fc285bf00b3033061dcd4e29298b227c6df fc642c33ef28829eda0119a0fe39fd9bc4b84051..98c1536852d1de9a978b11046e7414e79ed40b46
```

🚀 achow101 merged a pull request: "refactor: move util/pcp and util/netif to common/"
(https://github.com/bitcoin/bitcoin/pull/31011)

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2389806502)
> Ran Doxygen and inspected `ParseVerbosity` docs to confirm `[in]`-parameters were handled well etc.
Much appreciated

💬 achow101 commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2389837346)
ACK fa2b7d8d6b3f8d53199921e1e542072441b26fab

🚀 achow101 merged a pull request: "log: Enforce trailing newline"
(https://github.com/bitcoin/bitcoin/pull/30929)

👍 tdb3 approved a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2344305697)
code review and light test ACK 683b558a020f1632b0a3cbdaa165adbd1423281c

Good catch with age/services