Add witness data output to TxInError messages #8384

instagibbs commented at 7:15 PM on July 20, 2016: member

Makes for easier debugging.

MarcoFalke added the label RPC/REST/ZMQ on Jul 20, 2016

instagibbs force-pushed on Jul 20, 2016

mcelrath commented at 8:31 PM on July 20, 2016: none

Tested ACK

Works fine with signrawtransaction that fails to fully sign the transaction. (empty witness)

NicolasDorier commented at 2:53 PM on July 21, 2016: contributor

utACK 73c5eb18e64c7071bfba45029264effccbef3cf3

laanwj commented at 9:07 AM on August 3, 2016: member

Does this need to be tested in the RPC tests?

instagibbs commented at 1:05 PM on August 3, 2016: member

@laanwj added basic test check for new field

luke-jr referenced this in commit c0631fafaf on Oct 20, 2016

luke-jr referenced this in commit 34bbc0d3f2 on Oct 20, 2016

jtimon commented at 8:18 PM on January 25, 2017: contributor

utACK 34bbc0d Needs rebase.

instagibbs force-pushed on Jan 25, 2017

instagibbs commented at 8:47 PM on January 25, 2017: member

Rebased

instagibbs force-pushed on Jan 25, 2017

laanwj commented at 3:41 PM on January 26, 2017: member

utACK cb70b74

in src/rpc/rawtransaction.cpp:None in cb70b74d6a outdated

 581 | @@ -582,11 +582,16 @@ UniValue decodescript(const JSONRPCRequest& request)
 582 |  }
 583 |  
 584 |  /** Pushes a JSON object for script verification or signing errors to vErrorsRet. */
 585 | -static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std::string& strMessage)
 586 | +static void TxInErrorToJSON(const CTxIn& txin, const CScriptWitness witness, UniValue& vErrorsRet, const std::string& strMessage)

sipa commented at 5:06 PM on January 26, 2017:

Pass CScriptWitness by reference?

jtimon commented at 3:59 AM on February 3, 2017: contributor

ACK squashing b69789a8addc086da4515783808d869d592b13f0 into ef8726c7c9d0f6d57f0419017b09b80c1553e3ea

jtimon commented at 10:57 PM on February 18, 2017: contributor

Squash?

instagibbs force-pushed on Feb 21, 2017

instagibbs commented at 2:36 AM on February 21, 2017: member

squashed

instagibbs commented at 1:09 AM on March 3, 2017: member

This need anything for merge?

in src/rpc/rawtransaction.cpp:None in f6ad6af3fb outdated

 824 | @@ -820,9 +825,13 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
 825 |      // Sign what we can:
 826 |      for (unsigned int i = 0; i < mergedTx.vin.size(); i++) {
 827 |          CTxIn& txin = mergedTx.vin[i];
 828 | +        CScriptWitness witness;
 829 | +        if (txin.scriptWitness.stack.size() >= i + 1) {

jnewbery commented at 2:07 PM on March 3, 2017:

I think this is the wrong check. You're testing the size of the witness stack for this particular CTxIn, but I think what you intend is to check for the existence of a witness for this CTxIn. I think the test you need is:

if (!txin.scriptWitness.IsNull()) {

Your test will fail if (for example) TxIn in postion 3 has only 2 items on its witness stack.

instagibbs force-pushed on Mar 3, 2017

instagibbs commented at 4:31 AM on March 4, 2017: member

@jnewbery you're right, bad rebase. Sorry for not being careful. Check isn't even needed.

in src/rpc/rawtransaction.cpp:585 in a3ba1cf818 outdated

 581 | @@ -582,11 +582,16 @@ UniValue decodescript(const JSONRPCRequest& request)
 582 |  }
 583 |  
 584 |  /** Pushes a JSON object for script verification or signing errors to vErrorsRet. */
 585 | -static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std::string& strMessage)
 586 | +static void TxInErrorToJSON(const CTxIn& txin, const CScriptWitness& witness, UniValue& vErrorsRet, const std::string& strMessage)

mchrostowski commented at 11:30 PM on May 16, 2017:

Why take an extra argument here? In all use cases witness is txin.witness. More concise change if the method simply references txin.witness rather than changing code for each call.

in src/rpc/rawtransaction.cpp:594 in a3ba1cf818 outdated

 590 |      entry.push_back(Pair("vout", (uint64_t)txin.prevout.n));
 591 | +    UniValue txinwitness(UniValue::VARR);
 592 | +    for (unsigned int i = 0; i < witness.stack.size(); i++) {
 593 | +        txinwitness.push_back(HexStr(witness.stack[i].begin(), witness.stack[i].end()));
 594 | +    }
 595 | +    entry.push_back(Pair("txinwitness", txinwitness));

mchrostowski commented at 11:36 PM on May 16, 2017:

"txinwitness" here seems derived from the variable name, in the context of the entry object it is understood the entry represents a transaction input error, using "witness" instead matches the other entries closer. That is, the error has a "sequence" not a "txinnsequence" or similar.

mchrostowski changes_requested

mchrostowski commented at 11:52 PM on May 16, 2017: contributor

Minor change suggestions to make the PR smaller and its changes more consistent with existing code.

instagibbs force-pushed on May 17, 2017

Add witness data output to TxInError messages 9f7341b078

Expand signrawtransaction.py to cover error witness checking 6e9e026656

instagibbs force-pushed on May 17, 2017

instagibbs commented at 6:16 PM on May 17, 2017: member

@mchrostowski thanks, that was leftover from previous structure of code. Nits addressed.

sipa commented at 7:33 PM on May 17, 2017: member

utACK 6e9e02665674209b0b43f2aebecc7bb71e527054

sipa merged this on May 18, 2017

sipa closed this on May 18, 2017

sipa referenced this in commit e317c0d192 on May 18, 2017

luke-jr referenced this in commit 0daa720a7a on Jun 15, 2017

luke-jr referenced this in commit 2dee676ff1 on Jun 15, 2017

DrahtBot locked this on Sep 8, 2021