📝 fanquake opened a pull request: "releases: use LLVM 18 for macOS"
(https://github.com/bitcoin/bitcoin/pull/30022)
Bumps the Guix time-machine to include a bump to LLVM 18.1.4: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=837016fe33e11c6252ec24c423d1b6ae0cd7efd3. Can split this out given it effects all releases.
Needs another patch to Qt. It's internal libpng build is broken with the newer Clang, due to:
> A new Clang extension (see [here](https://releases.llvm.org/18.1.0/tools/clang/docs/ReleaseNotes.html?1=#target-os-detail)) is enabled for Darwin (Apple platform) targets. Clang now defines TA
...
(https://github.com/bitcoin/bitcoin/pull/30022)
Bumps the Guix time-machine to include a bump to LLVM 18.1.4: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=837016fe33e11c6252ec24c423d1b6ae0cd7efd3. Can split this out given it effects all releases.
Needs another patch to Qt. It's internal libpng build is broken with the newer Clang, due to:
> A new Clang extension (see [here](https://releases.llvm.org/18.1.0/tools/clang/docs/ReleaseNotes.html?1=#target-os-detail)) is enabled for Darwin (Apple platform) targets. Clang now defines TA
...
💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1587494035)
Actually `SNAPSHOT_BASE_HEIGHT` is 299, and here I am trying to make sure that the snapshot was deleted and therefore the node is now using it's previous chain which has less work. I changed it to `assert node.getblockcount() < SNAPSHOT_BASE_HEIGHT` for more clarity
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1587494035)
Actually `SNAPSHOT_BASE_HEIGHT` is 299, and here I am trying to make sure that the snapshot was deleted and therefore the node is now using it's previous chain which has less work. I changed it to `assert node.getblockcount() < SNAPSHOT_BASE_HEIGHT` for more clarity
💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1587496652)
Yes, good catch. This line was removed since the iteration is no longer needed based on your other comment below.
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1587496652)
Yes, good catch. This line was removed since the iteration is no longer needed based on your other comment below.
💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1587497230)
That's a good one. Thanks. I Fixed it.
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1587497230)
That's a good one. Thanks. I Fixed it.
💬 paplorinc commented on pull request "refactor, fuzz: Make 64-bit shift explicit":
(https://github.com/bitcoin/bitcoin/pull/30017#discussion_r1587506138)
I don't have MSVC to test, just seemed suspicious. If it was deliberate, please resolve this comment.
(https://github.com/bitcoin/bitcoin/pull/30017#discussion_r1587506138)
I don't have MSVC to test, just seemed suspicious. If it was deliberate, please resolve this comment.
💬 hebasto commented on pull request "refactor, fuzz: Make 64-bit shift explicit":
(https://github.com/bitcoin/bitcoin/pull/30017#discussion_r1587527245)
> I don't have MSVC to test, just seemed suspicious.
Do you mind elaborating your concerns?
> If it was deliberate, please resolve this comment.
The MSVC warning C4334 is caused by the type of the left-hand side operand in the `<<` operator. This PR is focused on resolving only this issue. I can't see reasons to introduce unrelated code modifications.
(https://github.com/bitcoin/bitcoin/pull/30017#discussion_r1587527245)
> I don't have MSVC to test, just seemed suspicious.
Do you mind elaborating your concerns?
> If it was deliberate, please resolve this comment.
The MSVC warning C4334 is caused by the type of the left-hand side operand in the `<<` operator. This PR is focused on resolving only this issue. I can't see reasons to introduce unrelated code modifications.
✅ laanwj closed an issue: "Objdump can't parse our Linux debug information"
(https://github.com/bitcoin/bitcoin/issues/30016)
(https://github.com/bitcoin/bitcoin/issues/30016)
💬 laanwj commented on issue "Objdump can't parse our Linux debug information":
(https://github.com/bitcoin/bitcoin/issues/30016#issuecomment-2090370225)
This was already solved upstream!
Diff: https://sourceware.org/git/?p=binutils-gdb.git;a=blobdiff;f=binutils/objdump.c;h=3d70df470f290f7e4dc46cf80bc075f3015e6c14;hp=5acaa54929dbd4bbe37d7a2195ca698bfca603d0;hb=635d05b88f4823f46ef1ddbb3d438db16c0f6e71;hpb=5ce0e02478cc79a260c7e29822450284a32b9b12
i expect this will land in binutils 2.43. No need to keep this issue open.
(https://github.com/bitcoin/bitcoin/issues/30016#issuecomment-2090370225)
This was already solved upstream!
Diff: https://sourceware.org/git/?p=binutils-gdb.git;a=blobdiff;f=binutils/objdump.c;h=3d70df470f290f7e4dc46cf80bc075f3015e6c14;hp=5acaa54929dbd4bbe37d7a2195ca698bfca603d0;hb=635d05b88f4823f46ef1ddbb3d438db16c0f6e71;hpb=5ce0e02478cc79a260c7e29822450284a32b9b12
i expect this will land in binutils 2.43. No need to keep this issue open.
💬 dergoegge commented on pull request "releases: use LLVM 18 for macOS":
(https://github.com/bitcoin/bitcoin/pull/30022#discussion_r1587550128)
this download link works but ubuntu-18?
(https://github.com/bitcoin/bitcoin/pull/30022#discussion_r1587550128)
this download link works but ubuntu-18?
🤔 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.