Converting LeafVersion into an enum #718

dr-orlovsky · 2021-11-24T23:04:30Z

The original LeafVersion implementation was just a newtype around u8. I think that having enum explicitly listing consensus script implementation rules may be more beneficial in terms of both code readibility and future use of multiple script types, where LeafVersion may operate as a context object provided to Script to specify interpretation rules for particular op codes.

sanket1729

concept ACK. I really like from_u8 over from_byte, but waiting to see what others think of it.

sanket1729 · 2021-11-24T23:12:27Z

src/util/taproot.rs

+        match version {
+            TAPROOT_LEAF_TAPSCRIPT => Ok(LeafVersion::TapScript),
+            TAPROOT_ANNEX_PREFIX => Err(TaprootError::InvalidTaprootLeafVersion(TAPROOT_ANNEX_PREFIX)),
+            even if even & TAPROOT_LEAF_MASK != even => Err(TaprootError::InvalidTaprootLeafVersion(even)),


nit: perhaps you want to name this odd?

Note that the rustdoc says 'even' as well.

Nice catch, the doc-comment should have said odd

Also the parameter name needs to be updated in the docs.

@dr-orlovsky, can you please address this. I am good with the rest of the PR.

Update the docs here (change to odd instead of even)

The code variable name should be odd instead of even

Thank you for spotting! Addressed with the new force-pushed version.

dr-orlovsky · 2021-11-25T08:41:05Z

I actually agree to switch back into *_u8 since it is not a byte in fact: it will be modified with parity flag

Kixunil

Concept ACK

Kixunil · 2021-12-02T20:15:16Z

src/util/taproot.rs

+            (LeafVersion::TapScript, true) => fmt::Display::fmt(&TAPROOT_LEAF_TAPSCRIPT, f),
+            (LeafVersion::Future(version), false) => {
+                f.write_str("future(")?;
+                fmt::Display::fmt(version, f)?;


I don't think formatter parameters should affect version number. E.g. "{:10}" would display future(.........1) (spaces instead of dots, GH is broken) instead of .future(1)
However, getting parameters right is quite annoying. :(

I think we should just always serialize in hex.

Hmm, version is the kind of thing that I consider naturally decimal.

That's fair.

Ok, "no opinion" on what format it should have :)

I agree with @apoelstra that the script "version", which is not ordinal, is better to be formatted as hex: it will be odd to see the next post-tapscript version as future(82) than future(0x52). Decimals will do a bad job here.

I agree with @sanket1729 that we should not use formatting when we use version "number" (actually version id) as a part of other string.

I fixed both of the problems and also changed from future(x) to future_script_0xXX so it will be more readable. When the version is printed without any other string in alternate representation, I pass the formatting flags to allow users to format it as they like (decimal, prefixed with zeros etc).

apoelstra · 2021-12-21T17:24:11Z

concept ACK. I also prefer _u8 over byte.

dr-orlovsky · 2021-12-30T00:21:31Z

@apoelstra @sanket1729 @Kixunil rebased and changed to *_u8

apoelstra

ACK 1ab9282

I am a little tempted to say that the two error conditions should be separate variants, but I think this is fine.

Edit: removed a leading 5 from my hash -- my 5 key and paste-key are right next to each other.

DON-MAC-256 · 2021-12-30T15:50:05Z

Concept ACK

In general, we do well to lean further into Rust's excellent enum system.

sanket1729

ACK, but one last thing

sanket1729 · 2021-12-30T17:39:24Z

src/util/taproot.rs

+        match version {
+            TAPROOT_LEAF_TAPSCRIPT => Ok(LeafVersion::TapScript),
+            TAPROOT_ANNEX_PREFIX => Err(TaprootError::InvalidTaprootLeafVersion(TAPROOT_ANNEX_PREFIX)),
+            even if even & TAPROOT_LEAF_MASK != even => Err(TaprootError::InvalidTaprootLeafVersion(even)),


@dr-orlovsky, can you please address this. I am good with the rest of the PR.

Update the docs here (change to odd instead of even)

The code variable name should be odd instead of even

src/util/taproot.rs

Kixunil · 2022-01-03T11:50:58Z

Note: let's not forget to add this to non_exhaustive TODO after it's merged.

dr-orlovsky · 2022-01-06T09:52:15Z

Addressed all comments. @Kixunil added to non-exhaustive todo (as a part of your comment #510 (comment)).

@apoelstra needs re-ACK

Kixunil

I think serialization issue is serious enough that I'd like at least some good argument why keep it.
I'd also really, really like to avoid weird states by using FutureLeafVersion.

The potential confusion related to from_u8 is not great although not strictly related to this PR, so willing to accept in followup PR.

src/util/taproot.rs

Kixunil · 2022-01-06T10:30:44Z

src/util/taproot.rs

-        LeafVersion(TAPROOT_LEAF_TAPSCRIPT)
-    }
+    /// Future leaf version
+    Future(u8)


This is essentially a public field that allows creating LeafVersion::Future(TAPROOT_LEAF_TAPSCRIPT) which could cause breakages/confusion.

I suggest:

#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct FutureLeafVersion(u8); // IMPORTANT: NO PUBLIC CONSTRUCTOR! // The only way to construct this is by converting u8 to LeafVersion and then extracting it impl FutureLeafVersion> { pub fn to_consensus(self) -> u8 { self.0 } } // How to represent it though? impl Display for FutureLeafVersion { ... }

Maybe also perform conversion from consensus encoding to cardinal representation so that we can use NonZeroU8

src/util/taproot.rs

dr-orlovsky · 2022-01-07T15:07:57Z

@Kixunil I did as you have suggested.

Interestingly enough disallowing invalid leaf versions breaks unit tests, since they include deserialization of invalid control blocks. I assume this must be allowed, otherwise taproot would be a hard fork and not a soft fork. Basically, we need to allow processing (but not constructing?) invalid leaf versions?!

I propose to add third variant (LeafVersion::Invalid(InvalidLeafVersion))) which will be produced only during deserialization and can´t be constructed manually. @apoelstra what do you think?

dr-orlovsky · 2022-01-07T15:16:25Z

src/util/taproot.rs

+    }
+}
+
+impl fmt::Display for FutureLeafVersion {


@Kixunil I think this (unlike LeafVersion) should be allowed to be displayed as a simple number with whatever formatting options were used by the caller.

This is an internal type which can be obtained only from matching over LeafVersion and thus will be rarely print directly.

Sure, I don't feel strongly about the representation. But maybe also implement hex traits?

I clearly remember that I was writing one here. Probably commit got lost through multiple rebases...

Kixunil · 2022-01-07T16:44:58Z

I don't understand why rejecting an invalid control block would be hard fork and not soft fork. Anyway, this is outside of my knowledge. If accepting invalid versions is truly needed then I'm not against having another variant/type but not currently entirely sure it's the best way.

Kixunil

impl Into<u8> doesn't seem right, so not ACKing yet.

src/util/taproot.rs

Kixunil · 2022-01-07T16:57:19Z

src/util/taproot.rs

+                where
+                    E: serde::de::Error,
+            {
+                LeafVersion::from_consensus(value).map_err(|err| E::custom(err))


I think E::unexpected_value would be nicer but wouldn't hold up the urgent PR. Doing it later is completely OK.

E::unexpected_value requires to list all the expected values, which makes this looking ugly

Not really, you can describe it with a string "consensus-encoded leaf version as u8" or something similar

src/util/taproot.rs

apoelstra · 2022-01-07T17:33:51Z

If our tests assume you can deserialize invalid control blocks then the tests are wrong. There is no reason to support this.

And no, disallowing invalid data does not make Taproot a hardfork.

dr-orlovsky · 2022-01-07T17:52:23Z

@apoelstra from what I understand, pre-taproot block may contain witness v1 output spent by anybody with arbitrary script (before taproot activation) including use of invalid control block.

The test which is failing is taken from bitcoin core, so probably we should double-check what it is... I will try to figure this out

Kixunil · 2022-01-07T17:53:37Z

@dr-orlovsky maybe they shouldn't be parsed as control blocks pre-taproot activation?

apoelstra · 2022-01-07T17:55:33Z

We don't need to support pre-Taproot Taproot-like spends in this library.

Kixunil · 2022-01-07T18:10:56Z

From README:

This library must not be used for consensus code

and

patches to fix specific consensus incompatibilities are welcome.

Suggest that it's up to PR author to decide.

IMO it'd be faster to just skip it now, releasing Taproot is more important.
If someone wants to do it (later), I'd prefer using some flag controlling whether Taproot is activated and avoiding parsing control blocks completely if it isn't. Similarly to how Core does it.

dr-orlovsky · 2022-01-07T19:37:38Z

Ok, rebased & removed failed tests with invalid control blocks. Ready for re-review

Kixunil

ACK a3642e8

Just non-critical style comments, can be addressed later or ignored.

Kixunil · 2022-01-07T21:55:24Z

src/util/taproot.rs

+    }
+}
+
+impl fmt::UpperHex for FutureLeafVersion {


doc comments for these impls saying it's in consensus encoding would be nice but not going to block this important PR.

Kixunil · 2022-01-07T21:56:30Z

src/util/taproot.rs

+    /// # Errors
+    /// - If the last bit of the `version` is odd.
+    /// - If the `version` is 0x50 ([`TAPROOT_ANNEX_PREFIX`]).
+    /// - If the `version` is not a valid [`LeafVersion`] byte.


Not nice anymore: we do not error on invalid LeafVersion bytes other than annex. Will provide a fix for this doc

Kixunil · 2022-01-07T22:01:27Z

src/util/taproot.rs

+            (LeafVersion::TapScript, true) => fmt::Display::fmt(&TAPROOT_LEAF_TAPSCRIPT, f),
+            (LeafVersion::Future(version), false) => write!(f, "future_script_{:#02x}", version.0),
+            (LeafVersion::Future(version), true) => fmt::Display::fmt(version, f),
+        }


