💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610540459)
> If we can't drop it, can we be sure that we encounter RTA_OIF before RTA_GATEWAY? Otherwise if (rta_gateway != nullptr) could trigger prematurely.
AFAIK the order of the attributes within a record can be arbitrary. But how can this go wrong? The `rta_gateway` check is only after going over all the attributes, right?
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610540459)
> If we can't drop it, can we be sure that we encounter RTA_OIF before RTA_GATEWAY? Otherwise if (rta_gateway != nullptr) could trigger prematurely.
AFAIK the order of the attributes within a record can be arbitrary. But how can this go wrong? The `rta_gateway` check is only after going over all the attributes, right?
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610543822)
Right, will update this (though yeah for POSIX operating systems there's no difference).
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610543822)
Right, will update this (though yeah for POSIX operating systems there's no difference).
💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610546508)
Probably worth doing now to make it more clear over time. Let me know what you think.
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610546508)
Probably worth doing now to make it more clear over time. Let me know what you think.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610547164)
For the FreeBSD versus Linux differences see the comments in the standalone tool here: https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1604635967
The netlink calls behave differently on Linux and FreeBSD, we don't know why this is.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610547164)
For the FreeBSD versus Linux differences see the comments in the standalone tool here: https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1604635967
The netlink calls behave differently on Linux and FreeBSD, we don't know why this is.
💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610554624)
> > I'm not sure we should rely on the `pong` alone because that might change over time?
>
> If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.
I don't think thats applicable here. We cannot freely change the initial negotiation phase (the version-verack window) without a BIP for the p2p protocol change. At the protocol level, only features negotiation messages are allowed in this phase. An
...
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610554624)
> > I'm not sure we should rely on the `pong` alone because that might change over time?
>
> If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.
I don't think thats applicable here. We cannot freely change the initial negotiation phase (the version-verack window) without a BIP for the p2p protocol change. At the protocol level, only features negotiation messages are allowed in this phase. An
...
💬 sdaftuar commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610556549)
Seems more readable to me, thanks.
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610556549)
Seems more readable to me, thanks.
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610559719)
> The rta_gateway check is only after going over all the attributes, right?
Oh wait, I misread the indentation, yes.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610559719)
> The rta_gateway check is only after going over all the attributes, right?
Oh wait, I misread the indentation, yes.
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610568498)
> The scope ID is extremely important for the default gateway, as it tends to be a scope-local address.
Aha: https://blogs.infoblox.com/ipv6-coe/fe80-1-is-a-perfectly-valid-ipv6-default-gateway-address/
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610568498)
> The scope ID is extremely important for the default gateway, as it tends to be a scope-local address.
Aha: https://blogs.infoblox.com/ipv6-coe/fe80-1-is-a-perfectly-valid-ipv6-default-gateway-address/
💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610572476)
> > > I'm not sure we should rely on the `pong` alone because that might change over time?
> >
> >
> > If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.
>
> I don't think thats applicable here. We cannot freely change the initial negotiation phase (the version-verack window) without a BIP for the p2p protocol change. At the protocol level, only features negotiation messages are allowed i
...
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610572476)
> > > I'm not sure we should rely on the `pong` alone because that might change over time?
> >
> >
> > If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.
>
> I don't think thats applicable here. We cannot freely change the initial negotiation phase (the version-verack window) without a BIP for the p2p protocol change. At the protocol level, only features negotiation messages are allowed i
...
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610572586)
Ah, some of these comments are worth preserving until we know more or can point to clear documentation elsewhere. I compressed them a bit:
```
// Linux IPv4 / IPv6 - this must be present, otherwise no gateway is found
// FreeBSD IPv4 - does not matter, the gateway is found with or without this
// FreeBSD IPv6 - this must be absent, otherwise no gateway is found
request.hdr.nlmsg_flags |= NLM_F_DUMP;
#ifdef __FreeBSD__
// Linux IPv4 / IPv6 this must be absent, other
...
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610572586)
Ah, some of these comments are worth preserving until we know more or can point to clear documentation elsewhere. I compressed them a bit:
```
// Linux IPv4 / IPv6 - this must be present, otherwise no gateway is found
// FreeBSD IPv4 - does not matter, the gateway is found with or without this
// FreeBSD IPv6 - this must be absent, otherwise no gateway is found
request.hdr.nlmsg_flags |= NLM_F_DUMP;
#ifdef __FreeBSD__
// Linux IPv4 / IPv6 this must be absent, other
...
💬 achow101 commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#issuecomment-2125651180)
ACK 6629d1d0f8285d1bf2d87341a856abe903f26c13
(https://github.com/bitcoin/bitcoin/pull/30118#issuecomment-2125651180)
ACK 6629d1d0f8285d1bf2d87341a856abe903f26c13
💬 sipa commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2125658959)
It's unclear to me whether the `UniValue` type has a move constructor, actually. If not, then these `std::move`s have no effect.
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2125658959)
It's unclear to me whether the `UniValue` type has a move constructor, actually. If not, then these `std::move`s have no effect.
💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2125672769)
> It's unclear to me whether the `UniValue` type has a move constructor, actually. If not, then these `std::move`s have no effect.
@sipa I worked up a quick bench to test exactly that: https://github.com/theuni/bitcoin/commit/35d5ffc793b59bd03b8f66bf68847ab773e914f3
The numbers look as you'd expect.
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 7,273,304.00 |
...
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2125672769)
> It's unclear to me whether the `UniValue` type has a move constructor, actually. If not, then these `std::move`s have no effect.
@sipa I worked up a quick bench to test exactly that: https://github.com/theuni/bitcoin/commit/35d5ffc793b59bd03b8f66bf68847ab773e914f3
The numbers look as you'd expect.
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 7,273,304.00 |
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610594332)
Sure, will add that.
FWIW, this is why i initially went with parsing the route tables from `/proc/net/...`, for Linux that's the most straightforward implementation. Netlink is a bit finnicky, though it should be stable (for the same OS) because it's what the tools like `ip` use.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610594332)
Sure, will add that.
FWIW, this is why i initially went with parsing the route tables from `/proc/net/...`, for Linux that's the most straightforward implementation. Netlink is a bit finnicky, though it should be stable (for the same OS) because it's what the tools like `ip` use.
💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1610597554)
As an alternative: How about making this a set of enums whose variants encode the various errors? Then we can add a function to convert the enums to strings (the enums and the conversion function could live in the kernel namespace) and use that to populate the vector for `GetMessages()`.
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1610597554)
As an alternative: How about making this a set of enums whose variants encode the various errors? Then we can add a function to convert the enums to strings (the enums and the conversion function could live in the kernel namespace) and use that to populate the vector for `GetMessages()`.
💬 mzumsande commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1610611867)
sure - updated.
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1610611867)
sure - updated.
💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610622809)
> Yes, that is what I am trying to say. The pong alone should be necessary and sufficient.
Ah ok. "goto the first message".. now we are sync. Nice bikeshedding from my end.
So yeah, we could wait only for the `pong` or introduce a new field on the `getpeerinfo` output (`fSuccessfullyConnected` with another name like "handshake_completed").
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610622809)
> Yes, that is what I am trying to say. The pong alone should be necessary and sufficient.
Ah ok. "goto the first message".. now we are sync. Nice bikeshedding from my end.
So yeah, we could wait only for the `pong` or introduce a new field on the `getpeerinfo` output (`fSuccessfullyConnected` with another name like "handshake_completed").
🤔 BenWestgate requested changes to a pull request: "contrib: verify-binaries accept full arch-platform specifier"
(https://github.com/bitcoin/bitcoin/pull/28418#pullrequestreview-2072230792)
Approach NACK
Does not seem to completely solve the "Download just one file" use case in the commit description for certain inputs. Will fail with error if the user attempts to specify further the file they want downloaded. More complex code than necessary that requires updates if new platforms or architectures are supported by Bitcoin.
This is too much shoe horning and a pattern match on the file name in SHA256SUM, after special handling `<version>[-rcN]` since these have special rules, s
...
(https://github.com/bitcoin/bitcoin/pull/28418#pullrequestreview-2072230792)
Approach NACK
Does not seem to completely solve the "Download just one file" use case in the commit description for certain inputs. Will fail with error if the user attempts to specify further the file they want downloaded. More complex code than necessary that requires updates if new platforms or architectures are supported by Bitcoin.
This is too much shoe horning and a pattern match on the file name in SHA256SUM, after special handling `<version>[-rcN]` since these have special rules, s
...
💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610579617)
This line may be too long 117 characters w/ comment
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610579617)
This line may be too long 117 characters w/ comment
💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610569530)
This sentence is better than master. I'm borrowing it.
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610569530)
This sentence is better than master. I'm borrowing it.