This repository has been archived by the owner on Apr 3, 2019. It is now read-only.
Remove Redeem Script Classification Requirement from Script.isScriptHashIn() #36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
January 5, 2016 11:28
- The bitcore-lib Script is primarily concerned with serializing, deserializing, building common bitcoin scripts, and extracting commonly needed information - Script.isScriptHashIn() requires the Script object to conclude whether a P2SH redeem script is valid, which in many cases, it cannot (nor should it) - The Script object should remain unopinionated, especially in light of a P2SH redeemscript, where the benefits of classification are dwarfed by the costs of it being incorrect - In the case of BIP 65's OP_CHECKLOCKTIMEVERIFY, this error causes consumers of bitcore-lib to incorrectly conclude that CLTV transactions are not P2SH spends - Over-zealous classification in this case is fragile and breaks forward compatibility - it should be removed until a more stable P2SH check is implemented
- Data transactions consist of OP_RETURN followed by a push of bytes to the stack - P2SH redeem scripts (generally) consist of signatures and relevant information followed by a push of bytes to the stack - To differentiate the two, we check that the first OP code is not OP_RETURN in Script.isScriptHashIn()
APTy
changed the title
added 2 commits
January 11, 2016 17:38
- Since we've relaxed our definition of what a valid redeemscript comprises of, we need to update the test to check for that. In this case, it's a script that simply pushes bytes to the stack.
APTy
force-pushed
the
fix-p2sh-classification
branch
from
730bfc2 to
2a54b18
Compare
|
Interesting, |
|
Right - so users of This patch has proven effective on a bitcore node for the past few weeks. I think it would be helpful to others, but let me know if you think a fix is more suitable on the Thanks! |
|
This may be something that we need to fix on the bitcore-node side. Without looking at the classification, we would need to have a reference to the output to know if it's standard pay-to-script-hash. Example: > var tx = bitcore.Transaction('0100000001ce5d8ced3c8c364b4f74dfd646b641b63887772f3a6b6dcf0ba8dc9b7bd12f4e010000008b483045022100d39707717cf2a172c98c932001b220c4ba9b2925e61d34574a8a9c7d8db3a03f022066bf18115b3e011d17b8879b088b90cd416fae825719a23e759dca151f016dfa01410470a47342c1d1ab469fbb3ec339b37b9da9ffea707aab41e127dcbac47fba2a3da5f1097d46a7e47788d9215a32e3d2a540cef0475e6a1901b9b037b3ffaf0bdaffffffff0292c67e09000000001976a914715832ea41ae50d5fc0c3a8e3f4dad12a2a20b7788aca6ca064b020000001976a914e780f5642809d1a0b24d6db71fcd17ff2e32343f88ac00000000');
> tx.toObject()
{ hash: '0cf9a0218b79625003b90cdacd1b6b215b1d4480d2e4ccd5ddc3cd72f20ccba5',
version: 1,
inputs:
[ { prevTxId: '4e2fd17b9bdca80bcf6d6b3a2f778738b641b646d6df744f4b368c3ced8c5dce',
outputIndex: 1,
sequenceNumber: 4294967295,
script: '483045022100d39707717cf2a172c98c932001b220c4ba9b2925e61d34574a8a9c7d8db3a03f022066bf18115b3e011d17b8879b088b90cd416fae825719a23e759dca151f016dfa01410470a47342c1d1ab469fbb3ec339b37b9da9ffea707aab41e127dcbac47fba2a3da5f1097d46a7e47788d9215a32e3d2a540cef0475e6a1901b9b037b3ffaf0bda',
scriptString: '72 0x3045022100d39707717cf2a172c98c932001b220c4ba9b2925e61d34574a8a9c7d8db3a03f022066bf18115b3e011d17b8879b088b90cd416fae825719a23e759dca151f016dfa01 65 0x0470a47342c1d1ab469fbb3ec339b37b9da9ffea707aab41e127dcbac47fba2a3da5f1097d46a7e47788d9215a32e3d2a540cef0475e6a1901b9b037b3ffaf0bda' } ],
outputs:
[ { satoshis: 159303314,
script: '76a914715832ea41ae50d5fc0c3a8e3f4dad12a2a20b7788ac' },
{ satoshis: 9848670886,
script: '76a914e780f5642809d1a0b24d6db71fcd17ff2e32343f88ac' } ],
nLockTime: 0 }
> var s = bitcore.Script('483045022100d39707717cf2a172c98c932001b220c4ba9b2925e61d34574a8a9c7d8db3a03f022066bf18115b3e011d17b8879b088b90cd416fae825719a23e759dca151f016dfa01410470a47342c1d1ab469fbb3ec339b37b9da9ffea707aab41e127dcbac47fba2a3da5f1097d46a7e47788d9215a32e3d2a540cef0475e6a1901b9b037b3ffaf0bda');
> s.isScriptHashIn()
true
> s.isPublicKeyHashIn()
true |
|
Closing this in favor of the |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change fixes an issue where the
Scriptobject would return anunknownscript type when trying to classify a redeem script inScript.isScriptHashIn(). Every type of redeem script must be defined inscript.jsin order to be recognized. At current, onlyMultisigandDataOutredeem script types are supported. In other words, new script types such as those used in CLTV payment channel transactions are not recognized.This can cause issues where Script implementations don't recognize a script, and, in the case of
bitcore-node, fail to index the transaction, causing issues like bitpay/insight-api#444 whereinsight-apiis unable to find a custom p2sh in its leveldb indexes. This is perhaps an implementation oversight on the part ofbitcore-nodebut the fact remains that there are many possible redeem scripts, and we shouldn't have to itemize every type for the Script object to recognize them.Due to the fact that there is little (if any) cost to incorrectly classifying a redeem script as valid after all other script types have been exhausted, and rather than define every possible redeem script type, we should remove this requirement and instead only verify basic attributes like length, ability to be serialized, etc.