script: previous_release.sh rewritten in python #19205

bliotti commented at 1:01 AM on June 8, 2020: contributor

Added functionality:

checks file hash before untarring when using the binary download option

fanquake added the label Scripts and tools on Jun 8, 2020

Rickyricky1987 approved

in contrib/devtools/previous-release.py:22 in 6e4a07c83c outdated

  17 | +import sys
  18 | +import hashlib
  19 | +
  20 | +def usage(args, extras):
  21 | +    print('Usage: {} [options] tag1 [tag2..tagN]'.format(sys.argv[0]))
  22 | +    print('Specify release tag(s), e.g.: {} v0.18.1'.format(sys.argv[0]))

Sjors commented at 9:58 AM on June 8, 2020:

Can you mention a release candidate example as well, e.g. v0.20.0rc2?

in contrib/devtools/previous-release.py:72 in 6e4a07c83c outdated

  67 | +        ret = subprocess.call(cmd)
  68 | +        if ret:
  69 | +            return ret
  70 | +
  71 | +    hasher = hashlib.sha256()
  72 | +    with open(tarball, "rb", encoding="utf-8") as afile:

Sjors commented at 10:05 AM on June 8, 2020:

encoding="utf-8" shouldn't be necessary and results in ValueError: binary mode doesn't take an encoding argument.

in contrib/devtools/previous-release.py:147 in 6e4a07c83c outdated

 142 | +        Path('bin').mkdir(exist_ok=True)
 143 | +        files = ['bitcoind', 'bitcoin-cli', 'bitcoin-tx']
 144 | +        for f in files:
 145 | +            Path('src/'+f).rename('bin/'+f)
 146 | +        if args.functional_tests:
 147 | +            Path('src/qt/bitcoin-qt').rename('bin/bitcoin-qt')

Sjors commented at 10:17 AM on June 8, 2020:

This script should not build QT (see below), so this can be dropped.

in contrib/devtools/previous-release.py:122 in 6e4a07c83c outdated

 117 | +        host = args.host
 118 | +        if args.depends:
 119 | +            with pushd('depends'):
 120 | +                no_qt = ''
 121 | +                if args.functional_tests:
 122 | +                    no_qt = 'NO_QT=1'

Sjors commented at 10:18 AM on June 8, 2020:

This should always be set.

in contrib/devtools/previous-release.py:185 in 6e4a07c83c outdated

 180 | +                if ret:
 181 | +                    return ret
 182 | +        return 0
 183 | +    args.config_flags = os.environ.get('CONFIG_FLAGS', '')
 184 | +    if args.functional_tests:
 185 | +        args.config_flags += ' --without-gui --disable-tests --disable-bench'

Sjors commented at 10:20 AM on June 8, 2020:

These flags should always be set.

Sjors changes_requested

Sjors commented at 10:22 AM on June 8, 2020: member

Concept ACK, thanks for working on this. 6e4a07c83ccc099e3b60d644d58e3f6558dca021 works on macOS 10.15.4 for the standard set of binaries we download, release candidates, and for compiling v0.20.0.

Can you squash your commits?

You can also remove previous_release.sh and switch over to this Python version (in a separate commit).

Are you using Python 3.5.6? (pyenv should do that automatically)

If you check the checksums then it makes sense to also check their PGP signature. Alternatively we might as well hard code the checksum for each tar.gz release in the source code, here. That way Travis might catch someone messing with bitcoincore.org.

I initially added -f Configure for functional tests as a way to opt-out of building QT. Looking at it again, I think we shouldn't build QT period. So I added inline suggestions to drop QT everywhere.

practicalswift commented at 11:16 AM on June 8, 2020: contributor

Concept ACK

Nice first-time contribution @bliotti - welcome as a contributor! :)

bliotti commented at 1:25 PM on June 8, 2020: contributor

Thanks, I have always wanted to contribute! I will follow up with the mentioned suggestions. :)

in contrib/devtools/previous-release.py:34 in 6e4a07c83c outdated

  29 | +    try:
  30 | +        yield
  31 | +    finally:
  32 | +        os.chdir(previous_dir)
  33 | +
  34 | +def download_binary(tag, args):

fanquake commented at 1:45 PM on June 8, 2020:

You could add return type annotations to function definitions. i.e:

def download_binary(tag, args) -> int:

in contrib/devtools/previous-release.py:131 in 6e4a07c83c outdated

  99 | +def build_release(tag, args):
 100 | +    if args.remove_dir:
 101 | +        if Path(tag).is_dir():
 102 | +            shutil.rmtree(tag)
 103 | +    if not Path(tag).is_dir():
 104 | +        output = subprocess.check_output(['git', 'tag', '-l', tag])

fanquake commented at 1:50 PM on June 8, 2020:

Could use subprocess.run(..., check=True) here and where you're using subprocess.call() etc; as opposed to the older subprocess API.

bliotti commented at 2:21 AM on June 9, 2020:

I changed the other subprocess.call methods to subprocess.run but when I used run for this line it hung up the script. I had to exit out of the tag list to continue the script. However check_output worked.

bliotti commented at 8:05 PM on June 8, 2020: contributor

Concept ACK, thanks for working on this. 6e4a07c83ccc099e3b60d644d58e3f6558dca021 works on macOS 10.15.4 for the standard set of binaries we download, release candidates, and for compiling v0.20.0.

Can you squash your commits?

You can also remove previous_release.sh and switch over to this Python version (in a separate commit).

Are you using Python 3.5.6? (pyenv should do that automatically)

If you check the checksums then it makes sense to also check their PGP signature. Alternatively we might as well hard code the checksum for each tar.gz release in the source code, here. That way Travis might catch someone messing with bitcoincore.org.

I initially added -f Configure for functional tests as a way to opt-out of building QT. Looking at it again, I think we shouldn't build QT period. So I added inline suggestions to drop QT everywhere.

Yes I will squash my commits.

Just to confirm, I need to squash all these commits into one commit that encompasses the addition of the py script, and then... push up one more commit for the removal of the bash script.

Most of the suggestions have been incorporated I just need to add the PGP signature check and I should have it up soon.

bliotti force-pushed on Jun 9, 2020

in contrib/devtools/previous-release.py:102 in 53d10682b4 outdated

  97 | +    # Bitcoin Core Release Signing Keys v0.11.0+
  98 | +    singingKey = "01EA5486DE18A882D4C2684590C8019E36C2E964"
  99 | +    keyServer = "hkp://keyserver.ubuntu.com:80"
 100 | +    primKeyFngPnt = '01EA 5486 DE18 A882 D4C2  6845 90C8 019E 36C2 E964'
 101 | +
 102 | +    # Refresh expired keys

Sjors commented at 9:30 AM on June 9, 2020:

I don't think you should mess with the developers PGP keys.

in contrib/devtools/previous-release.py:105 in 53d10682b4 outdated

 100 | +    primKeyFngPnt = '01EA 5486 DE18 A882 D4C2  6845 90C8 019E 36C2 E964'
 101 | +
 102 | +    # Refresh expired keys
 103 | +    subprocess.run(["gpg", "--keyserver", keyServer,
 104 | +                    "--recv-keys", singingKey],
 105 | +                   capture_output=True, text=True)

Sjors commented at 9:33 AM on June 9, 2020:

text=True causes an error for me: TypeError: __init__() got an unexpected keyword argument 'text'

Also:

  File "/Users/sjors/.pyenv/versions/3.5.6/lib/python3.5/subprocess.py", line 383, in run
    with Popen(*popenargs, **kwargs) as process:
TypeError: __init__() got an unexpected keyword argument 'capture_output'

Sjors commented at 9:37 AM on June 9, 2020: member

I restarted Travis for an unrelated failure (details in #19221)

It's probably better to move closes [#18132](/bitcoin-bitcoin/18132/) into the commit message body instead of the subject (needs a blank line). Title could be: test: switch usage of previous_release.sh to .py . Right now the changes are mixed between both commits; a single commit is fine too if it's tricky to untangle.

Let's not download and add PGP keys on behalf on the user. You could check if the key is present and fail otherwise.

MarcoFalke added the label Waiting for author on Jun 16, 2020

fjahr commented at 2:05 PM on June 18, 2020: member

Concept ACK

Great first contribution @bliotti! I will take a closer look one you have squashed the commits. You can make yourself a co-author as well, I think. And I think @Sjors mistyped, it should be script: ....

bliotti force-pushed on Jun 29, 2020

bliotti force-pushed on Jun 30, 2020

bliotti force-pushed on Jul 2, 2020

in test/functional/wallet_upgradewallet.py:31 in 60faac3351 outdated

  26 | @@ -27,7 +27,7 @@ def set_test_params(self):
  27 |          self.setup_clean_chain = True
  28 |          self.num_nodes = 3
  29 |          self.extra_args = [
  30 | -            ["-addresstype=bech32"], # current wallet version
  31 | +            ["-addresstype=bech32"],  # current wallet version
  32 |              ["-usehd=1"],            # v0.16.3 wallet

fjahr commented at 9:34 AM on July 2, 2020:

nit: the comments below the change are not aligned anymore

bliotti commented at 5:14 AM on July 3, 2020:

This was unintentional. This was from an auto formatting tool I was not paying attention to. It will be fixed and realigned.

in contrib/devtools/previous-release.py:98 in 45c76d7889 outdated

  93 | +            print("Checksum did not match")
  94 | +            Path(tarball).unlink()
  95 | +            return 1
  96 | +
  97 | +    # Bitcoin Core Release Signing Keys v0.11.0+
  98 | +    singingKey = "01EA5486DE18A882D4C2684590C8019E36C2E964"

fjahr commented at 9:38 AM on July 2, 2020:

nit: Not sure if that key's voice is good enough ;)

bliotti commented at 5:48 AM on July 3, 2020:

What are you thinking? Maybe there's no need for this check? I am ok with losing it.

fjahr commented at 9:12 AM on July 3, 2020:

sorry, I was just trying to be be funny. All I wanted to say was there is a typo: should probably be signingKey not singingKey :)

bliotti commented at 4:55 AM on July 4, 2020:

ohh lol... Totally went over my head. 😑 Thanks 👍

in test/functional/feature_backwards_compatibility.py:9 in 60faac3351 outdated

   5 | @@ -6,7 +6,7 @@
   6 |  
   7 |  Test various backwards compatibility scenarios. Download the previous node binaries:
   8 |  
   9 | -contrib/devtools/previous_release.sh -b v0.19.1 v0.18.1 v0.17.1 v0.16.3 v0.15.2
  10 | +contrib/devtools/previous_release.py -b v0.19.1 v0.18.1 v0.17.1 v0.16.3 v0.15.2

fjahr commented at 12:47 PM on July 2, 2020:

The .sh file name had an underscore while the .py file currently has a dash. Either rename the file or update the docs so that copy+paste of this line works again.

bliotti commented at 5:33 AM on July 3, 2020:

good catch.

fjahr approved

fjahr commented at 2:24 PM on July 2, 2020: member

tested ACK 60faac3351866730af783da3097a84b906ad8175

However, the docs should be updated. CI failure seems to be unrelated. I could restart Appveyor but it seems I can't restart that Travis build.

Also, the formatting changes in the second commit make this a longer review than necessary and I think many of them are not worth the noise they create. In some cases, the original authors even made an effort to line up comments etc. I don't use that style but it's not necessary to remove it unless you are editing that particular part of the file. For me, it's ok to keep them but other reviewers might disagree. If you keep them they should be moved into their own commit so that their current commit does what the commit message says: script: removed previous_release.sh. I would suggest to make it:

script: switch previous_release from .sh to .py version

Remove previous_release.sh

(the switch is more significant than the removal of the file).

And then the formatting changes move to refactor: Clean up functional compat test formatting.

EDIT: actually, the first commit message should be changed as well then because it doesn't do the switch now. Should be just something like script: Add previous_release.py.

in test/functional/wallet_upgradewallet.py:93 in 60faac3351 outdated

  94 | -        v15_2_wallet       = os.path.join(v15_2_node.datadir, "regtest/wallet.dat")
  95 | +        node_master_wallet_dir = os.path.join(
  96 | +            node_master.datadir, "regtest/wallets")
  97 | +        v16_3_wallet = os.path.join(
  98 | +            v16_3_node.datadir, "regtest/wallets/wallet.dat")
  99 | +        v15_2_wallet = os.path.join(v15_2_node.datadir, "regtest/wallet.dat")

MarcoFalke commented at 4:39 PM on July 2, 2020:

I don't really like the formatting changes here. This makes it harder to read, imo.

Maybe leave them out for now?

bliotti commented at 5:31 AM on July 3, 2020:

All of this formatting was unintentional. I didn't notice it auto formatted the file when I edited the one line above. It will be put back.

DrahtBot commented at 11:46 PM on July 2, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19013 (test: add v0.20.0 to backwards compatibility test by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

bliotti force-pushed on Jul 4, 2020

in contrib/devtools/previous_release.py:229 in 58b3fdfa3e outdated

 224 | +    parser.add_argument('-b', '--download-binary', action='store_true',
 225 | +                        help='Download release binary.')
 226 | +    parser.add_argument('-t', '--target-dir', action='store',
 227 | +                        help='Target directory.', default='releases')
 228 | +    args, extras = parser.parse_known_args()
 229 | +    sys.exit(main(args, extras))

MarcoFalke commented at 11:46 AM on July 4, 2020:

extras means tags?

If yes, this can be a positional argument with the + specifier (one or more instances)

bliotti commented at 12:57 PM on July 5, 2020:

added in 9c34aff

in contrib/devtools/previous_release.py:21 in 58b3fdfa3e outdated

  16 | +import subprocess
  17 | +import sys
  18 | +import hashlib
  19 | +
  20 | +
  21 | +def usage(args, extras) -> int:

MarcoFalke commented at 11:49 AM on July 4, 2020:

I think python prints the help by itself already, no?

bliotti commented at 12:57 PM on July 5, 2020:

removed usage in 9c34aff

fjahr commented at 12:03 PM on July 4, 2020: member

I think for the "co-authored-by:" to work as intended it needs to be at the end of the commit message. You can check that it shows both profile pictures and in Github it says 'bliotti and bboot committed'. You also only need to list the additional co-authors, not the author of the commit itself (yourself in this case).

See 4950e5d (#19055) for example.

script: Add previous_release.py

closes #18132
added GPG verify for binaries

co-authored-by: bboot <bboot@cisco.com>

e1e5960e10

Remove previous_release.sh 9c34aff393

bliotti force-pushed on Jul 5, 2020

fjahr commented at 9:30 PM on July 5, 2020: member

re-ACK 9c34aff39309b8adc99d347e07b6ddb5366498e9

Side-note: Waiting for author label can be removed.

bliotti commented at 10:04 PM on July 5, 2020: contributor

thanks for the help @fjahr :+1:

jnewbery removed the label Waiting for author on Jul 9, 2020

Sjors commented at 4:22 PM on July 9, 2020: member

tACK 9c34aff39309b8adc99d347e07b6ddb5366498e9

bliotti commented at 11:21 AM on July 20, 2020: contributor

Is there anything I need to do now?

fjahr commented at 11:28 AM on July 20, 2020: member

Is there anything I need to do now?

Not really, but you could respectfully ping someone to get another review/ACK. With three ACKs this should be ready for merge and if maintainers don't take notice then you could post a message in IRC that this might be RFM after a few days.

jonatack commented at 12:44 PM on July 20, 2020: member

Is there anything I need to do now? @bliotti welcome! May I suggest this section (particularly the last item) https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#finding-reviewers, followed by these steps:

Participate in https://bitcoincore.reviews and check out the resources listed at the bottom of the home page
Help out reviewing some of the hundreds of open issues and PRs that are waiting for testing or review, for instance among these by your kind reviewers :slightly_smiling_face: https://github.com/bitcoin/bitcoin/pulls/fjahr and https://github.com/bitcoin/bitcoin/pulls/Sjors

emsheets commented at 4:35 PM on July 20, 2020: none

Concept ACK, will test later today if test ACKs still needed at that time. Great stuff, @bliotti!!

MarcoFalke merged this on Jul 21, 2020

MarcoFalke closed this on Jul 21, 2020

MarcoFalke commented at 9:07 AM on July 21, 2020: member

I did some minor cleanups in #19560

sidhujag referenced this in commit 4dc1afd34b on Jul 21, 2020

bliotti commented at 6:19 PM on July 21, 2020: contributor

I did some minor cleanups in #19560

Thanks! @MarcoFalke

hebasto commented at 12:12 PM on August 26, 2020: member

Alternatively we might as well hard code the checksum for each tar.gz release in the source code, here. That way Travis might catch someone messing with bitcoincore.org.

Please see #19813.

MarcoFalke referenced this in commit c1e0c2ad3b on Aug 31, 2020

sidhujag referenced this in commit dd6081216a on Aug 31, 2020

DrahtBot locked this on Feb 15, 2022