search

sipa / bips

Bitcoin Improvement Proposals
bitcoin.org
145 stars 43 forks source link

Bad pushdata is not the only reason (in theory) GetOp can fail #133

Closed dgpv closed 4 years ago

dgpv commented 4 years ago

It is true that currently, the only reason ScriptPubKey.GetOp(pc, opcode) might fail (when pc is in bounds) is because the pushdata encoding length extends beyond the script.

But this detail is abstracted away in GetScriptOp(), and thus can change independently of the code in ExecuteWitnessProgram().

If an opcode is introduced later that can fail the decoding for other causes, the documentation/implementation introducing this new opcode would need to either to specify explicitly that failing to decode this new opcode has the same effect as bad pushdata in regards to tapscript validation, or change the logic of ExecuteWitnessProgram() to distinguish between failing pushdata or this new opcode.

I think it is better to specify that any failed opcode decoding (before OP_SUCCESSx) is encountered should mean 'fail', and the specific reason (bad pushdata, indeed the only possible reason currently) to be given as example.

sipa commented 4 years ago

I'm not sure about this. It is, as of this specification, the only reason for failure. If there are future proposals that modify the script execution semantics, they will need to specify explicitly what they apply to (legacy script, p2wsh, tapscript, ...) anyway. So I think it's better to be explicit.

dgpv commented 4 years ago

OK