show scriptSig signature hash types. fixes #3166
Please give this a look and let me know if you’d like it changed or if I mis-understood the issue request.
The fix basically appends the scriptSig signature hash types, within parentheses, onto the end of the signature(s) in the various “asm” json outputs. That’s just the first formatting idea that came to my mind.
Added some tests for this too.
NACK
This will definitely break things, and I don’t see a need given how easy the SIGHASH flags are to remember. All the standard ones can be interpreted by thinking in terms of upper and lower nibbles:
Upper nibble == 0:
001 - ALL
102 - NONE
203 - SINGLE
Upper nibble == 8:
081 - ANYONECANPAY | ALL
182 - ANYONECANPAY | NONE
283 - ANYONECANPAY | SINGLE
That’s it.
@petertodd That’s not really fair. Why was this an open issue, if this is not desirable?
Where is the risk of breakage? It doesn’t affect consensus code. Do people rely on the exact format of the dumped script format?
@laanwj They sure do! Granted, maybe we don’t want that, in which case we should delibrately break it. (make a note in the release notes please)
Why was this an open issue, if this is not desirable?
Well, I’m telling people why I think it’s not desirable. If I’m outnumbered on this, then I’ll at least ask if we could drop the ‘SIGHASH_’ prefix on this; having the full string is kinda long if it’s not meant to be parsed by anyone other than humans.
26+ ;
27 if (vch.size() <= 4)
28 return strprintf("%d", CScriptNum(vch, false).getint());
29 else
30- return HexStr(vch);
31+ return HexStr(vch) + (CheckSignatureEncoding(vch, SCRIPT_VERIFY_STRICTENC) ? ("(" + mapSigHashTypes[vch.back()] + ")") : "");
@petertodd Yes, let’s see what the others think here. I generally like it when people solve an actual open issue :)
Agree on dropping the SIGHASH_ prefix and adding mention to doc/release-notes.md.
@petertodd @laanwj thanks for the feedback.
I have removed the “SIGHASH_” from the text due to your feedback that it was too verbose. I was back and forth on that before I pushed it up, so I’m happy for your opinions.
I can see @petertodd’s concern about this being a breaking change for people scripted against it. It’s something to weigh. I don’t know that adding more flags is the answer. From working on another pull request, my understanding is that the JSON is equivalent to “verbose”.
FWIW, I kind of like the extra decoding output myself because it’s one less thing I have to think about. It’s a simple decode like @petertodd said, but having it just there in plain sight means I don’t have to remember, or go look it up.
I will add some release notes comments in doc/release-notes.md in a little bit after waiting to hear what other feedback is.
Hm. I don’t have a strong opinion about how it would work. I would have normally assumed some annotation on the asm output, or an additional lower level asm view.
My goal was mostly that it would be clearly indicated in some manner stronger than squnting at bytes. In particular, squinting fails when you only barely know there is something you need to squint at. An expliact flag list would be much more obvious and discoverable.
WRT compatiblity of the ASM output, we’ve certantly never promised to hold that encoding constant… it’s fine if things are reading it, but they’ll need to be revised if it changes.
@mruddy Actually, I was just asking around, and it looks like people are getting the message and not depending on the asm output format as much as before. As an example Counterparty switched to using python-bitcoinlib for that on my advice.
Maybe we should reopen this for v0.11 and simultaneously drop the ‘OP_’ prefix from the opcode names in the asm output? That’d likely break the remaining stuff pretty thoroughly in a clear way.
@mruddy Great, thanks!
FWIW a “OP_” prefix dropping change should definitely be in a separate commit so it can be debated separately. Also, if changing stuff like that doesn’t break any tests, keep in mind it’s a sign that maybe depending on the exact format of the asm output is a bad idea. :)
(yeah, the more I think about this, the more I think my original objection was wrong…)
26+ ;
27 if (vch.size() <= 4)
28 return strprintf("%d", CScriptNum(vch, false).getint());
29 else
30- return HexStr(vch);
31+ return HexStr(vch) + (CheckSignatureEncoding(vch, SCRIPT_VERIFY_STRICTENC, NULL) ? ("(" + mapSigHashTypes[vch.back()] + ")") : "");
Indexing a std::map with [x] adds an entry with the default value for the type (in this case, an empty string), if the key doesn’t exist in the map. This is not thread-safe, and makes it possible to consume some memory by providing unknown sigHashTypes.
mapSigHashTypes const.find(x) instead of [x] to find the value
3@@ -4,20 +4,9 @@
4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
5
6 #include "script.h"
7-
271+ (int(SIGHASH_SINGLE|SIGHASH_ANYONECANPAY), std::string("SINGLE|ANYONECANPAY"))
272+ ;
273+
274+std::string CScript::ValueString(const std::vector<unsigned char>& vch) const
275+{
276+ if (vch.size() <= 4)
5@@ -6,9 +6,13 @@
6 #ifndef BITCOIN_SCRIPT_SCRIPT_H
7 #define BITCOIN_SCRIPT_SCRIPT_H
8
9+#include "script/interpreter.h"
10+
11 #include <assert.h>
12+#include <boost/assign/list_of.hpp>
15@@ -16,8 +16,6 @@
16
17 using namespace std;
18
19-typedef vector<unsigned char> valtype;
I don’t like exposing valtype externally, the type name is too general. Would prefer to change the function signature to just
0bool CheckSignatureEncoding(const vector<unsigned char> &vchSig, unsigned int flags, ScriptError* serror) {
239@@ -238,11 +240,11 @@ bool CScript::IsPushOnly() const
240 return true;
241 }
242
243-std::string CScript::ToString() const
16 #include <stdint.h>
17 #include <string.h>
18 #include <string>
19 #include <vector>
20
21+#include <boost/assign/list_of.hpp>
4@@ -5,18 +5,11 @@
5
6 #include "script.h"
7
8+#include "script/interpreter.h"
Would it be enough to move the opcode constants from script/interpreter to script/script in this PR to avoid this new include? I haven’t tried to compile that but I think that should be enough.
About IsPayToPublicKeyHash, do you mean ValueString shouldn’t be a method of CScript ? Otherwise, since CScript::ValueString uses IsPayToPublicKeyHash, it has to be in the same level or below, but not above. If I have to suggest another place, I think I would go with script/standard, where other “static analysis” things are.
Leaning towards closing (with the associated comment/act on the related issue). Generally agree w/ the comment that this seems like a layering violation.
The intent is understandable, but it seems like the script show function is not the right place.
TxToUniv() dumps both script hex and script asm.
I wonder if an optional argument to script.ToString() would suffice?
Printing out the flags somewhere is certainly useful. bitcoin-tx utility and RPC both call TxToUnix() when dumping the script verbosely.
I don’t see how an optional argument to CScript::ToString would solve the layering concern.
That is caused by the script object needing to know if a piece of itself is a signature with a valid hash type. Right now, that logic is in the interpreter.
I spent some time looking at the layering.
I think IsValidSignatureEncoding could be moved over to script/bitcoinconsensus.cpp since it is entirely consensus-critical since BIP66.
Since the sig hash type is not part of consensus, only part of determining if a sig is standard, I think move IsDefinedHashtypeSignature and the sig hash types over to script/standard.cpp and .h respectively.
IsLowDERSignature is effectively dead code.
Then, CheckSignatureEncoding could also go to script/standard.cpp since it’s not consensus and does not seem like it could only ever be used by the interpreter.
I think at that point the layering concern is covered.
Next, what lead to the klutziness in my code, that I wound up not really liking, was the need to answer the question of if the current script was a scriptSig. A lot of what I did to ValueString was to avoid mis-interpreting non-scriptSig data as a signature.
It seems like a flag could be set on the script object, or more object oriented, just use a SignatureScript type object when these objects are created (and PubKeyScript objects in other cases). Then this decode ticket would be an override to SignatureScript::ToString.
But, refactoring all that would obscure this change and is a lot of work for this seemingly low-value sighash decode. It’d turn into a train of refactoring tickets followed by the last ticket to actually do the simple decode. There’s also probably a reason why all that refactoring would be misguided or error-prone that I don’t foresee yet.
All my hacking to avoid refactoring is what makes me think this should just be closed.
Also, TxToUniv is only used by bitcoin-tx, CScript::ToString is the common dependency of RPC and bitcoin-tx. The RPC dependencies go through, TxToJSON (decoderawtransaction and getrawtransaction) and ScriptPubKeyToJSON (decodescript). That reason, and thinking that a script should know how to show/display/decode itself, is why I targeted CScript::ToString as the place to make these changes.
I think IsValidSignatureEncoding could be moved over to script/bitcoinconsensus.cpp since it is entirely consensus-critical since BIP66.
It is in script/interpreter, so it’s already part of libconsensus, see: https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.am#L365
Since the sig hash type is not part of consensus… Then, CheckSignatureEncoding could also go to script/standard.cpp since it’s not consensus and does not seem like it could only ever be used by the interpreter.
The sighash types and CheckSignatureEncoding are definitely part of consensus (and are used by the interpreter), please, just leave them there. The only thing you need to move is the new code for converting to string to script/standard and I think all layering concerns are gone.
It is in script/interpreter, so it’s already part of libconsensus, see: https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.am#L365
That’s true. I was just thinking that determining whether a signature is validly encoded is generic enough that it could be useful without including the interpreter. Personal preference, I guess.
The sighash types and CheckSignatureEncoding are definitely part of consensus (and are used by the interpreter), please, just leave them there.
I was assuming that to begin with, but IsDefinedHashtypeSignature is only called when the flags include SCRIPT_VERIFY_STRICTENC. The comment for SCRIPT_VERIFY_STRICTENC even states, “not used or intended as a consensus rule”. The best I found was that scripts with a bad sighash type would fail to eval (and thus be non-standard), but that if they got into valid blocks we’d still have to accept those blocks. I suppose I could try it and see, but that’s what made me say that the sig hash type is not actually part of consensus. Maybe we’re using the word consensus in two different ways here.
The only thing you need to move is the new code for converting to string to script/standard and I think all layering concerns are gone.
Oh, you mean move ValueString and mapSigHashTypes over to script/standard and add the CScript as an argument to ValueString? Yes, that may make sense. And, if we move the sighash stuff from interpreter over to standard, then it’ll all be in the same place.
STRICTENC is indeed not technically consensus-critical, but it is part of the interpreter (as we want it for evaluation of standardness), which consensus-critical code depends on. I don’t see how you can disconnect it from the interpreter.
In general I prefer to have the basic data type definitions (script/script, CScript) to have as little dependencies and functionality as possible, as they get pulled into everything, including consensus code. Ideally, they only provide representatiin, serialization, deserialization, and basic construction. Something like ToString() is mostly for debugging purposes IMHO, and should not be used for actual conversion to well-defined other representation. See for example the core_io module for transaction/block conversion code.
Oh, you mean move ValueString and mapSigHashTypes over to script/standard and add the CScript as an argument to ValueString? Yes, that may make sense.
Yes. I like this PR, but the code should just be in script/standard instead of script/script or script/interpreter, it’s only that.
And, if we move the sighash stuff from interpreter over to standard, then it’ll all be in the same place.
No, the sighash stuff is consensus critical so you don’t want it in script/standard because you don’t want the rest of what’s in there for libconsensus. I wouldn’t oppose to move the sighash flags, SignatureHash, TransactionSignatureChecker and MutableTransactionSignatureChecker to a different file (still required for consensus), but I don’t think that’s a priority and is certainly not needed for this PR (or libconsensus).
@jtimon OK, I just rebased and made the layering changes that I think we were talking about. I didn’t squish them into one commit so that you could easily diff.
I did look at what @sipa was talking about. I see he added a FormatScript in core_write.cpp that I could probably build off of instead (never saw it before because it’s only referenced by tests). He might be onto something. At quick glance, I think I might be able to make things simpler by going the route he suggested. I’ll look into it more and may open an alternate pull request to this one. But at least the latest commit here kind of avoids the layering concern now, I think.
Now it’s actually worse because you’re putting the whole script/standard inside libconsensus. In fact, that’s what is giving you errors on travis https://travis-ci.org/bitcoin/bitcoin/jobs/72684953#L2093 because you haven’t adapted makefile.am to include script/standard in libconsensus.
But you’re very close, you can just move the rest of CScript::ToString to the new function in script/standard. That way you will be able to pass only a const CScript& as parameter (no additional vch param, which is nice) and you will be making libconsensus actually smaller instead of bigger. I’m not sure how many places CScript::ToString is called, but it is just a mechanical replace for ToString(const CScript&) [or whatever you want to call it].
Sorry if I haven’t clearer earlier about CScript::ToString: if you want to use the new function in script/standard, it cannot be a method of CScript but an independent function instead (because script/script should not depend on neither script/interpreter nor script/standard).
To try to be clearer: if you need to “de-encapsulate” anything from CScript - by creating a new child class that uses the protected arguments, by creating new getters or even by making a private/protected public - to be able to move CScript::ToString up, I’m totally fine with that.
I’m happy with any solution that ends up with equal or less code in libconsensus (https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.am#L365), anything else is secondary to me with rewards to this PR (it also makes it “less risky” [I don’t think your changes are risky even violating the layering]).
My preference would be to simply rename CScript::ToString() to script/standard::ToString(const CScript&) - a trivial change using regular expressions or patience (the compiler won’t let you get this little step wrong, anyway: just remove CScript::ToString and make the compiler happy with the new function). But better is better, so my only requirement for an utACK is what I said before: equal or less code in libconsensus (and less is better).
EDIT: So, to be completely clear. It’s not your fault that CScript::ToString() is in libconsensus, we just don’t want you to make it worse by improving the method/function in-place.
@jtimon that’s a lot of useful input, thanks! I took that and actually refactored these changes twice.
I put each of the two ways into the last two commits.
The first commit refactored into a couple of formatter classes. It worked, but the second/next idea seemed a little cleaner.
The second commit does away with the formatter class idea of the previous commit and just moves CScript::ToString out into a function in core_write.cpp.
I think this last commit converges your input with sipa’s input. I think the last one loops in everyone’s input, doesn’t layer violate, and reduces code from the consensus library.
I purposefully didn’t choose to sighhash decode some scriptSigs that were just going to be logged since those usages were tangential to what i was trying to actually update for this ticket.
Let me know what you think of the latest. Thanks!
You are welcomed. I’m fine with putting new things in core_io as @sipa suggested, just not in libconsensus. CTransaction::ToString now calls ScriptToAsmStr so instead of putting script/standard in libconsensus, now you’re putting core_io instead. The important thing is this: independently of where you chose to put the new code outside of libconsensus, you cannot include that new code from libconsensus. That’s why I keep posting a link to the list of files that are part of libconsensus: https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.am#L365
If you include more things in any of those files, travis will complain (unless you adapt the makefile properly to include the new file, in which case we will complain).
So, either don’t adapt CTransaction::ToScript to the new code (seems the simpler solution) or also take CTransaction::ToString out of consensus (seems better but also more work).
4@@ -5,6 +5,7 @@
5
6 #include "primitives/transaction.h"
7
8+#include "core_io.h"
11@@ -12,6 +12,7 @@
12 #include "primitives/transaction.h"
13 #include "script/script.h"
14 #include "script/sign.h"
15+#include "script/standard.h"
8@@ -9,6 +9,7 @@
9 #include "key.h"
10 #include "keystore.h"
11 #include "script/script.h"
12+#include "script/standard.h"
88+ }
89+ if (0 <= opcode && opcode <= OP_PUSHDATA4) {
90+ if (vch.size() <= static_cast<vector<unsigned char>::size_type>(4)) {
91+ str += strprintf("%d", CScriptNum(vch, false).getint());
92+ } else {
93+ if (fAttemptSighashDecode) {
These changes decode valid SIGHASH types on signatures in assembly (asm) representations of scriptSig scripts.
This squashed commit incorporates substantial helpful feedback from jtimon, laanwj, and sipa.
35@@ -36,7 +36,7 @@ std::string CTxIn::ToString() const
36 if (prevout.IsNull())
37 str += strprintf(", coinbase %s", HexStr(scriptSig));
38 else
39- str += strprintf(", scriptSig=%s", scriptSig.ToString().substr(0,24));
40+ str += strprintf(", scriptSig=%s", HexStr(scriptSig).substr(0, 24));
HexStr wasn’t used here before?
CScript::ToString: https://github.com/bitcoin/bitcoin/commit/af3208bfa6967d6b35aecf0ba35d9d6bf0f8317e#diff-f7ca24fb80ddba0f291cb66344ca6fcb