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

TODO: Add Tapscript / Taproot feature #1522

Closed
junderw opened this issue Dec 9, 2019 · 40 comments
Closed

TODO: Add Tapscript / Taproot feature #1522

junderw opened this issue Dec 9, 2019 · 40 comments

Comments

@junderw
Copy link
Member

junderw commented Dec 9, 2019

This will be a place to brainstorm ideas...

Needs:

  1. Tagged Hashes (maybe make a smaller separate package)
  2. Taproot builder (can we wedge this into the existing Payments API??? it seems to act similar to p2sh and p2wsh, and the lazy-loading should help greatly with performance due to the crypto operations to calculate the tweaked pubkey)
  3. Segwit v1 Address generation via the Payments(Taproot) API
  4. Schnorr signing module. (Modify tiny-secp256k1... not sure what to do with native JS though.)

Questions:

  1. When should this be implemented? I'm guessing we should wait for at least a testnet release of Taproot before spending time on it... But at the same time, it would help the development of taproot if we can look at taproot with fresh eyes / a new perspective and make suggestions based on problems while implementing.
  2. This is going to be a pretty big change... so maybe it should be a major version bump and that way we can reconsider existing API structure when adding Taproot.

Thoughts and comments welcome.

@junderw
Copy link
Member Author

junderw commented Jan 28, 2020

Working on it now.

Starting with tiny-secp256k1.

I am thinking of implementing TaggedHash and schnorr signing in tiny-secp256k1 itself since it is very bitcoin-specific. There is a PR currently up against secp256k1lib and I'll use that for the C++ addon for now.

WASM was considered to begin with for browsers but we'll put that off for now.

@andrewtoth
Copy link
Contributor

Is there any update on this? I would like to help out if possible.

@junderw
Copy link
Member Author

junderw commented Feb 21, 2021

Hi @andrewtoth, thanks a lot.

There are quite a few large things I need to work on in order to get this out.

You helped a ton with # 3, thanks.

In order of difficulty, the next step would be to create a TaggedHash module. You can take care of that if you'd like. If you want you can just write a single js file and post a gist or create your own repo and I can take it into the BitcoinJS org. If you don't want to maintain it, it should be simple enough so we won't mind maintaining it.

After that... 2 is code-wise next difficult... but concept-wise it's one of the most difficult. I have been debating whether to just scrap the Payment API for a similar API that is more aligned with Output Descriptors. I envision an API where the methods would be able to calculate and output ranges of addresses, and there would be a toOutputDescriptor() method that would output the checksummed output descriptor.

As far as tiny-secp256k1 is concerned... I just need to sit down and do the work... I am thinking of borrowing code from node-secp256k1 so we can update to using N-API for native bindings.

Also I think bitcoinjs-lib should remove tiny-secp256k1 as a direct dependency but rather have it be passed as an optional dependency, that way anyone who doesn't need key operations can use bitcoinjs-lib without installing native addons. Also, you could plug in your own tiny-secp256k1 (I am making a WASM version in rust).

I also want to try and get the WASM version usable for the browser...

It's a daunting task. Though the recent discussions of a deployment schedule have pushed things forward.

@andrewtoth
Copy link
Contributor

andrewtoth commented Feb 21, 2021

Ok, I will get started on a TaggedHash module. Leave that with me.

I have some general thoughts on how we should approach this. I think making big changes before releasing some basic taproot features is a bit too ambitious. It could make taproot adoption for single sig wallets harder than it needs to be.

I started with # 3 because I think getting the ecosystem upgraded to allow sending to taproot is the best way to encourage adoption. Many projects need to have assurance that a good percentage of users will be able to send to a taproot address before integrating receiving and spending. I appreciate your point that backporting BIP350 could break some assumptions so it likely isn't safe to do so. I think we should cut a release with just the address and output generation right now as v6, to give many slow moving projects the ability to upgrade easily.

Next we should add a payments.p2tr that can receive and spend from a single key. That should then be released as a minor v6.1. I think for the vast majority of single sig wallets this will allow them to integrate taproot without burdening them with having to also move away from using the payments API. Adding unnecessary friction to simply switch to a different payment type is the wrong approach IMO.

I think the project should wait for a v7 before adding any complex tapscript functionality that breaks existing API. Let me know your thoughts.

@junderw
Copy link
Member Author

junderw commented Feb 22, 2021

I am against releasing a v6 with just the address changes.

If you are really set on releasing the address change ASAP, add some new methods and revert the changes to the old methods. Then we can release it as a minor version change to v5, and switch back to the currently merged method for v6.

@junderw
Copy link
Member Author

junderw commented Feb 22, 2021

fromBech32m,
toBech32m,
bech32mFromOutputScript,
bech32mToOutputScript

They should probably contain a deprecation warning with console.warn saying they will be removed in v6 for integration in fromBech32 etc.

@junderw
Copy link
Member Author

junderw commented Feb 22, 2021

I would be willing to release that immediately.

@andrewtoth
Copy link
Contributor

