Add all standard TXO types to bitcoin-tx #8883

jnewbery · 2016-10-04T21:29:02Z

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
- multi-sig in Pay to Witness Script Hash, wrapped in P2SH
Pay to Witness Pub Key Hash
- Pay to Witness Pub Key Hash, wrapped in P2SH
Pay to Witness Script Hash
- Pay to Witness Script Hash, wrapped in P2SH

The PR also includes new test cases for all tx types.

fanquake · 2016-10-05T07:10:22Z

Windows builds are failing:

make[2]: Entering directory `/home/travis/build/bitcoin/bitcoin/build/src'
  CXX      bitcoin_tx-bitcoin-tx.o
../../src/bitcoin-tx.cpp: In function ‘void MutateTxAddOutMultiSig(CMutableTransaction&, const string&)’:
../../src/bitcoin-tx.cpp:309:5: error: ‘uint’ was not declared in this scope
     uint required = std::stoul(vStrInputParts[1]);
     ^
../../src/bitcoin-tx.cpp:309:10: error: expected ‘;’ before ‘required’
     uint required = std::stoul(vStrInputParts[1]);
          ^
../../src/bitcoin-tx.cpp:312:10: error: expected ‘;’ before ‘numkeys’
     uint numkeys = std::stoul(vStrInputParts[2]);
          ^
../../src/bitcoin-tx.cpp:315:33: error: ‘numkeys’ was not declared in this scope
     if (vStrInputParts.size() < numkeys + 3)
                                 ^
../../src/bitcoin-tx.cpp:318:9: error: ‘required’ was not declared in this scope
     if (required < 1 || required > 20 || numkeys < 1 || numkeys > 20 || numkeys < required)
         ^
../../src/bitcoin-tx.cpp:318:42: error: ‘numkeys’ was not declared in this scope
     if (required < 1 || required > 20 || numkeys < 1 || numkeys > 20 || numkeys < required)
                                          ^
../../src/bitcoin-tx.cpp:324:14: error: expected ‘;’ before ‘pos’
     for(uint pos = 1; pos <= numkeys; pos++) {
              ^
../../src/bitcoin-tx.cpp:324:23: error: ‘pos’ was not declared in this scope
     for(uint pos = 1; pos <= numkeys; pos++) {
                       ^
../../src/bitcoin-tx.cpp:324:30: error: ‘numkeys’ was not declared in this scope
     for(uint pos = 1; pos <= numkeys; pos++) {
                              ^
../../src/bitcoin-tx.cpp:334:34: error: ‘numkeys’ was not declared in this scope
     if (vStrInputParts.size() == numkeys + 4) {
                                  ^
../../src/bitcoin-tx.cpp:344:49: error: ‘required’ was not declared in this scope
     CScript scriptPubKey = GetScriptForMultisig(required, pubkeys);
                                                 ^
make[2]: *** [bitcoin_tx-bitcoin-tx.o] Error 1
make[2]: Leaving directory `/home/travis/build/bitcoin/bitcoin/build/src'
make[1]: *** [check-recursive] Error 1
make[1]: Leaving directory `/home/travis/build/bitcoin/bitcoin/build/src'
make: *** [check-recursive] Error 1

paveljanik · 2016-10-05T18:18:09Z

Concept ACK

New warning:

bitcoin-tx.cpp:283:25: warning: declaration shadows a local variable [-Wshadow]
        CBitcoinAddress addr(scriptPubKey);
                        ^
bitcoin-tx.cpp:265:21: note: previous declaration is here
    CBitcoinAddress addr(scriptPubKey);
                    ^
1 warning generated.

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

Thanks @paveljanik . Fixed in squashed commit.

paveljanik · 2016-10-05T19:18:30Z

-Wshadow clean now, thanks!

I'll read the changes later tonight.

paveljanik · 2016-10-05T20:19:14Z

src/bitcoin-tx.cpp

+            _("Optionally add the \"W\" flag to produce a pay-to-witness-script-hash output") + ". " +
+            _("Optionally add the \"S\" flag to wrap the output in a pay-to-script-hash."));
+        strUsage += HelpMessageOpt("outmultisig=VALUE:REQUIRED:PUBKEYS:PUBKEY1:PUBKEY2:....[:FLAGS]", _("Add Pay To n-of-m Multi-sig output to TX. n = REQUIRED, m = PUBKEYS") + ". " +
+            _("Optionally add the \"W\" flag to produce a pay-to-witness-script-hash output") + ". " +


Why do you sometimes translate dot at the end of sentence and sometimes not?

for some reason I get a compile error when I put the dot in the sentence:

bitcoin-tx.cpp:79:13: error: expected ')' _("Optionally add the \"W\" flag to produce a pay-to-witness-pubkey-hash output") + ". " + ^ bitcoin-tx.cpp:78:35: note: to match this '(' strUsage += HelpMessageOpt("outpubkey=VALUE:PUBKEY[:FLAGS]", _("Add pay-to-pubkey output to TX. ") ^ 1 error generated.

paveljanik · 2016-10-05T20:20:04Z

src/bitcoin-tx.cpp

-        (pos == 0) ||
-        (pos == (strInput.size() - 1)))
-        throw runtime_error("TX output missing separator");
+    // Seperate into VALUE:ADDRESS


Separate (e -> a)?

paveljanik · 2016-10-05T20:21:57Z

src/bitcoin-tx.cpp

-        throw runtime_error("TX output missing separator");
+    // Seperate into VALUE:ADDRESS
+    vector<string> vStrInputParts;
+    boost::split(vStrInputParts, strInput, boost::is_any_of(":"));


This adds another dependency on boost. But as we already use it in this file, it is OK. And definitely looks better ;-)

paveljanik · 2016-10-05T20:25:56Z

src/bitcoin-tx.cpp

+    vector<string> vStrInputParts;
+    boost::split(vStrInputParts, strInput, boost::is_any_of(":"));
+
+    // Extract and validate VALUE


$ grep "Extract and validate VALUE" bitcoin-tx.cpp // Extract and validate VALUE // Extract and validate VALUE // Extract and validate VALUE $

What about adding some function for this and share the code in all instances?

done in 79cd927

jonasschnelli · 2016-10-08T14:52:50Z

Concept ACK.

jonasschnelli · 2016-10-08T14:47:54Z

src/bitcoin-tx.cpp

+        bScriptHash = (flags.find("S") != string::npos);
+    }
+
+    if (bSegWit) {


Is there a reason for bSegWit and not directly use (flags.find("W") != string::pos)?

See below. The flags are not exclusive. I need to put the flags.find() test inside the (vStrInputParts.size() == 3) so I may as well parse for both flags at the same point and store the result in variables.

jonasschnelli · 2016-10-08T14:48:08Z

src/bitcoin-tx.cpp

+        // Call GetScriptForWitness() to build a P2WSH scriptPubKey
+        scriptPubKey = GetScriptForWitness(scriptPubKey);
+    }
+    if (bScriptHash) {


maybe a else if?

They're not exclusive. If both flags are used, we output a P2WPKH wrapped in P2SH.

jonasschnelli · 2016-10-08T14:49:05Z

src/bitcoin-tx.cpp

+    CPubKey pubkey(ParseHex(vStrInputParts[1]));
+    if (!pubkey.IsFullyValid())
+        throw runtime_error("invalid TX output pubkey");
+    CScript scriptPubKey = GetScriptForRawPubKey(pubkey);


Does this result in supporting pure P2PK (not P2PKH)?

Yes - without either flag, the TXO will be a P2PK.

With just the H flag, the TXO is a P2SH

With just the W flag, the TXO is a P2WPK

With both the H and W flags, the TXO is a P2WPK wrapped in a P2SH

laanwj · 2016-10-18T19:23:33Z

Needs rebase.

jnewbery · 2016-10-18T20:33:05Z

Rebased. @jonasschnelli - are you happy with my responses above?

jonasschnelli · 2016-10-19T12:13:24Z

@jnewbery: Yes.
utACK, will test now...

jonasschnelli · 2016-10-19T12:16:01Z

Travis is failing on the new tests:

Running test/bitcoin-util-test.py...
make[4]: Entering directory `/home/travis/build/bitcoin/bitcoin/build/src'
Output data mismatch for txcreatescript1.json
make[3]: *** [check-local] Error 1
make[3]: *** Waiting for unfinished jobs....

