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 2616
Build 3346: Bitcoin ABC Buildbot (legacy)
Build 3345: 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

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

906

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/
9 lines
1 line

Diff 3939

View Options

src/test/data/script_tests.json

Loading...
View Options

src/test/script_tests.cpp

Loading...