Add OP_CHECKSIGADD and OP_SUCCESSxxx by sanket1729 · Pull Request #644 · rust-bitcoin/rust-bitcoin

sanket1729 · 2021-09-07T10:35:19Z

Adds support for CHECKSIGADD and OP_SUCCESSxxx. BIP 342 link for reference https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki#specification .

Also makes a minor change to add INVALIDOPCODE (was previously OP_RETURN)

Kixunil · 2021-09-07T16:22:47Z

If I understand this correctly this change assumes everything is Tapscript. That doesn't seem right as old script will continue to exist. Perhaps put opcodes into different submodules?

sanket1729 · 2021-09-07T17:00:53Z

That doesn't seem right as old script will continue to exist. Perhaps put opcodes into different submodules?

Yes, this is correct. I am in favor of doing this, but this seemed too much code diff for opcodes that will never be used (and are not meant to be used). And I was not too concerned about this because this is handled correctly with ScriptContext in rust-miniscript.

If you feel like the diff(a new opcodes submodule along with APIs for it, and updating all usage of current APIs) is worth it, I don't mind updating the PR.

Kixunil · 2021-09-08T09:18:49Z

never be used (and are not meant to be used)

Ah, true, but I think so are OP_SUCCESSxxx until there is a new soft fork making them useful?

sanket1729 · 2021-09-08T14:48:33Z

Here is another possible alternative. We keep the opcodes as they are, only add CHECKSIGADD and classify API as shown below

/// Classification context for the opcode. Some opcodes like `OP_RESERVED`
/// abort the script in [`Legacy`] context, but will act as OP_SUCCESS in tapscript
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub enum ClassifyContext {
    /// Opcode used in tapscript context
    TapScript,
    /// Opcode used in legacy context
    Legacy,
}

