API for 4.0.0

[dcousens]

Just a bike shed for 3.0.0 4.0.0, if its necessary, and the currently open PRs:

#459 - Extraction of DER encoding
#456 - Remove message module (bitcoinjs-message)
#440 - Var naming conventions
#350 - secp256k1
#428 - Altcoin support

@jprichardson I was also thinking about TransactionBuilder, and what it represents.

It is basically a convenience wrapper around Transaction (the primitive) that makes everyone's life easier when building complex transactions, but, to be honest, it could easily be extracted from the core library.

Thoughts?
In general, I'm thinking we work on making this library primarily all the primitives for bitcoin operations such that other libraries can easily compose using them.
I'd like to see other crypto primitives like merkle trees, bloom filters and rolling bloom filters in the ecosystem too.

[dcousens]

Naturally, keeping that list of related modules is super helpful (BIPxx etc), as it helps keep things related and the ecosystem linked together.

[dcousens]

I think these decisions are becoming more and more important as more complicated structures and algorithms are becoming the norm (see opt-in RBF, CLTV, payment channels, etc).

[jprichardson]

It is basically a convenience wrapper around Transaction (the primitive) that makes everyone's life easier when building complex transactions, but, to be honest, it could easily be extracted from the core library.

Yes, you know I'd be in favor of breaking up the library into smaller modules =) I'd be willing to contribute any of my existing modules / names if you think they'd be of use. We can start fresh too.

In general, I'm thinking we work on making this library primarily all the primitives for bitcoin operations such that other libraries can easily compose using them.
I'd like to see other crypto primitives like merkle trees, bloom filters and rolling bloom filters in the ecosystem too.

Yes, definitely.

[jprichardson]

To add to the discussion, here's what I'd like:

• Identify which modules we can remove to their own package. Building a lean & mean core would allow us to iterate faster.
• Move to ES6.
• Fluent TxBuilder interface.
• Strongly consider secp256k1.
• Docs. These are a must. Much easier with a lean & mean core. I've come to really like GitBook.
• Consider dropping Mocha in place of something else. I've started using Tape and Ava (not at the same time of course). But I think there'd be a way to possibly build the docs automatically from the tests. I really need to document my ideas at some point. Mocha is a beast. Obviously only should be done w/ good reason. New test runner would have to bring a lot to the table.
• Generalizable serialization format #513 would allow the library to easily be used for any altcoin without explicitly supporting the altcoin. I'll be getting to this soon (few months) as I need it.

[dcousens]

Identify which modules we can remove to their own package. Building a lean & mean core would allow us to iterate faster.

Agreed, how do you want to go about that discussion?

Move to ES6.

Mixed thoughts on this. Its not much of a benefit to change existing code, but, new code sure.

Fluent TxBuilder interface.

Agreed, just need to figure out what is the most suitable without giving up the powerful [albeit, Java-like] parts of the existing API.

Strongly consider secp256k1.

Already have, I'm thinking that is the next step. @fanatid what is the status there OOI?
Do you think bn.js is ready for production use?

Docs. These are a must. Much easier with a lean & mean core. I've come to really like GitBook.

Absolutely. I'll check out GitBook :)

Consider dropping Mocha in place of something else

Tape.

#513

Targeting 3.0.0 with this might be very time consuming. Maybe 3.1.0 or 4.0.0?

[fanatid]

secp256k1-node v3.0.0-alpha.0 is ready, @wanderer checks PR and I hope he finish at the weekend.
I checked all functions that related with secp256k1 in bn.js and fixed all that I found, hope that it no longer contains bugs :)
Whats wrong with mocha? Tape support docs generation from tests?

[wanderer]

I also favor tape over mocha. After using mocha for a long time I found tape much easier to use and more consistent.

[rubensayshi]

👍 for using secp256k1-node, I've been running (an older version) in production for months now

tbh I'd prefer to keep the transactionbuilder in the same package, too much hassle to extract it and keep it in sync with the version of bitcoinjs-lib.
and imo it's pretty core to what most people do with this library...

[dcousens]

@rubensayshi I think the current roadmap is encompassing the extraction of:

• BIP32 to a separate bip32 module
• Remove message module #456
• Extraction of ECDSA (see secp256k1) and relevant encoding functions: Remove explicit DER/Compact decoding #459

Not much else.
Plans also include the creation of a BIP65 utilities module and a more extensive transaction creation module that adheres to best practice (BIP69, anti-fee sniping, etc).

[fanatid]

@dcousens do you want move bip32 to another module itself? or any volunteers are welcome?
Also, did you try use AVA? I found this test runner very nice, except that AVA doesn't support nested tests and don't work in browser :(

[dcousens]

@fanatid see https://github.com/bitcoinjs/bip32, that is the planned destination.
Any volunteers welcome to help :)

[fanatid]

@dcousens check my bip32 repository https://github.com/fanatid/bip32 if it's ok, push to bitcoinjs-lib/bip32 (I haven't permission to push there)

[jprichardson]

My concerns (some of these are pedantic):

• get methods should be dropped. Especially since we have ES6 and we've had ES5 properties for awhile i.e. I much prefer properties, they're more logical.
• What about removing the random feature? This way someone could just pass in the random bytes and it's one less dependency.
• What is fromString? Is it base58? hex? I think this should be renamed to add more clarity.
• I'd prefer ES6.
• Not a huge fan of typeforce as a dependency. Sorry @dcousens :) I'd much rather a Babel solution or even better, Flow.

Love that it uses secp256k1 :) Although, I'd prefer

try {
  var secp256k1 = require('secp256k1')
} catch () {
  var secp256k1 = require('secp256k1/js')
}

for electron environments.

Of course, these are just opinions. I don't feel strongly about any of them other than the get methods.

(Sorry, this probably isn't the right place to comment on this. I'm fine with this becoming bitcoinjs/bip32. If it is, I'll delete this comment and move it there.)

[fanatid]

@jprichardson

• bip32 code on ES5 will be almost indistinguishable from ES6, but I prefer ES6 of course!
• agree, we can drop get methods, but what name is suitable for getSerializedPublicKey?
• let's drop fromRandomSeed and make fromString as internal method, constructor will recognize object, base58 string and seed
• Flow can't replace typeforce, because working on compilation stage

[dcousens]

@jprichardson I'm totally on board with properties, but, without compilation warnings it can be very confusing as to what the behaviour should be if those properties are set.

What is fromString? Is it base58? hex? I think this should be renamed to add more clarity.

Where is fromString?

What about removing the random feature? This way someone could just pass in the random bytes and it's one less dependency.

What random feature?
What is fromRandomSeed?

constructor will recognize object, base58 string and seed

Multi-parameter constructors are just plain confusing. fromBase58Check should be fine?

Not a huge fan of typeforce as a dependency. Sorry @dcousens :) I'd much rather a Babel solution or even better, Flow.

They're not even in the same ball park? typeforce is for run time checks and validating data types that you can't trust.
Babel/flow would only provide compilation time validation.
Personally you should probably use both :), but I don't think forcing Babel/Flow on anyone for basic sanity type checking is the most reasonable solution either.

[dcousens]

What about removing the random feature? This way someone could just pass in the random bytes and it's one less dependency.

Do you mean makeRandom on ECPair?
I think forcing RNG input is OK, not great, but OK. Especially since users often screw that up... and what we have is actually safe.

[jprichardson]

Where is fromString?

https://github.com/fanatid/bip32/blob/c6c59a7d083c464f96236471752d2148d61b77a8/lib/index.js#L58

What is fromRandomSeed?

https://github.com/fanatid/bip32/blob/c6c59a7d083c464f96236471752d2148d61b77a8/lib/index.js#L50

