Misc. improvements and cleanups #4

brandonblack · 2022-03-18T22:59:53Z

No description provided.

* Move the (much simplified) type check function to types.ts * Use `Tapleaf` type a bit more (this might be a bad idea) * Be more consistent in the capitalization of `Taptree`

Because the taggedHash API is typed, these are compile-time checked and it's more clear w/o the constants.

* More clearly show the continuation and base cases in findScriptPath * Return undefined not empty path when no path is found * This would lead to generating an invalid witness * Tighten the type for HashTree to not allow 1-sided branch nodes

* Also added caching of `hashTree`, per todo. * Added a test for this functionality

The spec uses this notation because in a spec there's no such thing as reassigning a value. In real code it is appropriate to us accumulators or such.

junderw · 2022-03-19T00:16:14Z

ts_src/payments/p2tr.ts

@@ -85,6 +88,12 @@ export function p2tr(a: Payment, opts?: PaymentOpts): Payment {
    return a.witness.slice();
  });

+  const _hashTree = lazy.value(() => {


test/fixtures/p2tr.json

motorina0 · 2022-03-21T11:13:57Z

ts_src/types.ts

@@ -77,7 +77,22 @@ export interface Tapleaf {
  version?: number;
 }

-export type Taptree = Array<[Tapleaf, Tapleaf] | Tapleaf>;
+export const TAPLEAF_VERSION_MASK = 0xfe;


I'm not sure if the place for this constant is in types.
Maybe it will be extracted to a BIP341 module (see bitcoinjs#1780) together with isTapleaf, isTaptree in the future. OK to be here for now.

Yeah, I really didn't have a good place for it.

ts_src/payments/taprootutils.ts

motorina0 · 2022-03-21T11:26:37Z

Thank you for the PR!
I like simplified recursive functions for the tree parsing and the improved type checking.
I've added a few small comments.

brandonblack · 2022-03-21T14:58:21Z

Thank you for the feedback, pushed fixes.

ts_src/payments/p2tr.ts

brandonblack · 2022-03-21T20:49:39Z

I pushed a large commit over here: BitGo@db3bb8e that minimizes the need to thread eccLib around. That resolves the major concern I found while reviewing the PSBT changes, although I'm still learning the PSBT code, so may have further comments in coming days.

Hopefully it goes without saying that you are welcome to cherry-pick, squash, etc. any of my commits as you see fit into your branch.

Edit to add: I suspect that we're going to want to rethink having each p2tr created verify the tweakFn, both because it's an expensive operation, and because doing that makes it difficult to use with something like a MuSig tweak function that has an internal cache that is updated by the tweak operation.

junderw · 2022-03-21T23:27:23Z

NACK on removing eccLib from p2tr.

Perhaps it is because there is a lack of support from libraries and lots of people are "rolling their own" for now... but I have seen examples of people sending to v1 segwit programs where the 32 bytes following is not a valid point (even though it is less than P).

Bitcoin Core does not reject these, and they are essentially a black hole.

(Perhaps a standardness rule could be added to Bitcoin Core to prevent their broadcasting/propagation)

At the very least, we should require it for the address module, and anyone trying to get the address from p2tr.

But the requirement of being a valid point is especially concerning in this case since a v0 p2wsh is also 32 bytes, but it's a hash.

junderw · 2022-03-21T23:28:08Z

If Bitcoin Core included a standardness rule, I might consider it.

junderw · 2022-03-21T23:41:48Z

I made an issue for discussion here

junderw · 2022-03-21T23:43:17Z

I would be comfortable removing eccLib once an official release with that standardness rule is over 60%-ish of the observable network...

junderw · 2022-03-22T02:08:27Z

hmm... looks like consensus is opposite (standardness rule should come after every client/library under the sun has protected against it)
bitcoin/bitcoin#24106 (comment)

brandonblack · 2022-03-22T05:05:10Z

OK, I see your point.

I think more and more that we should drop the verifyEcc stuff though. Per a side chat, we're at least marginally concerned about performance, and those are rather expensive tests to be performing on each instantiation of p2tr. bitcoinjs-lib should either provide the ECC implementation, or accept that it may be injected with one that doesn't meet the expected requirements.

Given all of this, it might make sense to apply the Factory pattern to PSBT, make an ECC library required to use PSBT in general, and avoid the conditional/optional threading. That would then open up later refactors to replace types.isPoint with a proper point check for non-Taproot as well.

junderw · 2022-03-22T05:25:05Z

That is true. It's pretty expensive.

Perhaps we can cache eccLib...?

When comparing two objects, it returns true for equals when the object is the same reference.

We could use that and have some sort of eccLib cache that is rooted in the main library, and will use the cached library if none is passed in, and even if the same library is passed multiple times, skip the check if the reference to the library object is the same?

ie.

// eccCache.ts (do not export this in the index.ts, it should not be exposed to the public API)
export const _ECCLIB_CACHE: { eccLib?: TinySecp256k1Lib } = {};

// p2tr.ts
import { _ECCLIB_CACHE } from '../eccCache';

if (_ECCLIB_CACHE.eccLib !== eccLib) {
  check(eccLib);
  _ECCLIB_CACHE.eccLib = eccLib;
}

motorina0 · 2022-03-22T05:28:17Z

I think more and more that we should drop the verifyEcc stuff though. Per a side chat, we're at least marginally concerned about performance

this indeed can be a performance problem. One option is to add a skipEccValidation boolean flag to PaymentOpts:

export interface PaymentOpts {
  ...
  eccLib?: TinySecp256k1Interface;
  skipEccValidation?: boolean;
}

motorina0 · 2022-03-22T05:50:38Z

The caching approach would also solve the the "threading" of eccLib in PSBT.

But we can have the scenario where :

a p2tr(..., { eccLib: ecc }) call is made somewhere in the code
and later, in different parts of the code a p2tr(...) call is made (without eccLib) and it works (the eccLib is cached)
someone removes/adds a condition for the first call (with eccLib). Then the second call (without eccLib) fails

Same goes for new Psbt({ eccLib: ecc }) vs new Psbt()

Or... are we planning to use the cache only for removing multiple verify calls for eccLib?

junderw · 2022-03-22T06:02:12Z

We could do both.

Maybe even provide a public setter that will verify and set the cache so people don't have to use the weird options to begin with.

"Only insert this option once, any subsequent usage isn't needed" is a weird API.

motorina0 · 2022-03-22T08:44:48Z

Maybe even provide a public setter that will verify and set the cache...

Yes. I would prefer something explicit as in the code snippet below for several reasons:

if the eccLib is invalid, then it will fail at the initEccLib(...) invocation place (which can make it easy to investigate/debug), instead of failing inside p2tr or Psbt
if p2tr is used without an eccLib then the error message can clearly state that: "No ECC Library provided. You must call initEccLib(...) with a valid TinySecp256k1Interface instance"
- side note: rename TinySecp256k1Interface to Secp256k1Interface?
it makes the code-base a little easier to work with (no need to pass eccLib around)
it makes the public API a little simpler (no eccLib option for p2tr and Psbt)
calling initEccLib(...) is optional (only required if p2tr is used)
calling initEccLib(...) multiple times with the same TinySecp256k1Interface instance is a noop

// client code
import { payments, address, initEccLib } from 'bitcoinjs-lib'

initEccLib(clientEccLib)

brandonblack · 2022-03-22T14:17:40Z

I'm in full support of an explicit initEccLib and then having p2tr throw a useful error if it's used prior to that being called.

@motorina0 would you like me to whip that up, or you?

motorina0 · 2022-03-22T14:48:24Z

I would rather keep this PR smaller and focused on the review of the actual functionality.
I can do the initEccLib change. @junderw please 👍 if we have your blessing.

OttoAllmendinger · 2022-03-23T13:56:03Z

My concern with initEccLib is that it will bloat the require statement. In a given file we typically cannot assume that initEccLib() was called somewhere else, so we will end up with extra boilerplate anywhere we import bitcoinjs-lib.

How about we wrap methods that require an eccLib in a class?

class PaymentBuilder<TEccLib extends EccLib> {
  constructor(private eccLib: TEccLib) { ... }
  
  p2tr(...) { /* uses this.eccLib */ }
}

This way we can constrain the allowed ecc libs via the type system.

(Disclaimer: like @brandonblack I work at BitGo)

junderw · 2022-03-23T14:19:05Z

That boat has already sort of sailed with bip32/ecpair tbh.

The answer would be to have a single file that imports bitcoinjs, bip32/ecpair, and tiny-secp256k1, mashes them together, and re-exports the eccLib-included objects. Other files would then import from that one file.

OttoAllmendinger · 2022-03-24T14:23:18Z

That boat has already sort of sailed with bip32/ecpair tbh.

True, but unless I am missing something those deps do not have an init call.

If the recommendation is to wrap bitcoinjs-lib with crypto lib instances anyway, then I would favor the extra argument for consistency reason. Having a side-effecting call plus extra arguments is a strange mix.

motorina0 · 2022-03-24T18:10:16Z

those deps do not have an init call.

They kind of have (its a factory pattern):

import BIP32Factory from 'bip32';
import ECPairFactory from 'ecpair';
import { payments, address, initEccLib } from 'bitcoinjs-lib'
import * as ecc from 'tiny-secp256k1';

const bip32 = BIP32Factory(ecc);
const ECPair = ECPairFactory(ecc);

// suggested change:
initEccLib(ecc)
...

Having a side-effecting call plus extra arguments is a strange mix.

it is either the initEccLib OR the extra argument ({ ..., eccLib?: TinySecp256k1Interface }). It does not make sense to have both
I don't know if having an init() method is a "side-effect" call, but I get your point. It its more like "enable extra functionality". The lib itself is stateless.
the "extra argument" has the disadvantage of having to "thread" eccLib (as @brandonblack correctly observed for Psbt)
the init() has the disadvantage that the client application must make sure to call it at least once.
other options... ideas are welcomed

motorina0 · 2022-03-24T18:15:47Z

@brandonblack I would like to get these changes in the p2tr-v1 branch. Should I wait for the Psbt review, or is it OK for you to open a new PR?

brandonblack · 2022-03-24T19:11:28Z

Fine with merging this as is. Still getting my head around PSBT, but hopefully others can continue reviewing in the meantime.

junderw · 2022-03-24T23:13:21Z

Yeah merge this into the p2tr-v1 branch for now. We can continue discussion there.

motorina0 · 2022-03-25T10:57:08Z

PR merged! Thanks again @brandonblack
I have open this PR #5 against p2tr-v1 with the initEccLib chages.
Please have a look (and comment) before I merge it.

reardencode added 7 commits March 18, 2022 15:11

Declare tapscript version mask like the BIP

ddef6c8

Correct Taptree type

47f3158

* Move the (much simplified) type check function to types.ts * Use `Tapleaf` type a bit more (this might be a bad idea) * Be more consistent in the capitalization of `Taptree`

Consistent capitalization of tapleaf

37b0f1c

Don't use constants for tag prefixes

89785be

Because the taggedHash API is typed, these are compile-time checked and it's more clear w/o the constants.

Support p2tr with 1 script and no tree

299308a

* Also added caching of `hashTree`, per todo. * Added a test for this functionality

Remove unnecessary arrays of values

5f81cf4

The spec uses this notation because in a spec there's no such thing as reassigning a value. In real code it is appropriate to us accumulators or such.

brandonblack mentioned this pull request Mar 18, 2022

feat: add support for pay to taproot bitcoinjs/bitcoinjs-lib#1742

Merged

junderw reviewed Mar 19, 2022

View reviewed changes

test/fixtures/p2tr.json Outdated Show resolved Hide resolved

reardencode added 2 commits March 18, 2022 17:49

Improve tapleah hash parameter name

ce27c13

Fix indentation

4dd4a51

brandonblack force-pushed the p2tr-v1 branch from 68b17a8 to 4dd4a51 Compare March 19, 2022 00:49

motorina0 reviewed Mar 21, 2022

View reviewed changes

reardencode added 2 commits March 21, 2022 07:55

Add validation for redeem in scriptTree

5610d45

Improve comments and code clarity

ef5457e

motorina0 reviewed Mar 21, 2022

View reviewed changes

ts_src/payments/p2tr.ts Show resolved Hide resolved

motorina0 mentioned this pull request Mar 24, 2022

Do real secp256k1 point->curve checking bitcoinjs/bitcoinjs-lib#1786

Open

motorina0 merged commit 6ea2f8b into bitcoincoretech:p2tr-v1 Mar 25, 2022

motorina0 mentioned this pull request Mar 25, 2022

refactor: add explicit initialisation of the ecc library #5

Merged

Misc. improvements and cleanups #4

Misc. improvements and cleanups #4

Uh oh!

Conversation

brandonblack commented Mar 18, 2022

Uh oh!

junderw Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

motorina0 Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

brandonblack Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

motorina0 commented Mar 21, 2022

Uh oh!

brandonblack commented Mar 21, 2022

Uh oh!

Uh oh!

brandonblack commented Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junderw commented Mar 21, 2022

Uh oh!

junderw commented Mar 21, 2022

Uh oh!

junderw commented Mar 21, 2022

Uh oh!

junderw commented Mar 21, 2022

Uh oh!

junderw commented Mar 22, 2022

Uh oh!

brandonblack commented Mar 22, 2022

Uh oh!

junderw commented Mar 22, 2022

Uh oh!

motorina0 commented Mar 22, 2022

Uh oh!

motorina0 commented Mar 22, 2022

Uh oh!

junderw commented Mar 22, 2022

Uh oh!

motorina0 commented Mar 22, 2022

Uh oh!

brandonblack commented Mar 22, 2022

Uh oh!

motorina0 commented Mar 22, 2022

Uh oh!

OttoAllmendinger commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junderw commented Mar 23, 2022

Uh oh!

OttoAllmendinger commented Mar 24, 2022

Uh oh!

motorina0 commented Mar 24, 2022

Uh oh!

motorina0 commented Mar 24, 2022

Uh oh!

brandonblack commented Mar 24, 2022

Uh oh!

junderw commented Mar 24, 2022

Uh oh!

motorina0 commented Mar 25, 2022

Uh oh!

Uh oh!

brandonblack commented Mar 21, 2022 •

edited

Loading

OttoAllmendinger commented Mar 23, 2022 •

edited

Loading