update bitcoin-tx to output witness data #8817

jnewbery · 2016-09-26T21:08:28Z

In 7c4bf77 , @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.

sipa · 2016-09-28T02:46:19Z

utACK 76992bf

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

jonasschnelli · 2016-09-28T13:04:32Z

utACK 76992bf
Agree with @sipa about the tests.

jnewbery · 2016-09-28T17:53:57Z

I've added JSON test cases in #8829

maflcko · 2016-09-29T08:58:36Z

I assume this needs backport?

btcdrak · 2016-09-29T11:00:12Z

utACK 76992bf

fivepiece · 2016-09-29T18:28:42Z

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.
~~#8837 (comment)~~

Cleared.

jtimon · 2016-09-30T15:24:54Z

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

sipa · 2016-10-03T00:26:19Z

utACK 76992bf

gmaxwell · 2016-10-03T07:24:28Z

ACK

ryanofsky · 2016-10-03T19:30:26Z

src/core_write.cpp

+            if (!tx.wit.IsNull()) {
+                if (!tx.wit.vtxinwit[i].IsNull()) {
+                    UniValue txinwitness(UniValue::VARR);
+                    for (unsigned int j = 0; j < tx.wit.vtxinwit[i].scriptWitness.stack.size(); j++) {


Might be good to use BOOST_FOREACH here (for readability and to avoid copying the stack items).

We're trying to move away from BOOST in favour of c++11. See https://github.com/bitcoin/bitcoin/projects/3

In that case, for (const auto& item : tx.wit.vtxinwit[i].scriptWitness.stack) would be great.

Yep. Much better. Thanks!

ryanofsky · 2016-10-03T19:39:31Z

src/core_write.cpp

            o.pushKV("hex", HexStr(txin.scriptSig.begin(), txin.scriptSig.end()));
            in.pushKV("scriptSig", o);
+            if (!tx.wit.IsNull()) {
+                if (!tx.wit.vtxinwit[i].IsNull()) {


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.

I've added your (i < tx.wit.vtxinwit.size()) check and moved all checks into a single if statement.

ryanofsky · 2016-10-03T19:42:49Z

src/core_write.cpp

            o.pushKV("asm", ScriptToAsmStr(txin.scriptSig, true));
            o.pushKV("hex", HexStr(txin.scriptSig.begin(), txin.scriptSig.end()));
            in.pushKV("scriptSig", o);
+            if (!tx.wit.IsNull()) {


For clarity and efficiency would suggest moving this outside the for loop, since internally CTxWitness::IsNull loops over all the i's.

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.

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.

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.

jnewbery · 2016-10-03T22:25:15Z

I've applied @ryanofsky's changes and rebased.

maflcko · 2016-10-04T09:03:27Z

Fails with Output data mismatch for blanktx.json

maflcko · 2016-10-04T09:06:21Z

src/core_write.cpp

Why? Please keep it at BOOST or make it cpp11

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)

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.

laanwj · 2016-10-04T11:41:26Z

Failing test:


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

fanquake · 2016-10-05T04:02:04Z

Looks like all tests are now passing.

Making check in src
/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS check-local
PASS: test/test_bitcoin
PASS: qt/test/test_bitcoin-qt
============================================================================
Testsuite summary for Bitcoin Core 0.13.99
============================================================================
# TOTAL: 2
# PASS:  2
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================
Running test/bitcoin-util-test.py...
/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
PASS: tests
``

jnewbery · 2016-10-05T13:01:19Z

Commits squashed and commit comment updated.

4408558 Update bitcoin-tx to output witness data. (jonnynewbs)

Github-Pull: bitcoin#8817 Rebased-From: 4408558

laanwj added the Utils/log/libs label Sep 27, 2016

jnewbery mentioned this pull request Sep 27, 2016

Refactor TxToJSON() and ScriptPubKeyToJSON() #8824

Merged

jnewbery mentioned this pull request Sep 28, 2016

Add bitcoin-tx JSON tests #8829

Merged

maflcko added the Needs backport label Sep 29, 2016

maflcko added this to the 0.13.1 milestone Sep 29, 2016

ryanofsky reviewed Oct 3, 2016

View reviewed changes

jnewbery force-pushed the bitcoin-tx-witness branch 3 times, most recently from eb930c9 to 9f85db7 Compare October 3, 2016 22:23

maflcko reviewed Oct 4, 2016

View reviewed changes

Update bitcoin-tx to output witness data.

4408558

jnewbery force-pushed the bitcoin-tx-witness branch from 3a32aa8 to 4408558 Compare October 5, 2016 13:00

laanwj merged commit 4408558 into bitcoin:master Oct 13, 2016

laanwj added a commit that referenced this pull request Oct 13, 2016

Merge #8817: update bitcoin-tx to output witness data

e2b8c39

4408558 Update bitcoin-tx to output witness data. (jonnynewbs)

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Oct 13, 2016

Update bitcoin-tx to output witness data.

bcf3806

Github-Pull: bitcoin#8817 Rebased-From: 4408558

laanwj removed the Needs backport label Oct 13, 2016

jnewbery deleted the bitcoin-tx-witness branch October 14, 2016 09:23

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

update bitcoin-tx to output witness data #8817

update bitcoin-tx to output witness data #8817

Uh oh!

Conversation

jnewbery commented Sep 26, 2016

Uh oh!

sipa commented Sep 28, 2016

Uh oh!

jonasschnelli commented Sep 28, 2016

Uh oh!

jnewbery commented Sep 28, 2016

Uh oh!

maflcko commented Sep 29, 2016

Uh oh!

btcdrak commented Sep 29, 2016

Uh oh!

fivepiece commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Sep 30, 2016

Uh oh!

sipa commented Oct 3, 2016

Uh oh!

gmaxwell commented Oct 3, 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!

ryanofsky Oct 3, 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!

ryanofsky Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 3, 2016

Uh oh!

maflcko commented Oct 4, 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!

laanwj commented Oct 4, 2016

Uh oh!

fanquake commented Oct 5, 2016

Uh oh!

jnewbery commented Oct 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

fivepiece commented Sep 29, 2016 •

edited

Loading

ryanofsky Oct 3, 2016 •

edited

Loading

ryanofsky Oct 3, 2016 •

edited

Loading