🤔 Sjors reviewed a pull request: "[PoC, nomerge] IPv6 PCP pinhole test"
(https://github.com/bitcoin/bitcoin/pull/30005#pullrequestreview-2035221223)
Concept ACK. RFC 6887 is quite readable and simple for a client to support.
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?
Why not also use PCP for IPv4 NAT? If it's widely supported enough, maybe it lets us drop both upnp and natpmp from the dependencies (immediately or at some point in the future if there's actually a problem with them).
This appears to be trivial:
...
(https://github.com/bitcoin/bitcoin/pull/30005#pullrequestreview-2035221223)
Concept ACK. RFC 6887 is quite readable and simple for a client to support.
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?
Why not also use PCP for IPv4 NAT? If it's widely supported enough, maybe it lets us drop both upnp and natpmp from the dependencies (immediately or at some point in the future if there's actually a problem with them).
This appears to be trivial:
...
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587530201)
To make it easier to understand that 0x80 refers to R, which happens to be the most significant bit of second byte of the message, and 0x01 is the actual Opcode:
```h
// PCP Request Header. See section 7.1
constexpr uint8_t PCP_REQUEST = 0x00; // R = 0
// PCP Response Header. See section 7.2
constexpr uint8_t PCP_RESPONSE = 0x80; // R = 1
//! Map opcode. See section 19.2
constexpr uint8_t PCP_OP_MAP = 0x01;
//! Map request opcode.
constexpr uint8_t PCP_OP_MAP_REQUEST = PCP_REQUEST | P
...
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587530201)
To make it easier to understand that 0x80 refers to R, which happens to be the most significant bit of second byte of the message, and 0x01 is the actual Opcode:
```h
// PCP Request Header. See section 7.1
constexpr uint8_t PCP_REQUEST = 0x00; // R = 0
// PCP Response Header. See section 7.2
constexpr uint8_t PCP_RESPONSE = 0x80; // R = 1
//! Map opcode. See section 19.2
constexpr uint8_t PCP_OP_MAP = 0x01;
//! Map request opcode.
constexpr uint8_t PCP_OP_MAP_REQUEST = PCP_REQUEST | P
...
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587412293)
Did you mean `pcp6: Could not connect to gateway`?
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587412293)
Did you mean `pcp6: Could not connect to gateway`?
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587310888)
You could make this a permanent debugging utility by adding it to `bitcoin-util`.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587310888)
You could make this a permanent debugging utility by adding it to `bitcoin-util`.
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587549683)
We should explicitly set that we want this option to be mandatory (if we do indeed care that much):
> Its most significant bit indicates if this
> option is mandatory (0) or optional (1) to process.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587549683)
We should explicitly set that we want this option to be mandatory (if we do indeed care that much):
> Its most significant bit indicates if this
> option is mandatory (0) or optional (1) to process.
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587540017)
In a real implementation we should log the meaning of each result code, if known.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587540017)
In a real implementation we should log the meaning of each result code, if known.
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587560006)
IIUC this might as well be called `RequestPortMap`, with the clarification that this boils down to a pinhole for IPv6.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587560006)
IIUC this might as well be called `RequestPortMap`, with the clarification that this boils down to a pinhole for IPv6.
💬 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.
(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.
(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)?
(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.
(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
(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).
(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
(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
...
(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).
(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)
(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?
(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.
(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.
(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.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587616861)
Agree. There's not that many anyway.