jnewbery · 2016-10-24T14:39:21Z

I've fixed the failing testcases and squashed the commits. @jonasschnelli - please let me know if you have any further feedback for this PR.

jonasschnelli · 2016-10-28T08:59:41Z

slightly tested ACK e5fecbf64d1dad360490294d1aad79497dcb1de7.
Someone should verify the unit tests fixtures.

laanwj · 2016-12-14T09:24:30Z

utACK.
Needs rebase though.

jnewbery · 2016-12-23T13:44:34Z

rebased

maflcko · 2016-12-27T22:46:25Z

Travis fails with

2016-12-27 19:30:20,251 - ERROR - Output data mismatch for txcreatescript1.hex (format hex)

Edit: Also make sure to include all new files: #9436

This commit 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 - multi-sig in Pay to Witness Script Hash, wrapped in P2SH - Pay to Witness Pub Key Hash - Pay to Witness Pub Key Hash, wrapped in P2SH - Pay to Witness Script Hash - Pay to Witness Script Hash, wrapped in P2SH

This commit add testcases to test the following functions in bitcoin-tx: - add a pay to non-standard script output - add a P2SH output - add a P2WSH output - add a P2WSH wrapped in a P2SH output - add a pay to pub key output - add a P2WPKH output - add a P2WPKH wrapped in a P2SH output - add a bare multisig output - add a multisig in P2SH output - add a multisig in a P2WSH output - add a multisig in a P2WSH wrapped in as P2SH output

jnewbery · 2017-01-09T22:26:13Z

Rebased. I had to do a couple of large merges (removing std namespace and updating default tx version). @laanwj - let me know if there's anything I can do to help get this merged into master.

