New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add witness data output to TxInError messages #8384

Merged
merged 2 commits into from May 18, 2017

Conversation

Projects
None yet
9 participants
@instagibbs
Member

instagibbs commented Jul 20, 2016

Makes for easier debugging.

@mcelrath

This comment has been minimized.

Show comment
Hide comment
@mcelrath

mcelrath Jul 20, 2016

Tested ACK

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

Tested ACK

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

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
Member

NicolasDorier commented Jul 21, 2016

utACK 73c5eb1

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 3, 2016

Member

Does this need to be tested in the RPC tests?

Member

laanwj commented Aug 3, 2016

Does this need to be tested in the RPC tests?

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Aug 3, 2016

Member

@laanwj added basic test check for new field

Member

instagibbs commented Aug 3, 2016

@laanwj added basic test check for new field

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jan 25, 2017

Member

utACK 34bbc0d Needs rebase.

Member

jtimon commented Jan 25, 2017

utACK 34bbc0d Needs rebase.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Jan 25, 2017

Member

Rebased

Member

instagibbs commented Jan 25, 2017

Rebased

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 26, 2017

Member

utACK cb70b74

Member

laanwj commented Jan 26, 2017

utACK cb70b74

Show outdated Hide outdated src/rpc/rawtransaction.cpp
@@ -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)

This comment has been minimized.

@sipa

sipa Jan 26, 2017

Member

Pass CScriptWitness by reference?

@sipa

sipa Jan 26, 2017

Member

Pass CScriptWitness by reference?

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Feb 3, 2017

Member

ACK squashing b69789a into ef8726c

Member

jtimon commented Feb 3, 2017

ACK squashing b69789a into ef8726c

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Feb 18, 2017

Member

Squash?

Member

jtimon commented Feb 18, 2017

Squash?

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Feb 21, 2017

Member

squashed

Member

instagibbs commented Feb 21, 2017

squashed

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Mar 3, 2017

Member

This need anything for merge?

Member

instagibbs commented Mar 3, 2017

This need anything for merge?

Show outdated Hide outdated src/rpc/rawtransaction.cpp
@@ -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) {

This comment has been minimized.

@jnewbery

jnewbery Mar 3, 2017

Member

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.

@jnewbery

jnewbery Mar 3, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Mar 4, 2017

Member

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

Member

instagibbs commented Mar 4, 2017

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

@mchrostowski

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

Show outdated Hide outdated src/rpc/rawtransaction.cpp
@@ -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)

This comment has been minimized.

@mchrostowski

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

@mchrostowski

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

Show outdated Hide outdated src/rpc/rawtransaction.cpp
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));

This comment has been minimized.

@mchrostowski

mchrostowski 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

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

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs May 17, 2017

Member

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

Member

instagibbs commented May 17, 2017

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 17, 2017

Member

utACK 6e9e026

Member

sipa commented May 17, 2017

utACK 6e9e026

@sipa sipa merged commit 6e9e026 into bitcoin:master May 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request May 18, 2017

Merge #8384: Add witness data output to TxInError messages
6e9e026 Expand signrawtransaction.py to cover error witness checking (Gregory Sanders)
9f7341b Add witness data output to TxInError messages (Gregory Sanders)

Tree-SHA512: 6f2a758544fa2657f3a57051bdb80fb14cb10501c8ef4ccbab7a62d4b6a823e74f40991c8796248865def24619b620b859dc2bb08dc2cc72511c1cf3897ab1a9

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment