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

kdmukai · 2022-12-13T15:34:12Z

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:

TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_framework.py", line 563, in start_nodes
    node.wait_for_rpc_connection()
  File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_node.py", line 231, in wait_for_rpc_connection
    raise FailedToStartError(self._node_msg(
test_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:

$ codesign -v -d bitcoin-qt
bitcoin-qt: code object is not signed at all

DrahtBot · 2022-12-13T15:34:15Z

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.

maflcko · 2022-12-13T15:48:25Z

test/get_previous_releases.py

@@ -108,6 +108,10 @@ def download_binary(tag, args) -> int:
    platform = args.platform
    if tag < "v23" and platform in ["x86_64-apple-darwin", "arm64-apple-darwin"]:
        platform = "osx64"
+    elif tag == "v23.0" and platform == "arm64-apple-darwin":


does this not affect 24 and later as well?

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.

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.

hebasto · 2022-12-13T16:01:47Z

Approach NACK. Why not using self-signing:

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

?

kdmukai · 2022-12-13T16:16:47Z

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?

hebasto · 2022-12-13T16:26:41Z

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.

Sjors · 2022-12-13T16:42:55Z

Automated self signing sounds good to me (after the hash is verified).

Sjors

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.

Sjors · 2022-12-13T18:38:15Z

test/get_previous_releases.py

+
+        # Is it already signed?
+        result = subprocess.run(['codesign', '-v', '-d', binary_path], capture_output=True)
+        if "code object is not signed at all" in result.stderr.decode("utf-8"):


This seems brittle. Is there a return code we can use?

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.

hebasto · 2022-12-13T19:40:25Z

Approach ACK.

hebasto · 2022-12-13T19:55:53Z

test/get_previous_releases.py

+
+    if tag >= "v23" and platform == "arm64-apple-darwin":
+        # Starting with v23.0 there are arm64 binaries for ARM (M1/M2) macs, but they have to be signed to run
+        binary_path = '{cwd}/{tag}/bin/bitcoind'.format(cwd=os.getcwd(), tag=tag)


For new code f-strings are recommended:

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

See:

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-python

https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines

hebasto · 2022-12-13T19:59:04Z

test/get_previous_releases.py

@@ -148,6 +148,24 @@ def download_binary(tag, args) -> int:
    ret = subprocess.run(['tar', '-zxf', tarball, '-C', tag,
                          '--strip-components=1',
                          'bitcoin-{tag}'.format(tag=tag[1:])]).returncode
+


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

Suggested change

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

hebasto · 2022-12-13T20:18:15Z

@kdmukai you may want to squash these commits

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

kdmukai · 2022-12-13T20:25:33Z

@kdmukai you may want to squash these commits

Squashed!

Sjors · 2022-12-13T21:54:07Z

test/get_previous_releases.py

+            subprocess.run(['codesign', '-s', '-', binary_path], capture_output=True)
+
+            # Confirm success
+            result = subprocess.run(['codesign', '-v', '-d', binary_path], capture_output=True)


capture_output is Python 3.7 syntax (so this fails with Python 3.6.15)

Gah. Updated and tested against python 3.6.15.

Sjors · 2022-12-13T21:54:39Z

test/get_previous_releases.py

+        result = subprocess.run(['codesign', '-v', '-d', binary_path], capture_output=True)
+        if "code object is not signed at all" in result.stderr.decode("utf-8"):
+            # Have to self-sign the binary
+            subprocess.run(['codesign', '-s', '-', binary_path], capture_output=True)


This nicely returns 0 for me on Intel, but I haven't checked the failure mode.

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.

hebasto

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?

hebasto · 2022-12-14T00:03:26Z

test/get_previous_releases.py

    if ret:
        return ret


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.

Yes. I'll update and can also capture any stderr details if the extraction fails.

hebasto · 2022-12-14T08:10:32Z

test/get_previous_releases.py

@@ -148,6 +148,27 @@ def download_binary(tag, args) -> int:
    ret = subprocess.run(['tar', '-zxf', tarball, '-C', tag,
                          '--strip-components=1',
                          'bitcoin-{tag}'.format(tag=tag[1:])]).returncode
+
+    if tag >= "v23" and platform == "arm64-apple-darwin":
+        # Starting with v23.0 there are arm64 binaries for ARM (M1/M2) macs, but they have to be signed to run


nit: Mentioning of exact models, i.e. M1/M2, does not look future proof. Maybe pointing at arm64 architecture is enough?

It might be useful to keep the "marketing" terms around for a bit.

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.

kdmukai · 2022-12-15T06:03:59Z

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/".

kdmukai · 2022-12-15T14:59:58Z

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.

kdmukai · 2022-12-15T15:22:02Z

Minor follow-up change to use stderr=subprocess.DEVNULL to suppress the expected stderr output when we first check if a binary is signed.

hebasto

Tested c9ebfb6 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?

test/get_previous_releases.py

hebasto

ACK dc12f2e

…'t run unsigned arm64 binaries; self-sign when needed dc12f2e test: improve error msg on previous release tarball extraction failure (kdmukai) 7121fd8 test: self-sign previous release binaries for arm64 macOS (kdmukai) Pull request description: ## 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](https://news.ycombinator.com/item?id=26996578) 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: ``` TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_framework.py", line 563, in start_nodes node.wait_for_rpc_connection() File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_node.py", line 231, in wait_for_rpc_connection raise FailedToStartError(self._node_msg( test_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: bitcoin#15774 (comment) And @ fanquake noted on IRC that you can confirm which binaries are or are not signed via: ``` $ codesign -v -d bitcoin-qt bitcoin-qt: code object is not signed at all ``` ACKs for top commit: hebasto: ACK dc12f2e Tree-SHA512: 644895f8e97f5ffb3c4754c1db2c48abd77fa100c2058e3c896af04806596fc2b9c807a3f3a2add5be53301ad40ca2b8171585bd254e691f6eb38714d938396b

backport: bitcoin#24603, bitcoin#26694, bitcoin#24669, bitcoin#22546, bitcoin#22199, bitcoin#25817 (mac build)

DrahtBot added the Tests label Dec 13, 2022

maflcko reviewed Dec 13, 2022

View reviewed changes

kdmukai changed the title ~~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 Dec 13, 2022

Sjors reviewed Dec 13, 2022

View reviewed changes

hebasto reviewed Dec 13, 2022

View reviewed changes

kdmukai force-pushed the 2022-12-fix_mac_arm64_previous_releases branch from cf5f65d to 647d8df Compare December 13, 2022 20:24

Sjors reviewed Dec 13, 2022

View reviewed changes

kdmukai force-pushed the 2022-12-fix_mac_arm64_previous_releases branch from 5eb76c6 to f112c53 Compare December 13, 2022 23:21

hebasto reviewed Dec 14, 2022

View reviewed changes

kdmukai force-pushed the 2022-12-fix_mac_arm64_previous_releases branch from 412eb47 to cf79920 Compare December 15, 2022 14:49

kdmukai force-pushed the 2022-12-fix_mac_arm64_previous_releases branch from aabddc0 to c9ebfb6 Compare December 15, 2022 15:19

hebasto reviewed Dec 19, 2022

View reviewed changes

test/get_previous_releases.py Outdated Show resolved Hide resolved

test/get_previous_releases.py Outdated Show resolved Hide resolved

test: self-sign previous release binaries for arm64 macOS

7121fd8

kdmukai force-pushed the 2022-12-fix_mac_arm64_previous_releases branch from cdf6fdc to 7121fd8 Compare December 19, 2022 17:18

test: improve error msg on previous release tarball extraction failure

dc12f2e

hebasto approved these changes Dec 19, 2022

View reviewed changes

maflcko merged commit 6d40a1a into bitcoin:master Dec 21, 2022

hebasto mentioned this pull request Jan 2, 2023

Pathing Guide for Installation using macOS #26778

Closed

knst mentioned this pull request Nov 20, 2023

backport: bitcoin#24603, #26694, #24669, #22546, #22199, #25817 (mac build) dashpay/dash#5718

Merged

5 tasks

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Nov 24, 2023

Merge pull request #5718 from knst/mac-improvements

23eb7a7

backport: bitcoin#24603, bitcoin#26694, bitcoin#24669, bitcoin#22546, bitcoin#22199, bitcoin#25817 (mac build)

bitcoin locked and limited conversation to collaborators Dec 21, 2023

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

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

Uh oh!

Conversation

kdmukai commented Dec 13, 2022 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Problem

Solution in this PR

Longer term solution

Further info:

Uh oh!

DrahtBot commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kdmukai commented Dec 13, 2022

Uh oh!

hebasto commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 13, 2022

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 13, 2022

Uh oh!

kdmukai commented Dec 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmukai commented Dec 15, 2022

Uh oh!

kdmukai commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kdmukai commented Dec 15, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

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

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

kdmukai commented Dec 13, 2022 •

edited by maflcko

Loading

DrahtBot commented Dec 13, 2022 •

edited

Loading

hebasto commented Dec 13, 2022 •

edited

Loading

hebasto commented Dec 13, 2022 •

edited

Loading

hebasto Dec 13, 2022 •

edited

Loading

kdmukai commented Dec 15, 2022 •

edited

Loading