💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587415945)
See also the mockackable sockets introduced by @vasild in #26812, which - if this goes beyond just PoC - can be used to test how we handle the various failure modes.

💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587569941)
For IPv4 we should print the external address regardless, since we had no expectation.

💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587555746)
So this has the effect of expanding the message by 4 bytes, where we set the first byte to `PCP_OPTION_PREFER_FAILURE` and the rest to zero (since there's no option data)?

💬 paplorinc commented on pull request "refactor, fuzz: Make 64-bit shift explicit":
(https://github.com/bitcoin/bitcoin/pull/30017#discussion_r1587577987)
> Do you mind elaborating your concerns?

Looked like it's still on the LHS of a shift (`(size_bits + 1)) - 1U) << alignment_bits`), but it's not, no concerns, thanks for checking.

💬 maflcko commented on pull request "releases: use LLVM 18 for macOS":
(https://github.com/bitcoin/bitcoin/pull/30022#discussion_r1587581371)
> this download link works but ubuntu-18?

This should be fixed in LLVM 19: https://discourse.llvm.org/t/rfc-improve-binary-security/78121/56

💬 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