show scriptSig signature hash types in transaction decodes. fixes #3166 #5264
Conversation
|
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: Upper nibble == 8: 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)
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. |
src/script/script.cpp
Outdated
There was a problem hiding this comment.
Any special reason to use SCRIPT_VERIFY_STRICTENC instead of SCRIPT_VERIFY_DERSIG here?
There was a problem hiding this comment.
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.
|
@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 |
|
@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. |
|
Added some release notes. |
|
@mruddy Mind squashing those commits? |
|
sure thing, done |
|
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. |
|
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. |
|
@sipa What if the 'asm' format grew a way to specify comments? |
|
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. |
|
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 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. |
|
@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 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
force-pushed
the
iss3166
branch
3 times, most recently
from
bfbe34b to
628bbfc
Compare
|
@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. |
|
Eh, not important enough to keep open and keep re-basing the commit. |
|
I still think this makes sense, but after 0.10 obviously. |
|
@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! |
|
... 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. |
|
You are welcomed. I'm fine with putting new things in core_io as @sipa suggested, just not in libconsensus. 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). |
|
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. |
|
It turns out that resolving that last dependency was pretty easy. |
src/primitives/transaction.cpp
Outdated
There was a problem hiding this comment.
This is not necessary anymore, right?
|
This looks like a reasonable solution. |
|
@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! |
|
Thanks to you for your patience when improving this. Everything looks good, re-utACK. |
src/core_write.cpp
Outdated
There was a problem hiding this comment.
Nit: the !script.IsUnspendable() check can move here, I believe.
|
Untested ACK, but the tests look convincing. |
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.
|
@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. |
|
ut ACK - looks ready to merge |
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)
| 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)); |
There was a problem hiding this comment.
was there a reason HexStr wasn't used here before?
There was a problem hiding this comment.
It was just more verbose (someone's personal preference, I guess), see what was removed CScript::ToString: af3208b#diff-f7ca24fb80ddba0f291cb66344ca6fcb
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.
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.
249cc9d Avoid -Wshadow errors (random-zebra) 8e1ec9e Use fixed preallocation instead of costly GetSerializeSize (random-zebra) 9b801d0 Add optimized CSizeComputer serializers (random-zebra) 0035a54 Make CSerAction's ForRead() constexpr (random-zebra) 9730a3f Get rid of nType and nVersion (random-zebra) 25ce2bb Make GetSerializeSize a wrapper on top of CSizeComputer (random-zebra) 1b479db Make nType and nVersion private and sometimes const (random-zebra) 35f1755 Make streams' read and write return void (random-zebra) a395914 Remove unused ReadVersion and WriteVersion (random-zebra) 52e614c [WIP] Remove unused statement in serialization (random-zebra) 82a2021 Add COMPACTSIZE wrapper similar to VARINT for serialization (random-zebra) 13ad779 add bip32 pubkey serialization (random-zebra) 9e9b7b5 [QA] Update json files with sig hash type in ASM for bitcoin-util-test (random-zebra) 3383983 Resolve issue bitcoin#3166 (random-zebra) Pull request description: -Based on top of - [x] #1629 Backports the following serialization improvements from upstream and adds the required changes for the 2nd layer network and the legacy zerocoin code. - bitcoin#5264 > show scriptSig signature hash types. fixes bitcoin#3166 > > 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. - bitcoin#6215 > CExtPubKey should be serializable like CPubKey. > This would allow storing extended private and public key to support BIP32/HD wallets. - bitcoin#8068 (only commit 5249dac) This adds COMPACTSIZE wrapper similar to VARINT for serialization - bitcoin#8658 > As the line > ``` > nVersion = this->nVersion; > ``` > seems to have no meaning in READ and also in WRITE serialization op, let's remove it and see what our tests/travis will tell us. See bitcoin#8468 for previous discussion. - bitcoin#9039 > The commits in this pull request implement a sequence of changes: > > - Simplifications: > - **Remove unused ReadVersion and WriteVersion** CDataStream and CAutoFile had a ReadVersion and WriteVersion method that was never used. Remove them. > - **Make nType and nVersion private and sometimes const** Make the various stream implementations' nType and nVersion private and const (except in CDataStream where we really need a setter). > - **Make streams' read and write return void** The stream implementations have two layers (the upper one with operator<< and operator>>, and a lower one with read and write). The lower layer's return values are never used (nor should they, as they should only be used from the higher layer), so make them void. > - **Make GetSerializeSize a wrapper on top of CSizeComputer** Given that in default GetSerializeSize implementations we're already using CSizeComputer(), get rid of the specialized GetSerializeSize methods everywhere, and just use CSizeComputer. This removes a lot of code which isn't actually used anywhere. In a few places, this removes an actually more efficient size computing algorithm, which we'll bring back in the "Add optimized CSizeComputer serializers" commit later. > - **Get rid of nType and nVersion** The big change: remove the nType and nVersion as parameters to all serialization methods and functions. There is only one place where it's read and has an impact (in CAddress), and even there it does not impact any of the member objects' serializations. Instead, the few places that need nType or nVersion read it directly from the stream, through GetType and GetVersion calls which are added to all streams. > - **Avoid -Wshadow errors** As suggested by @paveljanik, remove the few remaining cases of variable shadowing in the serialization code. > - Optimizations: > - **Make CSerAction's ForRead() constexpr** The CSerAction's ForRead() method does not depend on any runtime data, so guarantee that requests to it can be optimized out by making it constexpr (suggested by @theuni in bitcoin#8580). > - **Add optimized CSizeComputer serializers** To get the advantages of faster GetSerializeSize implementations back, reintroduce them in the few places where they actually make a difference, in the form of a specialized Serialize implementation. This actually gets us in a better state than before, as these even get used when they're nested inside the serialization of another object. > - **Use fixed preallocation instead of costly GetSerializeSize** dbwrapper uses GetSerializeSize to compute the size of the buffer to preallocate. For some cases (specifically: CCoins) this requires a costly compression call. Avoid this by just using fixed size preallocations instead. > > This will make it easier to address @TheBlueMatt's comments in bitcoin#8580, resulting is a simpler and more efficient way to simultaneously deserialize+construct objects with const members from streams. ACKs for top commit: furszy: Long and nice PR 👌 , code review ACK 249cc9d . Fuzzbawls: ACK 249cc9d furszy: tested ACK 249cc9d and merging. Tree-SHA512: 56b07634b1e18871e7c9a99d412282c83b85f77f1672ec56330a1131fc7c234cd1ba3a053bdd210cc29f1e636ee374477ff614fa9a930329a7f8f912c5006232
9 participants
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.