But that would make https://github.com/bitcoinjs/bitcoinjs-lib/blob/master/ts_src/transaction_builder.ts#L231 no longer just work with taproot, which is the key benefit in my view.

@andrewtoth
Copy link
Contributor

andrewtoth commented Feb 22, 2021

Ok it would be better than nothing if users of v5 can do txBuilder.addOutput(baddress.bech32mToOutputScript(taproot_address), value) instead of having no way to decode the new addresses.

@junderw
Copy link
Member Author

junderw commented Feb 22, 2021

I made a new branch https://github.com/bitcoinjs/bitcoinjs-lib/tree/v5 (I haven't actually published up to this point yet, but the changes are minor, so we can add the new methods to this and publish it.)

You can add the new methods to that.

@junderw
Copy link
Member Author

junderw commented Feb 22, 2021

Also, unrelated, TransactionBuilder is deprecated and will be removed in v6, so people coding in v5 should be moving to PSBT anyways.

@andrewtoth
Copy link
Contributor

@junderw here's the TaggedHash module if you want to pull it into BitcoinJs https://github.com/andrewtoth/tagged-hash.

@andrewtoth
Copy link
Contributor

It can be optimized by saving the mid state after hashing the prefix. I left an issue comment browserify/sha.js#70. If it's worth it we can implement our own mid state savable implementation. I haven't found any.

motorina0 added a commit to bitcoincoretech/bitcoinjs-lib that referenced this issue Jun 24, 2021
motorina0 added a commit to bitcoincoretech/bitcoinjs-lib that referenced this issue Jun 25, 2021
motorina0 added a commit to bitcoincoretech/bitcoinjs-lib that referenced this issue Jun 25, 2021
@tylerlevine
Copy link
Contributor

Hi @junderw, I'm a software engineer at BitGo where we use a fork of bitcoinjs-lib for our utxo wallets. We're really interested in helping support taproot in bitcoinjs-lib, is there anything we can do to help with this? We would be willing to contribute our taproot code upstream

@junderw
Copy link
Member Author

junderw commented Jul 21, 2021

Hi!

Currently the plan is to use the tiny-secp256k1 WASM update, that way we can use the C library directly.

That said, this will shut out people who have older browsers.

One way I was thinking to solve this is to separate and make the tiny-secp256k1 module composable. So someone could implement a pure JS module with the same interface, and people could then plug in their own tiny-secp256k1-esque library.

Any thoughts on a WASM-only approach?

@tylerlevine
Copy link
Contributor

I think WASM support is wide spread enough that we wouldn't be too concerned with dropping support for older browsers.

However, my understanding is that this would require the unsafe-eval CSP rule enabled for pages which want to use WASM, which we don't currently allow (you can see a demo of this in action here). It looks like chromium is working on adding a wasm-unsafe-eval CSP rule which would allow this for WASM code only, but that is going to take a while to roll out.

I believe that means that these are our options:

  1. enable unsafe-eval to run WASM-based tiny-secp256k1
  2. use the upcoming modular tiny-secp256k1 and implement the interface in pure JS
  3. do something fancy like run a tiny-secp256k1 worker with unsafe-eval enabled
  4. wait for wasm-unsafe-eval to become implemented in mainline chrome

Is that right, or is there some other path forward that I'm missing here?

@junderw
Copy link
Member Author

junderw commented Jul 22, 2021

That sounds about right.
Thanks for sharing about the CSP rule behavior for WASM. I was not aware.

