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

💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2085298601)
Unfortunately I'm not able to find any documentation for this.

There is a UPnP plugin, but that's now what you're using (and probably best left uninstalled).

Anyway, people who run OPNsense will now how to manually forward a port, so that's not really the target demographic here.

💬 ajtowns commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-2085314148)
ACK e504b1fa1fa4d014b329dea81bfdf1aa55db238f ; LGTM and just checking the 1-of-3 case seems fine

📝 fanquake converted_to_draft 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.

👍 TheCharlatan approved a pull request: "doc: add LLVM instruction for macOS < 13"
(https://github.com/bitcoin/bitcoin/pull/29934#pullrequestreview-2031360698)
ACK 22574046c90c0662f3aa9b1baea074aff54f92a9

💬 hebasto commented on pull request "deploy: remove some tools when cross-compiling for macOS":
(https://github.com/bitcoin/bitcoin/pull/29890#issuecomment-2085348229)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/180.

👍 ryanofsky approved a pull request: "rpc: Remove index-based Arg accessor"
(https://github.com/bitcoin/bitcoin/pull/29997#pullrequestreview-2031418230)
Code review ACK fadb3eb57b4d207a678067b89caa45abf1f93702

💬 ryanofsky commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584855621)
In commit "rpc: Remove index-based Arg accessor" (fadb3eb57b4d207a678067b89caa45abf1f93702)

Should s/check_positional/check_named/

💬 maflcko commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584862055)
I think this means the positional `request.params` accessor, so this is better to leave as-is, no?