💬 fanquake commented on pull request "releases: use LLVM 18 for macOS":
(https://github.com/bitcoin/bitcoin/pull/30022#discussion_r1587589176)
Yea. While using the pre-built bins, we have to take what is available. For some reason the person building the x86_64 bins here has reverted to that. It also means we'll need to revert to installing libtinfo5 in the macOS CI, which is also annoying, because you also can't actually install that on Ubuntu 24.04 (although we currently still use 22.04 here, so it'll work for now).

👍 TheCharlatan approved a pull request: "refactor: Avoid unused-variable warning in init.cpp"
(https://github.com/bitcoin/bitcoin/pull/29968#pullrequestreview-2035683492)
ACK fa9abf968840745e428a86a9545aaa6c923415e2

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090448249)
Thanks for the review!

> IPv6 always returns multiple addresses. I'm not sure what the logic behind that is, but should we gossiping and pinholing all of them? Or is one "supposed" to be used?

That's a good question. Sometimes routers will give out temporary addresses as well as permanent addresses, i'm not sure it's possible to discover the intent of an address at application level. If so it is going to be OS specific. Currently in master the application Discover()s alll publicly routable
...

💬 fanquake commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090454836)
Could you rebase this on master, now that we are using the latest version of miniupnpc (#29707).

🚀 fanquake merged a pull request: "refactor: Avoid unused-variable warning in init.cpp"
(https://github.com/bitcoin/bitcoin/pull/29968)

💬 maflcko commented on pull request "releases: use LLVM 18 for macOS":
(https://github.com/bitcoin/bitcoin/pull/30022#discussion_r1587610125)
Or waiting after https://github.com/bitcoin/bitcoin/pull/21778, which should be fine as well, as there is no rush to use clang 18?

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587614730)
As i understand, mandatory or optional is a property of the opcode. `PCP_OPTION_PREFER_FAILURE` is `0x02`, so It's always mandatory.

We want the address and port that we request, if not possible, it's better to fail fast, instead of having to deal with having to cancel unwanted bindings etc.

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587616006)
Correct. The rest of the fields can be left at 0. i'll add a comment.

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587616861)
Agree. There's not that many anyway.

💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587618250)
Maybe I misread it, but I think we're implicitly doing:

```h
PCP_OPTION_MANDATORY = 0x00
... PCP_OPTION_PREFER_FAILURE | PCP_OPTION_MANDATORY
```

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587621612)
`Sock` is used below, which is the mockable socket. The only exception is construction, so yea for testing we'd want a function where the Sock is passed in.

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587626026)
It might be useful for troubleshooting, though i'm not sure how happy people will be to add networking stuff to bitcoin-util. Currently it's pure-function stuff only 😄

💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090487679)
> i'm kind of hestitant to touch that, given how hard it already is to find people to test these kind of things

For now we could add support in the test, but stick to IPv6 in the implementation. OTOH both UPnP and NAP-PMP are off by default. Adding another off-by-default checkbox "IPv6 pinhole" to the GUI seems pretty confusing, and probably doesn't increase the chance of users trying this.

💬 fanquake commented on pull request "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set":
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2090498778)
This will likely fail one last time, but should be good after #30012.

💬 instagibbs commented on pull request "opportunistic 1p1c followups":
(https://github.com/bitcoin/bitcoin/pull/30012#issuecomment-2090502979)
reACK https://github.com/bitcoin/bitcoin/pull/30012/commits/7f6fb73c82fdff2afe5edefcc335ba6707d5627d

just the non-pointer copying added

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090509155)
> Could you rebase this on master, now that we are using the latest version of miniupnpc (https://github.com/bitcoin/bitcoin/pull/29707).

Yes, but first want to address @Sjors's comments. Mind that this adds an utility and is orthogonal to UPnP. Adding IPv6 pinholing through UPnP would be useful too (for routers that support that, and not PCP), so we'll likely want to be able to do both in the eventual design like we do for natpmp/UPnP for IPv4.

> For now we could add support in the test,
...

👍 brunoerg approved a pull request: "contrib: Add asmap-tool"
(https://github.com/bitcoin/bitcoin/pull/28793#pullrequestreview-2035758814)
ACK 6abe772a17e09fe96e68cd3311280d5a30f6378b

I've been using this for a while but did some new tests to ensure.

I've tested encoding/decoding as well as the diff command with a very old file (1 year ago) and a newest one to test big changes - e.g.: `# 1296418840 (2^30.27) IPv4 addresses changed; 8741211000446919691919144517632603 (2^112.75) IPv6 addresses changed).`. I think code could be clearer but lgtm to merge as is.

💬 fanquake commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090522477)
> Mind that this adds an utility and is orthogonal to UPnP.

~0. Right, I have misunderstood what this is/does. This could be a useful tool (for some % of users of this software), but I don't think this repository is the right place for it to live, or that it fits into the set of things we should be maintaining here.

💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090546884)
This is a temporary PR for people to test this code, i intend to integrate it in bitcoind just like the current IPv4 port mapping stuff. I did not intend it to be a useful tool to keep around. This is why the title says "nomerge".

💬 fanquake commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090550845)
> I did not intend it to be a useful tool to keep around.

Fair enough, I'll -redirect my NACK to this thread: https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587310888.