Do you mean makeRandom on ECPair?

No, I'm referring to removing fromRandomSeed and thus removing random-bytes from fanatid/bip32.

They're not even in the same ball park? typeforce is for run time checks and validating data types that you can't trust. Babel/flow would only provide compilation time validation.

While this accurate for Flow, you can do runtime checking with Babel, that's why I included it. See: https://github.com/codemix/babel-plugin-typecheck

[dcousens]

Right. I thought this was a discussion RE 3.0.0 in respect to its current API. The bip32/ module is still completely WIP IMHO and discussion RE that should be moved there.

[jprichardson]

Right. I thought this was a discussion RE 3.0.0 in respect to its current API. The bip32/ module is still completely WIP IMHO and discussion RE that should be moved there.

Yes, you're right. My fault for commenting in that regard. When we get consensus (really just your decision in this case on fanatid/bip32 -> bitcoinjs/bip32... hehe). I'll ping back here and we can delete these bip32 related comments to keep the discussion on topic.

[dcousens]

@jprichardson do you have an example for a relative example of how we might use babel run time types for bitcoinjs-lib?

Also, would this mean non-babel users would essentially be left in the dust? (assuming they aren't already haha)

[jprichardson]

I think this example (https://github.com/codemix/babel-plugin-typecheck#what) is pretty solid:

function sendMessage (to: User, message: string): boolean {
  return socket.send(to, message);
}

to

function sendMessage(to, message) {
  var _socket$send;

  if (!(to instanceof User)) throw new TypeError("Value of argument 'to' violates contract.");
  if (typeof message !== "string") throw new TypeError("Value of argument 'message' violates contract.");
  _socket$send = socket.send(to, message);
  if (typeof _socket$send !== "boolean") throw new TypeError("Function 'sendMessage' return value violates contract.");
  return _socket$send;
}

Gives us both static compile-time checking and run-time checking. If we were to use Babel, we'd precompile as most do (unless they used Rollup and jsnext:main), the consumers would not get any static compile-time type checking (again, unless they used Rollup), but they'd get the run-time type checking.

[dcousens]

@jprichardson complete with crap error messages though? :/

[dcousens]

Also can I enforce more complicated types like a map of "Hex256bit": "Hex"?

[jprichardson]

Have we made consensus on switching to https://github.com/cryptocoinjs/secp256k1-node? Also, everyone cool with starting a 3.0.0 branch?

[jprichardson]

Also, @dcousens which modules are you alright with breaking out for 3.0.0 if any at all?

[dcousens]

@jprichardson pretty much all of them I thought.
I'm happy to move on to 3.0.0 now if needed.

If we do move on it now though, it means anyone wanting to use SegWit [which will probably be merged after] will likely have to upgrade... do we want to force that?

[jprichardson]

If we do move on it now though, it means anyone wanting to use SegWit will likely have to upgrade... do we want to force that?

Why would this be necessary? Couldn't we keep 2.x as the mainline for a bit?

[dcousens]

@jprichardson sorry, I mistook what you said above as a "lets make master 3.0.0 and start merging"

[dcousens]

Strongly consider secp256k1.

@jprichardson @fanatid it is your opinion that moving to BN.js would be a waste of time by comparison to secp256k1? Since, in an ideal world, we would use secp256k1.

Related to #623

[dcousens]

@ryanxcharles comment from February 2015 comes to mind as a good "goal" to continue to strive for...
But I also don't want to continue stalling on this blatant performance related issue.

[dcousens]

Bumping this to 4.0.0, as 3.0.0 is going to come early instead of 2.4.0

[dcousens]

In the spirit of #407, for the purposes of #513, #737 and #787 (and indirectly #866, even #899), I think the following point should be kept in mind for 4.0.0:

• Caching, for the sake of performance, is nearly always easier to do at a higher level of abstraction. Application level.

Think of ECPair, the main reason the abstraction exists is because

this.__Q = secp256k1.G.multiply(this.d)

is computationally expensive... without caching Q, performance is terrible.
But it is also expensive pre-emptively, so we don't even do that. We cache it in ECPair, on-demand. Boo impurity. Impurity hurts higher level caching abstractions.

The secondary reason, is getAddress was way too convenient to lose [at the time].
With #787, we will probably lose getAddress in favour of [hopefully] a much simpler, and shorter-hand API for the script templates.

That leaves Q caching as the sole reason to maintain ECPair - short of being capable of using the WIF everywhere, and a few encode/decode functions.
The decoding of Base58 is generally negligible by comparison.

If the secp256k1.G.multiply operation performance is improved, say by #737, then we could re-evaluate how useful that caching really is.

If a Buffer was used for pubkeys, and WIF string/Buffer for private keys... we could directly avoid the ECPair abstraction AND adhere to #407 allowing for library interop.

[dcousens]

To capture the motivation for the ideas being thrown around in https://github.com/bitcoinjs/playground, this is the ideal scenario for 4.0.0 script/address/classification API merger/refactor:

let { maybe witness, maybe input, output } = p2sh(p2wsh(p2pkh(pubkey, maybe signature)))
let { maybe witness, maybe input, output } = p2sh(p2wsh(p2ms(pubkeys, maybe signatures)))
let { output } = p2sh(p2wsh(p2ms(pubkeys)))
let { witness, input } = p2sh(p2wsh(p2ms(pubkeys, signatures))) // fails without pubkeys, missing redeemscript
let { witness, input } = p2ms([], signatures)

let { witness, output, pubKeys, signatures } = p2ms({ witness, scriptPubKey })

// ... or
let { output } = p2sh({ redeem: p2wsh(p2ms(pubkeys)) })

I want to capture requiredSigs in this API, and allow for easy chaining to reach any desired results.
Recursive P2SH is easily detected... classification is a matter of simply passing through a result or the parts that you have.

I don't this API is the cleanest when applied with TransactionBuilder.

I think TransactionBuilder as a stateful object was a mistake, and instead we should assume the user is going to provide all the data they can in one go.
They may try and build incomplete, but I don't think the flow of buildIncomplete... do some actions... then build, was that useful by comparison to simply doing fromTransaction in the post-haste situation?
Maybe?

@afk11 @dabura667

[dcousens]

I want to try and have a first-pass at a release candidate for this by 20th December 👍
Don't let our dreams be dreams!

[afk11]

I want to try and have a first-pass at a release candidate for this by 20th December 👍

Oh wow, that's a mission :) lets get it done

I'll start getting up to speed on it, only had time to glance at stuff recently :/

One thing I'd like to make sure is in there, basically don't construct any inputs the person hasn't provided the txOut for. Be nice to have it as it lets us expose some interesting stuff. It does reenforce however that assumptions can be made about a tx (fee wise, fully signed-ness, whatever) unless we have all inputs..

TransactionBuilder.prototype.input = function (nIn, txOut, {rs, ws}) {
    if (!(nIn in this.inputs)) {
        this.inputs[nIn] = this.buildInput(this.tx, nIn, txOut, {rs, ws})
    }
    return this.inputs[nIn]
}
TransactionBuilder.prototype.sign = function (txOut, {rs, ws}, key, hashtype) {
    this.input(nIn, txOut, {rs, ws}).sign(key, hashType)
}

so you get all sorts of nice stuff from inputs:

var input = txb.input(nIn, txOut, opts);
input.isFullySigned()
input.getSigHashUnsafe(hashType)
input.verify()
// and other goodies..

Am about to grab a flight, might try make a start.. awful tho, we have to many tests to keep happy ;)

[dcousens]

Am about to grab a flight, might try make a start.. awful tho, we have to many tests to keep happy ;)

