Add witness data output to TxInError messages #8384
Conversation
|
Tested ACK Works fine with |
|
utACK 73c5eb18e64c7071bfba45029264effccbef3cf3 |
|
Does this need to be tested in the RPC tests? |
|
@laanwj added basic test check for new field |
Github-Pull: bitcoin#8384 Rebased-From: 73c5eb18e64c7071bfba45029264effccbef3cf3
Github-Pull: bitcoin#8384 Rebased-From: 464c826d825e0e5c71a4e05fdc169184d9903abf
|
utACK 34bbc0d Needs rebase. |
|
Rebased |
af0b15e to
cb70b74
Compare
|
utACK cb70b74 |
src/rpc/rawtransaction.cpp
Outdated
| @@ -582,11 +582,16 @@ UniValue decodescript(const JSONRPCRequest& request) | |||
| } | |||
|
|
|||
| /** Pushes a JSON object for script verification or signing errors to vErrorsRet. */ | |||
| static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std::string& strMessage) | |||
| static void TxInErrorToJSON(const CTxIn& txin, const CScriptWitness witness, UniValue& vErrorsRet, const std::string& strMessage) | |||
|
Squash? |
|
squashed |
|
This need anything for merge? |
src/rpc/rawtransaction.cpp
Outdated
| @@ -820,9 +825,13 @@ UniValue signrawtransaction(const JSONRPCRequest& request) | |||
| // Sign what we can: | |||
| for (unsigned int i = 0; i < mergedTx.vin.size(); i++) { | |||
| CTxIn& txin = mergedTx.vin[i]; | |||
| CScriptWitness witness; | |||
| if (txin.scriptWitness.stack.size() >= i + 1) { | |||
There was a problem hiding this comment.
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.
41a0ec0 to
a3ba1cf
Compare
|
@jnewbery you're right, bad rebase. Sorry for not being careful. Check isn't even needed. |
src/rpc/rawtransaction.cpp
Outdated
| @@ -582,11 +582,16 @@ UniValue decodescript(const JSONRPCRequest& request) | |||
| } | |||
|
|
|||
| /** Pushes a JSON object for script verification or signing errors to vErrorsRet. */ | |||
| static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std::string& strMessage) | |||
| static void TxInErrorToJSON(const CTxIn& txin, const CScriptWitness& witness, UniValue& vErrorsRet, const std::string& strMessage) | |||
There was a problem hiding this comment.
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.
src/rpc/rawtransaction.cpp
Outdated
| for (unsigned int i = 0; i < witness.stack.size(); i++) { | ||
| txinwitness.push_back(HexStr(witness.stack[i].begin(), witness.stack[i].end())); | ||
| } | ||
| entry.push_back(Pair("txinwitness", txinwitness)); |
There was a problem hiding this comment.
"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 thanks, that was leftover from previous structure of code. Nits addressed. |
|
utACK 6e9e026 |
6e9e026 Expand signrawtransaction.py to cover error witness checking (Gregory Sanders) 9f7341b Add witness data output to TxInError messages (Gregory Sanders) Tree-SHA512: 6f2a758544fa2657f3a57051bdb80fb14cb10501c8ef4ccbab7a62d4b6a823e74f40991c8796248865def24619b620b859dc2bb08dc2cc72511c1cf3897ab1a9
Github-Pull: bitcoin#8384 Rebased-From: 9f7341b
Github-Pull: bitcoin#8384 Rebased-From: 6e9e026
Makes for easier debugging.