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.

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

Make TaprooBuilder::finalize able to return keyspend only #936

Merged
merged 1 commit into from Apr 20, 2022

Conversation

JeremyRubin
Copy link
Contributor

No description provided.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

One small comment

src/util/taproot.rs Show resolved Hide resolved
@dr-orlovsky dr-orlovsky added this to In review in Taproot Apr 5, 2022
@JeremyRubin
Copy link
Contributor Author

JeremyRubin commented Apr 7, 2022 via email

@tcharding
Copy link
Member

Ha! I missed the ownership thing during review as well, perhaps a code comment in the function mentioning it?

@sanket1729
Copy link
Member

Why is this important if the function takes ownership of the value?

The inner API from_node_info needs to consume the value. I think it is idiomatic of the builder pattern to consume values and generate new values.

It would be dropped in the error, no?

Yes, but this was in the previous case when the builder was empty. Perhaps, the correct thing to do here(and other builder APIs) is to return to the previous builder state in the error value.

@JeremyRubin
Copy link
Contributor Author

@sanket1729 that sounds like a bigger change because then it's a breaking API change vs a bug fix that can be minor released.

Can we merge this patch for a point release as-is and we can open a new issue for the above (change to a continuation that lets you continue?)

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 7969b7a

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 7969b7a

I missed the ownership thing as well :).

TBH I don't really follow the discussion here. Will hold off on merging until @sanket1729 agrees that it's resolved. But I'm fine to pull this into the release. It is technically adding extra functionality, which we aren't supposed to do, but it's small.

Taproot automation moved this from In review to Ready for merge Apr 18, 2022
@JeremyRubin
Copy link
Contributor Author

JeremyRubin commented Apr 20, 2022 via email

@sanket1729
Copy link
Member

I mean there are reasonable taprootspendinfos that cannot be built

Can you point to an example?

@sanket1729 sanket1729 merged commit 9f79f8d into rust-bitcoin:master Apr 20, 2022
Taproot automation moved this from Ready for merge to Done Apr 20, 2022
@JeremyRubin
Copy link
Contributor Author

JeremyRubin commented Apr 20, 2022 via email

sanket1729 added a commit to sanket1729/rust-bitcoin that referenced this pull request Jul 29, 2022
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 added a commit to sanket1729/rust-bitcoin that referenced this pull request Jul 29, 2022
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 added a commit to sanket1729/rust-bitcoin that referenced this pull request Jul 29, 2022
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 sanket1729 mentioned this pull request Jul 29, 2022
sanket1729 added a commit to sanket1729/rust-bitcoin that referenced this pull request Jul 29, 2022
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 added a commit that referenced this pull request Aug 1, 2022
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
@JeremyRubin
Copy link
Contributor Author

JeremyRubin commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants