Trivial improvements for TapTree type by dr-orlovsky · Pull Request #927 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2022-03-31T13:34:11Z

These are trivial fixes from extracted from now closed #922

apoelstra · 2022-04-01T17:12:05Z

src/util/psbt/map/output.rs

In f6dcd2cc56d942c33619bb526c089d171b04f9ab:

I think as_inner' is an extremely weird name for this. Yes, as an implementation detail a TapTreeis a finalizedTaprootBuilder`, but this doesn't necessarily make conceptual sense and I don't think we want to guarantee it.

I'd be ok with the same method named as_builder, or preferably into_builder which takes ownership.

I also didn't like the name into_inner. Let's rename them both.

Renamed & force-pushed. Also renamed from_inner and added standard conversion implementations which makes dev life easier (I found it really unnatural to work with the current TapTree/TaprootBuilder conversions in downstream crates due to lacking APIs).

tcharding · 2022-04-03T22:31:18Z

src/util/psbt/map/output.rs

Suggested change

/// A `TapTree` iff the `builder` builder is complete, otherwise return the inner as `Err`.

/// A `TapTree` iff the `builder` is complete, otherwise return the `builder` as `Err`.

Not part of this PR but the return section header could be pluralized i.e., # Returns.

Fixed elsewhere: https://github.com/rust-bitcoin/rust-bitcoin/pull/929/files#diff-8ed6474e7f75e74be2ab10a3102be1af7dcda692d83edceff3012296e3a7e04eR140

sanket1729

One major comment.

sanket1729 · 2022-04-04T11:41:34Z

src/util/psbt/map/output.rs

As mentioned elsewhere, this should implement ExactLenIterator instead

It does, since the iterator is core::slice::Iter, which indeed is ExactSizeIterator.
This method is just convenience.

Sorry, this happens in #924: b5470bf

I think we should add this using the len method then if really needed. Because the current implementation seems wrong to me. I think it is counting something else, not the number of leaves.

leaf_count may be clearer name?

It is not leaf count: in the future with hidden parts of the tree we will not know about how many leaves are hidden. So probably better to have it as known_scripts_count.

sanket1729 · 2022-04-04T11:49:11Z

src/util/psbt/map/output.rs

nit:

I would rather let users call as_builder instead rather than an indirect AsRef. Maybe it's just only me, but TapTree and TaprootBuilder are not similar conceptually in the operations they do. I would only conservatively implement AsRef when the conversion is super-obvious or should be hidden from the user.

AsRef is needed for generic function arguments

Indeed. It is required for generic functions, but I don't think we should be blindly following conventions. Sometimes are underlying the same things: Ex([u8; 32]) and TapLeafHash can be AsRef because both are both hashes.

TapTree and TaprootBuilder are not similar conceptually in the operations they do, unlike TapLeafHash and [0u8; 32].

TapTree is a tree, that should be viewed as a tree.

TaprootBuilder is a builder designed to construct the tree.

Same comment for Borrow. Many APIs for Builder don't make sense for TapTree, which is why I don't want indirect indirection using AsRef

I don't like leaking the internal representation at all. Is there a strong reason to do it?

Yes, otherwise we can't use TapTree in places where TaprootBuilder is required as an argument: fn some_stuff(tree: impl AsRef<TaprootBuilder>)

As mentioned in the above comment, I would rather users use conversion functions.

sanket1729 · 2022-04-04T11:53:06Z

src/util/psbt/map/output.rs

nit:
Same comment as the previous commit. Is there any reason to do this? Have we done this for inner types before? I think we should avoid implementing it unless we have a reason. Ref, Deref (IMO) should only be implemented on types that are super-obvious to convert and only if they are really needed.

Allowing function args that accept these is however another story.

There is a reason to do this and I always do that for the new types. Deref, otherwise, is not always useful for them.

Some information: https://itfanr.gitbooks.io/rust-doc-en/content/borrow-and-asref.html

Kixunil · 2022-04-05T15:09:34Z

src/util/psbt/map/output.rs

Note that as opposed to previous conversion, this one is OK.

Kixunil · 2022-04-05T15:10:38Z

src/util/psbt/map/output.rs

leaf_count may be clearer name?

sanket1729

Had an incomplete review in another tab

sanket1729 · 2022-04-04T20:41:09Z

src/util/psbt/map/output.rs

Indeed. It is required for generic functions, but I don't think we should be blindly following conventions. Sometimes are underlying the same things: Ex([u8; 32]) and TapLeafHash can be AsRef because both are both hashes.

TapTree and TaprootBuilder are not similar conceptually in the operations they do, unlike TapLeafHash and [0u8; 32].

TapTree is a tree, that should be viewed as a tree.

TaprootBuilder is a builder designed to construct the tree.

Same comment for Borrow. Many APIs for Builder don't make sense for TapTree, which is why I don't want indirect indirection using AsRef

sanket1729 · 2022-04-04T20:43:19Z

src/util/psbt/map/output.rs

I think we should add this using the len method then if really needed. Because the current implementation seems wrong to me. I think it is counting something else, not the number of leaves.

sanket1729 · 2022-04-05T20:01:45Z

I have moved this to 0.29 since this is not a bug fix and a slight feature improvement with some bugs. There are some discussions that might take some time to resolve.

