update bitcoin-tx to output witness data #8817

pull jnewbery wants to merge 1 commits into bitcoin:master from jnewbery:bitcoin-tx-witness changing 12 files +21 −1
  1. jnewbery commented at 9:08 pm on September 26, 2016: member

    In https://github.com/bitcoin/bitcoin/commit/7c4bf779e8b74e474551982a24f5acc265293abd , @jl2012 updated getrawtransaction() to return the witness data for transaction inputs spending P2WPK and P2WSH.

    bitcoin-tx doesn’t call into this function and instead calls TxToUniv() in core_write.cpp, so bitcoin-tx calls don’t return witness data.

    This PR adds code to TxToUniv() to return witness data.

    The full fix is to have getrawtransaction() call into TxToUniv() so there isn’t duplicate code. I’ll open a separate PR for that change.

  2. laanwj added the label Utils and libraries on Sep 27, 2016
  3. sipa commented at 2:46 am on September 28, 2016: member

    utACK 76992bf22987ead0c2643685a228769ef0932a1b

    Side note: it seems we have no tests for bitcoin-tx JSON functionality, despite there being some other tests for it. Perhaps it can be added (see src/test/data/bitcoin-util-test.py).

  4. jonasschnelli commented at 1:04 pm on September 28, 2016: contributor
    utACK 76992bf22987ead0c2643685a228769ef0932a1b Agree with @sipa about the tests.
  5. jnewbery commented at 5:53 pm on September 28, 2016: member
    I’ve added JSON test cases in #8829
  6. MarcoFalke added the label Needs backport on Sep 29, 2016
  7. MarcoFalke added this to the milestone 0.13.1 on Sep 29, 2016
  8. MarcoFalke commented at 8:58 am on September 29, 2016: member
    I assume this needs backport?
  9. btcdrak commented at 11:00 am on September 29, 2016: contributor
    utACK 76992bf
  10. fivepiece commented at 6:28 pm on September 29, 2016: contributor

    I’ll reference this here as well since it all seems (to me) to be setting some kind of precedent wrt blank transactions. Would love a bit of insight into this. https://github.com/bitcoin/bitcoin/pull/8837#issuecomment-250542708

    Cleared.

  11. jtimon commented at 3:24 pm on September 30, 2016: contributor
    Concept ACK. I believe it would be simpler and not harder for me to review if you go with your “full solution that doesn’t duplicate code” in the same PR, but that may just be me…
  12. sipa commented at 0:26 am on October 3, 2016: member
    utACK 76992bf22987ead0c2643685a228769ef0932a1b
  13. gmaxwell commented at 7:24 am on October 3, 2016: contributor
    ACK
  14. in src/core_write.cpp: in 76992bf229 outdated 
    167@@ -166,6 +168,16 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry)
168             o.pushKV("asm", ScriptToAsmStr(txin.scriptSig, true));
169             o.pushKV("hex", HexStr(txin.scriptSig.begin(), txin.scriptSig.end()));
170             in.pushKV("scriptSig", o);
171+            if (!tx.wit.IsNull()) {
172+                if (!tx.wit.vtxinwit[i].IsNull()) {
173+                    UniValue txinwitness(UniValue::VARR);
174+                    for (unsigned int j = 0; j < tx.wit.vtxinwit[i].scriptWitness.stack.size(); j++) {
    ryanofsky commented at 7:30 pm on October 3, 2016:
    Might be good to use BOOST_FOREACH here (for readability and to avoid copying the stack items).
    jnewbery commented at 8:11 pm on October 3, 2016:
    We’re trying to move away from BOOST in favour of c++11. See https://github.com/bitcoin/bitcoin/projects/3
    ryanofsky commented at 8:15 pm on October 3, 2016:
    In that case, for (const auto& item : tx.wit.vtxinwit[i].scriptWitness.stack) would be great.
    jnewbery commented at 9:33 pm on October 3, 2016:
    Yep. Much better. Thanks!
  15. in src/core_write.cpp: in 76992bf229 outdated 
    167@@ -166,6 +168,16 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry)
168             o.pushKV("asm", ScriptToAsmStr(txin.scriptSig, true));
169             o.pushKV("hex", HexStr(txin.scriptSig.begin(), txin.scriptSig.end()));
170             in.pushKV("scriptSig", o);
171+            if (!tx.wit.IsNull()) {
172+                if (!tx.wit.vtxinwit[i].IsNull()) {
    ryanofsky commented at 7:39 pm on October 3, 2016:

    This check is redundant with the check above, and might be worth removing.

    It would probably also be good to add an (i < tx.wit.vtxinwit.size()) check or assertion.

    jnewbery commented at 8:07 pm on October 3, 2016:
    I’ve added your (i < tx.wit.vtxinwit.size()) check and moved all checks into a single if statement.
  16. in src/core_write.cpp: in 76992bf229 outdated 
    167@@ -166,6 +168,16 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry)
168             o.pushKV("asm", ScriptToAsmStr(txin.scriptSig, true));
169             o.pushKV("hex", HexStr(txin.scriptSig.begin(), txin.scriptSig.end()));
170             in.pushKV("scriptSig", o);
171+            if (!tx.wit.IsNull()) {
    ryanofsky commented at 7:42 pm on October 3, 2016:
    For clarity and efficiency would suggest moving this outside the for loop, since internally CTxWitness::IsNull loops over all the i’s.
    jnewbery commented at 8:06 pm on October 3, 2016:
    I don’t think we need to be too concerned about efficiency here. ScriptPubKeyToUniv() isn’t heavily used, so looping through the CTxIn isn’t going to cause any performance issues.
    ryanofsky commented at 8:12 pm on October 3, 2016:
    Sounds good. I was suggesting this more for clarity than efficiency, though. Right now I would be surprised looking at this code to learn that if that any /single/ vtxinwit is null, then no output will be produced for all other vtxinwit. Adding a well named bool variable outside the for loop might make it a little clearer. Anyway, thanks for considering this, and for making the other changes.
    ryanofsky commented at 8:40 pm on October 3, 2016:
    Actually, I’m wrong about this. CTxWitness::IsNull returns true if all vtxinwits are null, not if any are null, so having this check isn’t actually misleading w.r.t the individual entries, just redundant.
  17. jnewbery force-pushed on Oct 3, 2016
  18. jnewbery force-pushed on Oct 3, 2016
  19. jnewbery force-pushed on Oct 3, 2016
  20. jnewbery commented at 10:25 pm on October 3, 2016: member
    I’ve applied @ryanofsky’s changes and rebased.
  21. MarcoFalke commented at 9:03 am on October 4, 2016: member
    Fails with Output data mismatch for blanktx.json
  22. in src/core_write.cpp: in 9f85db7c69 outdated 
    156     entry.pushKV("locktime", (int64_t)tx.nLockTime);
157 
158     UniValue vin(UniValue::VARR);
159-    BOOST_FOREACH(const CTxIn& txin, tx.vin) {
160+    for (unsigned int i = 0; i < tx.vin.size(); i++) {
161+        const CTxIn& txin = tx.vin[i];
    MarcoFalke commented at 9:06 am on October 4, 2016:
    Why? Please keep it at BOOST or make it cpp11
    laanwj commented at 9:47 am on October 4, 2016:
    I think it’s because he needs index i below. I’ve wondered this before: does c++11 have a canonical way to do enumerated iteration (like enumerate() in python or for i, a := range ... { in golang)
    jnewbery commented at 1:16 pm on October 4, 2016:
    That’s correct. I don’t think there’s a direct equivalent for python’s enumerate in c++11. The most common suggestions I can find on stackoverflow are zipping with an index or tracking the index outside the for loop. I think it’s clearer just to iterate through the indexes in the for loop.
  23. laanwj commented at 11:41 am on October 4, 2016: member

    Failing test:

    0
1make  check-TESTS check-local
2make[3]: Entering directory `/home/travis/build/bitcoin/bitcoin/build/src'
3Running test/bitcoin-util-test.py...
4make[4]: Entering directory `/home/travis/build/bitcoin/bitcoin/build/src'
5Output data mismatch for blanktx.json
6make[3]: *** [check-local] Error 1
7make[3]: *** Waiting for unfinished jobs....
  24. fanquake commented at 4:02 am on October 5, 2016: member

    Looks like all tests are now passing.

     0Making check in src
 1/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS check-local
 2PASS: test/test_bitcoin
 3PASS: qt/test/test_bitcoin-qt
 4============================================================================
 5Testsuite summary for Bitcoin Core 0.13.99
 6============================================================================
 7# TOTAL: 2
 8# PASS:  2
 9# SKIP:  0
10# XFAIL: 0
11# FAIL:  0
12# XPASS: 0
13# ERROR: 0
14============================================================================
15Running test/bitcoin-util-test.py...
16/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
17PASS: tests
18``
  25. Update bitcoin-tx to output witness data. 4408558843
  26. jnewbery force-pushed on Oct 5, 2016
  27. jnewbery commented at 1:01 pm on October 5, 2016: member
    Commits squashed and commit comment updated.
  28. laanwj merged this on Oct 13, 2016
  29. laanwj closed this on Oct 13, 2016
  30. laanwj referenced this in commit e2b8c394d6 on Oct 13, 2016
  31. laanwj referenced this in commit bcf3806f4c on Oct 13, 2016
  32. laanwj removed the label Needs backport on Oct 13, 2016
  33. jnewbery deleted the branch on Oct 14, 2016
  34. DrahtBot locked this on Sep 8, 2021


jnewbery sipa jonasschnelli MarcoFalke btcdrak fivepiece jtimon gmaxwell ryanofsky laanwj fanquake

Labels
Utils/log/libs

Milestone
0.13.1

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: 2024-12-19 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me