💬 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.
💬 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
```
(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.
(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.