Consensus: DIP-0020: Dash opcode updates - OP_CAT and OP_SPLIT. #3824

tomlaigna · 2020-11-21T17:41:47Z

This PR is intended as a partial implementation of https://github.com/dashpay/dips/blob/master/dip-0020.md
Enable and implement OP_CAT.
Rename OP_SUBSTR to OP_SPLIT and implement it.
Copied from https://reviews.bitcoinabc.org/D1097 mentioned in the document.

Idea is to receive feedback on this PR and if positive, proceed with the rest of the disabled opcodes.

UdjinM6 · 2020-11-21T21:46:37Z

Opcodes themselves look good but pls note that develop branch should be kept backwards compatible. You can't just (re)implement opcodes like that, you need to use some activation mechanism (could re-use DEPLOYMENT_V17 (#3808) for this) and implement corresponding tests i.e. (at least) smth like Bitcoin-ABC/bitcoin-abc@7ad4d79 + Bitcoin-ABC/bitcoin-abc@f103591.

tomlaigna · 2020-11-22T09:54:06Z

@UdjinM6 Thanks for the quick reaction!

If you can, please confirm my understanding that the following should be done here:

Add DEPLOYMENT_DIP0020 to src/chainparams.cpp or just reuse DEPLOYMENT_V17.
Add a script verification flag (maybe ENABLE_DIP0020_OPDCODES) that enables these opcodes and is automatically enabled if the DEPLOYMENT_DIP0020 or DEPLOYMENT_V17 criteria is active.
Implement test that checks this flag is activated properly.
Also fix the script tests to accommodate the enable flag.

UdjinM6 · 2020-11-22T15:22:42Z

Correct. Also, pls rebase this PR on top of the latest develop to fix simplepose test failures.

…rom OP_SUBSTR)

…ctually preserve the DIP0020 flag

UdjinM6

There are few things that need to be fixed:

pls see https://github.com/UdjinM6/dash/commits/pr3824
do we really need a dedicated opcodes_tests.cpp? It looks like all the required tests can be implemented via script_tests.cpp+script_test_json. Or am I missing smth?
pls see an inline question below

But in general, we are moving in the right direction I think :)

src/test/data/script_tests.json

UdjinM6

LGTM, utACK 👍

EDIT: re-indexed on testnet with --assumevalid=0 with no issues.

UdjinM6

Found one tiny issue UdjinM6@38b8127

UdjinM6

re-utACK

PastaPastaPasta

So far so good, one maybe typo?

src/policy/policy.h

PastaPastaPasta · 2020-12-01T16:07:13Z

Also, be aware of the timeline found here #3775, feature freeze on Dec. 7 is approaching quickly. Although that date may get pushed back.

tomlaigna · 2020-12-01T16:40:35Z

In the implementation this pr is based on, the mempool is cleared once the new opcodes are enabled. Same with replay protection. See Bitcoin-ABC/bitcoin-abc@f103591#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R2610
Is this necessary? It's missing here.

UdjinM6

one maybe typo?

oh, wow 🙈 typos are fine though, I make them all the time 😄

re-utACK

xdustinface

Looks good 👍 Just two nits, see the comments.

src/test/script_tests.cpp

test/functional/feature_dip0020_activation.py

UdjinM6

re-re-utACK :D

re

... the mempool is cleared once the new opcodes are enabled. ... It's missing here.

This code is in DisconnectTip, so it's once the new opcodes are disabled because of a reorg actually. Anyway, dropping the whole mempool sounds like a pretty bad idea imo. Even if a reorg would happen (which is quite rare in Dash btw) the block that would cause it would still activate the proposal, there is no option to not activate it. Reorging 2+ blocks is practically impossible thanks to a combination of a) Dash being the major X11 chain and b) ChainLocks. I think it's safe to avoid nuking the mempool in our case. For BCH it's different for obvious reasons (2% of the dsha256 and no 51% attack protection).

xdustinface

utACK

PastaPastaPasta · 2020-12-03T19:03:42Z

Does it really make sense to break this up into a number of PRs? because really, all of this needs to be merged for v0.17, or none of it.

UdjinM6 · 2020-12-03T19:16:26Z

all of this needs to be merged for v0.17, or none of it.

Why? I mean, ideally yes, it would be nice to have all of them activated at once but technically speaking, none of them require another one to function correctly, there is no atomicity requirement for DIP0020.

tomlaigna · 2020-12-03T19:45:55Z

Either way is fine by me. The main reason I only added two opcodes in this pr was that I was unsure how this will be received, so I didn't want to spend too much time adding all the opcodes. In any case I will be adding the rest of the opcodes soon.

UdjinM6 · 2020-12-03T20:13:07Z

I'd say let's merge this one and then work on other opcodes in following PR(s).

@PastaPastaPasta ?

PastaPastaPasta

can you explain why src/test/monolith_opcodes_string.cpp isn't / shouldn't be included?

src/script/interpreter.cpp

PastaPastaPasta · 2020-12-03T20:28:40Z

src/script/interpreter.h

@@ -95,6 +95,9 @@ enum
    // Signature(s) must be empty vector if a CHECK(MULTI)SIG operation failed
    //
    SCRIPT_VERIFY_NULLFAIL = (1U << 14),
+
+    // Enable the opcodes listed in DIP0020 (CAT, SPLIT, etc).


I would maybe list all opcodes here, no need to open the dip to see them

src/script/script.cpp

PastaPastaPasta · 2020-12-03T20:30:12Z

src/script/script_error.h

@@ -34,6 +34,7 @@ typedef enum ScriptError_t
    SCRIPT_ERR_INVALID_STACK_OPERATION,
    SCRIPT_ERR_INVALID_ALTSTACK_OPERATION,
    SCRIPT_ERR_UNBALANCED_CONDITIONAL,
+    SCRIPT_ERR_INVALID_SPLIT_RANGE,


Add the comment like upstream?

PastaPastaPasta · 2020-12-06T19:04:59Z

bump

tomlaigna · 2020-12-07T08:13:30Z

@PastaPastaPasta About the src/test/monolith_opcodes_string.cpp, we agreed with @UdjinM6 that they were basically doing the same thing the script_tests.cpp are doing so kind of redundant. I think it's fine.

PastaPastaPasta · 2020-12-09T19:13:36Z

I'm not sure why not you couldn't have src/test/monolith_opcodes_string.cpp... / why that wouldn't make sense. A bit of test redundancy wouldn't be the end of the world

EDIT: okay, I think @UdjinM6 resolved my concerns in slack. Basically, should treat these opcodes as all other opcodes, and other opcodes are only tested in script_tests(.cpp/.json)

PastaPastaPasta

@UdjinM6 what do you think of below?

src/script/interpreter.cpp

src/test/script_tests.cpp

src/validation.cpp

UdjinM6 · 2020-12-09T19:52:43Z

@PastaPastaPasta not a big fan of the new naming scheme personally tbh, so I'm fine either way

PastaPastaPasta · 2020-12-09T19:56:58Z

@PastaPastaPasta not a big fan of the new naming scheme personally tbh, so I'm fin either way

I don't have a strong prefernce, but I'd like to figure out what style we want to do and follow that

UdjinM6 · 2020-12-09T20:07:21Z

I don't have a strong prefernce, but I'd like to figure out what style we want to do and follow that

I don't think a backport PR like this is a good place to apply/enforce these rules. We could maybe start doing this in our own PRs introducing new features though...

PastaPastaPasta · 2020-12-09T20:10:05Z

I don't have a strong prefernce, but I'd like to figure out what style we want to do and follow that

Sounds good, please disregard formatting changes. Imo, please change the negative boolean

PastaPastaPasta · 2020-12-10T17:10:29Z

bump :D negative boolean

UdjinM6

utACK

xdustinface

utACK

PastaPastaPasta

utACK

…pay#3824) * DIP-0020: Dash opcode updates - enable OP_CAT and OP_SPLIT (renamed from OP_SUBSTR) * DIP-0020: Dash opcode updates - DEPLOYMENT_V17 activates dip0020 opcodes * Add/tweak MAX_SCRIPT_ELEMENT_SIZE tests for OP_CAT and OP_SPLIT * Check nDefaultMaxNumSize in OP_SPLIT tests * Purify DISABLED_OPCODE tests for OP_CAT and OP_SPLIT, fix DoTest to actually preserve the DIP0020 flag * Fix `warning: '&' within '|' [-Wbitwise-op-parentheses]` * Rework/simplify feature_dip0020_activation.py * DIP-0020: Remove functionally redundant tests. * Fix file permissions for feature_dip0020_activation.py * DIP-0020: fix typo * Update src/test/script_tests.cpp Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com> * Update test/functional/feature_dip0020_activation.py Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com> * DIP-0020: improve comments * DIP-0020: dont use negative booleans Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>

tomlaigna force-pushed the enable-splice-ops branch from 4f26da5 to 3c38593 Compare November 21, 2020 19:27

tomlaigna force-pushed the enable-splice-ops branch 2 times, most recently from 8e9a933 to 09404de Compare November 28, 2020 21:16

tomlaigna added 2 commits November 28, 2020 23:16

DIP-0020: Dash opcode updates - enable OP_CAT and OP_SPLIT (renamed f…

8b98018

…rom OP_SUBSTR)

DIP-0020: Dash opcode updates - DEPLOYMENT_V17 activates dip0020 opcodes

543522e

tomlaigna force-pushed the enable-splice-ops branch from 09404de to 543522e Compare November 28, 2020 21:17

UdjinM6 added 5 commits November 29, 2020 18:31

Add/tweak MAX_SCRIPT_ELEMENT_SIZE tests for OP_CAT and OP_SPLIT

6372fe1

Check nDefaultMaxNumSize in OP_SPLIT tests

d7f0a52

Purify DISABLED_OPCODE tests for OP_CAT and OP_SPLIT, fix DoTest to a…

115a0af

…ctually preserve the DIP0020 flag

Fix warning: '&' within '|' [-Wbitwise-op-parentheses]

5e15a01

Rework/simplify feature_dip0020_activation.py

531bfbb

UdjinM6 requested changes Nov 29, 2020

View reviewed changes

src/test/data/script_tests.json Show resolved Hide resolved

DIP-0020: Remove functionally redundant tests.

dbdec0c

UdjinM6 approved these changes Nov 30, 2020

View reviewed changes

UdjinM6 added this to the 17 milestone Nov 30, 2020

UdjinM6 requested review from PastaPastaPasta and xdustinface November 30, 2020 20:36

Fix file permissions for feature_dip0020_activation.py

38b8127

UdjinM6 requested changes Dec 1, 2020

View reviewed changes

UdjinM6 previously approved these changes Dec 1, 2020

View reviewed changes

PastaPastaPasta reviewed Dec 1, 2020

View reviewed changes

src/policy/policy.h Outdated Show resolved Hide resolved

DIP-0020: fix typo

f15cef6

PastaPastaPasta dismissed UdjinM6’s stale review via f15cef6 December 1, 2020 17:48

UdjinM6 previously approved these changes Dec 1, 2020

View reviewed changes

xdustinface suggested changes Dec 1, 2020

View reviewed changes

src/test/script_tests.cpp Outdated Show resolved Hide resolved

test/functional/feature_dip0020_activation.py Outdated Show resolved Hide resolved

UdjinM6 previously approved these changes Dec 2, 2020

View reviewed changes

xdustinface previously approved these changes Dec 2, 2020

View reviewed changes

UdjinM6 requested a review from PastaPastaPasta December 3, 2020 17:46

PastaPastaPasta requested changes Dec 3, 2020

View reviewed changes

DIP-0020: improve comments

ee355dc

tomlaigna dismissed stale reviews from xdustinface and UdjinM6 via ee355dc December 7, 2020 08:23

UdjinM6 requested a review from PastaPastaPasta December 8, 2020 00:42

PastaPastaPasta requested changes Dec 9, 2020

View reviewed changes

src/script/interpreter.cpp Show resolved Hide resolved

src/test/script_tests.cpp Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

DIP-0020: dont use negative booleans

