Skip to content

Add all standard TXO types to bitcoin-tx - #5

Closed
jnewbery wants to merge 1 commit into
ChaincodeResidency:masterfrom
jnewbery:bitcoin-tx-addstandardTXOs
Closed

Add all standard TXO types to bitcoin-tx#5
jnewbery wants to merge 1 commit into
ChaincodeResidency:masterfrom
jnewbery:bitcoin-tx-addstandardTXOs

Conversation

@jnewbery

Copy link
Copy Markdown

bitcoin-tx can currently create transactions with the following TXO types:

  • Pay to Pub Key Hash
  • null data (OP_RETURN)
  • Pay to Script Hash

This PR enhances bitcoin-tx so all remaining standard TXO types can be created:

  • Pay to Pub Key
  • Multi-sig
    • bare multi-sig
    • multi-sig in Pay To Script Hash
    • multi-sig in Pay to Witness Script Hash
  • Pay to Witness Pub Key
  • Pay to Witness Script Hash

@jnewbery

Copy link
Copy Markdown
Author

@czzarr do you mind code reviewing this for me? I think that passing the paytoscript* and paytowitness* options to the MutateTxAddOut*() functions is a bit ugly. If there were any more options I'd probably define some constants and pass as a single bitfield flag, but passing two bools is probably ok.

@czzarr czzarr left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure exactly I understand how you split the different functions. For instance, a P2SH should always be a P2SH, whether it wraps a segwit output or not. I think maybe a way to deal with that would be to add P2SH as a step instead of an alternative.

Comment thread src/bitcoin-tx.cpp
CBitcoinAddress addr(script);
scriptPubKey = GetScriptForDestination(addr.Get());
}
else if (paytowitnessscripthash) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think technically this is not an if/else situation. You should be able to wrap your P2WSH in a P2SH.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, it's possible to create a P2WSH wrapped in a P2SH, but I haven't implemented that in bitcoin-tx. If you think that's going to be useful, you could add separate commands for:

  • multisig in P2WSH wrapped in P2SH
  • any script in P2WSH wrapped in P2SH
  • P2WPK wrapped in P2SH

I don't think there's much use case for this, but feel free to create an additional PR on top of this if you think it'd be useful.

Comment thread src/bitcoin-tx.cpp
// This could either refer to the scriptPubKey (for a bare pay-to-script)
// or the redeem script (for a P2SH or P2WSH)

if (paytoscripthash) {

@czzarr czzarr Sep 26, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't understand why you need this, it is a given that it is a scripthash output (from the arguments).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

as discussed, this function MutateTxAddOutScript() can be used to add the following kinds of TXO:

  • bare script
  • P2SH
  • P2WSH

Currently, bitcoin-tx can only create bare script outputs. This flag paytoscripthash is used to signal that the TXO should be P2SH rather than a pay to script.

@jnewbery

jnewbery commented Oct 4, 2016

Copy link
Copy Markdown
Author

I've now merged all functionality into #10 . Closing this PR.

@jnewbery jnewbery closed this Oct 4, 2016
@jnewbery
jnewbery deleted the bitcoin-tx-addstandardTXOs branch October 11, 2016 09:09
jnewbery pushed a commit that referenced this pull request Dec 23, 2016
003a4ac Merge #5: fix typo
5254f14 [trivial] Fix typo
e7c0aab Merge #4: Fix some comments
d07cead Fix some comments

git-subtree-dir: src/crypto/ctaes
git-subtree-split: 003a4acfc273932ab8c2e276cde3b4f3541012dd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants