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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Aug 13 2019, 16:58
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

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 13 2019, 17:01
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg removed a reviewer: Restricted Project.

updated to use CheckMinimalPush

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 14 2019, 14:49
jasonbcox requested changes to this revision.Aug 14 2019, 22:10
jasonbcox added inline comments.
src/test/script_standard_tests.cpp
113 ↗(On Diff #10783)

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

115 ↗(On Diff #10783)

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.)

markblundeberg added a child revision: Restricted Differential Revision.Aug 14 2019, 23:11
jasonbcox changed the visibility from "Custom Policy" to "Custom Policy".Aug 15 2019, 16:02
jasonbcox accepted this revision.Aug 15 2019, 16:21
This revision is now accepted and ready to land.Aug 15 2019, 16:21
Fabien added a comment.Aug 15 2019, 18:37

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 marked an inline comment as done.Aug 15 2019, 20:54
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...

jasonbcox accepted this revision.Aug 15 2019, 21:25
Fabien accepted this revision.Aug 15 2019, 21:27
deadalnix changed the visibility from "Custom Policy" to "Public (No Login Required)".Fri, Aug 30, 16:37