Page MenuHomePhabricator

Add a NUM2BIN test with a 5 byte operand
ClosedPublic

Authored by schancel on May 22 2018, 18:25.

Details

Reviewers
jasonbcox
deadalnix
danconnolly
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING911d89718992: Add a NUM2BIN test with a 5 byte operand
rABC911d89718992: Add a NUM2BIN test with a 5 byte operand
Summary

The first operand to OP_NUM2BIN (the number that is to be converted into a binary) is special. It is numeric but can be larger than 4 bytes long and minimal encoding is not required. This revision adds a few tests that confirm this behavior.

  • Try to squeeze a 3 byte number into a 2 byte binary vector - should generate IMPOSSIBLE_ENCODING error
  • Convert a 5 byte number into a 5 byte binary - should pass
  • Convert a 5 byte number into a 6 byte binary - should pass and pad properly
  • Convert a 5 byte number into a 4 byte binary - should generate IMPOSSIBLE_ENCODING error
  • Convert a 520 byte number into a binary - should pass
  • Convert a number that is not minimally encoded - should pass, minimal encoding does not apply to the first operand
  • Shrink a number that is not minimally encoded - should pass
  • Use non-minimally encoded number for second operand - should fail, must be minimally encoded
  • Use 5 byte second operand - should fail with UNKNOWN_ERROR, second operand must be <= 4 bytes long, result too long would generate PUSH_SIZE error
Test Plan

make test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2615
Build 3344: Bitcoin ABC Buildbot (legacy)
Build 3343: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.May 22 2018, 18:25
jasonbcox added inline comments.
src/test/data/script_tests.json
902 ↗(On Diff #3934)

While we're at it, we should add some edge cases as well:

  • 0x05 0xabcdef4243 6 -> should pad appropriately
  • 0x05 0xabcdef4243 4 -> should error

I know this feels redundant, but having extra tests around the boundary condition where greater than 4-byte operands could be an issue, I think it's necessary.

jasonbcox requested changes to this revision.May 22 2018, 18:44
This revision now requires changes to proceed.May 22 2018, 18:44

@jasonbcox good plan. added new tests.

deadalnix requested changes to this revision.May 22 2018, 21:10
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/test/data/script_tests.json
904 ↗(On Diff #3935)

Please add checks that trigger SCRIPT_ERR_IMPOSSIBLE_ENCODING including for smaller integers.

src/test/script_tests.cpp
58 ↗(On Diff #3935)

The tell is that you needed to add it here.

This revision now requires changes to proceed.May 22 2018, 21:10

add several tests for NUM2BIN:

  • try to squeeze a 3 byte number into a 2 byte binary vector - IMPOSSIBLE_ENCODING error
  • convert a 520 byte number into a binary - should pass
  • convert a number that is not minimally encoded - should pass, minimal encoding does not apply to the first operand
  • use non-minimally encoded number for second operand - should fail, must be minimally encoded
  • use 5 byte second operand - should fail, second operand must be <= 4 bytes long
danconnolly edited the summary of this revision. (Show Details)

shrink a number that is not minimally encoded - should pass

Much better, thanks :)

deadalnix requested changes to this revision.May 23 2018, 09:59
deadalnix added inline comments.
src/test/data/script_tests.json
900 ↗(On Diff #3939)

If you are testing an edge case, you need to be testing both side of the edge.

906 ↗(On Diff #3939)

Move the expected result in the output script.

This revision now requires changes to proceed.May 23 2018, 09:59

add test for other side of IMPOSSIBLE_ENCODING edge case & move expected result to output script

This revision is now accepted and ready to land.May 30 2018, 16:27
schancel retitled this revision from add a new test with a 5 byte operand to NUM2BIN to confirm acceptance of operand larger than 4 bytes to Add a NUM2BIN test with a 5 byte operand.
schancel edited the summary of this revision. (Show Details)
schancel added a reviewer: danconnolly.
This revision was automatically updated to reflect the committed changes.
markblundeberg added inline comments.
src/test/data/script_tests.json
910 ↗(On Diff #4057)

note this test did not cover what it was supposed to cover; see D3347