Very nice that you implement alternate formatting, however the convention is that alternate true means more human-friendly/verbose representation, so it appears it's inverted here.

Kixunil · 2022-01-07T22:02:05Z

src/util/taproot.rs

+        match (self, f.alternate()) {
+            (LeafVersion::TapScript, false) => f.write_str("tapscript"),
+            (LeafVersion::TapScript, true) => fmt::Display::fmt(&TAPROOT_LEAF_TAPSCRIPT, f),
+            (LeafVersion::Future(version), false) => write!(f, "future_script_{:#02x}", version.0),


Super-unimortant: .0 could be dropped here.

Better not to: we may change the way FutureLeafVersion is displayed in a future and this will break formatting here

Kixunil · 2022-01-07T22:03:20Z

src/util/taproot.rs

+    }
+}
+
+impl fmt::UpperHex for LeafVersion {


Comment on these (consensus encoding) may also be helpful.

Kixunil · 2022-01-07T22:04:09Z

src/util/taproot.rs

+
+#[cfg(feature = "serde")]
+#[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
+impl<'de> ::serde::Deserialize<'de> for LeafVersion {


Shame you forgot about doc comment. Not critical.

TODO: create good first issue

#764

Hm, which kind of doc comment can be put here?

Something like "(De)serializes LeafVersion as u8 using consensus encoding."

sanket1729 · 2022-01-08T03:09:10Z

Interestingly enough disallowing invalid leaf versions breaks unit tests, since they include deserialization of invalid control blocks. I assume this must be allowed, otherwise taproot would be a hard fork and not a soft fork. Basically, we need to allow processing (but not constructing?) invalid leaf versions?!

@dr-orlovsky @apoelstra The current implementation is master correct. As I mentioned in the review, I think the futureleafversion is incorrect. I had those test vectors from cross-testing with bitcoin core. All these control blocks were accpeted by bitcoin core in regtest. So, if we cannot parse them, it is our bug!

From BIP 341:

In addition, in order to support some forms of static analysis that rely on being able to identify script spends without access to the output being spent, it is recommended ....

There can be an update to use leaf version, 0x02, but BIP341 does not recommend doing it. But an update would be consensus accepted. Thus, we must parse those leaf versions correctly.

sanket1729 · 2022-01-08T02:58:13Z

src/util/taproot.rs

+            0xE0 | 0xE2 | 0xE4 | 0xE6 | 0xE8 | 0xEA | 0xEC | 0xEE |
+            0xF0 | 0xF2 | 0xF4 | 0xF6 | 0xF8 | 0xFA | 0xFC | 0xFE |
+            // and also 0x66, 0x7e, 0x80, 0x84, 0x96, 0x98, 0xba, 0xbc, 0xbe
+            0x66 | 0x7E | 0x80 | 0x84 | 0x96 | 0x98 | 0xBA | 0xBC | 0xBE => Ok(FutureLeafVersion(version)),


The above values are merely a recommendation and not a consensus rule. I think we should be checking for only even values here.

sanket1729 · 2022-01-08T03:00:30Z

src/util/taproot.rs

@@ -790,16 +827,16 @@ impl LeafVersion {
        match version {
            TAPROOT_LEAF_TAPSCRIPT => Ok(LeafVersion::TapScript),
            TAPROOT_ANNEX_PREFIX => Err(TaprootError::InvalidTaprootLeafVersion(TAPROOT_ANNEX_PREFIX)),
-            odd if odd & TAPROOT_LEAF_MASK != odd => Err(TaprootError::InvalidTaprootLeafVersion(odd)),
-            future => Ok(LeafVersion::Future(future)),
+            0xC1 => Err(TaprootError::InvalidTaprootLeafVersion(0xC1)),


Any special reason why this needs a special line? I think this would be handled by the future match arm in the next line.

sanket1729 · 2022-01-08T03:10:18Z

src/util/taproot.rs

@@ -1059,6 +1096,8 @@ mod test {
    fn control_block_verify() {
        let secp  = Secp256k1::verification_only();
        // test vectors obtained from printing values in feature_taproot.py from bitcoin core
+        // excluding four test cases using invalid leaf version in the control block
+        // see details in <https://github.com/rust-bitcoin/rust-bitcoin/pull/718#issuecomment-1007596228>


No need to exclude these tests, if you change the above, the tests can then be uncommented. See #718 (comment)

Regardless they should definitely not be called invalid if they are not actually invalid.

Sorry for contributing to confusion instead of just checking the BIP myself.

@apoelstra sorry, now I do not understand at all what should be done with these tests or LeafVersion type.

@dr-orlovsky the tests should be restored. We should only reject odd values or 0x50, as the BIP says in footnote 7. Nothing else should be rejected.

How to classify all values which are not 0xC0, LeafVersion::Future?

Everything that isn't known or invalid is Future (even though the real-life future will not include BIPs that ignore the advice in BIP 341).

Kixunil

Even though I'm not happy about inverted alternate() proceeding is better.

ACK ef8a3a8

Kixunil · 2022-01-08T22:43:37Z

src/util/taproot.rs

+/// Inner type representing future (non-tapscript) leaf versions. See [`LeafVersion::Future`].
+///
+/// NB: NO PUBLIC CONSTRUCTOR!
+/// The only way to construct this is by converting u8 to LeafVersion and then extracting it.


TODO: create good first issue for rewriting to `u8` and [`LeafVersion`]

Sorry, I am not following what do you mean here

Putting backticks and square brackets around them :)

Created #763

src/util/taproot.rs

+    }
+}
+
+impl fmt::Display for FutureLeafVersion {


src/util/taproot.rs

+            (LeafVersion::TapScript, true) => fmt::Display::fmt(&TAPROOT_LEAF_TAPSCRIPT, f),
+            (LeafVersion::Future(version), false) => write!(f, "future_script_{:#02x}", version.0),
+            (LeafVersion::Future(version), true) => fmt::Display::fmt(version, f),
+        }


Kixunil · 2022-01-08T22:50:21Z

src/util/taproot.rs

+
+#[cfg(feature = "serde")]
+#[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
+impl<'de> ::serde::Deserialize<'de> for LeafVersion {


TODO: create good first issue

#764

sanket1729

crACK ef8a3a8. Waiting a day to let others complete review before merging.

src/util/taproot.rs

apoelstra

ACK ef8a3a8

dr-orlovsky · 2022-01-09T20:26:54Z

Addresses all your addressable requests in #761

7f06e91 LowerHex and UpperHex implementations for LeafVersion (Dr Maxim Orlovsky) 6a3f3aa Inverse alternative formatting for LeafVersion type (Dr Maxim Orlovsky) bec6694 Fix docs on error conditions in LeafVersion::from_consensus (Dr Maxim Orlovsky) 7c28b47 LowerHex and UpperHex implementations for FutureLeafVersion (Dr Maxim Orlovsky) Pull request description: Trivial post-merge fixups from review comments in #718 ACKs for top commit: Kixunil: ACK 7f06e91 sanket1729: ACK 7f06e91 Tree-SHA512: d94c4bd3d0b466287c8965103f74ecaba185d14c13b6c3f37d9fbe194343b3fc902fd2c7716554ad01fe28ff89cda933df199b7e8388a3fa6097028caf62522b

dr-orlovsky added this to the 0.28.0 milestone Nov 24, 2021

sanket1729 reviewed Nov 24, 2021

View reviewed changes

Kixunil reviewed Dec 2, 2021

View reviewed changes

apoelstra previously approved these changes Dec 30, 2021

View reviewed changes

sanket1729 reviewed Dec 30, 2021

View reviewed changes

RCasatta reviewed Dec 31, 2021

View reviewed changes

src/util/taproot.rs Show resolved Hide resolved

dr-orlovsky added this to In review in Taproot Jan 6, 2022

dr-orlovsky dismissed apoelstra’s stale review via 8ab926f January 6, 2022 09:49

dr-orlovsky mentioned this pull request Jan 6, 2022

Rust 2018 & MSRV bump 1.41.1 tracking issue #510

Closed

20 tasks

Kixunil requested changes Jan 6, 2022

View reviewed changes

sanket1729 mentioned this pull request Jan 7, 2022

Tracking issue for Taproot #503

Closed

20 tasks

dr-orlovsky commented Jan 7, 2022

View reviewed changes

dr-orlovsky added enhancement P-high High priority labels Jan 7, 2022

Kixunil reviewed Jan 7, 2022

View reviewed changes

dr-orlovsky added 2 commits January 7, 2022 20:27

Use TAPROOT_ANNEX_PREFIX in sighash module

2405417

Converting LeafVersion into an enum

67b8db0

dr-orlovsky requested review from Kixunil and apoelstra January 7, 2022 19:37

dr-orlovsky added 2 commits January 7, 2022 22:04

Make serde for LeafVersion to have byte representation

839c022

Improve docs in LeafVersion

b028385

Kixunil previously approved these changes Jan 7, 2022

View reviewed changes

Kixunil added the one ack PRs that have one ACK, so one more can progress them label Jan 7, 2022

sanket1729 reviewed Jan 8, 2022

View reviewed changes

dr-orlovsky dismissed Kixunil’s stale review via a75352c January 8, 2022 22:38

Introduce FutureLeafVersion

ef8a3a8

Kixunil approved these changes Jan 8, 2022

View reviewed changes

Taproot automation moved this from In review to Ready for merge Jan 9, 2022

sanket1729 approved these changes Jan 9, 2022

View reviewed changes

apoelstra reviewed Jan 9, 2022

View reviewed changes

src/util/taproot.rs Show resolved Hide resolved

apoelstra approved these changes Jan 9, 2022

View reviewed changes

apoelstra merged commit 8e9f99b into rust-bitcoin:master Jan 9, 2022

Taproot automation moved this from Ready for merge to Done Jan 9, 2022

dr-orlovsky mentioned this pull request Jan 9, 2022

Taproot trivial post-merge fixups #761

Merged

This was referenced Jan 9, 2022

Put backticks/square brackets around ` u8 and [LeafVersion]` #763

Closed

Document that serde impl of LeafVersion uses u8 in consensus encoding #764

Closed

tcharding mentioned this pull request Jan 10, 2022

Fix the mess around Parity rust-bitcoin/rust-secp256k1#370

Closed

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Converting LeafVersion into an enum #718

Converting LeafVersion into an enum #718

Conversation

dr-orlovsky commented Nov 24, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky commented Nov 25, 2021

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Dec 21, 2021

dr-orlovsky commented Dec 30, 2021

apoelstra left a comment • edited Loading

Choose a reason for hiding this comment

DON-MAC-256 commented Dec 30, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil commented Jan 3, 2022

dr-orlovsky commented Jan 6, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

dr-orlovsky commented Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Kixunil commented Jan 7, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Jan 7, 2022

dr-orlovsky commented Jan 7, 2022

Kixunil commented Jan 7, 2022 • edited Loading

apoelstra commented Jan 7, 2022

Kixunil commented Jan 7, 2022

dr-orlovsky commented Jan 7, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

apoelstra left a comment •

edited

Loading

Kixunil Jan 6, 2022 •

edited

Loading

dr-orlovsky commented Jan 7, 2022 •

edited

Loading

Kixunil commented Jan 7, 2022 •

edited

Loading

Kixunil Jan 8, 2022 •

edited

Loading

sanket1729 commented Jan 8, 2022 •

edited

Loading

sanket1729 Jan 8, 2022 •

edited

Loading

Kixunil Jan 9, 2022 •

edited

Loading

Kixunil Jan 8, 2022 •

edited

Loading