c34ff7d

UdjinM6 approved these changes Dec 11, 2020

View reviewed changes

UdjinM6 requested review from PastaPastaPasta and xdustinface December 11, 2020 10:05

xdustinface approved these changes Dec 11, 2020

View reviewed changes

PastaPastaPasta approved these changes Dec 11, 2020

View reviewed changes

PastaPastaPasta merged commit 5a026be into dashpay:develop Dec 11, 2020

thephez mentioned this pull request Jan 25, 2021

DIP-20 opcode support dashpay/dashcore-lib#218

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consensus: DIP-0020: Dash opcode updates - OP_CAT and OP_SPLIT. #3824

Consensus: DIP-0020: Dash opcode updates - OP_CAT and OP_SPLIT. #3824

tomlaigna commented Nov 21, 2020

UdjinM6 commented Nov 21, 2020 •

edited

Loading

tomlaigna commented Nov 22, 2020

UdjinM6 commented Nov 22, 2020

UdjinM6 left a comment

UdjinM6 left a comment •

edited

Loading

UdjinM6 left a comment

UdjinM6 left a comment

PastaPastaPasta left a comment

PastaPastaPasta commented Dec 1, 2020

tomlaigna commented Dec 1, 2020

UdjinM6 left a comment

xdustinface left a comment

UdjinM6 left a comment

xdustinface left a comment

PastaPastaPasta commented Dec 3, 2020

UdjinM6 commented Dec 3, 2020 •

edited

Loading

tomlaigna commented Dec 3, 2020

UdjinM6 commented Dec 3, 2020

PastaPastaPasta left a comment

PastaPastaPasta Dec 3, 2020

PastaPastaPasta Dec 3, 2020

PastaPastaPasta commented Dec 6, 2020

tomlaigna commented Dec 7, 2020

PastaPastaPasta commented Dec 9, 2020 •

edited

Loading

PastaPastaPasta left a comment

UdjinM6 commented Dec 9, 2020 •

edited

Loading

PastaPastaPasta commented Dec 9, 2020

UdjinM6 commented Dec 9, 2020

PastaPastaPasta commented Dec 9, 2020

PastaPastaPasta commented Dec 10, 2020

UdjinM6 left a comment

xdustinface left a comment

PastaPastaPasta left a comment

Consensus: DIP-0020: Dash opcode updates - OP_CAT and OP_SPLIT. #3824

Consensus: DIP-0020: Dash opcode updates - OP_CAT and OP_SPLIT. #3824

Conversation

tomlaigna commented Nov 21, 2020

UdjinM6 commented Nov 21, 2020 • edited Loading

tomlaigna commented Nov 22, 2020

UdjinM6 commented Nov 22, 2020

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment • edited Loading

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Dec 1, 2020

tomlaigna commented Dec 1, 2020

UdjinM6 left a comment

Choose a reason for hiding this comment

xdustinface left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

xdustinface left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Dec 3, 2020

UdjinM6 commented Dec 3, 2020 • edited Loading

tomlaigna commented Dec 3, 2020

UdjinM6 commented Dec 3, 2020

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta Dec 3, 2020

Choose a reason for hiding this comment

PastaPastaPasta Dec 3, 2020

Choose a reason for hiding this comment

PastaPastaPasta commented Dec 6, 2020

tomlaigna commented Dec 7, 2020

PastaPastaPasta commented Dec 9, 2020 • edited Loading

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 commented Dec 9, 2020 • edited Loading

PastaPastaPasta commented Dec 9, 2020

UdjinM6 commented Dec 9, 2020

PastaPastaPasta commented Dec 9, 2020

PastaPastaPasta commented Dec 10, 2020

UdjinM6 left a comment

Choose a reason for hiding this comment

xdustinface left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 commented Nov 21, 2020 •

edited

Loading

UdjinM6 left a comment •

edited

Loading

UdjinM6 commented Dec 3, 2020 •

edited

Loading

PastaPastaPasta commented Dec 9, 2020 •

edited

Loading

UdjinM6 commented Dec 9, 2020 •

edited

Loading