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

show scriptSig signature hash types in transaction decodes. fixes #3166 #5264

Merged
merged 1 commit into from Sep 25, 2015

Conversation

Projects
None yet
9 participants
@mruddy
Contributor

mruddy commented Nov 12, 2014

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.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 12, 2014

Contributor

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:

01 - ALL
02 - NONE
03 - SINGLE

Upper nibble == 8:

81 - ANYONECANPAY | ALL
82 - ANYONECANPAY | NONE
83 - ANYONECANPAY | SINGLE

That's it.

Contributor

petertodd commented Nov 12, 2014

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:

01 - ALL
02 - NONE
03 - SINGLE

Upper nibble == 8:

81 - ANYONECANPAY | ALL
82 - ANYONECANPAY | NONE
83 - ANYONECANPAY | SINGLE

That's it.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 12, 2014

Member

@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?

Member

laanwj commented Nov 12, 2014

@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 laanwj added the RPC/REST/ZMQ label Nov 12, 2014

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 12, 2014

Contributor

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

Contributor

petertodd commented Nov 12, 2014

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

@laanwj

View changes

Show outdated Hide outdated src/script/script.cpp
if (vch.size() <= 4)
return strprintf("%d", CScriptNum(vch, false).getint());
else
return HexStr(vch);
return HexStr(vch) + (CheckSignatureEncoding(vch, SCRIPT_VERIFY_STRICTENC) ? ("(" + mapSigHashTypes[vch.back()] + ")") : "");

This comment has been minimized.

@laanwj

laanwj Nov 12, 2014

Member

Any special reason to use SCRIPT_VERIFY_STRICTENC instead of SCRIPT_VERIFY_DERSIG here?

@laanwj

laanwj Nov 12, 2014

Member

Any special reason to use SCRIPT_VERIFY_STRICTENC instead of SCRIPT_VERIFY_DERSIG here?

This comment has been minimized.

@mruddy

mruddy Nov 12, 2014

Contributor

the reason why i went with SCRIPT_VERIFY_STRICTENC was because in CheckSignatureEncoding, SCRIPT_VERIFY_STRICTENC will get us some checking through IsDefinedHashtypeSignature as well in order to make sure that the hash type is one of the defined types. if it's not a defined hash type, then no decode gets appended to the hex encoding of the signature.

@mruddy

mruddy Nov 12, 2014

Contributor

the reason why i went with SCRIPT_VERIFY_STRICTENC was because in CheckSignatureEncoding, SCRIPT_VERIFY_STRICTENC will get us some checking through IsDefinedHashtypeSignature as well in order to make sure that the hash type is one of the defined types. if it's not a defined hash type, then no decode gets appended to the hex encoding of the signature.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 12, 2014

Member

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

Member

laanwj commented Nov 12, 2014

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

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Nov 12, 2014

Contributor

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

Contributor

mruddy commented Nov 12, 2014

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

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Nov 12, 2014

Contributor

Added some release notes.

Contributor

mruddy commented Nov 12, 2014

Added some release notes.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 12, 2014

Contributor

@mruddy Mind squashing those commits?

Contributor

petertodd commented Nov 12, 2014

@mruddy Mind squashing those commits?

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Nov 12, 2014

Contributor

sure thing, done

Contributor

mruddy commented Nov 12, 2014

sure thing, done

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 12, 2014

Member

I don't think we should be adding this to the existing 'asm' output, but perhaps an alternate decoding that goes into lower level detail.

Member

sipa commented Nov 12, 2014

I don't think we should be adding this to the existing 'asm' output, but perhaps an alternate decoding that goes into lower level detail.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Nov 12, 2014

Contributor

ok, thanks. if nobody pops up and says that they want this within a day or so, i'll go ahead and close this. it probably fits better somewhere else.

Contributor

mruddy commented Nov 12, 2014

ok, thanks. if nobody pops up and says that they want this within a day or so, i'll go ahead and close this. it probably fits better somewhere else.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 13, 2014

Member

