Add all standard TXO types to bitcoin-tx #8883

pull jnewbery wants to merge 4 commits into bitcoin:master from ChaincodeResidency:add-p2sh-segwit-options-to-bitcoin-tx changing 24 files +541 −31
  1. jnewbery commented at 9:29 PM on October 4, 2016: member

    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.

  2. fanquake commented at 7:10 AM on October 5, 2016: member

    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
  3. jnewbery force-pushed on Oct 5, 2016
  4. paveljanik commented at 6:18 PM on October 5, 2016: contributor

    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.
  5. jnewbery force-pushed on Oct 5, 2016
  6. jnewbery commented at 7:04 PM on October 5, 2016: member

    Thanks @paveljanik . Fixed in squashed commit.

  7. paveljanik commented at 7:18 PM on October 5, 2016: contributor

    -Wshadow clean now, thanks!

    I'll read the changes later tonight.

  8. in src/bitcoin-tx.cpp:None in f6bbe64607 outdated 
      82 | -        strUsage += HelpMessageOpt("outscript=VALUE:SCRIPT", _("Add raw script output to TX"));
  83 | +        strUsage += HelpMessageOpt("outscript=VALUE:SCRIPT[:FLAGS]", _("Add raw script output to TX") + ". " +
  84 | +            _("Optionally add the \"W\" flag to produce a pay-to-witness-script-hash output") + ". " +
  85 | +            _("Optionally add the \"S\" flag to wrap the output in a pay-to-script-hash."));
  86 | +        strUsage += HelpMessageOpt("outmultisig=VALUE:REQUIRED:PUBKEYS:PUBKEY1:PUBKEY2:....[:FLAGS]", _("Add Pay To n-of-m Multi-sig output to TX. n = REQUIRED, m = PUBKEYS") + ". " +
  87 | +            _("Optionally add the \"W\" flag to produce a pay-to-witness-script-hash output") + ". " +
    paveljanik commented at 8:19 PM on October 5, 2016:

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

    czzarr commented at 9:35 PM on October 5, 2016:

    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.
  9. in src/bitcoin-tx.cpp:None in f6bbe64607 outdated 
     227 | -    size_t pos = strInput.find(':');
 228 | -    if ((pos == string::npos) ||
 229 | -        (pos == 0) ||
 230 | -        (pos == (strInput.size() - 1)))
 231 | -        throw runtime_error("TX output missing separator");
 232 | +    // Seperate into VALUE:ADDRESS
    paveljanik commented at 8:20 PM on October 5, 2016:

    Separate (e -> a)?

    czzarr commented at 9:15 PM on October 5, 2016:

    fixed

  10. in src/bitcoin-tx.cpp:None in f6bbe64607 outdated 
     229 | -        (pos == 0) ||
 230 | -        (pos == (strInput.size() - 1)))
 231 | -        throw runtime_error("TX output missing separator");
 232 | +    // Seperate into VALUE:ADDRESS
 233 | +    vector<string> vStrInputParts;
 234 | +    boost::split(vStrInputParts, strInput, boost::is_any_of(":"));
    paveljanik commented at 8:21 PM on October 5, 2016:

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

  11. in src/bitcoin-tx.cpp:None in f6bbe64607 outdated 
     260 | +{
 261 | +    // Seperate into VALUE:PUBKEY[:FLAGS]
 262 | +    vector<string> vStrInputParts;
 263 | +    boost::split(vStrInputParts, strInput, boost::is_any_of(":"));
 264 | +
 265 | +    // Extract and validate VALUE
    paveljanik commented at 8:25 PM on October 5, 2016: 
    $ 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?

  12. in src/bitcoin-tx.cpp:None in 79cd9275db outdated 
     287 | +        string flags = vStrInputParts[2];
 288 | +        bSegWit = (flags.find("W") != string::npos);
 289 | +        bScriptHash = (flags.find("S") != string::npos);
 290 | +    }
 291 | +
 292 | +    if (bSegWit) {
    jonasschnelli commented at 2:47 PM on October 8, 2016:

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

    jnewbery commented at 9:04 AM on October 11, 2016:

    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.

  13. in src/bitcoin-tx.cpp:None in 79cd9275db outdated 
     291 | +
 292 | +    if (bSegWit) {
 293 | +        // Call GetScriptForWitness() to build a P2WSH scriptPubKey
 294 | +        scriptPubKey = GetScriptForWitness(scriptPubKey);
 295 | +    }
 296 | +    if (bScriptHash) {
    jonasschnelli commented at 2:48 PM on October 8, 2016:

    maybe a else if?

    jnewbery commented at 9:04 AM on October 11, 2016:

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

  14. in src/bitcoin-tx.cpp:None in 79cd9275db outdated 
     275 | +
 276 | +    // Extract and validate PUBKEY
 277 | +    CPubKey pubkey(ParseHex(vStrInputParts[1]));
 278 | +    if (!pubkey.IsFullyValid())
 279 | +        throw runtime_error("invalid TX output pubkey");
 280 | +    CScript scriptPubKey = GetScriptForRawPubKey(pubkey);
    jonasschnelli commented at 2:49 PM on October 8, 2016:

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

    jnewbery commented at 9:01 AM on October 11, 2016:

    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
  15. jonasschnelli added the label Scripts and tools on Oct 8, 2016
  16. jonasschnelli added the label Utils and libraries on Oct 8, 2016
  17. jonasschnelli removed the label Scripts and tools on Oct 8, 2016
  18. jonasschnelli commented at 2:52 PM on October 8, 2016: contributor

    Concept ACK.

  19. jonasschnelli changes_requested
  20. laanwj commented at 7:23 PM on October 18, 2016: member

    Needs rebase.

  21. jnewbery force-pushed on Oct 18, 2016
  22. jnewbery commented at 8:33 PM on October 18, 2016: member

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

  23. jonasschnelli commented at 12:13 PM on October 19, 2016: contributor

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

  24. jonasschnelli commented at 12:16 PM on October 19, 2016: contributor

    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....
  25. jnewbery force-pushed on Oct 19, 2016
  26. jnewbery force-pushed on Oct 24, 2016
  27. jnewbery commented at 2:39 PM on October 24, 2016: member

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

  28. jonasschnelli approved
  29. jonasschnelli commented at 8:59 AM on October 28, 2016: contributor

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

  30. laanwj commented at 9:24 AM on December 14, 2016: member

    utACK. Needs rebase though.

  31. laanwj added this to the milestone 0.15.0 on Dec 14, 2016
  32. laanwj added this to the milestone 0.14.0 on Dec 14, 2016
  33. laanwj removed this from the milestone 0.15.0 on Dec 14, 2016
  34. jnewbery force-pushed on Dec 23, 2016
  35. jnewbery commented at 1:44 PM on December 23, 2016: member

    rebased

  36. MarcoFalke commented at 10:46 PM on December 27, 2016: member

    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

  37. add p2sh and segwit options to bitcoin-tx outscript command 1814b089fb
  38. Add all transaction output types to bitcoin-tx.
    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
    61a153443e
  39. Add test cases to test new bitcoin-tx functionality
    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
    b7e144bb73
  40. jnewbery force-pushed on Dec 29, 2016
  41. testcases: explicitly specify transaction version 1 0c50909347
  42. jnewbery commented at 10:26 PM on January 9, 2017: member

    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.

  43. laanwj merged this on Jan 12, 2017
  44. laanwj closed this on Jan 12, 2017
  45. laanwj referenced this in commit d5d4ad87af on Jan 12, 2017
  46. jnewbery deleted the branch on Jan 12, 2017
  47. codablock referenced this in commit c2aaf2b233 on Jan 19, 2018
  48. codablock referenced this in commit 80cedcfaf9 on Jan 20, 2018
  49. codablock referenced this in commit a0d089eda1 on Jan 21, 2018
  50. andvgal referenced this in commit 4d7f261034 on Jan 6, 2019
  51. CryptoCentric referenced this in commit f5447c983d on Feb 27, 2019
  52. CryptoCentric referenced this in commit e11d1ae195 on Feb 27, 2019
  53. MarcoFalke locked this on Sep 8, 2021
Contributors
jnewberyfanquakepaveljanikczzarrjonasschnellilaanwjMarcoFalke
Labels
Utils/log/libs

Milestone
0.14.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me