Skip to content

Commit

Permalink
Added LSHIFT and RSHIFT opcode functionality and test
Browse files Browse the repository at this point in the history
  • Loading branch information
@Danconnolly
shaunOK authored and Danconnolly committed Aug 29, 2018
1 parent 86ad646 commit 74922dd
Show file tree
Hide file tree
Showing 3 changed files with 324 additions and 6 deletions.
100 changes: 98 additions & 2 deletions src/script/interpreter.cpp
View file
Expand Up @@ -34,6 +34,68 @@ inline bool set_error(ScriptError *ret, const ScriptError serror) {

} // namespace

inline uint8_t make_rshift_mask(size_t n) {
static uint8_t mask[] = {0xFF, 0xFE, 0xFC, 0xF8, 0xF0, 0xE0, 0xC0, 0x80};
return mask[n];
}

inline uint8_t make_lshift_mask(size_t n) {
static uint8_t mask[] = {0xFF, 0x7F, 0x3F, 0x1F, 0x0F, 0x07, 0x03, 0x01};
return mask[n];
}

// shift x right by n bits, implements OP_RSHIFT
static valtype RShift(const valtype &x, int n) {
int bit_shift = n % 8;
int byte_shift = n / 8;

uint8_t mask = make_rshift_mask(bit_shift);
uint8_t overflow_mask = ~mask;

valtype result(x.size(), 0x00);
for (int i = 0; i < (int)x.size(); i++) {
int k = i + byte_shift;
if (k < (int)x.size()) {
uint8_t val = (x[i] & mask);
val >>= bit_shift;
result[k] |= val;
}

if (k + 1 < (int)x.size()) {
uint8_t carryval = (x[i] & overflow_mask);
carryval <<= 8 - bit_shift;
result[k + 1] |= carryval;
}
}
return result;
}

// shift x left by n bits, implements OP_LSHIFT
static valtype LShift(const valtype &x, int n) {
int bit_shift = n % 8;
int byte_shift = n / 8;

uint8_t mask = make_lshift_mask(bit_shift);
uint8_t overflow_mask = ~mask;

valtype result(x.size(), 0x00);
for (int i = x.size() -1; i >= 0; i--) {
int k = i - byte_shift;
if (k >= 0) {
uint8_t val = (x[i] & mask);
val <<= bit_shift;
result[k] |= val;
}

if (k - 1 >= 0) {
uint8_t carryval = (x[i] & overflow_mask);
carryval >>= 8 - bit_shift;
result[k - 1] |= carryval;
}
}
return result;
}

