Skip to content

A few follow-ups for taproot signing #22275

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
Merged
fanquake merged 4 commits into from
Aug 23, 2021
Merged

A few follow-ups for taproot signing #22275

fanquake merged 4 commits into from
Aug 23, 2021

Conversation

@sipa
Copy link
Member

@sipa sipa commented Jun 18, 2021 •
edited
Loading

This addresses a few review comments from #21365 that were left at the time of merge (as well as some from #22166 applying to the commit it shared with #21365).

I do not think any are blockers for a 22.0 release.

@sipa sipa mentioned this pull request Jun 18, 2021
Merged
@sipa sipa force-pushed the 202106_taproot_sign_followup branch from 3d3f5ce to 38844bc Compare June 18, 2021 01:17
@sipa sipa mentioned this pull request Jun 18, 2021
Merged
@theStack
Copy link
Contributor

theStack commented Jun 18, 2021

Concept ACK

@achow101
Copy link
Member

achow101 commented Jun 18, 2021

aCK 38844bc18492a847a6a3aff000eb0e85571070dd

@sipa sipa force-pushed the 202106_taproot_sign_followup branch from 38844bc to 9336670 Compare June 18, 2021 20:21
@sipa
Copy link
Member Author

sipa commented Jun 18, 2021

Improved the comment in CKey::SignSchnorr to elaborate on the 3 variants of tweaking (pointed out here).

@Sjors
Copy link
Member

Sjors commented Jun 21, 2021

utACK 9336670

theStack
theStack approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK 9336670df5e0b8baa4169f13ba1d242ecae85d26

Checked that the introduced comments match the logic, verified by review that the code-changes are refactor-only (+ an added sanity check in TaprootBuilder::GetSpendData), built locally, ran unit tests and the functional test feature_taproot.py.

jonatack
jonatack reviewed Jun 27, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK 9336670df5e0b8baa4169f13ba1d242ecae85d26

@DrahtBot DrahtBot mentioned this pull request Jun 29, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 29, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22558 (psbt: Taproot fields for PSBT by achow101)
  • #22512 (Consolidate XOnlyPubKey lookup hack by achow101)
  • #22440 (Eliminate Signature Checker/Creator Ambiguity w/ LIFETIMEBOUND References by JeremyRubin)
  • #22364 (wallet: Make a tr() descriptor by default by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@sipa sipa force-pushed the 202106_taproot_sign_followup branch from 9336670 to f0fb5f8 Compare July 2, 2021 23:42
theStack
theStack reviewed Jul 3, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It seems like after rebasing, the CKey::SignSchnorr comment improvement (previously done in commit 8b1d3db) is missing now?

jonatack
jonatack reviewed Jul 4, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK per git range-diff 7a49fdc 9336670 f0fb5f8 modulo the missing CKey::SignSchnorr() doxygen improvement as mentioned in #22275 (review)

@DrahtBot DrahtBot mentioned this pull request Jul 14, 2021
Closed
This was referenced Jul 21, 2021
Merged
Merged
@sipa sipa force-pushed the 202106_taproot_sign_followup branch from f0fb5f8 to 08f57a0 Compare August 20, 2021 18:29
@sipa
Copy link
Member Author

sipa commented Aug 20, 2021

@theStack @jonatack Restored the accidentally deleted comment improvement.

theStack
theStack approved these changes Aug 21, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

re-ACK 08f57a0 🌴

Checked via git range-diff 9336670d...08f57a00 that since my last ACK, the only changes were jonatack's suggestions w.r.t. to doxygen comments wording and code style (#22275 (comment), #22275 (comment)) plus the rebase on master.

Zero-1729
Zero-1729 reviewed Aug 22, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

crACK 08f57a0 🧉

LGTM, changes check out and taproot tests passed locally.

jonatack
jonatack reviewed Aug 22, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code review ACK 08f57a0 per git range-diff e9d6eb1 9336670 08f57a0 followed by re-code review per commit to swap context back into memory and debug build/run unit tests + feature_taproot.py as a sanity check

@fanquake fanquake merged commit e826b22 into bitcoin:master Aug 23, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 23, 2021
@fanquake
Merge bitcoin#22275: A few follow-ups for taproot signing
a77514a 
08f57a0 Assert that IsComplete() in GetSpendData() (Pieter Wuille)
d8f4b97 Remove default nHashTypeIn arguments to MutableTransactionSignatureCreator (Pieter Wuille)
c7048aa Simplify SignTransaction precomputation loop (Pieter Wuille)
addb9b5 Improve comments in taproot signing logic (Pieter Wuille)

Pull request description:

  This addresses a few review comments from bitcoin#21365 that were left at the time of merge (as well as some from bitcoin#22166 applying to the commit it shared with bitcoin#21365).

  I do not think any are blockers for a 22.0 release.

ACKs for top commit:
  theStack:
    re-ACK 08f57a0 🌴
  Zero-1729:
    crACK 08f57a0
  jonatack:
    Code review ACK 08f57a0 per `git range-diff e9d6eb1 9336670 08f57a0` followed by re-code review per commit to swap context back into memory and debug build/run unit tests + feature_taproot.py as a sanity check

Tree-SHA512: 968750109ba8d6faf3016035a38f81c6aefb724c632a3cab0bbf43cf31b9d187b6b0fddd8772768f57338df11eb07ab9c4c6dacdf3cf35b71f29699c67a301ea
uvhw
uvhw reviewed Oct 31, 2021
View reviewed changes
Copy link

@uvhw uvhw left a comment

Choose a reason for hiding this comment

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

#14529

@bitcoin bitcoin locked and limited conversation to collaborators Oct 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@jonatack jonatack jonatack left review comments

@Zero-1729 Zero-1729 Zero-1729 left review comments

@uvhw uvhw uvhw left review comments

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Consensus

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@sipa @theStack @achow101 @Sjors @DrahtBot @jonatack @Zero-1729 @uvhw @fanquake