Page MenuHomePhabricator

Ensure checks are enforced in core_read.cpp for all push types
ClosedPublic

Authored by schancel on Apr 17 2018, 05:26.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
op_pushdata_tests
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2347
Build 2829: Bitcoin ABC Buildbot (legacy)
Build 2828: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
schancel retitled this revision from Add checks for OP_PUSHDATA in ParseScript to Clean up core_read.cpp.Apr 24 2018, 04:52
schancel edited the summary of this revision. (Show Details)
schancel retitled this revision from Clean up core_read.cpp to Ensure checks are enforced in core_read.cpp for all push types.
schancel edited the summary of this revision. (Show Details)

Remove extraneous comment

src/core_read.cpp
128 ↗(On Diff #3632)

It's not clear what that comment is doing here. Also, rather than explaining what something is not, it's probably a better idea to explain what it is.

168 ↗(On Diff #3632)

I'd move that part after the block line 122, so we get a clear separation between data and instructions. In addition, some more comments would be nice :)

src/test/core_io_tests.cpp
1018 ↗(On Diff #3632)

You should consider building these programatically.

Fix nits and build test case programatically.

deadalnix requested changes to this revision.May 1 2018, 23:02
deadalnix added inline comments.
src/core_read.cpp
52 ↗(On Diff #3643)

There is no reason to predeclare size_change.

126 ↗(On Diff #3643)

Explain what each block is for in comments.

129 ↗(On Diff #3643)

???

131 ↗(On Diff #3643)

Describe what it is rather than what it is not. What it is not is not helpful.

145 ↗(On Diff #3643)

Comment what this block is for.

src/test/core_io_tests.cpp
37 ↗(On Diff #3643)

PrintLE can be unit tested easily.

64 ↗(On Diff #3643)

This can do with a test, as it is really easy and would ensure this test what we think it test.

88 ↗(On Diff #3643)

This is inaccurate. It depends on flags passed down. Please don't assume things.

91 ↗(On Diff #3643)

This checks for something too large, but not something too small.

97 ↗(On Diff #3643)

Invalid size's size must also be tested.

111 ↗(On Diff #3643)

It doesn't work because 'help' is 5 bytes. There is no check that disallow this.

This revision now requires changes to proceed.May 1 2018, 23:02

Add comments per feedback, and remove wild *assert*.

src/core_read.cpp
129 ↗(On Diff #3643)

Leftover assert from when the code was laid out differently.

128 ↗(On Diff #3632)

Shouldn't be there. I need to fix my comment-blindness so I stop leaving them places they don't belong.

src/test/core_io_tests.cpp
111 ↗(On Diff #3643)

You're right. There was before though. You had me remove it. I didn't notice it was wrong now.

Remove extraneous test case

A few nits, but the logic looks good.

src/core_read.cpp
52 ↗(On Diff #3709)

There is no reason to predeclare size_change as its use is scoped.

148 ↗(On Diff #3709)

It can also be in the form of the opcode's name.

src/test/core_io_tests.cpp
111 ↗(On Diff #3709)

This can be moved in its own test case, also doesn't check various length.

114 ↗(On Diff #3709)

dito

deadalnix requested changes to this revision.May 2 2018, 15:29
This revision now requires changes to proceed.May 2 2018, 15:29
deadalnix added inline comments.
src/core_read.cpp
119 ↗(On Diff #3724)

I don't really get why this needs to be moved. It seems to me like the appropriate place is at line 66 as we are effectively doing a sort of bucket brigade here with these two variables.

This revision is now accepted and ready to land.May 2 2018, 21:43
This revision was automatically updated to reflect the committed changes.