Skip to content

add p2sh and segwit options to bitcoin-tx outscript command #10

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

add p2sh and segwit options to bitcoin-tx outscript command #10

wants to merge 3 commits into from

Conversation

@czzarr
Copy link

@czzarr czzarr commented Sep 30, 2016 •
edited
Loading

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
TheBlueMatt reviewed Sep 30, 2016
View reviewed changes
src/bitcoin-tx.cpp Outdated
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."));
Copy link

@TheBlueMatt TheBlueMatt Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@czzarr czzarr Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
?

Copy link

@TheBlueMatt TheBlueMatt Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that seems like a better API to me?

Copy link
Author

@czzarr czzarr Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I'll do that then

Copy link

@jnewbery jnewbery Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
TheBlueMatt reviewed Sep 30, 2016
View reviewed changes
Copy link

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

sdaftuar commented Sep 30, 2016 •
edited
Loading

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 czzarr force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from 6bd35e5 to 1a27e46 Compare September 30, 2016 20:43
@czzarr
Copy link
Author

czzarr commented Sep 30, 2016

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

@jnewbery
Copy link

jnewbery commented Oct 3, 2016

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
jnewbery reviewed Oct 4, 2016
View reviewed changes
src/bitcoin-tx.cpp
(pos == 0))
// separate VALUE:SCRIPT(:SEGWIT)(:P2SH)
std::vector<std::string> vStrInput;
boost::split(vStrInput, strInput, boost::is_any_of(":"));
Copy link

@jnewbery jnewbery Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

jnewbery commented Oct 4, 2016

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 jnewbery force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from 1a27e46 to e3ffc53 Compare October 4, 2016 19:55
@jnewbery
Copy link

jnewbery commented Oct 4, 2016

@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.

@jnewbery jnewbery mentioned this pull request Oct 4, 2016
Closed
czzarr
czzarr commented Oct 4, 2016
View reviewed changes
Copy link
Author

@czzarr czzarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/bitcoin-tx.cpp Outdated
Copy link
Author

@czzarr czzarr Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@jnewbery jnewbery Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agree.

src/bitcoin-tx.cpp Outdated
Copy link
Author

@czzarr czzarr Oct 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@jnewbery jnewbery Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

jnewbery commented Oct 4, 2016

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

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

jnewbery commented Oct 4, 2016

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

@jnewbery
Copy link

jnewbery commented Oct 4, 2016

PR opened: bitcoin#8883 . Closing this PR.

@jnewbery jnewbery closed this Oct 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt left review comments

@jnewbery jnewbery jnewbery left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@czzarr @sdaftuar @jnewbery @TheBlueMatt