@sipa What if the 'asm' format grew a way to specify comments?

Member

laanwj commented Nov 13, 2014

@sipa What if the 'asm' format grew a way to specify comments?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 13, 2014

Member

@gmaxwell can you comment here? You created #3166, so maybe you can illuminate how it should work

Member

laanwj commented Nov 13, 2014

@gmaxwell can you comment here? You created #3166, so maybe you can illuminate how it should work

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 13, 2014

Member

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.

Member

gmaxwell commented Nov 13, 2014

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

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Nov 18, 2014

Contributor

Closing because there doesn't seem to be much demand for this fix with the possibility that it breaks things scripted against the "asm" output value.

Contributor

mruddy commented Nov 18, 2014

Closing because there doesn't seem to be much demand for this fix with the possibility that it breaks things scripted against the "asm" output value.

@mruddy mruddy closed this Nov 18, 2014

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 18, 2014

Contributor

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

Contributor

petertodd commented Nov 18, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Nov 19, 2014

Contributor

@petertodd sure, re-opened. I have not gone through and made the "OP_" prefix changes yet. I figure that I'll have to do a bunch of reference checking to see what all I'm impacting, and then update or make some unit tests when I do that. So, more changes to come for that.

Contributor

mruddy commented Nov 19, 2014

@petertodd sure, re-opened. I have not gone through and made the "OP_" prefix changes yet. I figure that I'll have to do a bunch of reference checking to see what all I'm impacting, and then update or make some unit tests when I do that. So, more changes to come for that.

@mruddy mruddy reopened this Nov 19, 2014

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 20, 2014

Contributor

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

Contributor

petertodd commented Nov 20, 2014

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

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Nov 28, 2014

Contributor

@petertodd sorry for my delayed response... i just made #5392 for the "OP_" prefix changes. also, i rebased this request's commit so that it'll merge into the upstream once again.

Contributor

mruddy commented Nov 28, 2014

@petertodd sorry for my delayed response... i just made #5392 for the "OP_" prefix changes. also, i rebased this request's commit so that it'll merge into the upstream once again.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Dec 11, 2014

Contributor

Eh, not important enough to keep open and keep re-basing the commit.

Contributor

mruddy commented Dec 11, 2014

Eh, not important enough to keep open and keep re-basing the commit.

@mruddy mruddy closed this Dec 11, 2014

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 11, 2014

Member

I still think this makes sense, but after 0.10 obviously.
If this is closed unfixed with "don't bother" then so should #3166.

Member

laanwj commented Dec 11, 2014

I still think this makes sense, but after 0.10 obviously.
If this is closed unfixed with "don't bother" then so should #3166.

@laanwj laanwj reopened this Dec 11, 2014

@laanwj

View changes

Show outdated Hide outdated src/script/script.cpp
if (vch.size() <= 4)
return strprintf("%d", CScriptNum(vch, false).getint());
else
return HexStr(vch);
return HexStr(vch) + (CheckSignatureEncoding(vch, SCRIPT_VERIFY_STRICTENC, NULL) ? ("(" + mapSigHashTypes[vch.back()] + ")") : "");

This comment has been minimized.

@laanwj

laanwj Dec 11, 2014

Member

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.

  • please make mapSigHashTypes const
  • use .find(x) instead of [x] to find the value
@laanwj

laanwj Dec 11, 2014

Member

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.

  • please make mapSigHashTypes const
  • use .find(x) instead of [x] to find the value

This comment has been minimized.

@mruddy

mruddy Dec 12, 2014

Contributor

Thanks for the code review. Guarding that lookup with CheckSignatureEncoding I believe prevents that case, but you're right, I did not realize or intend for the [] operator to act that way. So, it'll be safer to make the change that you propose in-case someone changes CheckSignatureEncoding to act differently in the future. I'll get that done today and re-squash the commit.

@mruddy

mruddy Dec 12, 2014

Contributor

