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

fix: expose ripemd160 hash from boringssl #16454

Merged
merged 1 commit into from Jan 28, 2019
Merged

fix: expose ripemd160 hash from boringssl #16454

merged 1 commit into from Jan 28, 2019

Conversation

nornagon
Copy link
Member

Ref #16195

ripemd160 is available in openssl by default, but boringssl does not build it by default. This adds a patch to expose it from boringssl.

Checklist

Release Notes

Notes: Restored support for RIPEMD160 digest, which was lost when switching from OpenSSL to BoringSSL.

@nornagon nornagon changed the title fix: expose ripemd160 hash from boringssl fix: expose ripemd160 hash from boringssl Jan 22, 2019
@nornagon nornagon changed the title fix: expose ripemd160 hash from boringssl fix: expose ripemd160 hash from boringssl . Jan 22, 2019
@nornagon nornagon changed the title fix: expose ripemd160 hash from boringssl . fix: expose ripemd160 hash from boringssl Jan 22, 2019
ryzokuken
ryzokuken approved these changes Jan 28, 2019
View reviewed changes
Copy link

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

The technical implications of this LGTM, I'm more worried about who needs RIPEMD160 and why? I don't think it's a great idea, and that might be why Google removed the function from the default builds.

@nornagon
Copy link
Member Author

Yep, I agree that ripemd160 is not a wise choice of digest, but since it's right there I figured this was easier than arguing about it. If BoringSSL removes it upstream I'm happy to drop support for it in Electron.

@ryzokuken
Copy link

@nornagon sounds fair.

jkleinsc
jkleinsc approved these changes Jan 28, 2019
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Looks good to me assuming CI passes.

@nornagon
Copy link
Member Author

CI failures are unrelated, so I'm going to go ahead and merge.

@nornagon nornagon merged commit 138ba53 into master Jan 28, 2019
@release-clerk
Copy link

release-clerk bot commented Jan 28, 2019

Release Notes Persisted

Restored support for RIPEMD160 digest, which was lost when switching from OpenSSL to BoringSSL.

@nornagon nornagon deleted the ripemd160 branch January 28, 2019 21:36
@trop
Copy link
Contributor

trop bot commented Jan 28, 2019

I was unable to backport this PR to "4-0-x" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/4-0-x label Jan 28, 2019
@trop trop bot mentioned this pull request Jan 28, 2019
Merged
@trop
Copy link
Contributor

trop bot commented Jan 28, 2019

I have automatically backported this PR to "5-0-x", please check out #16572

nornagon added a commit that referenced this pull request Jan 28, 2019
@nornagon
fix: expose ripemd160 hash from boringssl (#16454)
bd0c8c4 
Ref #16195
@nornagon nornagon mentioned this pull request Jan 28, 2019
Merged
@trop
Copy link
Contributor

trop bot commented Jan 28, 2019

A maintainer has manually backported this PR to "4-0-x", please check out #16574

@trop
Copy link
Contributor

trop bot commented Jan 29, 2019

A maintainer has manually backported this PR to "4-0-x", please check out #16574

codebytere pushed a commit that referenced this pull request Jan 29, 2019
@nornagon @codebytere
fix: expose ripemd160 hash from boringssl (#16454) (#16574)
56276d2 
Ref #16195
@sirasistant
Copy link

This is why some cryptocurrency-related libraries require this hash:
https://en.bitcoin.it/wiki/Technical_background_of_version_1_Bitcoin_addresses

@ryzokuken
Copy link

@sirasistant I suppose to trim the hash (which is otherwise 32 bytes because SHA256)?

@sirasistant
Copy link

I think so too, 20 bytes vs 32. But the fact is that it's part of many blockchain network protocols, which are not precisely easy to change. Breaking the compatibility of electron with this hash in the future could be quite problematic for many apps built on electron.

@ryzokuken
Copy link

@sirasistant agreed, that's a fair reason for including RIPEMD160.

@sofianguy sofianguy added this to 5.0.0-beta.2 in 5.0.x Feb 4, 2019
monicanagent added a commit to monicanagent/cypherpoker.js that referenced this pull request Feb 18, 2019
@monicanagent
v0.4.0-alpha.2: (update for issue #31) Added desktop (Electron) launc…
a3a68f2 
…her for CypherPoker.JS that retains existing "web" and "server" structures and uses a SQLite 3 database for local storage; tested on Windows 10 and all functionality appears to work as expected (compared to existing web version); note that a "postinstall.js" script applies a patch to bitcoinjs-lib and BIP32 modules for compatibility with >= Electron 4.x.x (since switch to BoringSSL -- electron/electron#16454).
@monicanagent monicanagent mentioned this pull request Feb 18, 2019
Closed
@peterremote1980
Copy link

So, how to fix this problem? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@codebytere codebytere codebytere approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

Assignees
No one assigned
Labels
None yet
Projects
No open projects
5.0.x
Fixed in 5.0.0-beta.2
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nornagon @ryzokuken @sirasistant @peterremote1980 @jkleinsc @codebytere