Page MenuHomePhabricator

Add support for DIV and MOD opcodes
ClosedPublic

Authored by jasonbcox on Mar 18 2018, 02:40.

Details

Reviewers
schancel
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Restricted Maniphest Task
Commits
rSTAGING2c094ccb8320: Add support for DIV and MOD opcodes
rABC2c094ccb8320: Add support for DIV and MOD opcodes
Summary

Added implementation for OP_DIV and OP_MOD with minimal tests

Depends on D1223

Co-authored-by: Joshua Yabut <yabut.joshua@gmail.com>
Co-authored-by: Marcos Mayorga <mm@mm-studios.com>
Co-authored-by: Daniel Connolly <daniel@dconnolly.com>
Co-authored-by: Shammah Chancellor <shammah.chancellor@bitcoinabc.org>

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
src/test/monolith_opcodes.cpp
452–460 ↗(On Diff #3322)

These 4 CheckError() calls are now failing the tests. Everything else is passing fine. But I need to sleep, so will fix this later. If someone knows what's failing here, please let me know.

deadalnix requested changes to this revision.Mar 22 2018, 02:05
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/script/script_error.cpp
55 ↗(On Diff #3322)

Make the error message explicit.

57 ↗(On Diff #3322)

dito

src/test/data/script_tests.json
931 ↗(On Diff #3322)

Please add test for large numbers.

942 ↗(On Diff #3322)

dito

src/test/monolith_opcodes.cpp
105 ↗(On Diff #3322)

Please make the refactoring in a different diff.

436 ↗(On Diff #3322)

I think you should create a function to check for div and mod taking 4 paremeter: arguement's a and b passed to the ops, result of div and result of mod.

Then you can check all kind of properties, such as -a/b = -div, a/-b = -div and so on. This allows you to have many more test cases and grill the opcodes behavior.

453 ↗(On Diff #3322)

Please put comments on the line before the code it refers to rather than on the same line.

473 ↗(On Diff #3322)

If you create the function i u=suggested earlier, you'd have all these types of test case taken care of automagically.

This revision now requires changes to proceed.Mar 22 2018, 02:05

Refactoring DIV/MOD and cleanup BIN2NUM tests

movrcx added a subscriber: movrcx.
This comment was removed by movrcx.

Fixup: Minor typo in modulus error message

movrcx added a task: Restricted Maniphest Task.Mar 28 2018, 19:01
movrcx removed a task: Restricted Maniphest Task.
jasonbcox marked 2 inline comments as done.

Added some identity tests (note: a/b * b + a%b = a can't be done because OP_MUL is disabled)
Added long number tests
Reduced test implementation according to Amaury's feedback (CheckDiv and CheckMod)

jasonbcox edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Mar 29 2018, 08:10
deadalnix added inline comments.
src/test/monolith_opcodes.cpp
96 ↗(On Diff #3331)

This won't handle zero properly. An empty valtype should return a valtype with {0x80} when negated.

This also calls for a test for NegativeValtype.

506 ↗(On Diff #3331)

I think you should really have one check function that take result for div and mod. This ensure by design that you all test cases for both. It's not a big deal to have too many tests cases for one or the other.

This revision now requires changes to proceed.Mar 29 2018, 08:10
jasonbcox marked 2 inline comments as done.

Fixed up NegativeValtype, added tests for it, and merged CheckDiv + CheckMod = CheckDivMod

deadalnix requested changes to this revision.Mar 30 2018, 16:01

A few things:
1/ You should remove the stacktype everywhere, this distract from the values.
2/ Move the various identities in CheckDivMod . this ensure the ops are behaving the way we expect in general and reduce the number of test cases.
3/ It seems like you could just fusing the tests for div and mod, as it would make it apparent what error conditions are different or similar between the two and if cases where skipped. It would also remove confusion about what CheckDivMod are in which test for what reason as they clearly check for both.
4/ Considering error conditions are similar between the two, it's probably worth doing something similar to CheckDivMod but for error conditions, the way it's done for bitwise ops.

src/test/monolith_opcodes.cpp
99 ↗(On Diff #3336)
r[r.size() - 1] ^= 0x80;
111 ↗(On Diff #3336)

You don't need to say valtype everywhere if you put the function call on the left.

130 ↗(On Diff #3336)

likestamp

545 ↗(On Diff #3336)

sealofaproval

549 ↗(On Diff #3336)

You don't need to specify stacktype all over the place.

582 ↗(On Diff #3336)

Why no CheckDivMod ?

585 ↗(On Diff #3336)

Known results, such as a/a = 1 and a/1 = a and can also be added to CheckDivMod so they get grilled with many values of a.

616 ↗(On Diff #3336)

Put positive and negative zeros in there.

637 ↗(On Diff #3336)

This is also a property that can be checked by CheckDivMod on many values.

This revision now requires changes to proceed.Mar 30 2018, 16:01
jasonbcox marked 9 inline comments as done.

Addressed feedback, resulting in better test coverage

deadalnix requested changes to this revision.Mar 31 2018, 17:39

One missing test case + one small refactoring in the test code and we are there.

src/test/monolith_opcodes.cpp
671 ↗(On Diff #3348)

You are missing the minimally encoded zero: {}

734 ↗(On Diff #3348)

You should have a CheckDivModError as these error should happen for both div and mod.

743 ↗(On Diff #3348)

dito

772 ↗(On Diff #3348)

This would become redundant

781 ↗(On Diff #3348)

This would become redundant as well.

791 ↗(On Diff #3348)

And this can be merged in div_and_mod_opcode_tests

This revision now requires changes to proceed.Mar 31 2018, 17:39
jasonbcox marked 6 inline comments as done.

Fixed missing test case and code refactor/cleanup

This revision is now accepted and ready to land.Mar 31 2018, 18:06
This revision was automatically updated to reflect the committed changes.