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

Flesh out Schnorr/BIP340 functionality #50

Merged
merged 2 commits into from
Mar 13, 2022
Merged

Conversation

brandonblack
Copy link
Contributor

  • Add and export sign/verify sync for Schnorr
  • Add a cache for tagged hash prefixes
  • Simplify creating tag bytes for tagged hash
  • Export taggedHash and new taggedHashSync

@brandonblack
Copy link
Contributor Author

Started down this path to let noble-secp256k1 plug into bitcoinjs-lib https://github.com/bitcoinjs/bip32/blob/master/ts-src/bip32.ts#L58 as an alternative to tiny-secp256k1.

@brandonblack
Copy link
Contributor Author

brandonblack commented Feb 26, 2022

I added the remaining functions necessary to let this library be wrapped up and used with bitcoinjs-lib. You can see in the above mentioned PR where it's being tied together in the BitGo SDK.

edit to add link to the WIP wrapper: https://github.com/BitGo/BitGoJS/blob/bitcoinjs_lib_6_sync/modules/utxo-lib/src/noble_ecc.ts

This PR at least needs some tests for the new functions. I'll try to get those added early next week.

@paulmillr
Copy link
Owner

Great!

OttoAllmendinger pushed a commit to BitGo/BitGoJS that referenced this pull request Feb 28, 2022
* Schnorr signatures changed because lack of extra nonce data is now
  empty buffer instead of skipped
* tiny-secp256k1 no longer works in browser w/o WASM support, so
  switched crypto implementation to noble-secp256k1
* Required some moderately extensive changes to noble-secp256k1
  paulmillr/noble-secp256k1#50

Issue: BG-41154
@brandonblack
Copy link
Contributor Author

Alright, got those tests in. I see you already had vectors for the more complex two. Was there a strong reason they weren't implemented?

@paulmillr
Copy link
Owner

More complex two?

@brandonblack
Copy link
Contributor Author

pointAddScalar and pointMultiply already had test vectors, but weren't in the implementation.

@brandonblack
Copy link
Contributor Author

brandonblack commented Mar 1, 2022

  • Add a cache for tagged hash prefixes
  • Simplify creating tag bytes for tagged hash

BTW, I forgot to mention that I benchmarked many different ways of doing this, found all the ways that don't depend on nodejs Buffer to be slow, and settled on using a cache with one of the slower, but simpler methods.

For a million iterations of converting the tag to bytes, the original code is ~1s on my computer vs. the new at ~2s.

The only way that was marginally faster (.7s) was:

  const arr = new Uint8Array(tag.length);
  for (let j = 0; j < tag.length; j++) {
    arr[j] = tag.charCodeAt(j);
  }

README.md Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
* Add and export sign/verify sync for Schnorr
* Add a cache for tagged hash prefixes
* Simplify creating tag bytes for tagged hash
* Export taggedHash and new taggedHashSync
* Add and export some additional useful ecc utility functions
@paulmillr
Copy link
Owner

Going through the review, a bit slowly — sorry, in a war right now, hoping to merge this ASAP!

@brandonblack
Copy link
Contributor Author

in a war right now

Usually developers say that tongue in cheek because a server is throwing 500s, but you really mean it. Sorry. Stay safe out there!

@paulmillr
Copy link
Owner

@brandonblack why do you need these?

  privateAdd: (privateKey: PrivKey, tweak: Hex): Uint8Array
  privateNegate: (privateKey: PrivKey): Uint8Array
  pointAddScalar: (p: Hex, tweak: Hex, isCompressed?: boolean): Uint8Array
  pointMultiply: (p: Hex, tweak: Hex, isCompressed?: boolean): Uint8Array

Is it only for BIP32? I'd prefer to not add them into noble, because in this case we'll have two different methods of doing the same thing.

@brandonblack
Copy link
Contributor Author

Is it only for BIP32? I'd prefer to not add them into noble, because in this case we'll have two different methods of doing the same thing.

Yes, I added these for bitcoinjs/bip32 support.

https://github.com/BitGo/BitGoJS/pull/2054/files#diff-870b71613a9210366ce2c5f077e72747a328bb881c7121d7f890ba5dc4bc80f3R55-R71

brandonblack pushed a commit to BitGo/BitGoJS that referenced this pull request Mar 30, 2022
* Schnorr signatures changed because lack of extra nonce data is now
  empty buffer instead of skipped
* tiny-secp256k1 no longer works in browser w/o WASM support, so
  switched crypto implementation to noble-secp256k1
* Required some moderately extensive changes to noble-secp256k1
  paulmillr/noble-secp256k1#50

Issue: BG-41154
brandonblack pushed a commit to BitGo/BitGoJS that referenced this pull request Aug 19, 2022
* Schnorr signatures changed because lack of extra nonce data is now
  empty buffer instead of skipped
* tiny-secp256k1 no longer works in browser w/o WASM support, so
  switched crypto implementation to noble-secp256k1
* Required some moderately extensive changes to noble-secp256k1
  paulmillr/noble-secp256k1#50

Issue: BG-41154
brandonblack pushed a commit to BitGo/BitGoJS that referenced this pull request Aug 22, 2022
* Schnorr signatures changed because lack of extra nonce data is now
  empty buffer instead of skipped
* tiny-secp256k1 no longer works in browser w/o WASM support, so
  switched crypto implementation to noble-secp256k1
* Required some moderately extensive changes to noble-secp256k1
  paulmillr/noble-secp256k1#50

Issue: BG-41154
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants