feat: use dashcore-binaries.thepasta.org as keybase.pub is defunct and validate all .asc files #5820

PastaPastaPasta · 2024-01-14T04:47:05Z

Issue being fixed or feature implemented

keybase.pub isn't a thing anymore; instead use thepasta.org; also validate all .asc files

What was done?

How Has This Been Tested?

Validated 20.0.4, 20.0.3 and 20.0.2 with the script

Breaking Changes

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

knst · 2024-01-14T06:56:21Z

contrib/verifybinaries/verify.sh

@@ -25,7 +25,7 @@ TMPFILE="hashes.tmp"
 SIGNATUREFILENAME="SHA256SUMS.asc"
 RCSUBDIR="test"
 HOST1="https://github.com/dashpay/dash/releases/download/v"
-HOST2="https://pasta.keybase.pub/Dash-Core-Releases/"
+HOST2="https://dashcore-binaries.thepasta.org/file/dashcore-binaries/"


why not dash.org then?

Dash.org doesn't host any binaries at this point at least

we can setup releasing process to upload this binaries before official release but not somedays later as now

Huh? dash.org/downloads simply links to the GitHub artifacts

DashCoreAutoGuix · 2024-01-14T17:56:05Z

Guix Automation has began to build this PR tagged as v20.1.0-devpr5820.f3cdbbea. A new comment will be made when the image is pushed.

DashCoreAutoGuix · 2024-01-14T20:00:04Z

Guix Automation has began to build this PR tagged as v20.1.0-devpr5820.a07ba071. A new comment will be made when the image is pushed.

contrib/verifybinaries/verify.sh

UdjinM6

utACK

knst

First at all, bitcoin migrated this script from bash to python.
So, before adopt this script and writing any new code, let's migrate to python.

I backported here these changes (but not new code):

Secondly, I got this error when I am running script from this branch:

$ contrib/verifybinaries/verify.sh 20.0.3
pasta.keybase.pub and github.com signature files were not equal?

but for version 20.0.4 - work, 20.0.2 - works, 20.0.3 - doesn't.
Is there any bug? need to fix and test it for all reasonable many version (at least up to v19 IMO)

thirdly, Need to dashify contrib/verifybinaries/README.md - that's still not dashyfied by some reason... maybe out of scope of this PR, but if you want to mention this script in any release announcement or something like that - that should be dashified before.

Fourthly, Need to dashify path for tmp binaries also: /tmp/bitcoin_verify_binaries/ is currently - I fixed in my branch with backport (see commits)

And finally, I insist that it should be not your personal website; we have a nice candy domain dash.org and we need to utilize it!

PastaPastaPasta · 2024-01-18T02:15:10Z

NACK to this review

First at all, bitcoin migrated this script from bash to python. So, before adopt this script and writing any new code, let's migrate to python.

I backported here these changes (but not new code):

knst@aca17ad

knst@8636255

I don't really think it's really valuable to migrate this to python; if you'd like to re-implement the changes from this PR as well, then I guess we can move it to python. I don't really want to reimplement my changes

Secondly, I got this error when I am running script from this branch:
$ contrib/verifybinaries/verify.sh 20.0.3
pasta.keybase.pub and github.com signature files were not equal?
but for version 20.0.4 - work, 20.0.2 - works, 20.0.3 - doesn't. Is there any bug? need to fix and test it for all reasonable many version (at least up to v19 IMO)

I am hosting binaries back to 18.0.0 here. I have binaries back to 0.16.0.0 on my key base that I could move over to dashcore-binaries.thepasta.org.

try running it again. Locally I've seen it fail randomly with this see

> $ contrib/verifybinaries/verify.sh 18.0.2
pasta.keybase.pub and github.com signature files were not equal?
                                                                                                                                      
