💬 laanwj commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-2085021744)
> https://github.com/bitcoin/bitcoin/commit/068de7abc5bda9fd9928d7145265ae3a3a851de0 The idea behind the severity level logging is to ultimately have only one macro like Log(category, level) with one consistent, simple interface for everyone -- both developers updating logging in the code and people who read the logs -- and remove all the other macros when we're ready, simplifying the logging macros, tests, and benchmarks. This commit not only would add several more macros, but also with differe
...

💬 iw4p commented on pull request "refactor: convert string formatting to F-strings":
(https://github.com/bitcoin/bitcoin/pull/29969#issuecomment-2085022352)
Hi,
Thank you.
I touched this file to optimize it like using built-in python request library instead of curl and decreasing the request numbers, and one more PR. That's why I decided to go for converting strings to f-string.

📝 fanquake opened a pull request: "depends: fix Qt to not use `-q` with `ar`"
(https://github.com/bitcoin/bitcoin/pull/30004)
BusyBox `ar` doesn't implement `q`, and GNU `ar` treats `qs` like `r`, so replace the Qt usage of `ar cqs` with `ar rc`, which should work properly everywhere. Played around with making this work a different way, but couldn't seem to make it work properly. Open to other working suggestions (our other `AR` substitution still works correctly, it's just this single qmake bootstrap build invocation that is an issue.

✅ laanwj closed a pull request: "net: Modernize logging in UPnP and nat-pmp code"
(https://github.com/bitcoin/bitcoin/pull/29978)

💬 laanwj commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#issuecomment-2085024610)
Honestly, i don't feel like repeating this argument. i'll do everyone a favor and close this.

✅ glozow closed a pull request: "refactor: convert string formatting to F-strings"
(https://github.com/bitcoin/bitcoin/pull/29969)

💬 glozow commented on pull request "refactor: convert string formatting to F-strings":
(https://github.com/bitcoin/bitcoin/pull/29969#issuecomment-2085038645)
Thanks for your interest in contributing! CONTRIBUTING.md and developer-notes.md contain good resources for beginners. As we have 300+ pulls open right now, closing this test refactor to focus review attention on the others (see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring)

💬 stickies-v commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#issuecomment-2085041263)
> i prefer being explicit and not using macros myself TBH

`LogPrintLevel` is a macro too?

> The way it's done now, it's clear where the messages come from and what their level is

What #29256 proposes is that you'd be able to do exactly that with the shorthand `LogWarning(BCLog::NET, "natpmp: initnatpmp() failed with %d error.\n", r_init);`, hence why I thought you'd be interested in chiming in on that pull. Apologies for derailing this PR, that was not my intent.

📝 laanwj opened a pull request: "[PoC, nomerge] IPv6 PCP pinhole test"
(https://github.com/bitcoin/bitcoin/pull/30005)
The overarching goal is to increase the number of connectable nodes that are not in the big datacenters.

**Context:** IPv6 doesn't have NAT. Computers behind a router tend to get a globally routable address. However, by default this address is usually completely firewalled for incoming connections, as a security measure. See issue #17012.

This is where "pinholing" comes in. By sending a request, a machine on the network can request a port to be opened. This is similar to requesting a port
...

💬 laanwj commented on pull request "net: Modernize logging in UPnP and nat-pmp code":
(https://github.com/bitcoin/bitcoin/pull/29978#issuecomment-2085108270)
> What https://github.com/bitcoin/bitcoin/pull/29256 proposes is that you'd be able to do exactly that with the shorthand LogWarning(BCLog::NET, "natpmp: initnatpmp() failed with %d error.\n", r_init);, hence why I thought you'd be interested in chiming in on that pull. Apologies for derailing this PR, that was not my intent.

Thanks, i'll keep it in mind.

> Apologies for derailing this PR, that was not my intent.

No need for apologies ! you're right, i don't know what has been discussed
...

👍 brunoerg approved a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2031084551)
reACK 6d9083b249376d503621da7980ef7ae02e690e0b

💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2085153558)
> I think most of the criticism of this PR from AJ is saying that the changes here are unnecessary, not really that they are harmful,

I said "As I've already said, I think `m_log.SetCategory(NET); m_log.Debug("connected to foo")` is much worse than `LogDebug(NET, "connected to foo")`." Does that really not come across as me saying they're harmful? I really don't think I'm being **that** unclear. Anyway, I'll bow out, I'm already sorry for touching this.

✅ ajtowns closed a pull request: "logging: Update to new logging API"
(https://github.com/bitcoin/bitcoin/pull/29231)

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2085169447)
Rebased for #29985 (add/add).

💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2085177069)
Tried with a router running OPNsense:

```
2024-04-30T12:16:13Z [net:debug] pcp6: gateway: ...
2024-04-30T12:16:13Z [net:warning] pcp6: Opening pinhole for addr: ...
2024-04-30T12:16:14Z [net:debug] pcp6: Timeout
2024-04-30T12:16:14Z [net:debug] pcp6: Retrying (1)
2024-04-30T12:16:15Z [net:debug] pcp6: Timeout
2024-04-30T12:16:15Z [net:debug] pcp6: Retrying (2)
2024-04-30T12:16:16Z [net:debug] pcp6: Timeout
2024-04-30T12:16:16Z [net:debug] pcp6: Giving up after 3 tries
2024-04-30T12:1
...

💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1584709863)
In 6b4693ee859d6bddf559a0b32749b1dafe015ef4 "net: implement opening PRIVATE_BROADCAST connections": Just a question: why using `compare_exchange_weak` instead of `fetch_sub`?

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2085190180)
It may be some router setting to enable it, the more secure routers have these kind of protocols disabled by default. Or maybe it doesn't support it. There isn't any reply, not even 2 NOT_AUTHORIZED.

💬 laanwj commented on pull request "logging: Update to new logging API":
(https://github.com/bitcoin/bitcoin/pull/29231#issuecomment-2085194038)
@ajtowns Sorry, i didn't mean to derail this, i shouldn't have commented.

💬 laanwj commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#issuecomment-2085258103)
Concept ACK

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2085290323)
> > I think most of the criticism of this PR from AJ is saying that the changes here are unnecessary, not really that they are harmful,
>
> I said "As I've already said, I think `m_log.SetCategory(NET); m_log.Debug("connected to foo")` is much worse than `LogDebug(NET, "connected to foo")`." Does that really not come across as me saying they're harmful? I really don't think I'm being **that** unclear. Anyway, I'll bow out, I'm already sorry for touching this.

I think it's possible you migh
...