Pull minimal check out of CScriptNum constr. into IsMinimalArray
ClosedPublic
Actions

Authored by schancel on Mar 18 2018, 08:59.

Details

Reviewers

movrcx
jasonbcox
matiu
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING2b3932f10d03: Pull minimal check out of CScriptNum constr. into IsMinimalArray
rABC2b3932f10d03: Pull minimal check out of CScriptNum constr. into IsMinimalArray

Summary

Move the check for minimal encoded arrays out of CScriptNum's
constructor and into a free function. This enables it to be used
elsewhere, and avoid hitting exceptions.

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

minimalarray

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 2158
Build 2460: Bitcoin ABC Buildbot (legacy)
Build 2459: arc lint + arc unit

Event Timeline

schancel created this revision.Mar 18 2018, 08:59

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 18 2018, 08:59

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

schancel mentioned this in D1103: Add support for NUM2BIN, and BIN2NUM opcodes.Mar 18 2018, 09:03

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2158: Diff 3282.Mar 18 2018, 09:20

shadders added a subscriber: shadders.Mar 18 2018, 11:48

shadders added inline comments.

src/script/script.h
190	nit: Most constants in this file are declared at the top and use uppercase. Should follow the style as this constant can also be referenced in Num2Bin as a pre-check to ensure the input value is <= 4 bytes. Suggest placing it under: static const int MAX_SCRIPT_SIZE = 10000; static const int DEFAUL_MAX_NUM_BYTES = 4;

jasonbcox requested changes to this revision.Mar 18 2018, 15:31

jasonbcox added inline comments.

src/script/script.h
190	+1

This revision now requires changes to proceed.Mar 18 2018, 15:31

Fix nit

jasonbcox accepted this revision.Mar 18 2018, 18:32

This revision is now accepted and ready to land.Mar 18 2018, 18:32

Closed by commit rABC2b3932f10d03: Pull minimal check out of CScriptNum constr. into IsMinimalArray (authored by schancel). · Explain WhyMar 18 2018, 18:40

This revision was automatically updated to reflect the committed changes.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2166: Diff 3290.Mar 18 2018, 19:06

deadalnix added inline comments.Mar 19 2018, 11:45

src/script/script.h
35 ↗	(On Diff #3291)	This belong is CScriptNum like it was before. The name `DEFAULT_MAX_NUM_BYTES` has no meaning without context, which is why you added a comment to begin with.
194 ↗	(On Diff #3291)	Same thing, `IsMinimalArray` has no meaning outside of `CScriptNum`. Having it as a member called `IsMinimalEncoding` or something would be more appropriate.