0c50909 testcases: explicitly specify transaction version 1 (John Newbery) b7e144b Add test cases to test new bitcoin-tx functionality (jnewbery) 61a1534 Add all transaction output types to bitcoin-tx. (jnewbery) 1814b08 add p2sh and segwit options to bitcoin-tx outscript command (Stanislas Marion)

@codablock

…dablock committed on Jan 20, 2018 Use version 2 blocks for miner_tests … @codablock codablock committed on Jan 20, 2018 Merge bitcoin#7871: Manual block file pruning. … @laanwj @codablock laanwj authored and codablock committed on Jan 11, 2017 Merge bitcoin#9507: Fix use-after-free in CTxMemPool::removeConflicts() … @sipa @codablock sipa authored and codablock committed on Jan 11, 2017 Merge bitcoin#9297: Various RPC help outputs updated … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 12, 2017 Merge bitcoin#9416: travis: make distdir before make … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 12, 2017 Merge bitcoin#9520: Deprecate non-txindex getrawtransaction and bette… … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 12, 2017 Merge bitcoin#9518: Return height of last block pruned by pruneblockc… … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 12, 2017 Merge bitcoin#9472: Disentangle progress estimation from checkpoints … … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#8883: Add all standard TXO types to bitcoin-tx … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#9261: Add unstored orphans with rejected parents to rec… … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#9468: [Depends] Dependency updates for 0.14.0 … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#9222: Add 'subtractFeeFromAmount' option to 'fundrawtra… … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#9490: Replace FindLatestBefore used by importmuti with … … @sipa @codablock sipa authored and codablock committed on Jan 13, 2017 Merge bitcoin#9469: [depends] Qt 5.7.1 … @laanwj @codablock laanwj authored and codablock committed on Jan 15, 2017 Merge bitcoin#9380: Separate different uses of minimum fees … @laanwj @codablock laanwj authored and codablock committed on Jan 16, 2017 Remove SegWit related code in dash-tx @codablock codablock committed on Sep 21, 2017 Merge bitcoin#9561: Wake message handling thread when we receive a ne… … @sipa @codablock sipa authored and codablock committed on Jan 17, 2017 Merge bitcoin#9508: Remove unused Python imports … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 18, 2017 Merge bitcoin#9512: Fix various things -fsanitize complains about

jnewbery mentioned this pull request Oct 4, 2016

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

Closed

jnewbery force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from c23fa1d to ca0f851 Compare October 5, 2016 13:48

jnewbery force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from ca0f851 to f6bbe64 Compare October 5, 2016 19:03

paveljanik reviewed Oct 5, 2016

View reviewed changes

jonasschnelli added Scripts and tools Utils/log/libs and removed Scripts and tools labels Oct 8, 2016

jonasschnelli suggested changes Oct 8, 2016

View reviewed changes

jnewbery force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from 79cd927 to 1733547 Compare October 18, 2016 20:32

jnewbery force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch 2 times, most recently from f5b706d to e5fecbf Compare October 24, 2016 14:20

jonasschnelli approved these changes Oct 28, 2016

View reviewed changes

laanwj modified the milestones: 0.15.0, 0.14.0 Dec 14, 2016

jnewbery force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from e5fecbf to e0d9e1f Compare December 23, 2016 13:43

czzarr and others added 3 commits December 29, 2016 15:40

add p2sh and segwit options to bitcoin-tx outscript command

1814b08

jnewbery force-pushed the add-p2sh-segwit-options-to-bitcoin-tx branch from e0d9e1f to b7e144b Compare December 29, 2016 15:41

testcases: explicitly specify transaction version 1

0c50909

laanwj merged commit 0c50909 into bitcoin:master Jan 12, 2017

jnewbery deleted the add-p2sh-segwit-options-to-bitcoin-tx branch January 12, 2017 13:47

str4d mentioned this pull request Mar 14, 2018

CLI binary improvements zcash/zcash#3086

Merged

CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019

Merge bitcoin#8883: Add all standard TXO types to bitcoin-tx

f5447c9

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add all standard TXO types to bitcoin-tx #8883

Add all standard TXO types to bitcoin-tx #8883

Uh oh!

Conversation

jnewbery commented Oct 4, 2016

Uh oh!

fanquake commented Oct 5, 2016

Uh oh!

paveljanik commented Oct 5, 2016

Uh oh!

jnewbery commented Oct 5, 2016

Uh oh!

paveljanik commented Oct 5, 2016

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!

paveljanik Oct 5, 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!

jonasschnelli commented Oct 8, 2016

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!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 18, 2016

Uh oh!

jnewbery commented Oct 18, 2016

Uh oh!

jonasschnelli commented Oct 19, 2016

Uh oh!

jonasschnelli commented Oct 19, 2016

Uh oh!

jnewbery commented Oct 24, 2016

Uh oh!

jonasschnelli commented Oct 28, 2016

Uh oh!

laanwj commented Dec 14, 2016

Uh oh!

jnewbery commented Dec 23, 2016

Uh oh!

maflcko commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jan 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

paveljanik Oct 5, 2016 •

edited

Loading

maflcko commented Dec 27, 2016 •

edited

Loading