add p2sh and segwit options to bitcoin-tx outscript command by czzarr · Pull Request #10 · ChaincodeResidency/bitcoin

czzarr · 2016-09-30T19:34:33Z

This adds the option of creating p2sh, segwit and segwit wrapped into p2sh outputs from bitcoin-tx.

This is a proof-of-concept, just to check that it's ok on principle.

TheBlueMatt · 2016-09-30T19:42:52Z

        strUsage += HelpMessageOpt("-create", _("Create new, empty TX."));
        strUsage += HelpMessageOpt("-json", _("Select JSON output"));
        strUsage += HelpMessageOpt("-txid", _("Output only the hex-encoded transaction id of the resultant transaction."));
+        strUsage += HelpMessageOpt("-segwit", _("Produce a segwit output."));


Maybe 'Transform "outscript" commands to add a segwit output, instead.' - its a bit clearer whats going on then.

So for instance would you rather transform the outscript command such that it accepts the following:
VALUE:SCRIPT:SEGWIT
VALUE:SCRIPT:P2SH
VALUE:SCRIPT:SEGWIT:P2SH
?

Yea, that seems like a better API to me?

ok I'll do that then

I'd prefer adding a single new field for all flags, ie:

VALUE:SCRIPT[:FLAGS]

where FLAGS can be:

S for P2WSH,

H for P2SH and

SH for P2WSH wrapped in P2SH.

That makes the API more easily extensible if we want to add additional flags in future.

TheBlueMatt

Code looks good...kinda a strage API, though. Most of the API does exactly what the command was instead of having modifiers on what commands do. Still, not sure its incorrect, per se, so feel free to clarify the docs and PR upstream if you want more feedback.

sdaftuar · 2016-09-30T19:45:26Z

One thought -- bitcoin#8499 introduces code to prevent addwitnessaddress from working with an uncompressed pubkey. Perhaps bitcoin-tx should be doing the same, if it can tell that you're using an uncompressed pubkey with a segwit output?

Edit: meh, maybe not, i dunno...

czzarr · 2016-09-30T20:47:54Z

@sdaftuar shouldn't that check be put into GetScriptForWitness() ?

jnewbery · 2016-10-03T14:54:42Z

As discussed, this PR replicates the functionality of #5 . Rather than have two different PRs that implement different APIs for the same functionality, we should merge them before pushing upstream to bitcoin/bitcoin.

jnewbery · 2016-10-04T14:06:46Z

-        (pos == 0))
+    // separate VALUE:SCRIPT(:SEGWIT)(:P2SH)
+    std::vector<std::string> vStrInput;
+    boost::split(vStrInput, strInput, boost::is_any_of(":"));


There seems to be a general move away from Boost (https://github.com/bitcoin/bitcoin/projects/3). Sadly, I don't think there's anything in c++11 as simple as boost::split for splitting a string (see RegisterSet() for another way of splitting the string).

It's probably cleaner to keep this as is, but you may want to consider doing it without boost.

jnewbery · 2016-10-04T14:07:54Z

I've added a couple of comments. Let me know once you've addressed those and squashed your commit. I'll merge the rest of my functionality on top and then we can open a PR to bitcoin/bitcoin.

jnewbery · 2016-10-04T19:58:11Z

@czzarr , as discussed this morning, I've updated the API and merged my changes into this branch. I've also added test cases covering all the new functionality. Are you happy for me to go ahead and open the PR against bitcoin/bitcoin?

Note that I've rebased this branch and squashed your commits together, so you'll need to fetch the branch from origin and throw away your local changes.

czzarr

What do you think of S as a flag for scripthash instead of H?
Also made two other small comments, lmk what you think.

Rest looks great, props for the tests.

czzarr · 2016-10-04T20:12:13Z

nit: should probably be called MutateTxAddOutMultiSig to stay inline with the rest of the nomenclature.

Yes, agree.

czzarr · 2016-10-04T20:15:28Z

Question: do these options make sense for a pubkey output?

Yes - as discussed, this is the way to create P2WPKH (since you need to input the pubkey, not the address for that) and P2WPKH wrapped in P2SH.

jnewbery · 2016-10-04T20:38:37Z

Agree - I'll change the option to be S instead of H for scripthash

jnewbery · 2016-10-04T21:25:37Z

Thanks for the code review @czzarr. I've made suggested changes and squashed my commits. I'll open a PR against bitcoin/bitcoin.

jnewbery · 2016-10-04T21:30:10Z

PR opened: bitcoin#8883 . Closing this PR.

TheBlueMatt reviewed Sep 30, 2016

View reviewed changes

czzarr force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from 6bd35e5 to 1a27e46 Compare September 30, 2016 20:43

jnewbery reviewed Oct 4, 2016

View reviewed changes

add p2sh and segwit options to bitcoin-tx outscript command

7e19a32

jnewbery force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from 1a27e46 to e3ffc53 Compare October 4, 2016 19:55

jnewbery mentioned this pull request Oct 4, 2016

Add all standard TXO types to bitcoin-tx #5

Closed

czzarr commented Oct 4, 2016

View reviewed changes

jnewbery added 2 commits October 4, 2016 17:23

Add all transaction output types to bitcoin-tx.

dfa2ac6

Add test cases

c23fa1d

jnewbery force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from e3ffc53 to c23fa1d Compare October 4, 2016 21:24

jnewbery closed this Oct 4, 2016

Uh oh!

Conversation

czzarr commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czzarr commented Sep 30, 2016

Uh oh!

jnewbery commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 4, 2016

Uh oh!

jnewbery commented Oct 4, 2016

Uh oh!

czzarr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czzarr Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 4, 2016

Uh oh!

jnewbery commented Oct 4, 2016

Uh oh!

jnewbery commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

czzarr commented Sep 30, 2016 •

edited

Loading

TheBlueMatt Sep 30, 2016 •

edited

Loading

sdaftuar commented Sep 30, 2016 •

edited

Loading

czzarr Oct 4, 2016 •

edited

Loading