The current code contains a rather complex script template matching engine, which is only used for 3 particular script types (P2PK, P2PKH, multisig). The first two of these are trivial to match for otherwise, and a specialized matcher for multisig is both more compact and more efficient than a generic one.
The goal is being more flexible, so that for example larger standard multisigs inside SegWit outputs are easier to implement.
As a side-effect, it also gets rid of the pseudo opcodes hack.
185 | - // template matching params 186 | - OP_SMALLINTEGER = 0xfa, 187 | - OP_PUBKEYS = 0xfb, 188 | - OP_PUBKEYHASH = 0xfd, 189 | - OP_PUBKEY = 0xfe, 190 | -
Nice to get rid of these fake opcodes here.
73 | + opcodetype opcode; 74 | + valtype data; 75 | + CScript::const_iterator it = script.begin(); 76 | + if (script.size() < 1 || script.back() != OP_CHECKMULTISIG) return false; 77 | + 78 | + if (!script.GetOp(it, opcode, data) || (opcode != OP_0 && (opcode < OP_1 || opcode > OP_16))) return false;
Souldn't this return false if it's op_0 too? or are 0 required sigs multisigs valid?
key count can be 0, I think. The only checks in the interpreter I see are for negative numbers.
I'm trying to stick to the original logic as close as possible; optimizations / other tweaks can happen later.
78 | + if (!script.GetOp(it, opcode, data) || (opcode != OP_0 && (opcode < OP_1 || opcode > OP_16))) return false; 79 | + required = CScript::DecodeOP_N(opcode); 80 | + while (script.GetOp(it, opcode, data) && CPubKey::ValidSize(data)) { 81 | + pubkeys.emplace_back(std::move(data)); 82 | } 83 | + if (opcode != OP_0 && (opcode < OP_1 || opcode > OP_16)) return false;
Same here, shouldn't OP_0 be invalid here too?
If 0 is allowed, why not just opcode < OP_0 || opcode > OP_16?
OP_0 is 0, OP_1 is 81. They aren't continuous.
An isOP_N(opcode) helper function would make this (and the expression above) slightly better readable.
I added an IsSmallInteger function for this (which would be distinct from IsOP_N by not accepting OP_1NEGATE).
utACK modulo nits and modulo this kind of unrelated thing I don't understand. Perhaps a stupid question but, in https://github.com/bitcoin/bitcoin/blob/master/src/pubkey.h#L59 why are there 3 ways to indicate non-compressed and 2 ways to indicate compressed? shouldn't one for each be enough? also, should using an enum or macros or constants be more clear?
44 | - { 45 | - // Standard tx, sender provides pubkey, receiver adds signature 46 | - mTemplates.insert(std::make_pair(TX_PUBKEY, CScript() << OP_PUBKEY << OP_CHECKSIG)); 47 | + if (script.size() == 35 && script[0] == 33 && script[34] == OP_CHECKSIG) { 48 | + pubkey = valtype(script.begin() + 1, script.begin() + 34); 49 | + return CPubKey::ValidSize(pubkey);
Redundant check here? We know the size.
CpubKey::ValidSize checks the data-dependent size (depending on what the first byte is).
48 | + pubkey = valtype(script.begin() + 1, script.begin() + 34); 49 | + return CPubKey::ValidSize(pubkey); 50 | + } 51 | + if (script.size() == 67 && script[0] == 65 && script[66] == OP_CHECKSIG) { 52 | + pubkey = valtype(script.begin() + 1, script.begin() + 66); 53 | + return CPubKey::ValidSize(pubkey);
Redundant check here? We know the size.
We could just use proper constants in the lines above for more belt and suspenders.
light tACK
75 | + CScript::const_iterator it = script.begin(); 76 | + if (script.size() < 1 || script.back() != OP_CHECKMULTISIG) return false; 77 | + 78 | + if (!script.GetOp(it, opcode, data) || (opcode != OP_0 && (opcode < OP_1 || opcode > OP_16))) return false; 79 | + required = CScript::DecodeOP_N(opcode); 80 | + while (script.GetOp(it, opcode, data) && CPubKey::ValidSize(data)) {
Shouldn't it check the opcodes here?
Oh I guess not, all the push opcodes valid here will return non-empty data, and CPubKey::ValidSize rejects empty data.
Indeed (that's also what the old code does).
46 | - mTemplates.insert(std::make_pair(TX_PUBKEY, CScript() << OP_PUBKEY << OP_CHECKSIG)); 47 | + if (script.size() == 35 && script[0] == 33 && script[34] == OP_CHECKSIG) { 48 | + pubkey = valtype(script.begin() + 1, script.begin() + 34); 49 | + return CPubKey::ValidSize(pubkey); 50 | + } 51 | + if (script.size() == 67 && script[0] == 65 && script[66] == OP_CHECKSIG) {
CPubKey::PUBLIC_KEY_SIZE == 65, would add clarity
Done.
42 | - static std::multimap<txnouttype, CScript> mTemplates; 43 | - if (mTemplates.empty()) 44 | - { 45 | - // Standard tx, sender provides pubkey, receiver adds signature 46 | - mTemplates.insert(std::make_pair(TX_PUBKEY, CScript() << OP_PUBKEY << OP_CHECKSIG)); 47 | + if (script.size() == 35 && script[0] == 33 && script[34] == OP_CHECKSIG) {
CPubKey::COMPRESSED_PUBLIC_KEY_SIZE == 33, would add clarity
Done.
53 | + return CPubKey::ValidSize(pubkey); 54 | + } 55 | + return false; 56 | +} 57 | + 58 | +static bool MatchPayToPubkeyHash(const CScript& script, valtype& pubkeyhash)
See also IsToKeyID in compressor.cpp, which looks equivalent.
Could split this up as CScript::IsPayToPubkeyHash, and do the unpacking in Solver, ala IsPayToScriptHash. Would make the case handling more consistent in Solver.
I'd rather not merging with the versions in compressor - they may end up living separate lives as the standard templates change.
34 | @@ -35,22 +35,55 @@ const char* GetTxnOutputType(txnouttype t) 35 | return nullptr; 36 | } 37 | 38 | -bool Solver(const CScript& scriptPubKey, txnouttype& typeRet, std::vector<std::vector<unsigned char> >& vSolutionsRet) 39 | +static bool MatchPayToPubkey(const CScript& script, valtype& pubkey)
See also IsToPubKey in compressor, which looks equivalent.
Concept ACK! Much simpler way to handle these 3 cases.
Nice! utACK 2fc4fdd0945a3800af8213676182ebb2c63689d8
ACK.
86 | + while (script.GetOp(it, opcode, data) && CPubKey::ValidSize(data)) { 87 | + pubkeys.emplace_back(std::move(data)); 88 | } 89 | + if (!IsSmallInteger(opcode)) return false; 90 | + unsigned int keys = CScript::DecodeOP_N(opcode); 91 | + if (pubkeys.size() != keys || required == 0 || keys < required) return false;
nit: this required == 0 check could be an earlier return
Fixed.
221 | + std::vector<std::vector<unsigned char>> keys; 222 | + if (MatchMultisig(scriptPubKey, required, keys)) { 223 | + typeRet = TX_MULTISIG; 224 | + vSolutionsRet.push_back({static_cast<unsigned char>(required)}); 225 | + vSolutionsRet.insert(vSolutionsRet.end(), keys.begin(), keys.end()); 226 | + vSolutionsRet.push_back({static_cast<unsigned char>(keys.size())});
Maybe worth noting these casts are safe due to small integer limits?
Done.
utACK fa1f642
re-utACK 89736a9 just addresses comments above
88 | + pubkeys.emplace_back(std::move(data)); 89 | } 90 | + if (!IsSmallInteger(opcode)) return false; 91 | + unsigned int keys = CScript::DecodeOP_N(opcode); 92 | + if (pubkeys.size() != keys || keys < required) return false; 93 | + if (it + 1 != script.end()) return false;
nit
return it + 1 == script.end();
Done.
utACK 89736a9.
42 | - static std::multimap<txnouttype, CScript> mTemplates; 43 | - if (mTemplates.empty()) 44 | - { 45 | - // Standard tx, sender provides pubkey, receiver adds signature 46 | - mTemplates.insert(std::make_pair(TX_PUBKEY, CScript() << OP_PUBKEY << OP_CHECKSIG)); 47 | + if (script.size() == CPubKey::PUBLIC_KEY_SIZE + 2 && script[0] == CPubKey::PUBLIC_KEY_SIZE && script[CPubKey::PUBLIC_KEY_SIZE + 1] == OP_CHECKSIG) {
what about script.back() == OP_CHECKSIG?
Done.
The current code contains a rather complex script template matching engine,
which is only used for 3 particular script types (P2PK, P2PKH, multisig).
The first two of these are trivial to match for otherwise, and a specialized
matcher for multisig is both more compact and more efficient than a generic
one.
The goal is being more flexible, so that for example larger standard multisigs
inside SegWit outputs are more easy to implement.
As a side-effect, it also gets rid of the pseudo opcodes hack.
67 | +} 68 | + 69 | +/** Test for "small integer" script opcodes - OP_0 through OP_16. */ 70 | +static constexpr bool IsSmallInteger(opcodetype opcode) 71 | +{ 72 | + return opcode == OP_0 || (opcode >= OP_1 && opcode <= OP_16);
if you remove opcode == OP_0 here, you can remove if (required == 0) return false; below
Fixed.
utACK c814e2e
utACK c814e2e7e81fd01fcb07f4a28435741bdc463801
This change has the side effect that invalid pubkeys (with correct size of 33 or 65) in bare multisig are now non-standard (see #14104)