💬 ryanofsky 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-2389625672)
Thanks, I didn't read the previous discussion history, so my feedback is only based on the implementation of this PR. not on earlier comments.

I think tinyformat's original decision to throw exceptions when invalid format strings were specified was flexible and sound, and the additional consteval checking we were able to add on top of that was a nice improvement that should be extended more places.

I just don't see a need to rip out tinyformat's simple handling mechanism and replace it wit
...

👍 hodlinator approved a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2344029932)
ACK b6368fc285bf00b3033061dcd4e29298b227c6df

Reviewed `git range-diff master f83a4f6 b6368fc` + "add tx_in_orphanage" commit + GitHub Files Changed.

Thanks for addressing the latest feedback!

`build/test/functional/rpc_getorphantxs.py` passed locally.
Ran Doxygen and inspected `ParseVerbosity` docs to confirm `[in]`-parameters were handled well etc.

💬 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