If you want to go with 2 you can definitely fork off of our pre-WASM version if that makes it easier, which would require adding some new schnorr based functions for both the native and pure JS module. I am not sure whether the pure JS schnorr parts should be included upstream elliptic library or not (that's what we used for the pure JS in the pre-WASM version)

3 might be better though short term, then 4 once it's widespread.

@murchandamus
Copy link

murchandamus commented Jul 26, 2021

Hi @junderw,
I've been doing some outreach to get a sense whether projects and companies are going to be ready to send to Bech32m addresses when Taproot activates. The code changes in #1676 seem to indicate that bitcoinjs could be ready, but I gather here that the sending support wasn't released yet. Do you think that bitcoinjs will have sending support before Taproot goes live in November?

@junderw
Copy link
Member Author

junderw commented Jul 27, 2021

That's the plan.

@OttoAllmendinger
Copy link
Contributor

see also BitGo efforts here https://github.com/BitGo/bitcoinjs-lib

@junderw
Copy link
Member Author

junderw commented Oct 13, 2021

All this chatter made me feel bad, so I am working on getting schnorr into WASM.

https://github.com/bitcoinjs/tiny-secp256k1/tree/feature/schnorr

@junderw
Copy link
Member Author

junderw commented Nov 12, 2021

bip32 is now modular.
ecpair was giving trouble.

sighashv1 is now live on v6

p2tr not yet. BitGo has a lot of assumptions baked into p2tr which need to be taken out ie the dependency on priority queue.

I also looked at an alternative PR currentl up against this repo, but it was taking to long for me to figure out what I wanted to do, so I just released bare bones for v6.

Key spend isn't to hard. Can't use Psbt or Payments yet though.

I will keep this issue open until p2tr is merged, since script path payments are too hard to do manually, currently.

@junderw
Copy link
Member Author

junderw commented Nov 15, 2021

The taproot example I had up initially is bad.

It's not incorrect. Nor is it insecure necessarily.

But looking at BIP86, which requires a trick that was only recommended ("should") in BIP341... any example (like my initial one) which would fragment wallets into further compatibility nightmares (ie. "oh I support p2tr, but when I import to another wallet they get different addresses (because they don't do the trick)") is a bad example.

The new example up is more in-line with what the standard is looking to become.

However, it is very hacky...

I need to re-introduce private negate to tiny-secp256k1, since it looks like it will get a lot of use.
bitcoinjs/tiny-secp256k1#61

@junderw
Copy link
Member Author

junderw commented Nov 15, 2021

Actually..... perhaps this should be done at a lower level:

bitcoin-core/secp256k1#1015

@Overtorment
Copy link

will it work in pure js? we still have environments (like RN) who cant do WASM.

@junderw
Copy link
Member Author

junderw commented Nov 22, 2021

There are examples of projects using asmjs in order to shim wasm into React Native.

https://github.com/polkadot-js/wasm/blob/v4.4.1/packages/wasm-crypto/src/bridge.ts#L14-L32

However, this is something that should be implemented in tiny-secp256k1 itself.

Perhaps we would make a 3rd iteration of wasm_loader.ts which is for only react-native that uses asmjs.

https://github.com/polkadot-js/wasm/issues/19 has a back and forth, so it seems like even with the current solution, it is tricky to get working.

@Overtorment
Copy link

ive seen some npm packages for RN to run wasm. what they do is they create webview under the hood, and make it run wasm. hacky af.

@junderw
Copy link
Member Author

junderw commented Nov 23, 2021

I don't think that's what this solution is using.

While getting WASM to work on RN is "hacky af" for the implementer, tiny-secp256k1 native JS code was hacky af and that's why we got rid of it.

The elliptic library itself is as secure as JS can be... but the very nature of JS makes it near impossible to prevent side-channel private key recovery attacks... so it doesn't even try to prevent them. A device signing a transaction in BlueWallet is basically sending it's private key over any charging cable it's plugged into, or the power buses inside the phone when it's not plugged in.

Does everyone have a hacker with a spectrometer on your wall outlet? No. So maybe that is an acceptable risk.


One of the design concepts I had/have for v6 is to make tiny-secp256k1 modular and injectable. So if someone wanted to make a native JS version, they can do that. And they can fork the JS code from v1 to add schnorr if they want.

I might help out BitGo who says they will need to support native JS for the forseeable future, but I don't think native JS is secure and we should move away from it.

@vesparny
Copy link

@junderw I have a React Native app and managed to have v6 working using tiny-secp256k1@1.1.6
Is that, in your opinion, a viable solution while we find a good approach to leverage WASM in RN and switching to tiny-secp256k1@12 ?

Thanks

@junderw
Copy link
Member Author

junderw commented Nov 23, 2021

yes.

you just won't be able to use schnorr.

@Overtorment
Copy link

are you implementing schnorr in rust? i just had an idea to do what LDK does - compile it for android (jar) & ios (xcframework). i could then bake it as a react-native module and inject it as a dependency to bitcoinjs

@Adoliin
Copy link

Adoliin commented Dec 17, 2021

are there any updates to adding taproot to psbt class ?

@junderw
Copy link
Member Author

junderw commented Dec 17, 2021

Probably some time early next year. Things have come up in my personal life, so finding time outside of my day job has been difficult.

@Adoliin
Copy link

Adoliin commented Dec 18, 2021

Yeah sure, take all the time you need and I hope everything is okay :)

@motorina0
Copy link
Member

Hi All,
I just wanted to let you know that I have updated the p2tr PR (#1742), now it uses (an injectable) tiny-secp256k1 for the taproot ecc functions (previously it had custom logic).

The code to be reviewed has been reduced. The logic is mainly in the p2tr.ts and taprootutils.ts files.
Any review / comment / testcase / suggestion is appreciated.

@tlindi
Copy link

tlindi commented Nov 28, 2022

Any progress in this issue?
AFAIK iancoleman/bip39#507 is waiting for this.

@junderw
Copy link
Member Author

junderw commented Nov 28, 2022

Any progress in this issue? AFAIK iancoleman/bip39#507 is waiting for this.

It's basically done... but we would like more eyes on it.
One more thorough review by someone and I'll feel better about merging it.

@junderw
Copy link
Member Author

junderw commented Nov 29, 2022

Published v6.1.0-rc.0!!!!!

Thanks to @motorina0 for the great work and thanks to everyone who helped review the large PR.

I will leave it rc for 2 weeks, and if we get no issues, I'll publish it as v6.1.0

@supertestnet
Copy link

What is the latest status update?

@junderw
Copy link
Member Author

junderw commented Jan 30, 2023

Published as v6.1.0

@junderw junderw closed this as completed Jan 30, 2023
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

No branches or pull requests