bool CastToBool(const valtype &vch) {
for (size_t i = 0; i < vch.size(); i++) {
if (vch[i] != 0) {
Expand Down Expand Up @@ -285,13 +347,13 @@ static bool IsOpcodeDisabled(opcodetype opcode, uint32_t flags) {
switch (opcode) {
case OP_2MUL:
case OP_2DIV:
case OP_LSHIFT:
case OP_RSHIFT:
// Disabled opcodes.
return true;

case OP_INVERT:
case OP_MUL:
case OP_LSHIFT:
case OP_RSHIFT:
// Opcodes that have been reenabled.
if ((flags & SCRIPT_ENABLE_MAGNETIC_OPCODES) == 0) {
return true;
Expand Down Expand Up @@ -856,6 +918,40 @@ bool EvalScript(std::vector<valtype> &stack, const CScript &script,
}
} break;

case OP_LSHIFT: {
// (x n -- out)
if (stack.size() < 2) {
return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
}

const valtype vch1 = stacktop(-2);
CScriptNum n(stacktop(-1), fRequireMinimal);
if (n < 0) {
return set_error(serror, SCRIPT_ERR_INVALID_NUMBER_RANGE);
}

popstack(stack);
popstack(stack);
stack.push_back(LShift(vch1, n.getint()));
} break;

case OP_RSHIFT: {
// (x n -- out)
if (stack.size() < 2) {
return set_error( serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
}

const valtype vch1 = stacktop(-2);
CScriptNum n(stacktop(-1), fRequireMinimal);
if (n < 0) {
return set_error(serror, SCRIPT_ERR_INVALID_NUMBER_RANGE);
}

popstack(stack);
popstack(stack);
stack.push_back(RShift(vch1, n.getint()));
} break;

case OP_EQUAL:
case OP_EQUALVERIFY:
// case OP_NOTEQUAL: // use OP_NUMNOTEQUAL
Expand Down
50 changes: 46 additions & 4 deletions src/test/data/script_tests.json
View file
Expand Up @@ -957,10 +957,10 @@
["2 2 0 IF MOD ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC,MAGNETIC_OPCODES", "DISABLED_OPCODE", "MOD disabled"],
["2 2 0 IF LSHIFT ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC", "DISABLED_OPCODE", "LSHIFT disabled"],
["2 2 0 IF LSHIFT ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC,MONOLITH_OPCODES", "DISABLED_OPCODE", "LSHIFT disabled"],
["2 2 0 IF LSHIFT ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC,MAGNETIC_OPCODES", "DISABLED_OPCODE", "LSHIFT disabled"],
["2 2 0 IF LSHIFT ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT enabled"],
["2 2 0 IF RSHIFT ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC", "DISABLED_OPCODE", "RSHIFT disabled"],
["2 2 0 IF RSHIFT ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC,MONOLITH_OPCODES", "DISABLED_OPCODE", "RSHIFT disabled"],
["2 2 0 IF RSHIFT ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC,MAGNETIC_OPCODES", "DISABLED_OPCODE", "RSHIFT disabled"],
["2 2 0 IF RSHIFT ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT enabled"],

["Bitwise opcodes"],
["AND"],
Expand Down Expand Up @@ -1008,6 +1008,48 @@
["0x03801234", "INVERT 0x037FEDCB EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "INVERT, 3 bytes"],
["0x088012348012341234", "INVERT 0x087FEDCB7FEDCBEDCB EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "INVERT, 8 bytes"],

["LSHIFT"],
["2 LSHIFT", "8 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "INVALID_STACK_OPERATION", "LSHIFT, invalid parameter count"],
["2 -1 LSHIFT", "8 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "INVALID_NUMBER_RANGE", "LSHIFT, n must be >= 0"],
["2 2 LSHIFT", "8 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "simple LSHIFT"],
["24 0 LSHIFT", "24 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT, n = 0"],
["0x01FF 0 LSHIFT", "0x01FF EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT 8 bit value by 0"],
["0x01FF 1 LSHIFT", "0x01FE EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT 8 bit value by 1"],
["0x01FF 2 LSHIFT", "0x01FC EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT 8 bit value by 2"],
["0x01FF 3 LSHIFT", "0x01F8 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT 8 bit value by 3"],
["0x01FF 4 LSHIFT", "0x01F0 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT 8 bit value by 4"],
["0x01FF 5 LSHIFT", "0x01E0 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT 8 bit value by 5"],
["0x01FF 6 LSHIFT", "0x01C0 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT 8 bit value by 6"],
["0x01FF 7 LSHIFT", "0x0180 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT 8 bit value by 7"],
["0x01FF 8 LSHIFT", "0x0100 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT 8 bit value by 8"],
["0x020080 1 LSHIFT", "0x020100 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT over byte boundary"],
["0x03008000 1 LSHIFT", "0x03010000 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT over byte boundary"],
["0x03000080 1 LSHIFT", "0x03000100 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT over byte boundary"],
["0x03800000 1 LSHIFT", "0x03000000 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT over byte boundary"],
["0x085462725AFE7647D2 2 LSHIFT", "0x085189C96BF9D91F48 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT, 8 bytes long"],
["0x0131 0x010A LSHIFT", "0x0100 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "LSHIFT, n > len(x)"],

["RSHIFT"],
["2 RSHIFT", "1 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "INVALID_STACK_OPERATION", "RSHIFT, invalid parameter count"],
["2 -1 RSHIFT", "8 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "INVALID_NUMBER_RANGE", "RSHIFT, n must be >= 0"],
["2 1 RSHIFT", "1 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "simple RSHIFT"],
["24 0 RSHIFT", "24 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT, n = 0"],
["0x01FF 0 RSHIFT", "0x01FF EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT 8 bit value by 0"],
["0x01FF 1 RSHIFT", "0x017F EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT 8 bit value by 1"],
["0x01FF 2 RSHIFT", "0x013F EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT 8 bit value by 2"],
["0x01FF 3 RSHIFT", "0x011F EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT 8 bit value by 3"],
["0x01FF 4 RSHIFT", "0x010F EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT 8 bit value by 4"],
["0x01FF 5 RSHIFT", "0x0107 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT 8 bit value by 5"],
["0x01FF 6 RSHIFT", "0x0103 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT 8 bit value by 6"],
["0x01FF 7 RSHIFT", "0x0101 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT 8 bit value by 7"],
["0x01FF 8 RSHIFT", "0x0100 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT 8 bit value by 8"],
["0x020100 1 RSHIFT", "0x020080 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT over byte boundary"],
["0x03010000 1 RSHIFT", "0x03008000 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT over byte boundary"],
["0x03000100 1 RSHIFT", "0x03000080 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT over byte boundary"],
["0x03000001 1 RSHIFT", "0x03000000 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT over byte boundary"],
["0x085462725AFE7647D2 2 RSHIFT", "0x0815189C96BF9D91F4 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT, 8 bytes long"],
["0x0131 0x010A RSHIFT", "0x0100 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "RSHIFT, n > len(x)"],

["Arithmetic Opcodes"],
["MUL"],
["MUL", "4 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "INVALID_STACK_OPERATION", "no parameters"],
Expand Down Expand Up @@ -1138,9 +1180,9 @@
["2 2DIV", "1 EQUAL", "P2SH,STRICTENC", "DISABLED_OPCODE", "disabled"],
["2 2DIV", "1 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "DISABLED_OPCODE", "disabled"],
["2 2 LSHIFT", "8 EQUAL", "P2SH,STRICTENC", "DISABLED_OPCODE", "disabled"],
["2 2 LSHIFT", "8 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "DISABLED_OPCODE", "disabled"],
["2 2 LSHIFT", "8 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "enabled"],
["2 1 RSHIFT", "1 EQUAL", "P2SH,STRICTENC", "DISABLED_OPCODE", "disabled"],
["2 1 RSHIFT", "1 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "DISABLED_OPCODE", "disabled"],
["2 1 RSHIFT", "1 EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "enabled"],
["0x0100 INVERT", "0x01FF EQUAL", "P2SH,STRICTENC", "DISABLED_OPCODE", "disabled"],
["0x0100 INVERT", "0x01FF EQUAL", "P2SH,STRICTENC,MAGNETIC_OPCODES", "OK", "enabled"],

Expand Down

6 comments on commit 74922dd

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented on 74922dd Aug 30, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm expecting everyone will be be so distracted by the convoluted table implementations of 256 - (1 << b) and (1 << 8 - b) - 1 that they will never notice the lack of range checking on n with respect to x.size(), resulting in out of bounds accesses.

This also appears to lack sign extension, so I am expecting numbertype negative inputs to not be handled in a consistent way. Leaving values zero-byte padded after right shift seems odd at least not without seeing a document that describes a theory of how someone would use these opcodes. I stopped looking after the out-of-range a few lines in, so there might be other issues too.

This certainly isn't consistent with how the opcodes originally worked: Script originally used bignums and the utility of arithmetic opcodes on bignums was obvious: you could use them to implement things like RSA and the paillier cryptosystem. Arithmetic operations on size limited types could be useful too, but there would probably need to be a consistent interface-- not some operations randomly padding outputs and others not.

@jmcorgan
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greg, you are too generous with your expertise, especially when it benefits people who have declared you both incompetent and evil. But I bet there will be furious behind the scenes activity to take your advice and fix things in a way that tries to avoid attribution to your analysis.

@shinsyotta
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff.

@nitroxplicit
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greg, I couldn't possibly agree more with Jmcorgan, these people aren't deserving of your kindness but I guess that's what you the better person. Great stuff

@Danconnolly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your comment. However, the algorithm used is not susceptible to an out of range positive value and is never called with a negative value.

@RufusYoakam
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Greg, I do see the range checks on n are in the CScriptNum

script.h 342:
int getint() const {
if (m_value > std::numeric_limits::max())
return std::numeric_limits::max();
else if (m_value < std::numeric_limits::min())
return std::numeric_limits::min();
return m_value;
}

RShift() and LShift() are always called with n.getint() in the current codebase but for futureproofing it might make more sense to pass CScriptNum to those functions and to call getint() from within.

It's good to see you take an interest in this project. I for one am looking forward to your continued feedback.

Please sign in to comment.