Throw around any crazy ideas in the "playground" (maybe I should rename it "bitcoinjs-lab" - haha).
We'll get it tested.

[dcousens]

I think ECPair and HDNode should be renamed/migrated to something higher-level.

Private keys are 32-byte Buffers.
Public keys are 33/65-byte Buffers.

The existing ECPair concept is simply an optimization for the existing EC library (which is gone in 4.0.0), and could easily be taken as a responsibility by the higher level abstraction.

HDNode/ECPair could be merged, with a BIP32 extended key using the bip32 package underneath.

Any thoughts on an API around that?
We'd want to capture network AND address/script types.
BIP32 derivation paths might be a bit too far, but, still, its in the same realm.

[dabura667]

From issue 999:

ECPair and HDNode should have a scriptType attribute to make transaction building easier for users.

[dcousens]

The issues around this are relatively frequent now with P2PKH being gradually decomissioned.

#804
#854
#927
#934
#964
#966
#995
#996

[dcousens]

@dabura667 alternatively, if we simplify the keys down (as we said, remove .getAddress), and instead a simplified ECPair (as mentioned previously) and the external bip32 package, perhaps the new address functions are more than enough?

The key structures should probably only support the minimal ECDSA functionality, and a public key getter...
Anything higher may be out-of-scope for this library?