At this rate, we will never have a release until we have a perfect taproot release(which is basically impossible). It has already been months since we started the RC cycle that we might have had two major releases.

sanket1729 · 2022-04-05T20:35:36Z

@dr-orlovsky, do you have an objection to the 0.29 tag for this?

dr-orlovsky · 2022-04-05T20:49:48Z

Removed everything you suggested to remove, @Kixunil and @sanket1729. I will not object to postpone it. But if you have time, I think it will be cool to get the rest of the pr (which is really trivial) just not to require breaking changes after.

sanket1729

utACK bcd9555ab096c3345c02d8d506abdbb11a867277.

tcharding · 2022-04-05T22:32:31Z

FWIW, looks good to me.

Kixunil

Just one detail, strong ACK renaming.

src/util/psbt/map/output.rs

dr-orlovsky · 2022-04-06T11:52:54Z

Force-pushed to remove as_builder and replace it with to_builder, as @Kixunil has requested.

Kixunil

ACK 798c08ece96308489174df8b5af69c2d055adc0b

tcharding

ACK (excluding into_ vs to_, I admit total defeat in trying to fully understand the naming conventions for as/into/to).

tcharding · 2022-04-20T05:55:47Z

excluding into_ vs to_, I admit total defeat in trying to fully understand the naming conventions for as/into/to

I rescind my statement of confusion, as if this minute I am in total understanding of that damnable table and ACK the naming into_builder and to_builder :)

tcharding · 2022-04-20T06:12:33Z

lolz, this one has rc-fix and milestone 0.29 - lets totally get rid of the rc-fix label soon as we do this release :)

dr-orlovsky · 2022-04-20T08:32:25Z

There was a conflict with the master; I have rebased. @sanket1729 @tcharding pls re-ACK

Kixunil

ACK 4cdff06

sanket1729

ACK 4cdff06

apoelstra

ACK 4cdff06

dr-orlovsky added minor API Change This PR should get a minor version bump RC fix labels Mar 31, 2022

dr-orlovsky added this to the 0.28.0 milestone Mar 31, 2022

dr-orlovsky mentioned this pull request Mar 31, 2022

Taproot-related 0.28.0 RC2 fixes #922

Closed

dr-orlovsky changed the title ~~Small taproot fixes~~ Trivial improvements for TapTree type Mar 31, 2022

dr-orlovsky added the trivial Obvious, easy and quick to review (few lines or doc-only...) label Mar 31, 2022

apoelstra reviewed Apr 1, 2022

View reviewed changes

tcharding reviewed Apr 3, 2022

View reviewed changes

sanket1729 reviewed Apr 4, 2022

View reviewed changes

Kixunil reviewed Apr 5, 2022

View reviewed changes

sanket1729 reviewed Apr 5, 2022

View reviewed changes

sanket1729 modified the milestones: 0.28.0, 0.29.0 Apr 5, 2022

dr-orlovsky mentioned this pull request Apr 5, 2022

Release 0.28.0 #944

Merged

sanket1729 previously approved these changes Apr 5, 2022

View reviewed changes

Kixunil requested changes Apr 6, 2022

View reviewed changes

src/util/psbt/map/output.rs Outdated Show resolved Hide resolved

dr-orlovsky dismissed sanket1729’s stale review via 798c08e April 6, 2022 11:52

Kixunil previously approved these changes Apr 6, 2022

View reviewed changes

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

tcharding previously approved these changes Apr 19, 2022

View reviewed changes

dr-orlovsky added 3 commits April 20, 2022 10:28

Make TapTree::node_info public

f9d8d0d

Rename TapTree::into_builder

2192737

Rename TapTree::from_builder

410412f

dr-orlovsky added 2 commits April 20, 2022 10:30

Implement From<TapTree> for TaprootBuilder

a12e7c7

Add convenience method TapTree:to_builder

4cdff06

dr-orlovsky dismissed stale reviews from tcharding and Kixunil via 4cdff06 April 20, 2022 08:31

Kixunil approved these changes Apr 20, 2022

View reviewed changes

sanket1729 approved these changes Apr 20, 2022

View reviewed changes

apoelstra approved these changes Apr 20, 2022

View reviewed changes

apoelstra merged commit 6b57a02 into rust-bitcoin:master Apr 20, 2022

	/// A `TapTree` iff the `builder` builder is complete, otherwise return the inner as `Err`.
	/// A `TapTree` iff the `builder` is complete, otherwise return the `builder` as `Err`.

Conversation

dr-orlovsky commented Mar 31, 2022

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!

tcharding Apr 3, 2022 • 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 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!

sanket1729 Apr 4, 2022 • 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!

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 Apr 4, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

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!

sanket1729 Apr 4, 2022 • 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!

sanket1729 commented Apr 5, 2022

Uh oh!

sanket1729 commented Apr 5, 2022

Uh oh!

dr-orlovsky commented Apr 5, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Apr 5, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dr-orlovsky commented Apr 6, 2022

Uh oh!

tcharding Apr 3, 2022 •

edited

Loading

sanket1729 Apr 4, 2022 •

edited

Loading

sanket1729 Apr 4, 2022 •

edited

Loading

sanket1729 Apr 4, 2022 •

edited

Loading

tcharding commented Apr 20, 2022 •

edited

Loading