impl All {
    /// Classifies an Opcode into a broad class
    #[inline]
    pub fn classify(self, ctx: ClassifyContext) -> Class;

apoelstra · 2021-09-08T21:02:39Z

I like this alternative ... we already have issues with opcodes having multiple names (e.g. OP_TRUE vs OP_1, or OP_0 vs OP_FALSE vs OP_PUSHBYTES_0, or OP_NOPx vs OP_CSV, etc etc). So I think it's okay to rely on the classifier to figure out what an opcode "really" is.

sanket1729 · 2021-09-13T14:19:42Z

Oops, I think I misclicked close

Kixunil · 2021-09-13T14:51:57Z

src/blockdata/opcodes.rs

+        // 3 opcodes
+        match ctx {
+            ClassifyContext::TapScript => // 3 opcodes
+                if self == all::OP_VERIF || self == all::OP_VERNOTIF ||


Nit: I guess that match (self, ctx) would be more readable.

dr-orlovsky

Concept ACK. While I definetly prefer having dedicated script types, suitable for context-specific tasks (like PubkeyScript, SigScript, TapScript etc, as I did in bitcoin_scripts crate), I understand that for the sake of Taproot implementation speed we should go with this for now.

Will be able to review against the spec tomorrow.

apoelstra

ACK b0ad674

dr-orlovsky

Handling of OP_CHECKMULTISIG and OP_CHECKMULTISIGVERIFY is missed. I will do a separate commit + PR to your branch to refactor classificator match as suggested by @Kixunil to make it more readable and address this issue

dr-orlovsky · 2021-09-14T05:33:22Z

src/blockdata/opcodes.rs

+                } else if all::OP_PUSHNUM_1.code <= self.code &&
+                        self.code <= all::OP_PUSHNUM_16.code {


Nit:

all::PUSHNUM.code..=all::OP_PUSHNUM_16.code.contains(self.code) is more idiomatic; otherwise we will have clippy warning here (when we will start using it)

Will change it. It was a copy-paste of the currently existing code

.contains does not work for 1.29 :(

Added to #510 so we don't forget it afterwards.

dr-orlovsky · 2021-09-14T05:33:54Z

src/blockdata/opcodes.rs

-        } else {
-            Class::Ordinary(Ordinary::try_from_all(self).unwrap())
+    pub fn classify(self, ctx: ClassifyContext) -> Class {
+        // 3 opcodes


Nit: we can do use all::* here and get rid of a lot of visual clutter with all:: prefixes

dr-orlovsky · 2021-09-14T05:35:22Z

src/blockdata/opcodes.rs

+                        (self.code >= 126 && self.code <= 129) ||
+                        (self.code >= 131 && self.code <= 134) ||
+                        (self.code >= 137 && self.code <= 138) ||
+                        (self.code >= 141 && self.code <= 142) ||
+                        (self.code >= 149 && self.code <= 153) ||
+                        (self.code >= 187 && self.code <= 254) {


Nit: 126..=129.contains(self.code) ... is more idiomatic (see my commend below)

dr-orlovsky · 2021-09-14T05:35:44Z

src/blockdata/opcodes.rs

+                        (self.code >= 187 && self.code <= 254) {
+                    Class::SuccessOp
+                // 1 opcode
+                } else if self == all::OP_RETURN{


Nit: space before {

dr-orlovsky · 2021-09-14T05:40:28Z

src/blockdata/opcodes.rs

+                        (self.code >= 187 && self.code <= 254) {
+                    Class::SuccessOp
+                // 1 opcode
+                } else if self == all::OP_RETURN{


Also OP_CHECKMULTISIG and OP_CHECKMULTISIGVERIFY according to BIP-342 must be classified as ReturnOp here. Potentially we need to rename ReturnOp into Terminating (as they named altogether in spec).

sanket1729 · 2021-09-14T06:17:07Z

I will do a separate commit + PR to your branch to refactor classificator match as suggested by
Kixunil to make it more readable and address this issue

Thanks :)

dr-orlovsky · 2021-09-14T06:23:14Z

@sanket1729 sorry it happened not to be a PR but just commit - I didn't expect that I can push to your branch directly :( Should I force-push without my commit and to a PR from outside instead?

sanket1729

ACK c12304012469aaa307bea2ae35e812e21aa697fd (modulo whitespaces). I know Andrew specifically does not like them :P

sanket1729 · 2021-09-14T06:42:57Z

src/blockdata/opcodes.rs

minor nit(don't fix unless needed): This was also an error in my code. 61 opcodes for Legacy and 60 for Tapscript.

Fixed since force-push was anyway needed due to end-line space character removal requirement :D

sanket1729 · 2021-09-14T06:48:21Z

sanket1729 sorry it happened not to be a PR but just commit - I didn't expect that I can push to your branch directly :( Should I force-push without my commit and to a PR from outside instead?

No worries. Looks like I explicitly gave all maintainers the permission to edit it.

dr-orlovsky

tACK c12304012469aaa307bea2ae35e812e21aa697fd

src/blockdata/opcodes.rs

…classifier

dr-orlovsky

tACK c75f3ef

sanket1729

ACK c75f3ef

apoelstra · 2021-09-24T22:42:02Z

src/blockdata/opcodes.rs

+            | (OP_RESERVED1, ctx) | (OP_RESERVED2, ctx)
+            | (OP_VER, ctx) if ctx == ClassifyContext::Legacy => Class::ReturnOp,
+
+            // 71 opcodes operating equally to `OP_RETURN` only in Legacy context


Thank you for doing all this counting, but honestly these numbers were originally there so I could check they all added up to 255 -- and now that we have multiple contexts, they don't, so there isn't so much value here :)

apoelstra

ACK c75f3ef

sanket1729 closed this Sep 8, 2021

sanket1729 force-pushed the tap_opcodes branch from 1283b4f to bd5d875 Compare September 8, 2021 14:54

sanket1729 reopened this Sep 13, 2021

sanket1729 added 2 commits September 13, 2021 07:45

Add CHECKSIGADD and update classify API

c252b36

Add tests for opcode classification

b0ad674

sanket1729 force-pushed the tap_opcodes branch from d3db64f to b0ad674 Compare September 13, 2021 14:45

sanket1729 mentioned this pull request Sep 13, 2021

Tracking issue for Taproot #503

Closed

20 tasks

Kixunil reviewed Sep 13, 2021

View reviewed changes

dr-orlovsky reviewed Sep 13, 2021

View reviewed changes

dr-orlovsky added the API break This PR requires a version bump for the next release label Sep 13, 2021

dr-orlovsky added this to the 0.28.0 milestone Sep 13, 2021

apoelstra previously approved these changes Sep 14, 2021

View reviewed changes

dr-orlovsky suggested changes Sep 14, 2021

View reviewed changes

dr-orlovsky dismissed apoelstra’s stale review via a7664b6 September 14, 2021 06:21

dr-orlovsky force-pushed the tap_opcodes branch from a7664b6 to c123040 Compare September 14, 2021 06:32

sanket1729 commented Sep 14, 2021

View reviewed changes

dr-orlovsky previously approved these changes Sep 14, 2021

View reviewed changes

sanket1729 commented Sep 14, 2021

View reviewed changes

src/blockdata/opcodes.rs Outdated Show resolved Hide resolved

Kixunil mentioned this pull request Sep 14, 2021

Rust 2018 & MSRV bump 1.41.1 tracking issue #510

Closed

20 tasks

dr-orlovsky dismissed their stale review via 60ab984 September 14, 2021 11:57

dr-orlovsky force-pushed the tap_opcodes branch from c123040 to 60ab984 Compare September 14, 2021 11:57

Handling CHECKMULTISIG(VERIFY) ops in TapScript context; refactoring …

c75f3ef

…classifier

dr-orlovsky force-pushed the tap_opcodes branch from 60ab984 to c75f3ef Compare September 14, 2021 11:58

dr-orlovsky approved these changes Sep 14, 2021

View reviewed changes

sanket1729 commented Sep 14, 2021

View reviewed changes

apoelstra reviewed Sep 24, 2021

View reviewed changes

apoelstra approved these changes Sep 24, 2021

View reviewed changes

apoelstra merged commit 9fe840c into rust-bitcoin:master Sep 24, 2021

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

		} else if all::OP_PUSHNUM_1.code <= self.code &&
		self.code <= all::OP_PUSHNUM_16.code {

Conversation

sanket1729 commented Sep 7, 2021

Uh oh!

Kixunil commented Sep 7, 2021

Uh oh!

sanket1729 commented Sep 7, 2021

Uh oh!

Kixunil commented Sep 8, 2021

Uh oh!

sanket1729 commented Sep 8, 2021

Uh oh!

apoelstra commented Sep 8, 2021

Uh oh!

sanket1729 commented Sep 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Sep 14, 2021

Uh oh!

dr-orlovsky commented Sep 14, 2021

Uh oh!

sanket1729 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Sep 14, 2021

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

dr-orlovsky left a comment •

edited

Loading

sanket1729 left a comment •

edited

Loading

dr-orlovsky Sep 14, 2021 •

edited

Loading