Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update finalize API #1151

Merged
merged 4 commits into from
Aug 1, 2022
Merged

Update finalize API #1151

merged 4 commits into from
Aug 1, 2022

Conversation

sanket1729
Copy link
Member

@sanket1729 sanket1729 commented Jul 29, 2022 •
edited
Loading

Found while reviewing rust-bitcoin/rust-miniscript#450 . There is also a BUG fix in the second commit that would have let users spendinfo from incomplete trees.

The bug was introduced in #936 which I am responsible for ACKing

This was referenced Jul 29, 2022
Merged
Merged
@tcharding tcharding added bug P-high High priority labels Jul 29, 2022
tcharding
tcharding previously approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e72b9b93b29b8bb34873dbe43a88721451c32263

Kixunil
Kixunil previously approved these changes Jul 29, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e72b9b93b29b8bb34873dbe43a88721451c32263

But I'd prefer a dedicated type if you have the time.

src/util/taproot.rs
}
_ => Err(TaprootBuilderError::IncompleteTree),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps:

pub struct IncompleteTree {
    pub builder: TaprootBuilder,
    len: usize,
}

// impl Display & co

This would provide more information about the error while still allowing to keep the builder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be overkill. What the user needs to know is that the tree wasn't finalized, which they learn from the error return. If they want to know the specific length they can call .len() themselves.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also possibly nicer error message. But if people find it too much effort, it's OK.

@Kixunil Kixunil added this to the 0.29.0 milestone Jul 29, 2022
apoelstra
apoelstra reviewed Jul 29, 2022
View reviewed changes
apoelstra
apoelstra previously approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e72b9b93b29b8bb34873dbe43a88721451c32263

@apoelstra
Copy link
Member

I'm happy to merge this but will give @sanket1729 a moment to look at the nits.

@sanket1729
Update TaprootBuilder::finalize
0b88051 
This commit does two things:
1) BUG FIX: We should have checked that the length of the branch is 1
before computing the spend_info on it. This was introduced in rust-bitcoin#936, but
slipped past my review :(
2) Update the return type to return the consumed `self` value. This
function can only error when there the tree building is not complete.
Returning TaprootBuilderError causes issues in downstream projects that
need to deal with error cases which cannot happen in this function
@sanket1729
Add breaking test that allowed incomplete builders to be created
aaadd25
@sanket1729
Rename is_finalized to is_finalizable
870ad59 
I really liked the is_complete naming, but that was changed earlier in b0f3992
Was also suggested by Andrew rust-bitcoin#929 (comment)
@sanket1729 sanket1729 dismissed stale reviews from apoelstra, Kixunil, and tcharding via 870ad59 July 29, 2022 20:04
@sanket1729 sanket1729 force-pushed the taproot_builder_updates branch from e72b9b9 to 870ad59 Compare July 29, 2022 20:04
@sanket1729
Copy link
Member Author

@apoelstra @tcharding @Kixunil force pushed. I changed the Err(self) to unreachable!. I really think that adding more code for a nice display message is not worth it.

I would ACK this if someone later added a wrapper error type around this. But right now, I don't want to do this in this PR.

apoelstra
apoelstra approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 870ad59

Kixunil
Kixunil approved these changes Jul 30, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 870ad59

tcharding
tcharding approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 870ad59

@sanket1729 sanket1729 merged commit bb43962 into rust-bitcoin:master Aug 1, 2022
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
Merge rust-bitcoin/rust-bitcoin#1151: Update finalize API
0306e67 
870ad59 Rename is_finalized to is_finalizable (sanket1729)
aaadd25 Add breaking test that allowed incomplete builders to be created (sanket1729)
0b88051 Update TaprootBuilder::finalize (sanket1729)
5813ec7 Return EmptyTree instead of OverCompleteTree when there are no scripts to add (sanket1729)

Pull request description:

  Found while reviewing rust-bitcoin/rust-miniscript#450 . There is also a BUG fix in the second commit that would have let users spendinfo from incomplete trees.

  The bug was introduced in #936 which I am responsible for ACKing

ACKs for top commit:
  apoelstra:
    ACK 870ad59
  Kixunil:
    ACK 870ad59
  tcharding:
    ACK 870ad59

Tree-SHA512: 61442bd95df6912865cbecdc207f330b241e7fbb88b5e915243b2b48a756bea9eb29cb28d8c8249647a0a2ac514df4737bddab695f63075bd55284be5be228ff
@Kixunil Kixunil added the API break This PR requires a version bump for the next release label Aug 1, 2022
apoelstra added a commit that referenced this pull request Aug 8, 2022
@apoelstra
Merge #1109: Release tracking PR - v0.29.0
8a30169 
110b5d8 Bump version to v0.29.0 (Tobin C. Harding)

Pull request description:

  Add changelog notes and bump the version number to v0.29.0.

  ## Notes

  I attempted to go through all the PRs since last release, please sing out if you had a PR merged that is not mentioned and you would like it mentioned.

  The changelog notes can be changed or improved, please do not take me writing them to imply I know exactly what goes on round here - I just made an effort to save others having to do it.

  ## TODO
  - [x]  merge all 'required' PRs
    - #1131
    - #1137
    - #1129
    - #1151
    - #1165 (add release notes still)
  - [x] Ensure all green from the CI run on: rust-bitcoin/rust-miniscript#450
  - [ ] Carry out (and improve) the #1106

ACKs for top commit:
  tcharding:
    ACK 110b5d8
  Kixunil:
    ACK 110b5d8
  apoelstra:
    ACK 110b5d8
  sanket1729:
    reACK 110b5d8

Tree-SHA512: d00c70534476794c01cd694ea9a23affef947c4f913b14344405272bc99cc00763f1ac755cc677e7afbd3dbef573d786251c9093d5dbafd76ee0cf86ca3b0ebd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

@tcharding tcharding tcharding approved these changes

Assignees
No one assigned
Labels
API break This PR requires a version bump for the next release bug P-high High priority
Projects
None yet
Milestone
0.29.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sanket1729 @apoelstra @Kixunil @tcharding