Feature/create tx #160

matiu · 2014-03-15T15:37:16Z

This implements the method #create in Transactions. Its aims to provide a much simpler interface to create new transactions. Some auxiliary functions were added in Transaction and Address. The test were updated to cover the new code.

The raw output transaction from some given inputs are compared in the tests with the (hardcoded) output of createrawtransaction from bitcoind. The parameters to generate those outputs in bitcoind are included for validation.

~~In a future PR the transaction signing (#sign) will be implemented~~
.sign is provided to sign a transaction using a given key array.

ryanxcharles · 2014-03-15T22:52:31Z

Address.js

+
+  return answer;
+};
+


Good idea. It might also be a good idea to be able to set the network. The way the version() function works is that you can either get or set the network. The same thing could work here. If arguments[0] is set, set the network to that network. Otherwise, just get the current network.

setting (changing) the network of an existing address seems strange to me :) Do you think that is a real use case?

I added the same to PrivateKey.js

It's exactly the same use case as setting the version. You probably wouldn't use it to set the network after, say, creating an address from an address string. But it would be a convenient way to set the network after creating an address from a hash.

For instance, rather than this:

var addr = new bitcore.Address(network.addressVersion, hash);

...which is confusing and has to be looked up every time, to this:

var addr = new bitcore.Address(hash); addr.network('testnet');

This would require changing the constructor to detect that a buffer was being input as the first argument.

Another possible interface:

var addr = new bitcore.Address('testnet', hash);

Then self.network('testnet') would be called from within the constructor.

I'm not committed to doing this now, maybe this should be in a different PR. But it just occurred to me because the functionality of network() mirrors that of version(), but is more user-friendly.

I see. I usually create address which address string, in which the network info is embedded, i didn't think about initializing with a hash.

I like both:
var addr = new bitcore.Address(hash);
addr.network('testnet');
and
var addr = new bitcore.Address('testnet', hash);

both seems simple and straightforward to implement. Will create a new ticket for that, ok?

Added here:

#161

matiu · 2014-03-16T00:32:39Z

updated with signing capabilities.

ryanxcharles · 2014-03-16T02:12:43Z

PrivateKey.js

+    answer = testnet;
+
+  return answer;
+};


See my above comment for Address, which is the same for this.

added here:
#162

ryanxcharles · 2014-03-16T14:46:23Z

Great work on this @matiu . Will continue code reviewing today.

matiu · 2014-03-17T02:59:06Z

Updated:

better coin selection for inputs (yet, there are a lot of enhancements that can be done)
dynamic fees (that could trigger rescaning utxos)
simplified interface:
- .create (utxos, outs, keys, opts) will select the coins, generate the tx and sign it.
- .selectUnspent, .prepare and .sign are used by .create internally. They can be used from the outside to customize the txs.
- more tests were added.
- tests pass in browser.
- CreateTX example / Readme were not updated yet. They will be updated once there is consensus about the API (Update CreateTX.js and Readme example once .create() is merged #166)

jgarzik · 2014-03-17T10:26:03Z

Mostly sounds good. I thought it was agreed that signing was a separate operation from create?

matiu · 2014-03-17T12:27:46Z

@jgarzik it an be done in 1 step:

   vat tx = Transaction.create(utxos, outs,keys);

Or in 2 (or more):

   vat tx = Transaction.create(utxos,outs);
   tx.sign(utxos, keys);

or

   vat tx = Transaction.create(utxos,outs);
   tx.sign(utxos, key1);
   tx.sign(utxos, key2);
   [..]
   if (tx.isComplete()) {
      ...
   }

The 1 step creation seems to be a good idea for simpler use cases.

maraoz · 2014-03-17T13:35:42Z

Awesome work @matiu! This will be a great addition to bitcore. I like the 1-step transaction creation. Is there a reason for requiring 2 steps @jgarzik ?

ryanxcharles · 2014-03-17T13:38:03Z

Certainly for multisig you need to be able to sign it just once, and then pass the partially signed transaction onto others to continue signing. Seems like "signing" should be a separate event that follows creation of the transaction.

jgarzik · 2014-03-17T13:41:44Z

The logic behind making signing a separate step is that it is security-sensitive. Technically signing is easy to incorporate, but making it a separate step forces people to think about what they are signing, IMO (or forces programmers to think about what they are designing). Making it a separate step is easier to test, and more in line with multisig workflow.

(note: this is not "do it that way" message, just explaining the rationale)

maraoz · 2014-03-17T14:11:01Z

Good point. Would you say it shouldn't be possible to make it in one step? I like @matiu's idea of having two ways: the simplified one, with just 1 step:

vat tx = Transaction.create(utxos, outs,keys);

and the separated one, with:

vat tx = Transaction.create(utxos,outs);
tx.sign(utxos, key1);
tx.sign(utxos, key2);

The other issue I see with this API is that it assumes ouput scripts are pubkeyhash (which is OK for a first simple API, but we should at least be thinking how we would incorporate other tx output types)

ryanxcharles · 2014-03-18T04:59:45Z

Still code reviewing. I definitely think the .create function should not include the .sign function. Instead, .create should only create, but not sign, the transaction. The two could be combined into a .createAndSign function for convenience.

jrpereira · 2014-03-18T05:22:12Z

How about making it so you can chain the calls, monad-style? All it takes is for the Transaction to keep a pointer to the utxos (it's already in memory, anyway). That way you could chain the calls and have "sign" only take the keys, which considering the above examples:

vat tx = Transaction.create(utxos, outs).sign(keys);

or

vat tx = Transaction.create(utxos,outs)
         .sign(key1)
         .sign(key2);

matiu · 2014-03-18T13:02:10Z

@jrpereira a similar schema was the original proposal, but was discarded to prevent Transaction storing the utxos silently.

@ryanxcharles Do you think the most common use case will be to first create and the sign the tx? I like you idea, seems simple to understand, and friendly to new users. Will do that now.

ryanxcharles · 2014-03-18T13:32:10Z

Since I think the bitcoin world is going to move to multisig, I think the most standard use-case will be to create the transaction first, and then sign it later. Signing it upon creation only makes sense if you have all the keys.

matiu · 2014-03-18T14:41:41Z

updated with @ryanxcharles comments.

Current constructors are:

createAndSign
create

related methods: (these needed to be updated to support p2sh)

sign (will throw error if try to sign anything other that TX_PUBKEYHASH)
isComplete

Used Internally:

createWithFee (creates a TX with fixed fee, and previously selected utxos)
calcSize
getSize
_sumOutputs
_scriptForAddress (this supports p2sh)

jrpereira · 2014-03-18T14:43:07Z

@ryanxcharles, that makes sense, but doesn't that mean you'll still need the utxos somewhere? Seeing as we're not copying the object (js is copy-on-write, so just storing a pointer to it), isn't the Transaction object an appropriate place to store a reference to this data, particularly if you'll be needing it for later sign operations?

There could be something like a .finalize() call that "seals" the transaction and releases the utxos reference.

matiu · 2014-03-18T15:50:11Z

@jrpereira I think the current PR address all all the comments:

utxos are not stored on the Transaction object. create returns the new tx, and the list of the selected utxos for later signing. That list can also be used in wallet software to flag those outputs, even if the tx is not broadcasted yet.

Finally createAndSign provides a simpler interface for certain use cases.

ryanxcharles · 2014-03-18T15:54:05Z

@matiu, ACK code changes. Unfortunately there is now a conflict, probably because I pulled in your other PR first. Can you fix the conflicts and update the PR so I can pull it in?

matiu · 2014-03-18T15:55:01Z

no prob, working on it.

On Tue, Mar 18, 2014 at 12:54 PM, Ryan X. Charles
notifications@github.comwrote:

@matiu https://github.com/matiu, ACK code changes. Unfortunately there
is now a conflict, probably because I pulled in your other PR first. Can
you fix the conflicts and update the PR so I can pull it in?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/160#issuecomment-37949812
.

Matías Alejo Garcia
Skype/Twitter: @EMATIU
Roads? Where we're going, we don't need roads!

matiu · 2014-03-18T15:57:43Z

updated.

Feature/create tx

matiu added 12 commits March 14, 2014 17:38

selectUnspent function and tests

ada9274

tx creation working. more tests needed

671d372

#create for Transaction and tests

706162e

#create for Transaction and tests

35f5c9c

#create for Transaction and tests

128662c

add comment to generate hardcoded dara

ced6c91

remove outdated example

d473b40

add example to createKey

9720954

fix variable names

b6e6ad2

add test for #network in Address

7d1df26

add uncompressed key generation example

5bf652d

add network detection in private key and tests

807a726

ryanxcharles reviewed Mar 15, 2014
View reviewed changes

matiu added 3 commits March 15, 2014 21:04

ignore tags file

a6463a3

add TX signing. Support to p2pubkeyhash

a2041d5

add limit to try-verify loop

530f05f

fix error in commit

242f438

ryanxcharles reviewed Mar 16, 2014
View reviewed changes

remove uncompressed key example

aac13a8

This was referenced Mar 16, 2014

Add a simple way to sign Transactions and examples of usage #125

Closed

better Address constructor #161

Closed

better utxo selection, combining inputs different confimations steps

42d30f4

matiu added 3 commits March 16, 2014 20:50

new interfase for create TX (select + prepare + sign)

0c83ecf

dynamic fee

041f06a

add comment regarding coins selection

eb8763a

matiu mentioned this pull request Mar 17, 2014

Update CreateTX.js and Readme example once .create() is merged #166

Closed

updated interfase to create: create and createAndSign

aab52ad

fix conflics on testdata

c30cc36

ryanxcharles pushed a commit that referenced this pull request Mar 18, 2014

Merge pull request #160 from matiu/feature/create-tx

806e424

Feature/create tx

ryanxcharles merged commit 806e424 into bitpay:master Mar 18, 2014

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/create tx #160

Feature/create tx #160

matiu commented Mar 15, 2014

ryanxcharles Mar 15, 2014

matiu Mar 16, 2014

ryanxcharles Mar 16, 2014

matiu Mar 16, 2014

matiu Mar 16, 2014

matiu commented Mar 16, 2014

ryanxcharles Mar 16, 2014

matiu Mar 16, 2014

ryanxcharles commented Mar 16, 2014

matiu commented Mar 17, 2014

jgarzik commented Mar 17, 2014

matiu commented Mar 17, 2014

maraoz commented Mar 17, 2014

ryanxcharles commented Mar 17, 2014

jgarzik commented Mar 17, 2014

maraoz commented Mar 17, 2014

ryanxcharles commented Mar 18, 2014

jrpereira commented Mar 18, 2014

matiu commented Mar 18, 2014

ryanxcharles commented Mar 18, 2014

matiu commented Mar 18, 2014

jrpereira commented Mar 18, 2014

matiu commented Mar 18, 2014

ryanxcharles commented Mar 18, 2014

matiu commented Mar 18, 2014

matiu commented Mar 18, 2014

Feature/create tx #160

Feature/create tx #160

Conversation

matiu commented Mar 15, 2014

ryanxcharles Mar 15, 2014

Choose a reason for hiding this comment

matiu Mar 16, 2014

Choose a reason for hiding this comment

ryanxcharles Mar 16, 2014

Choose a reason for hiding this comment

matiu Mar 16, 2014

Choose a reason for hiding this comment

matiu Mar 16, 2014

Choose a reason for hiding this comment

matiu commented Mar 16, 2014

ryanxcharles Mar 16, 2014

Choose a reason for hiding this comment

matiu Mar 16, 2014

Choose a reason for hiding this comment

ryanxcharles commented Mar 16, 2014

matiu commented Mar 17, 2014

jgarzik commented Mar 17, 2014

matiu commented Mar 17, 2014

maraoz commented Mar 17, 2014

ryanxcharles commented Mar 17, 2014

jgarzik commented Mar 17, 2014

maraoz commented Mar 17, 2014

ryanxcharles commented Mar 18, 2014

jrpereira commented Mar 18, 2014

matiu commented Mar 18, 2014

ryanxcharles commented Mar 18, 2014

matiu commented Mar 18, 2014

jrpereira commented Mar 18, 2014

matiu commented Mar 18, 2014

ryanxcharles commented Mar 18, 2014

matiu commented Mar 18, 2014

matiu commented Mar 18, 2014