🤔 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?
🤔 edilmedeiros reviewed a pull request: "doc: Update mentions of generating bitcoin.conf"
(https://github.com/bitcoin/bitcoin/pull/30154#pullrequestreview-2072542001)
Good catch.
I thought this would not make sense for people getting the binaries, but seems that none of the binary distributions include the `doc` folder. IIUIC, this change affects only people who is getting core from source. If so, the update does make sense.
(https://github.com/bitcoin/bitcoin/pull/30154#pullrequestreview-2072542001)
Good catch.
I thought this would not make sense for people getting the binaries, but seems that none of the binary distributions include the `doc` folder. IIUIC, this change affects only people who is getting core from source. If so, the update does make sense.
💬 edilmedeiros commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#discussion_r1610761009)
I don't like this change that much since this hardens the logical link between this file and `contrib/devtools/README.md` without any kind of automation to check if e.g. the section `gen-bitcoin-conf.sh` still exist.
(https://github.com/bitcoin/bitcoin/pull/30154#discussion_r1610761009)
I don't like this change that much since this hardens the logical link between this file and `contrib/devtools/README.md` without any kind of automation to check if e.g. the section `gen-bitcoin-conf.sh` still exist.
💬 edilmedeiros commented on pull request "depends: Fetch miniupnpc sources from an alternative website":
(https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2125912025)
> > > Which one? http://miniupnp.free.fr/ without HTTPS?
> >
> >
> > If that does keep the sha256 hash the same then maybe.
>
> It does. Switched to that website.
This is the same URL I used to update the MacOS package in the macports project a couple of months ago, so there will be more people watching it.
(https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2125912025)
> > > Which one? http://miniupnp.free.fr/ without HTTPS?
> >
> >
> > If that does keep the sha256 hash the same then maybe.
>
> It does. Switched to that website.
This is the same URL I used to update the MacOS package in the macports project a couple of months ago, so there will be more people watching it.
🤔 luke-jr requested changes to a pull request: "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection"
(https://github.com/bitcoin-core/gui/pull/815#pullrequestreview-2072577992)
I'm pretty sure you can just modify the existing line in `setWalletActionsEnabled` to:
```c++
historyAction->setEnabled(enabled && !isPrivacyModeActivated());
```
(https://github.com/bitcoin-core/gui/pull/815#pullrequestreview-2072577992)
I'm pretty sure you can just modify the existing line in `setWalletActionsEnabled` to:
```c++
historyAction->setEnabled(enabled && !isPrivacyModeActivated());
```
💬 luke-jr commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#discussion_r1610785803)
Why move this up top? Now it's after the signal is connected, so there's a tiny chance of a race...
(https://github.com/bitcoin-core/gui/pull/815#discussion_r1610785803)
Why move this up top? Now it's after the signal is connected, so there's a tiny chance of a race...
💬 luke-jr commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#discussion_r1610789375)
This shouldn't be necessary.
(https://github.com/bitcoin-core/gui/pull/815#discussion_r1610789375)
This shouldn't be necessary.
💬 luke-jr commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610799043)
nit
```suggestion
virtual std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() = 0;
```
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610799043)
nit
```suggestion
virtual std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() = 0;
```
💬 luke-jr commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610802187)
AFAIK Tor hidden services do still have port numbers
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1610802187)
AFAIK Tor hidden services do still have port numbers