CHECKSIGFROMSTACK has VERIFY semantics

[maaku]

If the signature fails validation in CHECKSIGFROMSTACK, serror is set and execution terminates even if this isn't from within the VERIFY version of the opcode. Therefore both opcodes have VERIFY semantics and differ only on whether the TRUE/FALSE result is left on the stack prior to termination. I do not believe this is the intended behavior.

[instagibbs]

Agreed, the logic should simply match checksigverify unless I'm missing some detail.

[roconnor-blockstream]

CHECKSIGFROMSTACK should be eliminated entirely.
There are no circumstances whereby we want to do all the work of performing and ECDSA signature check and not have it succeed.

Scripts that need to do optional verification should instead wrap the CHECKSIGFROMSTACKVERIFY with an IF clause to bypass it based on witness information, which saves, literally everyone, the time that would be wasted on a failed signature check.

The same reasoning applies to CHECKSIG.
The only reason CHECKSIG is used in standard scripts is to satisfy the rule that to be valid a program must leave a non-zero item on the stack. We should still eliminate CHECKSIG and either

1. use CHECKSIGVERIFY 1 at the end of standard scripts, or
2. add verify semantics to CHECKSIG, making it equivalent to CHECKSIGVERIFY 1, or
3. replace the requirement that the program leave a non-zero item on the stack with a requirement that the stack must be completely empty.

[roconnor-blockstream]

See also the SCRIPT_VERIFY_NULLFAIL policy, which should be made mandatory, along with all the other STANDARD_SCRIPT_VERIFY_FLAGS in policy.h.

[maaku]

Copied from #203 :

Russell's comments is jogging a distant memory that the original behavior was actually a purposeful design. One of the Alpha features was going to be the removal of expensive non-VERIFY checks, particularly the CHECKSIG codes so we can do batch validation, however this ended up being too invasive as it breaks standard P2PKH scripts, among other things. So only the new opcodes, CHECKSIGFROMSTACK for example, got this behavior. And it seems to be fully undocumented.

So either this behavior needs to be better documented, the actual engineering work done to make the other CHECKSIG opcodes have the same semantics, or this PR needs to be accepted and the behavior normalized.

IMHO the best approach is to normalize semantics with how CHECKSIG works by accepting the changes in this pull request, and then doing batch validation when non-verify CHECKSIG* opcodes are the last opcodes to be executed. But given the history I'm not sure this is something any one of us can decide unilaterally.

[apoelstra]

Closing since this is fixed in Tapscript, see #1020