Replace GetScriptForWitness with GetScriptForDestination #16841

meshcollider · 2019-09-10T01:49:31Z

As per this TODO in the code:

TODO: replace calls to GetScriptForWitness with GetScriptForDestination using the various witness-specific CTxDestination subtypes.

The commit "Add additional check for P2SH before adding extra wrapper" also adds an additional check that the scriptPubKey is a P2SH before auto-wrapping the witness script. We shouldn't wrap the witness script if not. Note: #16251 is even better than this check, please review that.

DrahtBot · 2019-09-10T02:30:00Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Simplify hash.h interface using Spans #19326 (Simplify hash.h interface using Spans by sipa)

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.

DrahtBot · 2019-09-11T06:08:05Z

Needs rebase

theStack · 2019-09-15T09:49:44Z

src/test/txvalidationcache_tests.cpp

I'd suggest not mixing up the semantically identical PKHash(pubkey) (line 161) and pubkey.GetID() (line 162) here for getting from a public key to a public key hash, see also comment below.

theStack · 2019-09-15T09:51:58Z

Looks good to me. Just one pedantic addition:

To get from public key to public key hash, two variants are in use:

using method CPubKey::GetID(), returns type CKeyID (inherited from uint160)
using class constructor PKHash::PKHash(const CPubKey& pubkey), returns type PKHash (also inherited from uint160), internally calls method GetID()

I'm quite new to the Bitcoin code base, but personally I would find the PKHash(somePubKey) way more expressive and readable than somePubKey.getID() as the term ID doesn't imply that this has anything to do with a hash at all.
Is there even a reason for two distinct data types PKHash and CKeyID which currently seem to serve exactly the same purpose?

meshcollider · 2019-09-15T21:48:58Z

@theStack thanks for the review!

The difference between PKHash and CKeyID is to make the difference between the two uses clear. PKHash is used as a destination type, whereas CKeyID is used internally to identify the key. It was introduced in #15452

theStack · 2019-09-19T20:40:39Z

The difference between PKHash and CKeyID is to make the difference between the two uses clear. PKHash is used as a destination type, whereas CKeyID is used internally to identify the key. It was introduced in #15452

Okay, thanks for the explanation, this brought some clarity. So whenever a public key hash is only used for further processing but is not yet a destination, .GetID() is the way to go I guess?
Seeing it from this perspective, using PKHash(...) as argument for GetScriptForDestination() and on the other hand using .GetID() as argument for WitnessV0KeyHash(...) makes perfect sense and my review comment on line 162 is obsolete.

achow101

Concept ACK

achow101 · 2020-06-11T18:22:31Z

src/rpc/rawtransaction_util.cpp

This was already done in #18484. I think there's a hidden merge conflict here.

achow101 · 2020-06-11T18:29:47Z

src/test/transaction_tests.cpp

nit: It might be easier to read if you made a variable for these scripts instead of copying the same script generation line around.

this is resolved in the third commit(not to reviewers)

DrahtBot · 2020-06-21T10:25:23Z

🐙 This pull request conflicts with the target branch and needs rebase.

meshcollider · 2020-07-02T00:16:22Z

Rebased and review comments addressed

achow101 · 2020-07-02T01:05:22Z

ACK ed266f7ca52fd0df2dd2e25bb9fdf9e5c6888eb9

src/test/script_standard_tests.cpp

src/test/transaction_tests.cpp

meshcollider · 2020-07-02T19:15:30Z

@sipa thanks for the review, but all your comments were already addressed in the third commit :)

jonatack

ACK ed266f7ca52fd0df2dd2e25bb9fdf9e5c6888eb9

Edit: modulo #16841 (comment)

jonatack · 2020-07-03T06:47:51Z

Sanity check: with only the changes in src/bitcoin-tx.cpp, the unit tests pass.

DrahtBot · 2020-08-03T16:27:24Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

achow101 · 2020-08-13T21:10:22Z

re-ACK 7966aa4 only changes since last is rebase.

instagibbs · 2020-08-14T13:28:31Z

ACK 7966aa4

nice to see it gone, much easier to read

jonatack

Code review re-ACK 7966aa4 per git range-diff b4d0366 ed266f7 7966aa4

