Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove OpenSSL #4740

Merged
merged 15 commits into from
Oct 1, 2020
Merged

Remove OpenSSL #4740

merged 15 commits into from
Oct 1, 2020

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Sep 23, 2020 •
edited
Loading

Includes changes cherry-picked from the following upstream PRs:

Closes #145.

@str4d str4d added C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. A-dependencies Area: Dependencies labels Sep 23, 2020
@str4d str4d added this to the Core Sprint 2020-37 milestone Sep 23, 2020
@str4d str4d requested a review from daira September 23, 2020 00:33
@str4d
Copy link
Contributor Author

str4d commented Sep 23, 2020

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Sep 23, 2020

⌛ Trying commit 5193861 with merge 5bea6d8...

zkbot added a commit that referenced this pull request Sep 23, 2020
@zkbot
Auto merge of #4740 - str4d:remove-openssl, r=<try>
5bea6d8 
Remove OpenSSL

Includes changes cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7095
- bitcoin/bitcoin#17165
  - Only the commit removing SSL lib detection (we have long since removed the rest).
- bitcoin/bitcoin#17265
  - We had already migrated away from OpenSSL for randomness.
- bitcoin/bitcoin#17515
  - Only the second commit.
@zkbot
Copy link
Contributor

zkbot commented Sep 23, 2020

💔 Test failed - pr-try

@str4d str4d marked this pull request as draft September 23, 2020 00:53
@str4d
Copy link
Contributor Author

str4d commented Sep 23, 2020

Blocking this PR on #4643, which makes some changes that remove merge conflicts I'd rather not unpick.

@zkbot
Copy link
Contributor

zkbot commented Sep 23, 2020

☔ The latest upstream changes (presumably #4720) made this pull request unmergeable. Please resolve the merge conflicts.

@str4d str4d marked this pull request as ready for review September 29, 2020 23:53
@str4d
Copy link
Contributor Author

str4d commented Sep 29, 2020

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Sep 29, 2020

⌛ Trying commit bab8537 with merge 8777d2884eeed2a0643cadb247b1561da3520cfa...

@zkbot
Copy link
Contributor

zkbot commented Sep 30, 2020

☀️ Test successful - pr-try
State: approved= try=True

@daira
Copy link
Contributor

daira commented Sep 30, 2020 •
edited
Loading

#4484 (comment)

The problem with applying bitcoin/bitcoin#7095 is that the CScriptNum implementation from Bitcoin 0.10.0 already had undefined behaviour that was fixed in #4454. If we apply that fix also to the CScriptNum10 implementation, then we would essentially be comparing the current CScriptNum with itself, which is not a useful test.

I believe a more useful test would be to compare against fixed test vectors that we can compute manually (or in Python).

Edit: I have a workaround that just asserts that the problematic cases are not exercised by the comparison tests; testing it now.

Edit: turns out they were exercised by the comparison tests. Will fix.

@daira
Minor additional OpenSSL scouring.
b165ead 
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
nuttycom
nuttycom approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

daira added 4 commits October 1, 2020 00:34
@daira
Avoid undefined behaviour in scriptnum tests.
9ba10a8 
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira
Add assertions for CScriptNum[10] +/- int64_t to avoid the possibilit…
f432fe5 
…y of UB.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira
It's unnecessary to pass int64_t by const reference.
932d627 
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira
Cosmetics in CScriptNum code and tests.
ce0654e 
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@str4d
Copy link
Contributor Author

str4d commented Sep 30, 2020

ACK the commits pushed by @daira.

daira
daira approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK (but I can't review my own changes, i.e. the last 5 commits).

@daira
Copy link
Contributor

daira commented Sep 30, 2020

@nuttycom can you also review the last 4 commits?

nuttycom
nuttycom approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK of last 4 commits.

@daira
Copy link
Contributor

daira commented Oct 1, 2020

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Oct 1, 2020

📌 Commit ce0654e has been approved by daira

@zkbot
Copy link
Contributor

zkbot commented Oct 1, 2020

⌛ Testing commit ce0654e with merge b5fa52b...

@zkbot
Copy link
Contributor

zkbot commented Oct 1, 2020

☀️ Test successful - pr-merge
Approved by: daira
Pushing b5fa52b to master...

@zkbot zkbot merged commit b5fa52b into zcash:master Oct 1, 2020
This was referenced Oct 1, 2020
Merged
Merged
Closed
Closed
@daira
Copy link
Contributor

daira commented Oct 1, 2020

tenor (1)

@str4d str4d deleted the remove-openssl branch October 1, 2020 08:44
@daira
Copy link
Contributor

daira commented Oct 1, 2020 •
edited
Loading

This broke the macOS (and CentOS) builds: #4755. @str4d has a fix in #4756.

str4d added a commit to str4d/zcash that referenced this pull request Oct 1, 2020
@str4d
build: Remove a stray -lcrypto
be7876a 
zcash#4740 removed OpenSSL from our depends system, so we were
not satisfying this linker flag intentionally. Configurations where a
target OpenSSL could not be found by the linker, such as cross-compiles
and native macOS builds, were therefore failing. However, OpenSSL is
likely available in the system for all the "supported builders" (which
are all Linux-based), so the linker was likely being satisfied by that
library, enabling the previous PR to be merged.
@str4d str4d mentioned this pull request Oct 1, 2020
Merged
zkbot added a commit that referenced this pull request Oct 1, 2020
@zkbot
Auto merge of #4756 - str4d:4755-exorcise-openssl, r=str4d
939c5df 
build: Remove a stray -lcrypto

#4740 removed OpenSSL from our depends system, so we were
not satisfying this linker flag intentionally. Configurations where a
target OpenSSL could not be found by the linker, such as cross-compiles
and native macOS builds, were therefore failing. However, OpenSSL is
likely available in the system for all the "supported builders" (which
are all Linux-based), so the linker was likely being satisfied by that
library, enabling the previous PR to be merged.
@daira daira mentioned this pull request Oct 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuttycom nuttycom nuttycom approved these changes

@daira daira daira approved these changes

Assignees
No one assigned
Labels
A-dependencies Area: Dependencies C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase.
Projects
None yet
Milestone
Core Sprint 2020-39
Development

Successfully merging this pull request may close these issues.

Remove OpenSSL dependency. Ref: NCC-2016-007
8 participants
@str4d @zkbot @daira @nuttycom @r3ld3v @laanwj @practicalswift @fanquake