Page MenuHomePhabricator
Differential D3865

recognize bare multisigs as standard only when using minimal pushes
ClosedPublic
Actions

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

Details

Reviewers
deadalnix
jasonbcox
Fabien
Group Reviewers
Restricted Project
Commits
rSTAGINGf820194d70de: recognize bare multisigs as standard only when using minimal pushes
rABCf820194d70de: recognize bare multisigs as standard only when using minimal pushes
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 7167
Build 12379: Bitcoin ABC Buildbot (legacy)
Build 12378: arc lint + arc unit

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)
markblundeberg updated this revision to Diff 10760.Aug 13 2019, 17:01

arc update to make it follow standard procedure

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 13 2019, 17:01
markblundeberg updated this revision to Diff 10783.Aug 14 2019, 14:49
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg removed a reviewer: Restricted Project.
markblundeberg added a parent revision: D3872: make CheckMinimalPush available to codebase.

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
markblundeberg updated this revision to Diff 10803.Aug 14 2019, 23:00

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: D3880: Add wallet acceptance / mempool acceptance tests for non-standard variants.Aug 14 2019, 23:11
jasonbcox mentioned this in D3880: Add wallet acceptance / mempool acceptance tests for non-standard variants.Aug 14 2019, 23:34
markblundeberg changed the visibility from "Custom Policy" to "Custom Policy".Aug 15 2019, 04:26
jasonbcox changed the visibility from "Custom Policy" to "Custom Policy".Aug 15 2019, 16:02
teamcity added a comment.Aug 15 2019, 16:02

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 15 2019, 16:12

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7156: Diff 10803.Aug 15 2019, 16:12
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.

markblundeberg updated this revision to Diff 10819.Aug 15 2019, 20:54

add nonnegative check per Fabien's suggestion

teamcity added a comment.Aug 15 2019, 20:54

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 15 2019, 20:54

Build Bitcoin-ABC / Diffs / Diff Testing started.

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

teamcity added a comment.Aug 15 2019, 21:00

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7167: Diff 10819.Aug 15 2019, 21:00
jasonbcox accepted this revision.Aug 15 2019, 21:25
Fabien accepted this revision.Aug 15 2019, 21:27
Closed by commit rABCf820194d70de: recognize bare multisigs as standard only when using minimal pushes (authored by Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com>). · Explain WhyAug 15 2019, 22:01
This revision was automatically updated to reflect the committed changes.
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> added a commit: rABCf820194d70de: recognize bare multisigs as standard only when using minimal pushes.
deadalnix changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 30 2019, 16:37
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> added a commit: rSTAGINGf820194d70de: recognize bare multisigs as standard only when using minimal pushes.Sep 19 2019, 01:51

Revision Contents
Changeset List

PathSize
src/
script/
4 lines
test/
106 lines

Diff 10819

View Options

src/script/standard.cpp

Loading...
View Options

src/test/script_standard_tests.cpp

Loading...