jonatack · 2020-08-14T19:18:14Z

Perhaps note #16841 (comment) / keep in mind.

sipa · 2020-08-14T19:22:13Z

utACK

…tination 7966aa4 Add variables for repeated scripts (MeshCollider) fec8336 Remove GetScriptForWitness function (MeshCollider) b887060 Replace usage of GetScriptForWitness with GetScriptForDestination (MeshCollider) Pull request description: As per this TODO in the code: > TODO: replace calls to GetScriptForWitness with GetScriptForDestination using the various witness-specific CTxDestination subtypes. The commit "Add additional check for P2SH before adding extra wrapper" also adds an additional check that the scriptPubKey is a P2SH before auto-wrapping the witness script. We shouldn't wrap the witness script if not. Note: bitcoin#16251 is even better than this check, please review that. ACKs for top commit: instagibbs: ACK bitcoin@7966aa4 jonatack: Code review re-ACK 7966aa4 per `git range-diff b4d0366 ed266f7 7966aa4` achow101: re-ACK 7966aa4 only changes since last is rebase. Tree-SHA512: 3449e0e83bd842acc7c94544a85367da97ac20d859eefc1a618caef0c98204398c266fe8fb9600b78326df5175402e1ae4a132eb766e2c4485e7cda6a2a95c43

meshcollider added Refactoring Wallet labels Sep 10, 2019

This was referenced Sep 10, 2019

bitcoin-tx: fix comment about pay-to-witness script type ("addoutpubkey" command) #16833

Closed

rpc: Improve signrawtransaction error reporting #16251

Merged

DrahtBot added the Needs rebase label Sep 11, 2019

theStack reviewed Sep 15, 2019

View reviewed changes

meshcollider removed the Needs rebase label Sep 22, 2019

jonatack mentioned this pull request Oct 10, 2019

Future PRs bitcoin-core-review-club/website#14

Closed

DrahtBot mentioned this pull request Feb 11, 2020

Disallow automatic conversion between disparate hash types #17938

Merged

DrahtBot mentioned this pull request May 30, 2020

scripted-diff: TxoutType C++11 scoped enum class #19114

Merged

achow101 reviewed Jun 11, 2020

View reviewed changes

DrahtBot mentioned this pull request Jun 19, 2020

Simplify hash.h interface using Spans #19326

Merged

DrahtBot added the Needs rebase label Jun 21, 2020

meshcollider removed the Needs rebase label Jul 2, 2020

sipa reviewed Jul 2, 2020

View reviewed changes

jonatack reviewed Jul 3, 2020

View reviewed changes

DrahtBot added the Needs rebase label Aug 3, 2020

meshcollider added 2 commits August 14, 2020 08:44

Replace usage of GetScriptForWitness with GetScriptForDestination

b887060

Remove GetScriptForWitness function

fec8336

Add variables for repeated scripts

7966aa4

meshcollider removed the Needs rebase label Aug 13, 2020

fanquake requested a review from instagibbs August 14, 2020 07:33

jonatack reviewed Aug 14, 2020

View reviewed changes

fanquake merged commit d052f5e into bitcoin:master Aug 15, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Replace GetScriptForWitness with GetScriptForDestination #16841

Replace GetScriptForWitness with GetScriptForDestination #16841

Uh oh!

Conversation

meshcollider commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

DrahtBot commented Sep 11, 2019

Uh oh!

theStack Sep 15, 2019

Choose a reason for hiding this comment

Uh oh!

theStack commented Sep 15, 2019

Uh oh!

meshcollider commented Sep 15, 2019

Uh oh!

theStack commented Sep 19, 2019

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

instagibbs Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 21, 2020

Uh oh!

meshcollider commented Jul 2, 2020

Uh oh!

achow101 commented Jul 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

meshcollider commented Jul 2, 2020

Uh oh!

jonatack left a comment • 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!

jonatack commented Jul 3, 2020

Uh oh!

DrahtBot commented Aug 3, 2020

Uh oh!

achow101 commented Aug 13, 2020

Uh oh!

instagibbs commented Aug 14, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 14, 2020

Uh oh!

sipa commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

meshcollider commented Sep 10, 2019 •

edited

Loading

DrahtBot commented Sep 10, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading