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

Follow-up extra comments on taproot code and tests #20207

Merged
merged 8 commits into from
Dec 1, 2020

Conversation

sipa
Copy link
Member

@sipa sipa commented Oct 20, 2020

Addressing some review comments raised here: #19953 (review) and #19953 (review)

Copy link
Contributor

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Code review ACK 51475be, thanks for the clarifications

Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

ACK

@sipa
Copy link
Member Author

sipa commented Oct 21, 2020

Added a few more things to address @MarcoFalke's review comments #19953 (review).

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

ACK

Couldn't find the doc comment change for #19953 (comment) though

test/functional/test_framework/key.py Outdated Show resolved Hide resolved
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 22, 2020

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

Conflicts

No conflicts as of last run.

@maflcko
Copy link
Member

maflcko commented Oct 22, 2020

ACK 029d2d9

only changes comments and tests

@maflcko
Copy link
Member

maflcko commented Oct 25, 2020

@ariard Mind to re-ACK?

Copy link
Contributor

@ariard ariard left a comment

Choose a reason for hiding this comment

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

ACK 029d2d9 pending on fixing the confusion around the new comment on "maximum stack size limit"

src/policy/policy.h Outdated Show resolved Hide resolved
@maflcko
Copy link
Member

maflcko commented Nov 2, 2020

re-ACK 4f10965

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 modulo minor comments below

test/functional/test_framework/script.py Outdated Show resolved Hide resolved
src/pubkey.h Outdated Show resolved Hide resolved
src/policy/policy.h Outdated Show resolved Hide resolved
@ariard
Copy link
Contributor

ariard commented Nov 8, 2020

ACK 4f10965

@sipa
Copy link
Member Author

sipa commented Nov 25, 2020

Rebased.

@jonatack
Copy link
Member

git range-diff 5009159 4f10965 4b8720f LGTM...some of the changes seem to have been lost in the last rebase per git range-diff 5009159 4f10965 0efa4da

@sipa
Copy link
Member Author

sipa commented Nov 26, 2020

@jonatack Thanks for catching that, fixed.

@jonatack
Copy link
Member

ACK 2d8099c per git range-diff 5009159 4f10965 2d8099c

@ariard
Copy link
Contributor

ariard commented Dec 1, 2020

ACK 2d8099c, only changes are comment light improvements on IsValid/IsWitnessStandard.

@maflcko maflcko merged commit f17e8ba into bitcoin:master Dec 1, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 1, 2020
2d8099c Mention units of MAX_STANDARD_ policy constants (Pieter Wuille)
84e29c7 Mention in validation that IsWitnessStandard tests for P2TR (Pieter Wuille)
f867cbc Clean up assets test minimizer LDFLAGS (Pieter Wuille)
ea0e786 Document additional IsWitnessStandard behavior (Pieter Wuille)
6040de9 Add comments on CPubKey::IsValid (Pieter Wuille)
8dbb7de Add comments to VerifyTaprootCommitment (Pieter Wuille)
cdf900c Document need_vin_vout_mismatch argument to make_spender (Pieter Wuille)
18246ed Fix and improve taproot_construct comments (Pieter Wuille)

Pull request description:

  Addressing some review comments raised here: bitcoin#19953 (review) and bitcoin#19953 (review)

ACKs for top commit:
  jonatack:
    ACK 2d8099c per `git range-diff 5009159 4f10965 2d8099c`
  ariard:
    ACK 2d8099c, only changes are comment light improvements on IsValid/IsWitnessStandard.

Tree-SHA512: c4881546c379ea8efc7ef99a43cbf3b9cd3f9dde5fd97a07ee66f2b593c78aef0bd8784853c5c9c737b66c269241a1048bbbdd6c964a3d872efd8ba0ec410b68
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants