Implement TryFrom #1007

tcharding · 2022-05-25T07:20:38Z

Audit the whole codebase checking for any method that is of the form from_foo where foo is not an interesting identifier (like 'consensus' and 'standard'). Implement TryFrom for any such methods, deprecating the original.

Done as separate patches so any can be easily dropped if not liked.

apoelstra

ACK c898301

sanket1729

LGTM c898301, but needs rebase

sanket1729 · 2022-06-01T21:30:15Z

src/util/sighash.rs

+            0x81 => AllPlusAnyoneCanPay,
+            0x82 => NonePlusAnyoneCanPay,
+            0x83 => SinglePlusAnyoneCanPay,
+            0xFF => Reserved,


Opened a new issue about removing the variant here

Linking the issue here: #1030

tcharding · 2022-06-02T00:18:37Z

Changes in force push:

rebased .. twice
Used to_u8 instead of deprecated into_u8

apoelstra

ACK 602c17a

Kixunil

Would've acked but I'm not sure about sighash.

Kixunil · 2022-06-16T17:00:53Z

src/util/address.rs

+    fn try_from(opcode: opcodes::All) -> Result<Self, Self::Error> {
+        match opcode.to_u8() {
+            0 => Ok(WitnessVersion::V0),
+            version if version >= opcodes::all::OP_PUSHNUM_1.to_u8() && version <= opcodes::all::OP_PUSHNUM_16.to_u8() =>


Doesn't clippy complain about this? I don't like the lint much anyway, just surprised.

Super bizarre, it seems that to trigger the 'manual range' lint there has to be an = sign. This does not trigger the lint

let version = 0u8; if version > 1 && version < 10 { in_range = true; }

But this does (added =):

let version = 0u8; if version >= 1 && version < 10 { in_range = true; }

src/util/taproot.rs

src/util/sighash.rs

tcharding · 2022-06-16T23:42:49Z

Changes in force push:

Added the additional TryFrom<&[sha256::Hash]> version as suggested
rebased

Remaining open question: what to do about sighash? I do think this PR is a move in the correct direction, possibly we could tackle the naming of try_from vs from_consensus at a later date? @Kixunil do you think we can find consensus quickly/easily or is it a thorny one?

Kixunil · 2022-06-17T07:53:55Z

src/util/taproot.rs

+    ///
+    /// If inner proof length is more than [`TAPROOT_CONTROL_MAX_NODE_COUNT`] (128).
+    fn try_from(v: &[sha256::Hash]) -> Result<Self, Self::Error> {
+        let v = v.to_vec();


This wasn't my point, the caller can already do this. :) My point was to check the length here so that allocation is avoided in the error case. If you want to avoid duplicating checking logic I have a trick for that:

// non-public fn from_collection<T: AsRef<[sha256::Hash]> + Into<Vec<sha256::Hash>>>(collection: T) -> Result<Self, TaprootError> { if collection.as_ref().len() > TAPROOT_CONTROL_MAX_NODE_COUNT { Err(TaprootError::InvalidMerkleTreeDepth(collection.as_ref().len())) } else { Ok(TaprootMerkleBranch(collection.into())) } } // just directly call `Self::from_collection(v)` in all `TryFrom` impls

You could also use macros for TryFrom and also add Box<[sha256::Hash]> as well.

Nice trick :) Implemented as suggested, thanks.

tcharding · 2022-06-19T23:59:04Z

Changes in force push: Amend the final commit adding a macro as suggested.

tcharding · 2022-06-20T00:43:49Z

Weird, the CI run is not showing?

apoelstra

ACK 6ef1ecc

tcharding · 2022-06-24T02:08:58Z

Changes in force push:

rebased on master
Removed TryFrom<u8> for SchnorrSighashType patch, replaced with "rename from_u8 -> from_consensus_u8" patch
Re-ordered patches so the TryFrom ones were all together and the re-name was last.

apoelstra · 2022-06-24T12:32:04Z

Intermediate commits seem not to compile now.

We have a bunch of 'from' methods that are fallible; `TryFrom` became available in Rust 1.34 so we can use it now we have bumped our MSRV. Implement the various `WitnessVersion` from methods using `TryFrom` and deprecate the originals.

TryFrom` became available in Rust 1.34 so we can use it now we have bumped our MSRV. Implement `TryFrom<TaprootBuilder>` for `TapTree` and deprecate the `from_builder` method.

TryFrom` became available in Rust 1.34 so we can use it now we have bumped our MSRV. Implement `TryFrom<Key>` for `ProprietaryKey` and deprecate the `from_key` method.

TryFrom` became available in Rust 1.34 so we can use it now we have bumped our MSRV. Add a macro for implementing `TryFrom` for various lists of `sha256::Hash` types. Use the macro to for vec, slice, and boxed slice.

The `u8` parameter in the `SchnorrSighashType` constructor is a consensus valid `u8`. Re-name the constructor to make this explicit. Deprecate `from_u8` as is typical.

tcharding · 2022-06-28T00:07:52Z

Force push is rebase to pick up clippy fix from #1063, no changes.

apoelstra

ACK b29ff9b

(Having local compilation errors but just due to rust-bitcoin/rust-secp256k1#466 )

Kixunil

ACK b29ff9b

b29ff9b Rename SchnorrSighashType::from_u8 -> from_consensus_u8 (Tobin C. Harding) af16286 Implement TryFrom sha256::Hash for TaprootMerkleBranch (Tobin C. Harding) 6b7b440 Implement TryFrom<Key> for ProprietaryKey (Tobin C. Harding) 5c49fe7 Implement TryFrom<TaprootBuilder> for TapTree (Tobin C. Harding) 632a5db Implement TryFrom for WitnessVersion (Tobin C. Harding) Pull request description: Audit the whole codebase checking for any method that is of the form `from_foo` where foo is not an interesting identifier (like 'consensus' and 'standard'). Implement `TryFrom` for any such methods, deprecating the original. Done as separate patches so any can be easily dropped if not liked. ACKs for top commit: apoelstra: ACK b29ff9b Kixunil: ACK b29ff9b Tree-SHA512: 40f1d96b505891080df1f7a9b3507979b0279a9e0f9d7cd32598bdc16c866785e6b13d5cb1face5ba50e3bc8484a5cd9c7f430d7abc86db9609962476dacd467

tcharding mentioned this pull request May 25, 2022

Rust 2018 & MSRV bump 1.41.1 tracking issue #510

Closed

20 tasks

apoelstra previously approved these changes May 27, 2022

View reviewed changes

sanket1729 previously approved these changes Jun 1, 2022

View reviewed changes

tcharding dismissed stale reviews from sanket1729 and apoelstra via af94774 June 2, 2022 00:16

tcharding force-pushed the 05-25-try-from branch 2 times, most recently from af94774 to 602c17a Compare June 2, 2022 00:18

apoelstra previously approved these changes Jun 2, 2022

View reviewed changes

Kixunil reviewed Jun 16, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via ec7f73b June 16, 2022 23:29

tcharding force-pushed the 05-25-try-from branch from ec7f73b to 553c4a2 Compare June 16, 2022 23:40

Kixunil reviewed Jun 17, 2022

View reviewed changes

tcharding force-pushed the 05-25-try-from branch from 553c4a2 to 6ef1ecc Compare June 19, 2022 23:58

apoelstra previously approved these changes Jun 20, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via b162ea1 June 24, 2022 02:07

tcharding force-pushed the 05-25-try-from branch from 6ef1ecc to b162ea1 Compare June 24, 2022 02:07

tcharding added this to the 0.29.0 milestone Jun 24, 2022

tcharding added 5 commits June 28, 2022 10:04

Implement TryFrom for WitnessVersion

632a5db

We have a bunch of 'from' methods that are fallible; `TryFrom` became available in Rust 1.34 so we can use it now we have bumped our MSRV. Implement the various `WitnessVersion` from methods using `TryFrom` and deprecate the originals.

Implement TryFrom<TaprootBuilder> for TapTree

5c49fe7

TryFrom` became available in Rust 1.34 so we can use it now we have bumped our MSRV. Implement `TryFrom<TaprootBuilder>` for `TapTree` and deprecate the `from_builder` method.

Implement TryFrom<Key> for ProprietaryKey

6b7b440

TryFrom` became available in Rust 1.34 so we can use it now we have bumped our MSRV. Implement `TryFrom<Key>` for `ProprietaryKey` and deprecate the `from_key` method.

Implement TryFrom sha256::Hash for TaprootMerkleBranch

af16286

TryFrom` became available in Rust 1.34 so we can use it now we have bumped our MSRV. Add a macro for implementing `TryFrom` for various lists of `sha256::Hash` types. Use the macro to for vec, slice, and boxed slice.

Rename SchnorrSighashType::from_u8 -> from_consensus_u8

b29ff9b

The `u8` parameter in the `SchnorrSighashType` constructor is a consensus valid `u8`. Re-name the constructor to make this explicit. Deprecate `from_u8` as is typical.

tcharding force-pushed the 05-25-try-from branch from b162ea1 to b29ff9b Compare June 28, 2022 00:05

apoelstra approved these changes Jun 28, 2022

View reviewed changes

Kixunil approved these changes Jun 29, 2022

View reviewed changes

apoelstra merged commit 022ab0b into rust-bitcoin:master Jun 29, 2022

tcharding deleted the 05-25-try-from branch June 29, 2022 23:53

Kixunil added API break This PR requires a version bump for the next release release notes mention and removed API break This PR requires a version bump for the next release labels Jun 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement TryFrom #1007

Implement TryFrom #1007

tcharding commented May 25, 2022 •

edited

apoelstra left a comment

sanket1729 left a comment •

edited

sanket1729 Jun 1, 2022

Eunoia1729 Jun 1, 2022

tcharding commented Jun 2, 2022

apoelstra left a comment

Kixunil left a comment

Kixunil Jun 16, 2022

tcharding Jun 16, 2022

tcharding commented Jun 16, 2022

Kixunil Jun 17, 2022

tcharding Jun 19, 2022

tcharding commented Jun 19, 2022 •

edited

tcharding commented Jun 20, 2022

apoelstra left a comment

tcharding commented Jun 24, 2022

apoelstra commented Jun 24, 2022

tcharding commented Jun 28, 2022

apoelstra left a comment

Kixunil left a comment

Implement TryFrom #1007

Implement TryFrom #1007

Conversation

tcharding commented May 25, 2022 • edited

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment • edited

Choose a reason for hiding this comment

sanket1729 Jun 1, 2022

Choose a reason for hiding this comment

Eunoia1729 Jun 1, 2022

Choose a reason for hiding this comment

tcharding commented Jun 2, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil Jun 16, 2022

Choose a reason for hiding this comment

tcharding Jun 16, 2022

Choose a reason for hiding this comment

tcharding commented Jun 16, 2022

Kixunil Jun 17, 2022

Choose a reason for hiding this comment

tcharding Jun 19, 2022

Choose a reason for hiding this comment

tcharding commented Jun 19, 2022 • edited

tcharding commented Jun 20, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jun 24, 2022

apoelstra commented Jun 24, 2022

tcharding commented Jun 28, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented May 25, 2022 •

edited

sanket1729 left a comment •

edited

tcharding commented Jun 19, 2022 •

edited