💬 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.
💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610635130)
First letter capitalization should match your strings on 304:305, if they are kept.
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610635130)
First letter capitalization should match your strings on 304:305, if they are kept.
💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610578325)
This will be more work to maintain than not worrying about what part of the string is what and only care about finding matches in SHA256SUMS.
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610578325)
This will be more work to maintain than not worrying about what part of the string is what and only care about finding matches in SHA256SUMS.
💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610568644)
I don't see the point of bumping the version numbers in the examples. They'll always get old.
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610568644)
I don't see the point of bumping the version numbers in the examples. They'll always get old.
💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610610813)
The `VERSION_PLATFORM` element `"apple"` will never match a fully specified release version string, it will always match `"apple-darwin"` first:
```
ben@ben:~/guix.sigs$ for file in $(grep all.SHA256SUMS <<< $(find .) | sed '/.asc/d' | uniq); do cat $file; done | grep -c "apple"
4551
ben@ben:~/guix.sigs$ for file in $(grep all.SHA256SUMS <<< $(find .) | sed '/.asc/d' | uniq); do cat $file; done | grep -c "apple-darwin"
4551
```
Including both `"apple"` and `"apple-darwin"` in `VERSION_PLA
...
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610610813)
The `VERSION_PLATFORM` element `"apple"` will never match a fully specified release version string, it will always match `"apple-darwin"` first:
```
ben@ben:~/guix.sigs$ for file in $(grep all.SHA256SUMS <<< $(find .) | sed '/.asc/d' | uniq); do cat $file; done | grep -c "apple"
4551
ben@ben:~/guix.sigs$ for file in $(grep all.SHA256SUMS <<< $(find .) | sed '/.asc/d' | uniq); do cat $file; done | grep -c "apple-darwin"
4551
```
Including both `"apple"` and `"apple-darwin"` in `VERSION_PLA
...
💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610571290)
I also added an "-x86_64-linux-gnu" example here as it's likely among the top choices a user wants to specifically download & verify.
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610571290)
I also added an "-x86_64-linux-gnu" example here as it's likely among the top choices a user wants to specifically download & verify.
💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610627298)
This prevents people from specifying a version string like:
```
./verify.py pub 27.0-win64-setup.exe
./verify.py pub 27.0-win64.zip
```
To fetch one of the specific files to download as we also have diversity in file extensions among -arch-platforms.
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610627298)
This prevents people from specifying a version string like:
```
./verify.py pub 27.0-win64-setup.exe
./verify.py pub 27.0-win64.zip
```
To fetch one of the specific files to download as we also have diversity in file extensions among -arch-platforms.
💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610635610)
Suggest changing to:
```python3
f"Example architectures: {VERSION_ARCHES}\n"
f"Example platforms: {VERSION_PLATFORMS}\n"
```
or
```python3
f"Suggested architectures: {VERSION_ARCHES}\n"
f"Suggested platforms: {VERSION_PLATFORMS}\n"
```
so it can become out of date but not become wrong.
(https://github.com/bitcoin/bitcoin/pull/28418#discussion_r1610635610)
Suggest changing to:
```python3
f"Example architectures: {VERSION_ARCHES}\n"
f"Example platforms: {VERSION_PLATFORMS}\n"
```
or
```python3
f"Suggested architectures: {VERSION_ARCHES}\n"
f"Suggested platforms: {VERSION_PLATFORMS}\n"
```
so it can become out of date but not become wrong.
💬 BenWestgate commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2125872340)
crACK https://github.com/bitcoin/bitcoin/commit/84900ac34f6888b7a851d0a6a5885192155f865c
Have not tested on a Windows machine.
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2125872340)
crACK https://github.com/bitcoin/bitcoin/commit/84900ac34f6888b7a851d0a6a5885192155f865c
Have not tested on a Windows machine.
💬 BenWestgate commented on pull request "guix: Use DOS newlines for SHA256SUMS files":
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2125882826)
> The UX for downloading two verification files sucks IMO. I think we should just go for the clearsign option, even if it's exclusive.
>
> Thoughts?
We have [`./contrib/verify-binaries/verify.py`](https://github.com/bitcoin/bitcoin/tree/master/contrib/verify-binaries) in this repository to make verification UX better. I just reviewed a PR and made a PR to it that makes it able to let a single file be downloaded & verified. Changes to here require changes to that script, right?
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2125882826)
> The UX for downloading two verification files sucks IMO. I think we should just go for the clearsign option, even if it's exclusive.
>
> Thoughts?
We have [`./contrib/verify-binaries/verify.py`](https://github.com/bitcoin/bitcoin/tree/master/contrib/verify-binaries) in this repository to make verification UX better. I just reviewed a PR and made a PR to it that makes it able to let a single file be downloaded & verified. Changes to here require changes to that script, right?