Consensus: DIP-0020: Dash opcode updates. Remaining dip0020 opcodes#3893
Conversation
50a6f90 to
b465f32
Compare
|
OP_LEFT and OP_RIGHT are removed and their opcodes are now assigned to OP_NUM2BIN = 0x80 and OP_BIN2NUM = 0x81. The functionality of OP_LEFT and OP_RIGHT was replaced by OP_SPLIT. Unit tests are again omitted like before as they pretty much check the same stuff as the script tests. But I don't have a strong opinion in here though, as long as everything is covered by tests. Also using the same activation mechanism as before. |
|
Looks good overall 👍 I would probably extend json data with some edge cases from cpp unit tests e.g. smth like UdjinM6@3314de6#diff-ef0159635ee0fee8864cd9570a04337cabb63e9c49a9e96d7b746194ecaa427dR936-R941 etc. and move other re-enabled opcode checks to where they belong to like UdjinM6@3314de6#diff-ef0159635ee0fee8864cd9570a04337cabb63e9c49a9e96d7b746194ecaa427dL914 -> UdjinM6@3314de6#diff-ef0159635ee0fee8864cd9570a04337cabb63e9c49a9e96d7b746194ecaa427dR927 like we did for SPLIT and CAT earlier. |
54d3394 to
5fb32d9
Compare
|
It was easier for me to just add the unit tests than to try to (re)write the tests to json. I guess extra testing doesn't hurt too much? |
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
It was easier for me to just add the unit tests than to try to (re)write the tests to json. I guess extra testing doesn't hurt too much?
Yeah, I was hoping to keep script tests data in one place but it looks like we can't really do this because of some test data being generated on the fly... ok, I guess having some extra won't make much difference in this case, let's keep it as is.
Well, just one tiny issue then, otherwise looks good imo 👍
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
See comments, primarily nits, formatting, comments
|
Updated formatting, I think i got them all. |
…verify json tests
…d unit tests back
7d50781 to
4110572
Compare
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Sorry, more formatting stuff!
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
@tomthoros Looks like you change some unrelated ones also, see below.
Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>
Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>
Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>
|
Whoops got carried away I guess. I see there is some tool to help with formatting, clang-format-diffpy. Haven't tried, but does that format references and brackets, etc as well? |
…ashpay#3893) * DIP-0020: Dash opcode updates - enable AND, OR, XOR * DIP-0020: Dash opcode updates - enable DIV, MOD * DIP-0020: Dash opcode updates - enable BIN2NUM, NUM2BIN * DIP-0020: Dash opcode updates - enable CHECKDATASIG, CHECKDATASIGVERIFY (no tests) * DIP-0020: Dash opcode updates - fix whitespace * DIP-0020: Dash opcode updates - add checkdatasig and datacheckdatasigverify json tests * More AND_OR_XOR tests * DIP-0020: Dash opcode updates - move opcodes enabled tests around. Add unit tests back * DIP-0020: Dash opcode updates - sort BITCOIN_TESTS aplhabetically * DIP-0020: Dash opcode updates - formatting * DIP-0020: Dash opcode updates - formatting * DIP-0020: Dash opcode updates - formatting references alignment * DIP-0020: Dash opcode updates - formatting references alignment * Update src/script/interpreter.h Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com> * Update src/script/interpreter.h Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com> * Update src/test/script_tests.cpp Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com> Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>
4 participants
As CAT/SPLIT were added, the rest of the opcodes brought out in https://github.com/dashpay/dips/blob/master/dip-0020.md had to follow.
Currently missing script tests for checkdatasig opcodes. The ones added in the reference implementation https://reviews.bitcoinabc.org/D1621 don't work out of the box, seemingly because some changes in the signature format. I will add them when I figure out how they work. Also the unit tests for other opodes have been omitted, but I will reevaluate what may be useful to include.