[dcousens]

From #927 (comment)

const bitcoin = require('bitcoinjs-lib')
const pubKey = bitcoin.HDNode.fromBase58('ypub6XMTwf6NSvfzYYgVgdNWRNfMTiQt4rSjZbEk8qoCnBGhUD2rsgZ2A8pexgzaGLKgySZxqxrctDpAVU8QtfxqfX8QUAhtFmGFUFx9B51TVg8')
  .derive(0)
  .derive(27)
  .getPublicKeyBuffer()

const { address } = bitcoin.payments.p2sh({
  redeem: bitcoin.payments.p2wpk({
    pubkey: pubKey
  })
})

And for signing,

let txb = new Transaction()
txb.addInput(..., ...)
txb.addOutput(..., ...)

txb.signForWitnessV0(0, unspent.value, SIGHASH_ALL, function (hash) {
  let signature = keyPair.sign(hash, SIGHASH_ALL)

  // expects { input, witness } at the least 
  return bitcoin.payments.p2sh({
    redeem: bitcoin.payments.p2wpk({
      pubkey: pubKey,
      signature: signature
    })
  })
}) 

Thoughts? ping @afk11

[dcousens]

For multisig

let signatures = [OPS.OP_0, OPS.OP_0]
let pubkeys = [alice, bob, keyPair.getPublicKeyBuffer()]

txb.sign(0, SIGHASH_ALL, function signFn (hash) {
  let signature = keyPair.sign(hash, SIGHASH_ALL)
  signatures[1] = signature

  // expects { input } at the least 
  return bitcoin.payments.p2sh({
    redeem: bitcoin.payments.p2ms({
      m: 2,
      pubkeys,
      signatures,
      allowIncomplete: true
    })
  })
})

The callback isn't async, so it may be an odd pattern, but I think it keeps the data flow clear and encapsulated. (not tied to the idea though)

[dcousens]

If we replace TransactionBuilder with the above, we miss out on:

• Multisig-re-ordering (resolved by supporting that feature in payments.p2ms)
• State being carried for us

Admittedly, the last point being what we actually want to get rid of because of how much complexity it entails and in the end, the above is insanely easy.

That said, we could have the payments result being stored as state and given as a second parameter state to the signFn.

[dcousens]

For standard P2PKH (assuming we don't add any conveniences)

txb.sign(0, SIGHASH_ALL, function (hash) {
  return bitcoin.payments.p2pkh({
    pubkey: keyPair.getPublicKeyBuffer(),
    signature: keyPair.sign(hash, SIGHASH_ALL)
  })
})

[dcousens]

Closing in favour of
#1048
#1047
#1046
#1045