> $ contrib/verifybinaries/verify.sh 18.0.2
stat: illegal option -- c
usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file ...]
contrib/verifybinaries/verify.sh: line 138: [: -eq: unary operator expected
Downloading dashcore-18.0.2-aarch64-linux-gnu.tar.gz

thirdly, Need to dashify contrib/verifybinaries/README.md - that's still not dashyfied by some reason... maybe out of scope of this PR, but if you want to mention this script in any release announcement or something like that - that should be dashified before.

Won't be mentioned anywhere; just for me really

Fourthly, Need to dashify path for tmp binaries also: /tmp/bitcoin_verify_binaries/ is currently - I fixed in my branch with backport (see commits)

Feel free to make a new PR with this; this is a nit imo.

And finally, I insist that it should be not your personal website; we have a nice candy domain dash.org and we need to utilize it!

Sure; but I don't control dash.org, so it would take a lot more effort to get that setup. I don't object to there being a "binaries.dash.org" but I can't really control that. Latte is looking into a potential s3 bucket or something like that that mirrors, but I see no reason why this should block. Previously it pointed to pasta.keybase.pub, now it points to dashcore-binaries.thepasta.org since keybase.pub doesn't exist. Also, this isn't REALLY meant to be for public consumption; the purpose of dashcore-binaries.thepasta.org is simply that it's a place I can store binaries but it's not taking up ~50 gigs on my local machine.

…fy.sh with python rewrite c86b9a6 contrib: remove verify.sh (Sebastian Falbesoner) c84838e contrib: binary verification script verify.sh rewritten in python (Sebastian Falbesoner) Pull request description: The rationale for the PR is the same as for bitcoin#18132: > Most of our test scripts are written in python. We don't have enough reviewers for bash scripts and they tend to be clumsy anyway. Especially when it comes to argument parsing. Note that there are still a lot of things that could be improved in this replacement (e.g. using regexps for version string parsing, adding type annotations, dividing up into more functions, getting a pylint score closer to 10, etc.), but I found the original shell script quite hard to read, so it's possibly still a good first step for an improvement. ~Not sure though if it's worth the reviewers time, and if it's even continued to be used long-term (maybe there are plans to merge it with `get_previous_releases.py`, which partly does the same?), so chasing for Concept ACKs right now.~ ACKs for top commit: laanwj: Tested and code review ACK c86b9a6 Tree-SHA512: f7949eead4ef7e5913fe273923ae5c5299408db485146cf996cdf6f8ad8c0ee4f4b30bb6b08a5964000d97b2ae2e7a1bdc88d11c613c16d2d135d80b444e3b16

fbbb2d4 lint: Fix spelling errors in comments (fyquah) Pull request description: Found some spelling errors while running spelling linter bitcoin#21245 This PR fixes them. ACKs for top commit: fanquake: ACK fbbb2d4 - I thought we just fixed all of these. Tree-SHA512: 95525040001f94e899b778c616cb66ebafb679dff88835b66fccf6349d8eb942d6b7374c536a44e393f13156bce9a32ed57e6a82bb02074d2b3cddb2696addb2

Due to keybase.pub is defunct

knst

Won't be mentioned anywhere; just for me really

so far as it is true, most of my comments are not relevant then and migration to dash.org is out-of-scope of this PR.

Please, check my branch that does backports and re-implement your code:

I don't really think it's really valuable to migrate this to python;

There's very few people who is able to write a good code on bash and I am not one of them to be able to review it properly, yet see comments.

Beside that, the failures are difficult to debug, they have no backtrace, etc.
Just look to this random failure (from your own comment):

> $ contrib/verifybinaries/verify.sh 18.0.2
stat: illegal option -- c
usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file ...]
contrib/verifybinaries/verify.sh: line 138: [: -eq: unary operator expected
Downloading dashcore-18.0.2-aarch64-linux-gnu.tar.gz

why does it continue running and downloads dashcore-18.0.2-aarch64-linux-gnu.tar.gz if that's something failed?

knst · 2024-01-18T08:07:08Z

contrib/verifybinaries/verify.sh

+
+      echo "gpg output:"
+      # shellcheck disable=SC2001
+      echo "$GPGOUT"|sed 's/^/\t/g'


bitcoin steady is ridding of sed over codebase while here it is introduced

knst · 2024-01-18T08:07:47Z

contrib/verifybinaries/verify.sh

@@ -149,6 +149,26 @@ for file in $FILES
 do
   echo "Downloading $file"
   wget --quiet -N "$HOST1$BASEDIR$file"
+   wget --quiet -N "$HOST1$BASEDIR$file.asc"
+
+   GPGOUT=$(gpg --verify "$file.asc" 2>&1)


missing quotes around $()

Suggested change

GPGOUT=$(gpg --verify "$file.asc" 2>&1)

GPGOUT="$(gpg --verify "$file.asc" 2>&1)"

knst · 2024-01-18T08:08:55Z

contrib/verifybinaries/verify.sh

+      echo "gpg output:"
+      # shellcheck disable=SC2001
+      echo "$GPGOUT"|sed 's/^/\t/g'
+      clean_up $SIGNATUREFILENAME $SIGNATUREFILENAME.2 $TMPFILE


missing quotes over $SIGNATUREFILENAME, $TMPFILE

if the file name will have space once or something other you maybe very surprised with a result

knst · 2024-01-18T08:10:31Z

contrib/verifybinaries/verify.sh

@@ -149,6 +149,26 @@ for file in $FILES
 do
   echo "Downloading $file"
   wget --quiet -N "$HOST1$BASEDIR$file"
+   wget --quiet -N "$HOST1$BASEDIR$file.asc"


there's no check for wget result - if it would fail in the middle file instead failure here you will get a failure in gpg command

PastaPastaPasta · 2024-01-18T15:48:31Z

New python version looks good!

knst

ACK (slightly tested)

UdjinM6

utACK

PastaPastaPasta added this to the 20.1 milestone Jan 14, 2024

knst reviewed Jan 14, 2024

View reviewed changes

PastaPastaPasta added the guix-build label Jan 14, 2024

PastaPastaPasta force-pushed the feat-verify-binaries branch from f3cdbbe to a07ba07 Compare January 14, 2024 19:59

PastaPastaPasta force-pushed the feat-verify-binaries branch from a07ba07 to 4c5f651 Compare January 15, 2024 02:00

PastaPastaPasta requested review from UdjinM6 and knst January 16, 2024 21:08

UdjinM6 requested changes Jan 16, 2024

View reviewed changes

contrib/verifybinaries/verify.sh Outdated Show resolved Hide resolved

UdjinM6 previously approved these changes Jan 16, 2024

View reviewed changes

knst requested changes Jan 17, 2024

View reviewed changes

laanwj and others added 4 commits January 18, 2024 15:59

feat: use dashcore-binaries.thepasta.org in verifybinaries script

a1c01f1

Due to keybase.pub is defunct

feat: validate all .asc files by verifybinaries script

f7db372

knst reviewed Jan 18, 2024

View reviewed changes

PastaPastaPasta dismissed UdjinM6’s stale review via f7db372 January 18, 2024 15:48

PastaPastaPasta force-pushed the feat-verify-binaries branch from d6e3e6c to f7db372 Compare January 18, 2024 15:48

knst approved these changes Jan 18, 2024

View reviewed changes

UdjinM6 approved these changes Jan 19, 2024

View reviewed changes

PastaPastaPasta merged commit 7b9623f into dashpay:develop Jan 19, 2024
4 of 9 checks passed

PastaPastaPasta deleted the feat-verify-binaries branch January 19, 2024 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use dashcore-binaries.thepasta.org as keybase.pub is defunct and validate all .asc files #5820

feat: use dashcore-binaries.thepasta.org as keybase.pub is defunct and validate all .asc files #5820

PastaPastaPasta commented Jan 14, 2024

knst Jan 14, 2024

PastaPastaPasta Jan 14, 2024

knst Jan 14, 2024

PastaPastaPasta Jan 14, 2024

DashCoreAutoGuix commented Jan 14, 2024

DashCoreAutoGuix commented Jan 14, 2024

UdjinM6 left a comment

knst left a comment •

edited

Loading

PastaPastaPasta commented Jan 18, 2024

knst left a comment

knst Jan 18, 2024

knst Jan 18, 2024

knst Jan 18, 2024

knst Jan 18, 2024

PastaPastaPasta commented Jan 18, 2024

knst left a comment

UdjinM6 left a comment

	GPGOUT=$(gpg --verify "$file.asc" 2>&1)
	GPGOUT="$(gpg --verify "$file.asc" 2>&1)"

feat: use dashcore-binaries.thepasta.org as keybase.pub is defunct and validate all .asc files #5820

feat: use dashcore-binaries.thepasta.org as keybase.pub is defunct and validate all .asc files #5820

Conversation

PastaPastaPasta commented Jan 14, 2024

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DashCoreAutoGuix commented Jan 14, 2024

DashCoreAutoGuix commented Jan 14, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

knst left a comment • edited Loading

Choose a reason for hiding this comment

PastaPastaPasta commented Jan 18, 2024

knst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PastaPastaPasta commented Jan 18, 2024

knst left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

knst left a comment •

edited

Loading