Thanks for the code review. Guarding that lookup with CheckSignatureEncoding I believe prevents that case, but you're right, I did not realize or intend for the [] operator to act that way. So, it'll be safer to make the change that you propose in-case someone changes CheckSignatureEncoding to act differently in the future. I'll get that done today and re-squash the commit.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Dec 12, 2014

Member

I really don't like a script show function to make assumptions about the data inside it. It feels like a layer violation.

Member

sipa commented Dec 12, 2014

I really don't like a script show function to make assumptions about the data inside it. It feels like a layer violation.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 12, 2014

Member

@sipa you could argue that, but in practice that's what disassemblers do, given incomplete semantic information they have to make guesses. As long as the information is only shown in what is an informative comment for the user, I don't think it hurts.

Member

laanwj commented Dec 12, 2014

@sipa you could argue that, but in practice that's what disassemblers do, given incomplete semantic information they have to make guesses. As long as the information is only shown in what is an informative comment for the user, I don't think it hurts.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Dec 12, 2014

Contributor

@laanwj ready for review. i added a couple more test cases, and re-squashed the commit after re-basing to the latest upstream.

Contributor

mruddy commented Dec 12, 2014

@laanwj ready for review. i added a couple more test cases, and re-squashed the commit after re-basing to the latest upstream.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Dec 13, 2014

Contributor

While doing more testing, I found that I was able to make some OP_RETURN data look like a signature and cause an erroneous decode.
I added some handling for that and did a little re-factoring at the same time.
I also changed the parentheses to square brackets for consistency with an existing error case that would output square brackets.

Contributor

mruddy commented Dec 13, 2014

While doing more testing, I found that I was able to make some OP_RETURN data look like a signature and cause an erroneous decode.
I added some handling for that and did a little re-factoring at the same time.
I also changed the parentheses to square brackets for consistency with an existing error case that would output square brackets.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Dec 21, 2014

Contributor

Tightened up a couple more cases and added unit tests.

Contributor

mruddy commented Dec 21, 2014

Tightened up a couple more cases and added unit tests.

@Diapolo

View changes

Show outdated Hide outdated src/script/script.cpp
@@ -4,20 +4,9 @@
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include "script.h"

This comment has been minimized.

@Diapolo

Diapolo Dec 21, 2014

Pleaso don't remove that line, it is here to separate the own header (script.h) from other core headers.

@Diapolo

Diapolo Dec 21, 2014

Pleaso don't remove that line, it is here to separate the own header (script.h) from other core headers.

@Diapolo

View changes

