Replace GetScriptForWitness with GetScriptForDestination #16841

meshcollider commented at 1:49 AM on September 10, 2019: contributor

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.

meshcollider added the label Refactoring on Sep 10, 2019

meshcollider added the label Wallet on Sep 10, 2019

DrahtBot commented at 2:30 AM on September 10, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 added the label Needs rebase on Sep 11, 2019

DrahtBot commented at 6:08 AM on September 11, 2019: member

Needs rebase

in src/test/txvalidationcache_tests.cpp:160 in 8e020b8c41 outdated

 158 | @@ -159,7 +159,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
 159 |      CScript p2pk_scriptPubKey = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
 160 |      CScript p2sh_scriptPubKey = GetScriptForDestination(ScriptHash(p2pk_scriptPubKey));
 161 |      CScript p2pkh_scriptPubKey = GetScriptForDestination(PKHash(coinbaseKey.GetPubKey()));
 162 | -    CScript p2wpkh_scriptPubKey = GetScriptForWitness(p2pkh_scriptPubKey);
 163 | +    CScript p2wpkh_scriptPubKey = GetScriptForDestination(WitnessV0KeyHash(coinbaseKey.GetPubKey().GetID()));

theStack commented at 9:49 AM on September 15, 2019:

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 commented at 9:51 AM on September 15, 2019: member

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 commented at 9:48 PM on September 15, 2019: contributor

@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 commented at 8:40 PM on September 19, 2019: member

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.

meshcollider removed the label Needs rebase on Sep 22, 2019

in src/rpc/rawtransaction_util.cpp:219 in 00ab0b4cc0 outdated

 215 | @@ -216,7 +216,7 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst
 216 |                  keystore->AddCScript(script);
 217 |                  // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
 218 |                  // This is done for redeemScript only for compatibility, it is encouraged to use the explicit witnessScript field instead.
 219 | -                CScript witness_output_script{GetScriptForWitness(script)};
 220 | +                CScript witness_output_script{GetScriptForDestination(WitnessV0ScriptHash(script))};

achow101 commented at 6:22 PM on June 11, 2020:

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

in src/test/transaction_tests.cpp:545 in 00ab0b4cc0 outdated

 545 | -    BOOST_CHECK(keystore.AddCScript(GetScriptForWitness(scriptMulti)));
 546 | +    BOOST_CHECK(keystore.AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubkey1.GetID()))));
 547 | +    BOOST_CHECK(keystore.AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubkey2.GetID()))));
 548 | +    BOOST_CHECK(keystore.AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubkey1L.GetID()))));
 549 | +    BOOST_CHECK(keystore.AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubkey2L.GetID()))));
 550 | +    BOOST_CHECK(keystore.AddCScript(GetScriptForDestination(WitnessV0ScriptHash(scriptMulti))));

achow101 commented at 6:29 PM on June 11, 2020:

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

instagibbs commented at 1:28 PM on August 14, 2020:

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

achow101 commented at 6:30 PM on June 11, 2020: member

Concept ACK

DrahtBot added the label Needs rebase on Jun 21, 2020

DrahtBot commented at 10:25 AM on June 21, 2020: member

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

meshcollider removed the label Needs rebase on Jul 2, 2020

meshcollider commented at 12:16 AM on July 2, 2020: contributor

Rebased and review comments addressed

achow101 commented at 1:05 AM on July 2, 2020: member

ACK ed266f7ca52fd0df2dd2e25bb9fdf9e5c6888eb9

in src/test/script_standard_tests.cpp:355 in a4b9f20579 outdated

 355 | -    witnessScript << ToByteVector(pubkeys[0]) << OP_CHECKSIG;
 356 | +    // WitnessV0KeyHash
 357 |      expected.clear();
 358 |      expected << OP_0 << ToByteVector(pubkeys[0].GetID());
 359 | -    result = GetScriptForWitness(witnessScript);
 360 | +    result = GetScriptForDestination(WitnessV0KeyHash(Hash160(ToByteVector(pubkeys[0]))));

sipa commented at 1:09 AM on July 2, 2020:

This can be GetScriptForDestination(WitnessV0KeyHash(pubkeys[0])).

sipa commented at 11:10 PM on July 3, 2020:

This one isn't addressed yet.

in src/test/script_standard_tests.cpp:357 in a4b9f20579 outdated

 361 |      BOOST_CHECK(result == expected);
 362 | -
 363 | -    witnessScript.clear();
 364 | -    witnessScript << OP_DUP << OP_HASH160 << ToByteVector(pubkeys[0].GetID()) << OP_EQUALVERIFY << OP_CHECKSIG;
 365 | -    result = GetScriptForWitness(witnessScript);
 366 | +    result = GetScriptForDestination(WitnessV0KeyHash(pubkeys[0].GetID()));

sipa commented at 1:10 AM on July 2, 2020:

This can be GetScriptForDestination(WitnessV0KeyHash(pubkeys[0])).

achow101 commented at 2:45 AM on July 2, 2020:

This would make this test the same as the one above. I think this should remain GetID and the above can be the pubkey to test the different constructors.

in src/test/transaction_tests.cpp:502 in a4b9f20579 outdated

 498 | @@ -499,13 +499,13 @@ BOOST_AUTO_TEST_CASE(test_witness)
 499 |      BOOST_CHECK(keystore.AddCScript(scriptPubkey1L));
 500 |      BOOST_CHECK(keystore.AddCScript(scriptPubkey2L));
 501 |      BOOST_CHECK(keystore.AddCScript(scriptMulti));
 502 | -    BOOST_CHECK(keystore.AddCScript(GetScriptForWitness(scriptPubkey1)));
 503 | -    BOOST_CHECK(keystore.AddCScript(GetScriptForWitness(scriptPubkey2)));
 504 | -    BOOST_CHECK(keystore.AddCScript(GetScriptForWitness(scriptPubkey1L)));
 505 | -    BOOST_CHECK(keystore.AddCScript(GetScriptForWitness(scriptPubkey2L)));
 506 | -    BOOST_CHECK(keystore.AddCScript(GetScriptForWitness(scriptMulti)));
 507 | +    BOOST_CHECK(keystore.AddCScript(GetScriptForDestination(WitnessV0KeyHash(pubkey1.GetID()))));