Page MenuHomePhabricator

recognize bare multisigs as standard only when using minimal pushes
ClosedPublic

Authored by markblundeberg on Aug 13 2019, 16:58.

Details

Summary

Since we're adopting SCRIPT_VERIFY_MINIMALDATA as a hard consensus rule, it doesn't make sense to allow "spendable" but actually unspendable outputs as standard.

While we're adding tests for this, also test some other nonminimal forms of standard outputs.

Depends on D3872

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D3865
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7144
Build 12333: Bitcoin ABC Buildbot (legacy)
Build 12332: arc lint + arc unit

Event Timeline

markblundeberg created this object with visibility "Custom Policy".
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg edited the summary of this revision. (Show Details)

arc update to make it follow standard procedure

markblundeberg edited the summary of this revision. (Show Details)
markblundeberg removed a reviewer: Restricted Project.

updated to use CheckMinimalPush

jasonbcox requested changes to this revision.Aug 14 2019, 22:10
jasonbcox added inline comments.
src/test/script_standard_tests.cpp
113

nit: each set of tests should be commented to make it clear which script type is intended to be tested.

115

Loop through all OP_PUSHDATAx that could be used to make non-minimal forms to improve test coverage.

This revision now requires changes to proceed.Aug 14 2019, 22:10

rebase; increase test coverage and add comments.

also added sanity check that opcode <= OP_PUSHDATA4 before calling CheckMinimalPush to avoid assert death.
(should be unnecessary since CPubKey::ValidSize excludes 0-size keys.)

This revision is now accepted and ready to land.Aug 15 2019, 16:21

Apart from the inline comment (that really can be a non issue in practice) the code looks good to me.

src/script/standard.cpp
85 ↗(On Diff #10803)

Just in case you can an opcode < 0 test as well. It appears that the code makes a cast from an uint32_t to the opcodetype enum, which my understanding is that it is undefined behavior if the value is not in the enum list. If for some reason you manage to fall into the assert from CheckMinimalPush() you get a efficient DoS vector.

add nonnegative check per Fabien's suggestion

markblundeberg added inline comments.
src/script/standard.cpp
85 ↗(On Diff #10803)

Makes sense! I didn't realize that enum behaviour could be so weird, yeah...

deadalnix changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 30 2019, 16:37