Show outdated Hide outdated src/script/script.cpp
std::string CScript::ValueString(const std::vector<unsigned char>& vch) const
{
if (vch.size() <= 4)

This comment has been minimized.

@Diapolo

Diapolo Dec 21, 2014

Just asking, but isn't this strictly a signed/unsigned comparison as .size() returns a size_t?

@Diapolo

Diapolo Dec 21, 2014

Just asking, but isn't this strictly a signed/unsigned comparison as .size() returns a size_t?

@Diapolo

View changes

Show outdated Hide outdated src/script/script.h
#include <assert.h>
#include <boost/assign/list_of.hpp>

This comment has been minimized.

@Diapolo

Diapolo Dec 21, 2014

Nit: Can you place this after a newline below the standard C/C++ headers?

@Diapolo

Diapolo Dec 21, 2014

Nit: Can you place this after a newline below the standard C/C++ headers?

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Dec 21, 2014

Contributor

@Diapolo thanks for the review. I've addressed your three comments in the latest updated commit.

Contributor

mruddy commented Dec 21, 2014

@Diapolo thanks for the review. I've addressed your three comments in the latest updated commit.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 26, 2015

Member

Needs rebase.

Member

laanwj commented Mar 26, 2015

Needs rebase.

@laanwj

View changes

Show outdated Hide outdated src/script/interpreter.cpp
@@ -16,8 +16,6 @@
using namespace std;
typedef vector<unsigned char> valtype;

This comment has been minimized.

@laanwj

laanwj Mar 26, 2015

Member

I don't like exposing valtype externally, the type name is too general. Would prefer to change the function signature to just

bool CheckSignatureEncoding(const vector<unsigned char> &vchSig, unsigned int flags, ScriptError* serror) {
@laanwj

laanwj Mar 26, 2015

Member

I don't like exposing valtype externally, the type name is too general. Would prefer to change the function signature to just

bool CheckSignatureEncoding(const vector<unsigned char> &vchSig, unsigned int flags, ScriptError* serror) {
@jtimon

View changes

Show outdated Hide outdated src/script/script.cpp
@@ -238,11 +240,11 @@ bool CScript::IsPushOnly() const
return true;
}
std::string CScript::ToString() const

This comment has been minimized.

@jtimon

jtimon Jul 4, 2015

Member

why remove std:: ?

@jtimon

jtimon Jul 4, 2015

Member

why remove std:: ?

This comment has been minimized.

@mruddy

mruddy Jul 4, 2015

Contributor

Just for consistency within the file. That method was the only one using the prefix in that file because of the "using namespace std" up on line 12. That's all I was thinking when I did that.

@mruddy

mruddy Jul 4, 2015

Contributor

Just for consistency within the file. That method was the only one using the prefix in that file because of the "using namespace std" up on line 12. That's all I was thinking when I did that.

This comment has been minimized.

@jtimon

jtimon Jul 4, 2015

Member

We're doing the opposite in other places. That doesn't mean we should do it here. If you were touching the lines anyway I wouldn't have said anything, but increasing the diff only for consistency doesn't seem very interesting in my opinion. Anyway, feel free to ignore, just a small nit.

@jtimon

jtimon Jul 4, 2015

Member

We're doing the opposite in other places. That doesn't mean we should do it here. If you were touching the lines anyway I wouldn't have said anything, but increasing the diff only for consistency doesn't seem very interesting in my opinion. Anyway, feel free to ignore, just a small nit.

This comment has been minimized.

@mruddy

mruddy Jul 4, 2015

Contributor

I agree with you, I'm going to put the prefixes back to reduce the diff in the next commit. Thanks for the info and for reviewing these changes.

@mruddy

mruddy Jul 4, 2015

Contributor

I agree with you, I'm going to put the prefixes back to reduce the diff in the next commit. Thanks for the info and for reviewing these changes.

@jtimon

View changes

Show outdated Hide outdated src/script/script.h
#include <stdexcept>
#include <stdint.h>
#include <string.h>
#include <string>
#include <vector>
#include <boost/assign/list_of.hpp>

This comment has been minimized.

@jtimon

jtimon Jul 4, 2015

Member

Is this actually used in the .h ? If not, why not move it to the cpp or wherever it is used?

@jtimon

jtimon Jul 4, 2015

Member

Is this actually used in the .h ? If not, why not move it to the cpp or wherever it is used?

This comment has been minimized.

@mruddy

mruddy Jul 4, 2015

Contributor

You might be right. I'll double check. I might have needed it at one point and then moved things around. If it's only needed in the cpp, I'll move it.

@mruddy

mruddy Jul 4, 2015

Contributor

You might be right. I'll double check. I might have needed it at one point and then moved things around. If it's only needed in the cpp, I'll move it.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 4, 2015

Contributor

@jtimon your two feedback items are incorporated into this latest commit, thanks!

Contributor

mruddy commented Jul 4, 2015

@jtimon your two feedback items are incorporated into this latest commit, thanks!

@jtimon

View changes

Show outdated Hide outdated src/script/script.cpp
@@ -5,18 +5,11 @@
#include "script.h"
#include "script/interpreter.h"

This comment has been minimized.

@jtimon

jtimon Jul 4, 2015

Member

Oh, I hadn't noticed the circular dependence...maybe an additional moveonly commit would solve that?
mhmm, moving CheckSignatureEncoding and the flags from interpreter to script would be an option. Maybe move the flags to consensus/consensus instead?
Another option is not having anything depending on the flags in script/script: CScript methods can be turned into functions taking a CScript as parameter.
Or maybe we can just let the circular dependency be for now.
Mhmm, I would like to know what other people think about this...I still don't even know what option I like more.

@jtimon

jtimon Jul 4, 2015

Member

Oh, I hadn't noticed the circular dependence...maybe an additional moveonly commit would solve that?
mhmm, moving CheckSignatureEncoding and the flags from interpreter to script would be an option. Maybe move the flags to consensus/consensus instead?
Another option is not having anything depending on the flags in script/script: CScript methods can be turned into functions taking a CScript as parameter.
Or maybe we can just let the circular dependency be for now.
Mhmm, I would like to know what other people think about this...I still don't even know what option I like more.

This comment has been minimized.

@laanwj

laanwj Jul 20, 2015

Member

Yes, script should probably not depend on the interpreter.
I think we should move the disassembly logic out of the core/consensus data structure to separate utility functions.
E.g. also IsPayToPublicKeyHash() doesn't belong in CScript itself, it belongs at a higher level.

@laanwj

laanwj Jul 20, 2015

Member

Yes, script should probably not depend on the interpreter.
I think we should move the disassembly logic out of the core/consensus data structure to separate utility functions.
E.g. also IsPayToPublicKeyHash() doesn't belong in CScript itself, it belongs at a higher level.

This comment has been minimized.

@jtimon

jtimon Jul 21, 2015

Member

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.

@jtimon

jtimon Jul 21, 2015

Member

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.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 23, 2015

Contributor

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.

Contributor

jgarzik commented Jul 23, 2015

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.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 23, 2015

Contributor

Yes, it should probably be closed. The more I worked on these changes, the less I liked them. I suppose I'll wait a day and close it unless someone objects.

Contributor

mruddy commented Jul 23, 2015

Yes, it should probably be closed. The more I worked on these changes, the less I liked them. I suppose I'll wait a day and close it unless someone objects.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 24, 2015

Member

I agree the intent is good, and it can be done, just in script/standard rather than script/script.
If closing this PR and reopening another one that does the same in another file is clearer, let's do that.

Member

jtimon commented Jul 24, 2015

I agree the intent is good, and it can be done, just in script/standard rather than script/script.
If closing this PR and reopening another one that does the same in another file is clearer, let's do that.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 24, 2015

Contributor

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.

Contributor

jgarzik commented Jul 24, 2015

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.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 24, 2015

Contributor

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.

Contributor

mruddy commented Jul 24, 2015

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.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 24, 2015

Member

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.

Member

jtimon commented Jul 24, 2015

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.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 25, 2015

Contributor

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.

Contributor

mruddy commented Jul 25, 2015

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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 25, 2015

Member
Member

sipa commented Jul 25, 2015

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 25, 2015

Member

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

Member

jtimon commented Jul 25, 2015

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

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 26, 2015

Contributor

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

Contributor

mruddy commented Jul 26, 2015

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

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 26, 2015

Member

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

Member

jtimon commented Jul 26, 2015

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

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 26, 2015

Member

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.

Member

jtimon commented Jul 26, 2015

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.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 28, 2015

Contributor

@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!

Contributor

mruddy commented Jul 28, 2015

@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!

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 28, 2015

Contributor

... or not. I see the travis windows builds failed because primitives/transaction.cpp is still part of libconsensus and references the ScriptToAsmStr for ToString'ing CTxIn and CTxOut objects. Guess I'll have to move those around too.

Contributor

mruddy commented Jul 28, 2015

... or not. I see the travis windows builds failed because primitives/transaction.cpp is still part of libconsensus and references the ScriptToAsmStr for ToString'ing CTxIn and CTxOut objects. Guess I'll have to move those around too.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 28, 2015

Member

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

Member

jtimon commented Jul 28, 2015

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

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 28, 2015

Member

Another possibility is to leave the new code in libconsensus like you had at the beginning (not my preference), but it still couldn't be in script/script since you need script/interpreter and that would cause a circular dependency. So even leaving the new code in libconsensus, you always need to rename CScript::ToString and move it our of script/script.

Member

jtimon commented Jul 28, 2015

Another possibility is to leave the new code in libconsensus like you had at the beginning (not my preference), but it still couldn't be in script/script since you need script/interpreter and that would cause a circular dependency. So even leaving the new code in libconsensus, you always need to rename CScript::ToString and move it our of script/script.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 28, 2015

Contributor

It turns out that resolving that last dependency was pretty easy.
I simply stopped using that script assembly formatting in the CTxIn and CTxOut ToString methods.
Those ToString's were only called by very low value debug and log print statements and the formatted scripts were being truncated immediately too. So, converting to hex instead in those cases isn't losing much, if anything at all, in my opinion. Doing it this way allowed the removal of the original CScript::ToString from libconsensus.

Contributor

mruddy commented Jul 28, 2015

It turns out that resolving that last dependency was pretty easy.
I simply stopped using that script assembly formatting in the CTxIn and CTxOut ToString methods.
Those ToString's were only called by very low value debug and log print statements and the formatted scripts were being truncated immediately too. So, converting to hex instead in those cases isn't losing much, if anything at all, in my opinion. Doing it this way allowed the removal of the original CScript::ToString from libconsensus.

@jtimon

View changes

Show outdated Hide outdated src/primitives/transaction.cpp
@@ -5,6 +5,7 @@
#include "primitives/transaction.h"
#include "core_io.h"

This comment has been minimized.

@jtimon

jtimon Jul 29, 2015

Member

This is not necessary anymore, right?

@jtimon

jtimon Jul 29, 2015

Member

This is not necessary anymore, right?

@jtimon

View changes

Show outdated Hide outdated src/bitcoin-tx.cpp
@@ -12,6 +12,7 @@
#include "primitives/transaction.h"
#include "script/script.h"
#include "script/sign.h"
#include "script/standard.h"

This comment has been minimized.

@jtimon

jtimon Jul 29, 2015

Member

This is core_io now.

@jtimon

jtimon Jul 29, 2015

Member

This is core_io now.

@jtimon

View changes

Show outdated Hide outdated src/test/script_tests.cpp
@@ -9,6 +9,7 @@
#include "key.h"
#include "keystore.h"
#include "script/script.h"
#include "script/standard.h"

This comment has been minimized.

@jtimon

jtimon Jul 29, 2015

Member

This is core_io now.

@jtimon

jtimon Jul 29, 2015

Member

This is core_io now.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 29, 2015

Member

This looks like a reasonable solution.
Apart from the latest little nits, consensus-safe-review ACK (I also slightly reviewed the new functions, but didn't look much at the tests).

Member

jtimon commented Jul 29, 2015

This looks like a reasonable solution.
Apart from the latest little nits, consensus-safe-review ACK (I also slightly reviewed the new functions, but didn't look much at the tests).

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 29, 2015

Contributor

@jtimon You are correct about those latest nits. I made the updates and squashed+rebased everything back down to the one latest commit. I also updated the release notes description at the same time. Should be all good now. Thanks for the help, it was good working with you on this!

Contributor

mruddy commented Jul 29, 2015

@jtimon You are correct about those latest nits. I made the updates and squashed+rebased everything back down to the one latest commit. I also updated the release notes description at the same time. Should be all good now. Thanks for the help, it was good working with you on this!

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 29, 2015

Member

Thanks to you for your patience when improving this. Everything looks good, re-utACK.

Member

jtimon commented Jul 29, 2015

Thanks to you for your patience when improving this. Everything looks good, re-utACK.

@sipa

View changes

Show outdated Hide outdated src/core_write.cpp
if (vch.size() <= static_cast<vector<unsigned char>::size_type>(4)) {
str += strprintf("%d", CScriptNum(vch, false).getint());
} else {
if (fAttemptSighashDecode) {

This comment has been minimized.

@sipa

sipa Jul 30, 2015

Member

Nit: the !script.IsUnspendable() check can move here, I believe.

@sipa

sipa Jul 30, 2015

Member

Nit: the !script.IsUnspendable() check can move here, I believe.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 30, 2015

Member

Untested ACK, but the tests look convincing.

Member

sipa commented Jul 30, 2015

Untested ACK, but the tests look convincing.

Resolve issue 3166.
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.
@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Jul 31, 2015

Contributor

@sipa nit addressed. I also added a new test case just to cover a specific low probability case related to the nit to make sure it was covered.

Contributor

mruddy commented Jul 31, 2015

@sipa nit addressed. I also added a new test case just to cover a specific low probability case related to the nit to make sure it was covered.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 15, 2015

Contributor

ut ACK - looks ready to merge

Contributor

jgarzik commented Sep 15, 2015

ut ACK - looks ready to merge

@laanwj laanwj merged commit af3208b into bitcoin:master Sep 25, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Sep 25, 2015

Merge pull request #5264
af3208b Resolve issue 3166. 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. (mruddy)

@mruddy mruddy deleted the mruddy:iss3166 branch Sep 29, 2015

zkbot added a commit to zcash/zcash that referenced this pull request Apr 17, 2018

Auto merge of #3180 - str4d:transaction-serialization, r=<try>
Upstream serialization improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5264
- bitcoin/bitcoin#6914
- bitcoin/bitcoin#6215
- bitcoin/bitcoin#8068
  - Only the `COMPACTSIZE` wrapper commit
- bitcoin/bitcoin#8658
- bitcoin/bitcoin#8708
  - Only the serializer variadics commit
- bitcoin/bitcoin#9039
- bitcoin/bitcoin#9125
  - Only the first two commits (the last two block on other upstream PRs)

Part of #2074.
@@ -36,7 +36,7 @@ std::string CTxIn::ToString() const
if (prevout.IsNull())
str += strprintf(", coinbase %s", HexStr(scriptSig));
else
str += strprintf(", scriptSig=%s", scriptSig.ToString().substr(0,24));
str += strprintf(", scriptSig=%s", HexStr(scriptSig).substr(0, 24));

This comment has been minimized.

@arielgabizon

arielgabizon Apr 18, 2018

was there a reason HexStr wasn't used here before?

@arielgabizon

arielgabizon Apr 18, 2018

was there a reason HexStr wasn't used here before?

This comment has been minimized.

@mruddy

mruddy Apr 18, 2018

Contributor

It was just more verbose (someone's personal preference, I guess), see what was removed CScript::ToString: af3208b#diff-f7ca24fb80ddba0f291cb66344ca6fcb

@mruddy

mruddy Apr 18, 2018

Contributor

It was just more verbose (someone's personal preference, I guess), see what was removed CScript::ToString: af3208b#diff-f7ca24fb80ddba0f291cb66344ca6fcb

zkbot added a commit to zcash/zcash that referenced this pull request Apr 18, 2018

Auto merge of #3180 - str4d:transaction-serialization, r=<try>
Upstream serialization improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5264
- bitcoin/bitcoin#6914
- bitcoin/bitcoin#6215
- bitcoin/bitcoin#8068
  - Only the `COMPACTSIZE` wrapper commit
- bitcoin/bitcoin#8658
- bitcoin/bitcoin#8708
  - Only the serializer variadics commit
- bitcoin/bitcoin#9039
- bitcoin/bitcoin#9125
  - Only the first two commits (the last two block on other upstream PRs)

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Apr 19, 2018

Auto merge of #3180 - str4d:transaction-serialization, r=ebfull
Upstream serialization improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5264
- bitcoin/bitcoin#6914
- bitcoin/bitcoin#6215
- bitcoin/bitcoin#8068
  - Only the `COMPACTSIZE` wrapper commit
- bitcoin/bitcoin#8658
- bitcoin/bitcoin#8708
  - Only the serializer variadics commit
- bitcoin/bitcoin#9039
- bitcoin/bitcoin#9125
  - Only the first two commits (the last two block on other upstream PRs)

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