test: get_previous_releases.py: M1/M2 macs can’t run unsigned arm64 binaries; self-sign when needed #26694

pull kdmukai wants to merge 2 commits into bitcoin:master from kdmukai:2022-12-fix_mac_arm64_previous_releases changing 1 files +30 −1
  1. kdmukai commented at 3:34 pm on December 13, 2022: contributor

    The Problem

    If you run test/get_previous_releases.py -b on an M1 or M2 mac, you’ll get an unsigned v23.0 binary in the arm64 tarball. macOS sets stricter requirements on ARM binaries so the unsigned arm64 binary is apparently completely unusable without being signed/notarized(?).

    This means that any test that depends on a previous release (e.g. wallet_backwards_compatibility.py) will fail because the v23.0 node cannot launch:

    0TestFramework (ERROR): Assertion failed
1Traceback (most recent call last):
2  File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_framework.py", line 563, in start_nodes
3    node.wait_for_rpc_connection()
4  File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_node.py", line 231, in wait_for_rpc_connection
5    raise FailedToStartError(self._node_msg(
6test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status -9 during initialization

    This can also be confirmed by downloading bitcoin-23.0-arm64-apple-darwin.tar.gz (https://bitcoincore.org/bin/bitcoin-core-23.0/) and trying to run any of the binaries manually on an M1 or M2 mac.

    Solution in this PR

    (UPDATED) Per @ hebasto, we can self-sign the arm64 binaries. This PR checks each binary in the previous release’s “bin/” and verifies if the arm64 binary is signed. If not, attempt to self-sign and confirm success.

    (note: an earlier version of this PR downloaded the x86_64 binary as a workaround but this approach has been discarded)

    Longer term solution

    If possible, produce signed arm64 binaries in a future v23.x tarball?

    Note that this same problem affects the new v24.0.1 arm64 tarball so perhaps a signed v24.x.x tarball would also be ideal?

    That being said, this PR will check all current and future arm64 binaries and self-sign as needed, so perhaps we need not worry about pre-signing the tarball binaries. And I did test a version of get_previous_releases.py that includes the new v24.0.1 binaries and it successfully self-signed both v23.0 and v24.0.1, as expected.

    Further info:

    Somewhat related to: #15774 (comment)

    And @ fanquake noted on IRC that you can confirm which binaries are or are not signed via:

    0$ codesign -v -d bitcoin-qt
1bitcoin-qt: code object is not signed at all
  2. DrahtBot commented at 3:34 pm on December 13, 2022: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hebasto
    Concept ACK Sjors

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Tests on Dec 13, 2022
  4. in test/get_previous_releases.py:111 in 3394ca4200 outdated 
    107@@ -108,6 +108,10 @@ def download_binary(tag, args) -> int:
108     platform = args.platform
109     if tag < "v23" and platform in ["x86_64-apple-darwin", "arm64-apple-darwin"]:
110         platform = "osx64"
111+    elif tag == "v23.0" and platform == "arm64-apple-darwin":
    maflcko commented at 3:48 pm on December 13, 2022:
    does this not affect 24 and later as well?
    kdmukai commented at 3:51 pm on December 13, 2022:
    Yes, it already affects the current v24.0.1 arm64 tarball. But that is not yet being pulled into the get_previous_releases.py list and I figured it’s possible/likely(?) that a subsequent v24.x release will have the proper signing before any v24 is part of this backwards compatibility list.
    maflcko commented at 4:09 pm on December 13, 2022:

    See also #26586

    Once there is a pull with a fix, it can set the tag here. But I am not aware of a pull/plan yet, so it seems best to avoid assuming it.

  5. hebasto commented at 4:01 pm on December 13, 2022: member

    Approach NACK. Why not using self-signing:

    0% codesign -s - ./releases/v23.0/bin/bitcoind

    ?

  6. kdmukai commented at 4:16 pm on December 13, 2022: contributor

    Approach NACK. Why not using self-signing: @hebasto I just confirmed that manual self-signing works. If that’s the preferred workaround, I’d still rather have that step automated in get_previous_releases.py. Adds a little more complication (I’m not sure yet what error conditions might occur when called from python, if any) but seems easy enough to add after the tarball is extracted.

    I’ll wait a bit for more feedback to come in. If I do end up altering the approach in this PR, is it preferable to just amend the title and description or is it better to close this PR and open a new one?

  7. hebasto commented at 4:26 pm on December 13, 2022: member

    I’d still rather have that step automated in get_previous_releases.py.

    This is what I meant :)

    If I do end up altering the approach in this PR, is it preferable to just amend the title and description or is it better to close this PR and open a new one?

    The former.

  8. Sjors commented at 4:42 pm on December 13, 2022: member
    Automated self signing sounds good to me (after the hash is verified).
  9. kdmukai renamed this:
    test: `get_previous_releases.py`: M1/M2 macs can't run the unsigned v23.0 arm64 binary; fall back to x86_64 binary
    test: `get_previous_releases.py`: M1/M2 macs can't run unsigned arm64 binaries; self-sign when needed
    on Dec 13, 2022
  10. in test/get_previous_releases.py:158 in 8c79786149 outdated 
    153+        # Starting with v23.0 there are arm64 binaries for ARM (M1/M2) macs, but they have to be signed to run
154+        binary_path = '{cwd}/{tag}/bin/bitcoind'.format(cwd=os.getcwd(), tag=tag)
155+
156+        # Is it already signed?
157+        result = subprocess.run(['codesign', '-v', '-d', binary_path], capture_output=True)
158+        if "code object is not signed at all" in result.stderr.decode("utf-8"):
    Sjors commented at 6:38 pm on December 13, 2022:
    This seems brittle. Is there a return code we can use?
    kdmukai commented at 7:51 pm on December 13, 2022:

    Agreed. But success or error (e.g. using an unrecognized flag) both return 1.

    The output ends up in stderr in either case, too. Unfortunately.

  11. Sjors commented at 6:38 pm on December 13, 2022: member

    Concept ACK @kdmukai you may want to squash these commits, and use a commit message that makes it clear what this is about when you later look it up on the master branch, e.g. test: self-sign bitcoind binary on arm64 macOS.

    Also, don’t use @ name in the commit message, because that results in spam notifications for that person when altcoins cherry-pick it.

  12. hebasto commented at 7:40 pm on December 13, 2022: member
    Approach ACK.
  13. in test/get_previous_releases.py:154 in 8c79786149 outdated 
    147@@ -148,6 +148,24 @@ def download_binary(tag, args) -> int:
148     ret = subprocess.run(['tar', '-zxf', tarball, '-C', tag,
149                           '--strip-components=1',
150                           'bitcoin-{tag}'.format(tag=tag[1:])]).returncode
151+    
152+    if tag >= "v23" and platform == "arm64-apple-darwin":
153+        # Starting with v23.0 there are arm64 binaries for ARM (M1/M2) macs, but they have to be signed to run
154+        binary_path = '{cwd}/{tag}/bin/bitcoind'.format(cwd=os.getcwd(), tag=tag)
    hebasto commented at 7:55 pm on December 13, 2022:

    For new code f-strings are recommended:

    Use f'{x}' for string formatting in preference to '{}'.format(x) or '%s' % x.

    See:

  14. in test/get_previous_releases.py:151 in 8c79786149 outdated 
    147@@ -148,6 +148,24 @@ def download_binary(tag, args) -> int:
148     ret = subprocess.run(['tar', '-zxf', tarball, '-C', tag,
149                           '--strip-components=1',
150                           'bitcoin-{tag}'.format(tag=tag[1:])]).returncode
151+
    hebasto commented at 7:59 pm on December 13, 2022:

    Extra spaces must be removed to pass ./test/lint/lint-whitespace.py:

    See: https://api.cirrus-ci.com/v1/task/6542625554038784/logs/lint.log

  15. hebasto commented at 8:18 pm on December 13, 2022: member

    @kdmukai you may want to squash these commits

    See: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

  16. kdmukai force-pushed on Dec 13, 2022
  17. kdmukai commented at 8:25 pm on December 13, 2022: contributor

    @kdmukai you may want to squash these commits

    Squashed!

  18. in test/get_previous_releases.py:163 in 647d8df5d3 outdated 
    158+        if "code object is not signed at all" in result.stderr.decode("utf-8"):
159+            # Have to self-sign the binary
160+            subprocess.run(['codesign', '-s', '-', binary_path], capture_output=True)
161+
162+            # Confirm success
163+            result = subprocess.run(['codesign', '-v', '-d', binary_path], capture_output=True)
    Sjors commented at 9:54 pm on December 13, 2022:
    capture_output is Python 3.7 syntax (so this fails with Python 3.6.15)
    kdmukai commented at 11:22 pm on December 13, 2022:
    Gah. Updated and tested against python 3.6.15.
  19. in test/get_previous_releases.py:160 in 647d8df5d3 outdated 
    155+
156+        # Is it already signed?
157+        result = subprocess.run(['codesign', '-v', '-d', binary_path], capture_output=True)
158+        if "code object is not signed at all" in result.stderr.decode("utf-8"):
159+            # Have to self-sign the binary
160+            subprocess.run(['codesign', '-s', '-', binary_path], capture_output=True)
    Sjors commented at 9:54 pm on December 13, 2022:
    This nicely returns 0 for me on Intel, but I haven’t checked the failure mode.
    kdmukai commented at 11:06 pm on December 13, 2022:

    You’re right. It returns 1 if the -v check doesn’t find a signature, but the subsequent calls do return 0 (on successful self-sign and on verifying that new signature).

    This will allow me to simplify the checks and not rely on the stderr messages. Update coming.

  20. kdmukai force-pushed on Dec 13, 2022
  21. in test/get_previous_releases.py:173 in f112c53dae outdated 
    168+                print(f'Failed to verify the self-signed {tag} arm64 binary; {result.stderr.decode("utf-8")}')
169+                return 1
170+            print(f"Successfully self-signed {tag} arm64 binary")
171+
172     if ret:
173         return ret
    hebasto commented at 0:03 am on December 14, 2022:
    Those lines seem to follow the tar invocation, as it is currently on the master branch. It is reasonable to exit early if the tar command fails.
    kdmukai commented at 5:59 am on December 15, 2022:
    Yes. I’ll update and can also capture any stderr details if the extraction fails.
  22. in test/get_previous_releases.py:153 in f112c53dae outdated 
    147@@ -148,6 +148,27 @@ def download_binary(tag, args) -> int:
148     ret = subprocess.run(['tar', '-zxf', tarball, '-C', tag,
149                           '--strip-components=1',
150                           'bitcoin-{tag}'.format(tag=tag[1:])]).returncode
151+
152+    if tag >= "v23" and platform == "arm64-apple-darwin":
153+        # Starting with v23.0 there are arm64 binaries for ARM (M1/M2) macs, but they have to be signed to run
    hebasto commented at 8:10 am on December 14, 2022:
    nit: Mentioning of exact models, i.e. M1/M2, does not look future proof. Maybe pointing at arm64 architecture is enough?
    Sjors commented at 6:10 pm on December 14, 2022:
    It might be useful to keep the “marketing” terms around for a bit.
    kdmukai commented at 5:58 am on December 15, 2022:

    Until I started getting into Docker I hadn’t been all that aware of cpu architecture details. I wouldn’t have immediately associated arm64 with an M1 mac.

    But I don’t feel strongly either way about the comment.

  23. hebasto commented at 8:13 am on December 14, 2022: member
    Functional tests can be run with substitution of bitcoind with bitcoin-qt or bitcoin-node using the BITCOIND variable. Does it make sense to sign not only bitcoind binary?
  24. kdmukai commented at 6:03 am on December 15, 2022: contributor

    Functional tests can be run with substitution of bitcoind with bitcoin-qt or bitcoin-node using the BITCOIND variable. Does it make sense to sign not only bitcoind binary?

    So far there are no tarballs with bitcoin-node but I’ll update it to just self-sign all the binaries it finds in each arm64 “bin/”.

  25. kdmukai force-pushed on Dec 15, 2022
  26. kdmukai commented at 2:59 pm on December 15, 2022: contributor

    Updated.

    • Signs ALL binaries in an arm64 release’s “bin/” directory, per @hebasto.
    • Keeps the tarball extraction wrapup code together, per @hebasto. Adds a slightly more useful error message.
    • Simplified error reporting; my earlier versions captured stderr and added them to print() but simply leaving stderr uncaptured outputs to the terminal anyway.
    • More PEP-8 friendly formatting on the long subprocess.run() calls.
      • Slightly opinionated decision on where to place the closing parens. My understanding is that PEP-8 is agnostic and I find it easier to read when the closing parens is on its own line, especially when there’s an attribute or function call added to the end of the closing parens.
  27. kdmukai force-pushed on Dec 15, 2022
  28. kdmukai commented at 3:22 pm on December 15, 2022: contributor
    • Minor follow-up change to use stderr=subprocess.DEVNULL to suppress the expected stderr output when we first check if a binary is signed.
  29. in test/get_previous_releases.py:152 in c9ebfb66ea outdated 
    147@@ -148,10 +148,38 @@ def download_binary(tag, args) -> int:
148     ret = subprocess.run(['tar', '-zxf', tarball, '-C', tag,
149                           '--strip-components=1',
150                           'bitcoin-{tag}'.format(tag=tag[1:])]).returncode
151-    if ret:
152+    if ret != 0:
153+        print("Failed to extract the {tag} tarball")
    hebasto commented at 1:26 pm on December 19, 2022: 
    0        print(f"Failed to extract the {tag} tarball")
    hebasto commented at 1:27 pm on December 19, 2022:
    This change looks good, but I’d prefer to see it its own commit, to keep every commit focused.
    kdmukai commented at 5:13 pm on December 19, 2022:
    Gah. Thank you for your patience and careful eye.
  30. hebasto commented at 1:29 pm on December 19, 2022: member

    Tested c9ebfb66ea4943ceeb01430613c050e3ea4cc2e6 on mac M1. Binaries got signed.

    While moving in the right direction, I reckon this change is not complete, because without Rosetta it is still not possible to run ./test/functional/wallet_backwards_compatibility.py on arm64. Perhaps, it makes sense do disable some releases in the latter, doesn’t it? Maybe in a follow up?

  31. test: self-sign previous release binaries for arm64 macOS 7121fd8fa7
  32. kdmukai force-pushed on Dec 19, 2022
  33. test: improve error msg on previous release tarball extraction failure dc12f2e212
  34. hebasto approved
  35. hebasto commented at 6:34 pm on December 19, 2022: member
    ACK dc12f2e212dfacbe238cf68eb454b9ec71169bbc
  36. maflcko merged this on Dec 21, 2022
  37. maflcko closed this on Dec 21, 2022
  38. knst referenced this in commit 2170604f33 on Nov 20, 2023
  39. knst referenced this in commit 0417209266 on Nov 20, 2023
  40. knst referenced this in commit cca5a467de on Nov 20, 2023
  41. PastaPastaPasta referenced this in commit d7bdf8e424 on Nov 24, 2023
  42. PastaPastaPasta referenced this in commit 23eb7a754b on Nov 24, 2023
  43. ogabrielides referenced this in commit 7c966c9db0 on Dec 4, 2023
  44. bitcoin locked this on Dec 21, 2023


kdmukai DrahtBot maflcko hebasto Sjors

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me