[script] Unit tests for script/standard and IsMine functions.

jimpo commented at 0:52 am on August 23, 2017: contributor

Simply adding unit test coverage.

in src/script/ismine.cpp:49 in 0d8d5d6fd4 outdated

45@@ -46,6 +46,8 @@ isminetype IsMine(const CKeyStore &keystore, const CTxDestination& dest, bool& i
46 
47 isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool& isInvalid, SigVersion sigversion)
48 {
49+    isInvalid = false;

promag commented at 4:32 am on August 23, 2017:

Remove.

jimpo commented at 4:53 pm on August 23, 2017:

Could you please explain why? Since isValid is a return parameter, I think it should get set to false from an initial value of true if the script is not invalid.

promag commented at 6:18 pm on September 8, 2017:

Ok, got it.

promag commented at 4:33 am on August 23, 2017: member

Remove periods from commit messages.

laanwj added the label Wallet on Aug 23, 2017

laanwj added the label Tests on Aug 23, 2017

laanwj removed the label Wallet on Aug 23, 2017

in src/test/script_standard_tests.cpp:514 in 0d8d5d6fd4 outdated

509+    {
510+        CBasicKeyStore keystore;
511+        keystore.AddKey(keys[0]);
512+
513+        CScript witnessScript;
514+        witnessScript << OP_DUP << OP_HASH160 << ToByteVector(pubkeys[0].GetID()) << OP_EQUALVERIFY << OP_CHECKSIG;

jonasschnelli commented at 10:51 am on August 23, 2017:

This variable is unused.

jimpo force-pushed on Aug 23, 2017

gmaxwell commented at 0:26 am on August 26, 2017: contributor

Without a6564cc, P2WPKH scripts where the pubkey is uncompressed are incorrectly

sounds correct to change; but how would a P2WPKH redeem script for such a key end up in the wallet in the first place? (trying to gauge severity; if it’s only accessible inside the code it’s good to fix but not e.g. something we need to worry about end users hitting.)

jimpo commented at 0:30 am on August 26, 2017: contributor

@gmaxwell I haven’t looked through the wallet code enough to know. I assume it would not happen, and it’s just a bug in some internals that wouldn’t be exercised, but I’m not 100% sure. I came across it through unit tests, not end-to-end testing of the wallet.

gmaxwell commented at 0:31 am on August 26, 2017: contributor

Great okay! just making sure you weren’t aware of any.

sipa commented at 7:52 pm on August 27, 2017: member

@jimpo If a6564cc has any effect, I think it’s a sign there is a more severe issue, namely that the CKeyID and the CKey in the CBasicKeyStore’s map are not in sync with each other (as the CKey stores the compressedness too). I’d rather investigate that discrepancy first.

EDIT: it seems it’s just the test code that’s wrong. You’re calling AddKeyPubKey with a CKey and CPubKey that don’t correspond (different compressedness).

in src/test/script_standard_tests.cpp:438 in e6eed352fb outdated

433+        result = IsMine(keystore, scriptPubKey, isInvalid);
434+        BOOST_CHECK_EQUAL(result, ISMINE_NO);
435+        BOOST_CHECK(!isInvalid);
436+
437+        // Keystore has key
438+        keystore.AddKeyPubKey(keys[0], uncompressedPubkey0);

sipa commented at 7:57 pm on August 27, 2017:

You’re adding a compressed CKey (keys[0]) with an uncompressed CPubKey, which is incorrect use of the function. You should first change the CKey’s compressedness.

Perhaps this warrants a comment on AddKeyPubKey or even an assert. It should alleviate the need for a6564cca7d443789671778f769a24bbb5a4a0652 though.

jimpo commented at 8:28 pm on August 27, 2017:

Thanks, will change.

jimpo force-pushed on Aug 28, 2017

jimpo commented at 4:41 pm on August 28, 2017: contributor

Thanks for the review @sipa. I had forgotten that CKey tracks the compressedness. Updated the test and removed the change to CBasicKeyStore.

gmaxwell commented at 6:44 pm on September 8, 2017: contributor

utACK

in src/test/script_standard_tests.cpp:86 in 323dd0e2b7 outdated

81+    BOOST_CHECK(solutions[2] == ToByteVector(pubkeys[1]));
82+    BOOST_CHECK(solutions[3] == ToByteVector(pubkeys[2]));
83+    BOOST_CHECK(solutions[4] == std::vector<unsigned char>({3}));
84+
85+    // TX_NULL_DATA
86+    solutions.clear();

MarcoFalke commented at 10:11 am on September 20, 2017:

why? It is the task of the solver to set the size of the vector to 0. Otherwise you could as well remove the BOOST_CHECK_EQUAL(solutions.size(), 0);, no?

jimpo commented at 6:09 pm on September 20, 2017:

Good point.

MarcoFalke commented at 10:12 am on September 20, 2017: member

utACK 323dd0e

jimpo force-pushed on Sep 20, 2017

in src/test/script_standard_tests.cpp:323 in 184a07d5b6 outdated

318+        keys[i].MakeNewKey(true);
319+        pubkeys[i] = keys[i].GetPubKey();
320+    }
321+
322+    CScript expected, result;
323+    CTxDestination dest;

TheBlueMatt commented at 11:32 pm on September 20, 2017:

I believe this is practically unused.

jimpo commented at 7:25 pm on September 21, 2017:

Thanks, removed.

TheBlueMatt commented at 1:00 am on September 21, 2017: member

utACK 184a07d5b6131c13238397db1b1e9a0a1c58c54f

[script] Unit tests for script/standard functions d7afe2d157

[script] Unit tests for IsMine

Does not test watch-only addresses.

7a1e873b27

jimpo force-pushed on Sep 21, 2017

laanwj merged this on Sep 21, 2017

laanwj closed this on Sep 21, 2017

laanwj referenced this in commit 49f3d57eeb on Sep 21, 2017

jimpo deleted the branch on Sep 21, 2017

in src/test/script_standard_tests.cpp:233 in 7a1e873b27

228+    s << OP_0 << ToByteVector(pubkey);
229+    BOOST_CHECK(!ExtractDestination(s, address));
230+
231+    // TX_WITNESS_V0_SCRIPTHASH
232+    s.clear();
233+    s << OP_0 << ToByteVector(CScriptID(redeemScript));

sipa commented at 10:11 pm on September 23, 2017:

Posthumous review (and fixed in #11167): this does not construct a valid TX_WITNESS_V0_SCRIPTHASH but a TX_WITNESS_V0_KEYHASH (as the pushed size is 20, not 32)

MarcoFalke referenced this in commit 315c716dd6 on Oct 3, 2017

MarcoFalke referenced this in commit 4199562271 on Oct 3, 2017

MarcoFalke referenced this in commit 5bb32e8359 on Oct 3, 2017

MarcoFalke referenced this in commit 79ccb2fbc1 on Oct 3, 2017

MarcoFalke referenced this in commit 794a80eee3 on Oct 3, 2017

MarcoFalke referenced this in commit 2c4ff35a8f on Oct 3, 2017

codablock referenced this in commit eb68d88b94 on Sep 24, 2019

zkbot referenced this in commit 900ed4555f on Dec 19, 2019

barrystyle referenced this in commit 1b69ccd92b on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

[script] Unit tests for script/standard and IsMine functions. #11116