WitnessVersion type #617

dr-orlovsky · 2021-06-13T09:55:20Z

Reopening as a new PR to #562 (github does not allow to re-open force-pushed branches) since there might be changes during the review at this level unrelated to #563 (which will be in draft mode until this is merged - and rebased after the merge)

sgeisler

utACK 3645c63

Looks pretty good. I really like the function documentation describing all error cases applicable to the function (especially when it's only a subset of the error enum).

src/util/address.rs

jrawsthorne

You mentioned here about using WitnessVersion in the is_* methods on Script. Don't know if you want to do that here.

src/util/address.rs

dr-orlovsky · 2021-06-13T21:59:08Z

@jrawsthorne well, tried that and it looks like it adds a lot more code instead of simplifying functions. So just made it for only is_witness_program and not the rest of is_* methods (which will require error testing instead of simple opcode equivalence test used now)

clarkmoody

I prefer this approach, since it "makes impossible states impossible." I've just got a couple preference nits.

clarkmoody · 2021-06-14T03:00:11Z

src/util/address.rs

+    /// # Errors
+    /// If the integer does not corresponds to any witness version, errors with
+    /// [`Error::InvalidWitnessVersion`]
+    pub fn from_no(no: u8) -> Result<Self, Error> {


Nit: use num instead of no for a number variable? Also, this function would probably be better named from_u8

It was originally named from_u8, but later I decided it is a bad name, since there are two different u8-based representations for witness version: by script byte (0, 0x51-0x60) and as version number (incremental from 0 to 16). So while I'm fine with using from_num instead of from_no, it will be confusing to use from_u8.

Possibly raw_num or raw_u8?

+1 for num instead of no

clarkmoody · 2021-06-14T03:01:42Z

src/util/address.rs

+    /// NB: this is not the same as an integer representation of the opcode signifying witness
+    /// version in bitcoin script. Thus, there is no function to directly convert witness version
+    /// into a byte since the conversion requires context (bitcoin script or just a version number)
+    pub fn into_no(self) -> u8 {


Nit: I would prefer into_u8

Commented above: the same reason for the naming here

Kixunil

Just a few doc suggestions.

Kixunil · 2021-06-14T12:32:08Z

src/util/address.rs

+///
+/// Structure helps to limit possible version of the witness according to the
+/// specification; if a plain `u8` type will be used instead it will mean that
+/// version > 16, which is incorrect.


Did you mean to say "if a plain u8 type was used instead it would mean that version may be > 16, which is incorrect"?

src/util/address.rs

Kixunil · 2021-06-14T12:35:38Z

src/util/address.rs

+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let version = u8::from_str(s).map_err(|err| Error::UnparsableWitnessVersion(err))?;


I believe s.parse::<u8>() is considered idiomatic.

src/util/address.rs

Kixunil · 2021-06-14T12:49:27Z

src/util/address.rs

@@ -329,7 +499,7 @@ impl Address {
    }

    /// Check whether or not the address is following Bitcoin
-    /// standardness rules.
+    /// standard rules.


Not a native speaker but the original seemed correct. The rules check if the address is standard.

src/util/address.rs

src/blockdata/script.rs

dr-orlovsky · 2021-06-16T06:14:36Z

Addressed all suggestions with a separate commit

Kixunil · 2021-06-16T08:37:30Z

src/util/address.rs

@@ -159,8 +159,8 @@ impl FromStr for AddressType {
 /// to 16 (inclusive).
 ///
 /// Structure helps to limit possible version of the witness according to the
-/// specification; if a plain `u8` type will be used instead it will mean that
-/// version > 16, which is incorrect.
+/// specification; if a plain `u8` type was be used instead it would mean that


Nit: "was be"

sanket1729

ack 4ecfd68

dr-orlovsky · 2021-06-19T07:58:50Z

Needed rebase, merged master into this branch so the review will not require to go through all commits again. Also fixed @Kixunil nit in docs.

Ned re-ACK from @sanket1729 and review from @apoelstra before the merge since this is intrusive and important change

src/util/address.rs

sgeisler

utACK 409cb91

src/util/address.rs

sgeisler

utACK 8a99a3f

apoelstra · 2021-07-07T20:43:56Z

Can you clean up the history by removing the merge commit and rebasing on top of master?

apoelstra · 2021-07-07T21:36:32Z

ack 8a99a3f aside from history

dr-orlovsky · 2021-08-01T18:39:53Z

@apoelstra done

sanket1729

ACK a8571f2

dr-orlovsky · 2021-08-10T08:35:24Z

@sanket1729 @sgeisler had to force-push due to conflicts with recent merges. Re-ACK will be highly appreciated

sgeisler

re-ACK ecc4008

Just looked at the diff-diff using git range-diff 8a99a3f...ecc400826c5eebf4c8bf3f28219af348a1ba0191

apoelstra · 2021-08-30T17:44:15Z

re-utACK ecc4008

sanket1729

ACK ecc4008

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

dr-orlovsky added this to the 0.27.0 milestone Jun 13, 2021

This was referenced Jun 13, 2021

WitnessVersion type #562

Closed

Taproot P2TR address #563

Merged

sgeisler previously approved these changes Jun 13, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

jrawsthorne reviewed Jun 13, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

dr-orlovsky dismissed sgeisler’s stale review via 7d1e2b4 June 13, 2021 21:44

clarkmoody reviewed Jun 14, 2021

View reviewed changes

Kixunil previously approved these changes Jun 14, 2021

View reviewed changes

sanket1729 reviewed Jun 15, 2021

View reviewed changes

src/util/address.rs Show resolved Hide resolved

src/blockdata/script.rs Show resolved Hide resolved

dr-orlovsky dismissed Kixunil’s stale review via 4ecfd68 June 16, 2021 06:14

dr-orlovsky requested review from clarkmoody, sanket1729 and sgeisler June 16, 2021 06:14

Kixunil reviewed Jun 16, 2021

View reviewed changes

dr-orlovsky added this to In review in Taproot Jun 16, 2021

sanket1729 previously approved these changes Jun 16, 2021

View reviewed changes

dr-orlovsky dismissed sanket1729’s stale review via 409cb91 June 19, 2021 07:57

Kixunil reviewed Jun 20, 2021

View reviewed changes

src/util/address.rs Show resolved Hide resolved

dr-orlovsky requested a review from sanket1729 June 27, 2021 12:43

sgeisler previously approved these changes Jun 27, 2021

View reviewed changes

src/util/address.rs Show resolved Hide resolved

dr-orlovsky dismissed sgeisler’s stale review via 8a99a3f June 27, 2021 13:23

sgeisler previously approved these changes Jun 27, 2021

View reviewed changes

Kixunil mentioned this pull request Jul 15, 2021

Moving bunch of stuff from my payjoin crate here #630

Open

dr-orlovsky dismissed sgeisler’s stale review via a8571f2 August 1, 2021 18:39

sanket1729 previously approved these changes Aug 1, 2021

View reviewed changes

dr-orlovsky added 2 commits August 10, 2021 10:34

WitnessVersion type

64c1ec0

Updating Script::is_witness_program to use new WitnessVersion

ecc4008

dr-orlovsky dismissed sanket1729’s stale review via ecc4008 August 10, 2021 08:34

sgeisler approved these changes Aug 10, 2021

View reviewed changes

dr-orlovsky mentioned this pull request Aug 26, 2021

Transfer WitnessVersion to rust-bitcoin BP-WG/descriptor-wallet#7

Closed

Taproot automation moved this from In review to Ready for merge Aug 30, 2021

apoelstra approved these changes Aug 30, 2021

View reviewed changes

sanket1729 approved these changes Sep 7, 2021

View reviewed changes

apoelstra merged commit 2a655f4 into rust-bitcoin:master Sep 8, 2021

Taproot automation moved this from Ready for merge to Done Sep 8, 2021

dr-orlovsky mentioned this pull request Sep 29, 2021

Tracking issue for Taproot #503

Closed

20 tasks

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WitnessVersion type #617

WitnessVersion type #617

dr-orlovsky commented Jun 13, 2021

sgeisler left a comment

jrawsthorne left a comment

dr-orlovsky commented Jun 13, 2021

clarkmoody left a comment

clarkmoody Jun 14, 2021

dr-orlovsky Jun 14, 2021 •

edited

Kixunil Jun 14, 2021

sanket1729 Jun 15, 2021

clarkmoody Jun 14, 2021

dr-orlovsky Jun 14, 2021

Kixunil left a comment

Kixunil Jun 14, 2021

Kixunil Jun 14, 2021

Kixunil Jun 14, 2021

clarkmoody Jun 14, 2021

dr-orlovsky commented Jun 16, 2021

Kixunil Jun 16, 2021

sanket1729 left a comment

dr-orlovsky commented Jun 19, 2021

sgeisler left a comment

sgeisler left a comment

apoelstra commented Jul 7, 2021

apoelstra commented Jul 7, 2021

dr-orlovsky commented Aug 1, 2021

sanket1729 left a comment

dr-orlovsky commented Aug 10, 2021

sgeisler left a comment

apoelstra commented Aug 30, 2021

sanket1729 left a comment

WitnessVersion type #617

WitnessVersion type #617

Conversation

dr-orlovsky commented Jun 13, 2021

sgeisler left a comment

Choose a reason for hiding this comment

jrawsthorne left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Jun 13, 2021

clarkmoody left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky Jun 14, 2021 • edited

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

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 Jun 16, 2021

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Jun 19, 2021

sgeisler left a comment

Choose a reason for hiding this comment

sgeisler left a comment

Choose a reason for hiding this comment

apoelstra commented Jul 7, 2021

apoelstra commented Jul 7, 2021

dr-orlovsky commented Aug 1, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Aug 10, 2021

sgeisler left a comment

Choose a reason for hiding this comment

apoelstra commented Aug 30, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

dr-orlovsky Jun 14, 2021 •

edited