Page MenuHomePhabricator
Differential D1438

Add a NUM2BIN test with a 5 byte operand
ClosedPublic
Actions

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

danconnolly created this revision.May 22 2018, 18:25
Owners added a reviewer: Restricted Owners Package.May 22 2018, 18:25
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 22 2018, 18:25
teamcity added a subscriber: teamcity.May 22 2018, 18:26

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

teamcity added a comment.May 22 2018, 18:27

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2611: Diff 3934!May 22 2018, 18:27
jasonbcox added a subscriber: jasonbcox.May 22 2018, 18:44
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
danconnolly updated this revision to Diff 3935.EditedMay 22 2018, 19:50

@jasonbcox good plan. added new tests.

teamcity added a comment.May 22 2018, 19:52

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

Harbormaster failed remote builds in B2612: Diff 3935!May 22 2018, 19:53
teamcity added a comment.May 22 2018, 19:53

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

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
danconnolly updated this revision to Diff 3938.May 22 2018, 22:20

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
teamcity added a comment.May 22 2018, 22:21

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

teamcity added a comment.May 22 2018, 22:22

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2615: Diff 3938!May 22 2018, 22:22
danconnolly edited the summary of this revision. (Show Details)May 22 2018, 22:29
danconnolly updated this revision to Diff 3939.May 22 2018, 22:30
danconnolly edited the summary of this revision. (Show Details)

shrink a number that is not minimally encoded - should pass

teamcity added a comment.May 22 2018, 22:31

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

teamcity added a comment.May 22 2018, 22:31

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2616: Diff 3939!May 22 2018, 22:31
danconnolly edited the summary of this revision. (Show Details)May 22 2018, 22:40
jasonbcox accepted this revision.May 22 2018, 23:17

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
danconnolly updated this revision to Diff 4030.May 30 2018, 15:24

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

teamcity added a comment.May 30 2018, 15:26

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B2678: Diff 4030.May 30 2018, 16:05
teamcity added a comment.May 30 2018, 16:05

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing passed.

deadalnix accepted this revision.May 30 2018, 16:27
This revision is now accepted and ready to land.May 30 2018, 16:27
schancel commandeered this revision.Jun 4 2018, 23:30
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.
Closed by commit rABC911d89718992: Add a NUM2BIN test with a 5 byte operand (authored by danconnolly, committed by schancel). · Explain WhyJun 4 2018, 23:34
This revision was automatically updated to reflect the committed changes.
markblundeberg added a subscriber: markblundeberg.Jun 15 2019, 02:41
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

Revision Contents
Changeset List

PathSize
src/
test/
data/
8 lines
1 line

Diff 3938

View Options

src/test/data/script_tests.json

Loading...
View Options

src/test/script_tests.cpp

Loading...