Page MenuHomePhabricator

fix scriptSig analysis in sign.cpp
ClosedPublic

Authored by markblundeberg on Jan 12 2020, 05:45.

Details

Summary

This is used for analyzing partially unsigned transactions, and exhibits
weird behaviour where it allows any opcodes (including CPU-intensive ones)
to be run, and then it analyses the aftermath.

Also, this is a misuse of MANDATORY_SCRIPT_VERIFY_FLAGS as it here we
shouldn't care about how invalid transactions are sometimes banned/not
banned in mempool acceptance. In Core they have STRICTENC here which
is pretty weird, since it doesn't really have any effect (invalidly
encoded signatures still get through since they are just pushed).

This changes the evaluation to only proceed if the script is properly
push-only, and uses neutral flags to build a stack out of the push
opcodes.

(In principle we could enforce MINIMALDATA, but, it doesn't hurt to
allow nonstandard pushes since all the pushes will get canonicalized by
PushAll after signing.)

Test Plan

ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fix_mandatory_sign
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8871
Build 15717: Default Diff Build & Tests
Build 15716: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Jan 12 2020, 15:13
markblundeberg retitled this revision from fic scriptSig analysis in sign.cpp to fix scriptSig analysis in sign.cpp.Jan 13 2020, 00:08