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.

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

Use blockstack.js W3C crypto #1966

Open
wants to merge 20 commits into
base: develop
from
Open

Use blockstack.js W3C crypto #1966

wants to merge 20 commits into from

Conversation

@zone117x
Copy link
Member

zone117x commented Nov 7, 2019

This PR drastically speeds up the user registration and restoration process, by using the native Web Crypto operations in blockstack.js v21-alpha blockstack/blockstack.js#691.

Some sample benchmarks using a Pixel 3 XL, on both Chrome and Firefox:

  • ~10 seconds when using browser.blockstack.org.
  • ~2 seconds when using this branch.

Additional performance testing is needed with lower-end Android phones, and regression testing is needed on all platforms.

This PR has an absurd changeset from the process of trying to debug errors while getting this working. Notably, all the synchronous code that needed to be refactored into asynchronous.

Debugging capabilities essentially were non-existent -- like readable stack traces, breakpoints in mocha tests, breakpoints within Chrome, etc.

Some of these notable tangental changes are:

  • Completely removed Flow. Few files had Flow annotations, and its build process involvement was a major pain when debugging.
  • Updated all the linting related dependencies.
  • Updated most of the toolchain dependencies (babel, webpack).

Now that the material code that actually needs changed is known, it is possible to cherrypick out only those changes. Let me know if we want to try that.

zone117x added 16 commits Oct 21, 2019
….js: update to latest bip39 (had breaking async changes) for baseline before migrating to blockstack.js
…tions
…9 updates
…d refactoring
…operations
…packages
@moxiegirl

This comment has been minimized.

Copy link
Contributor

moxiegirl commented Nov 13, 2019

@zone117x Does this work require that we update all the package.json files on the samples?

…t are no longer needed
@zone117x zone117x marked this pull request as ready for review Nov 18, 2019
@hstove
hstove approved these changes Nov 19, 2019
Copy link
Collaborator

hstove left a comment

Overall these are fantastic changes. I was a bit intimidated by the number of files changed, but the vast majority are simple changes due to removing Flow and @utils/@common resolvers.

The actual crypto code looks sound to me, and is familiar since I had to do some similar migration away from the HDNode interface for @blockstack/keychain.

I am curious about where exactly we're getting the W3C crypto polyfill. Is it because of the Babel changes, which include core-js? I didn't see anything in core-js about W3C crypto specifically. Or, are they just coming from blockstack.js?

I am impressed and grateful for this PR. I haven't finished a full functional QA yet, but there are tons of QOL improvements in this code.

.babelrc Show resolved Hide resolved
@@ -3,13 +3,14 @@ import React, { Component } from 'react'
import { connect } from 'react-redux'
import { bindActionCreators } from 'redux'

import { HDNode } from 'bitcoinjs-lib'
import { bip32 } from 'bitcoinjs-lib'
import * as bip39 from 'bip39'

This comment has been minimized.

Copy link
@hstove

hstove Nov 19, 2019

Collaborator

I had to do a bunch of these changes as well in @blockstack/keychain. I'm not sure if it's worth more refactoring to bring that package in here (maybe that would help QA that package) but it's worth keeping in mind.

} from '@utils'
import { isCoreEndpointDisabled } from '@utils/window-utils'
} from '../../../utils'
import { isCoreEndpointDisabled } from '../../../utils/window-utils'

This comment has been minimized.

Copy link
@hstove

hstove Nov 19, 2019

Collaborator

Another point for the future (if ever) - since packaging a core node is gone, we could probably remove this and other code references.

const { password } = this.state

const updateProfileUrls = localIdentities.map((identity, index) =>

This comment has been minimized.

Copy link
@hstove

hstove Nov 19, 2019

Collaborator

Good refactor - this file could certainly use more of it

app/js/utils/zone-utils.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@@ -10,7 +10,7 @@ requireHacker.hook('webp', () => 'module.exports = ""')
requireHacker.hook('css', () => 'module.exports = ""')

// Stub out workers
const workers = ['encrypt', 'decrypt', 'crypto-check']
const workers = []

This comment has been minimized.

Copy link
@hstove

hstove Nov 19, 2019

Collaborator

Were these not necessary because of the webpack Workbox plugin?

app/js/utils/name-utils.js Show resolved Hide resolved
@@ -130,25 +131,14 @@ module.exports = {
options: { minimize: true }
}
]
},
{
test: /\.worker\.js$/,

This comment has been minimized.

Copy link
@hstove

hstove Nov 19, 2019

Collaborator

Ah, so we just aren't using Web Workers anymore, since these are now async?

This comment has been minimized.

Copy link
@zone117x

zone117x Nov 19, 2019

Author Member

Correct. Should I get rid of the rest of the workerize related dependencies and setup?

This comment has been minimized.

Copy link
@hstove

hstove Nov 19, 2019

Collaborator

It wouldn't hurt to remove the dependency - is there other setup I'm missing?

@hstove

This comment has been minimized.

Copy link
Collaborator

hstove commented Nov 19, 2019

Functional QA I'm doing:

  • Wallet testing
    • Sending BTC
  • Profile testing
    • Change profile image
    • Change profile info
    • Github social proof (not working?)
  • Decrypting master keychain
  • Sign in with recovery code
  • Sign in with seed
  • Getting new identities
  • Registering a new identity on an existing keychain
  • Signing up from scratch

Signing in with my magic recovery code still seems kind of slow and UI blocking - though not completely. Might have just been a one-off. Signing up was quick and definitely not blocking.

Github social proofs didn't work, but they're probably already broken? I know we changed the cheerio import, so wanted to double check that.

@hstove

This comment has been minimized.

Copy link
Collaborator

hstove commented Nov 19, 2019

I'm seeing a weird issue with sending a BTC payment. I have $30 USD in my wallet, about 0.004 BTC, and no matter how much I try and send, I get "Not enough coin to fund fees transaction fees. Fees would be 5824, specified spend is 10". I see this comes from blockstack.js - is this an issue of not having the right UXTO's to put together a TX?

@zone117x

This comment has been minimized.

Copy link
Member Author

zone117x commented Nov 19, 2019

@hstove

I am curious about where exactly we're getting the W3C crypto polyfill.

The W3C crypto usage comes from blockstack.js, in this PR blockstack/blockstack.js#737

It's not really a crypto (Node.js module) polyfill, because there are no synchronous W3C crypto APIs available to create a polyfill. That blockstack.js PR directly uses the environment-native crypto APIs.

@zone117x

This comment has been minimized.

Copy link
Member Author

zone117x commented Nov 20, 2019

@hstove

I'm seeing a weird issue with sending a BTC payment

This appears to be an existing behavior, sending small btc amounts can trigger an incorrect error message. Created an issue: #1984

@timstackblock

This comment has been minimized.

Copy link
Contributor

timstackblock commented Dec 11, 2019

@zone117x @hstove I cant send anything out of the wallet with this new release

Screen Shot 2019-12-11 at 11 27 28 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.