[script] Unit tests for script/standard and IsMine functions. #11116
Conversation
| @@ -46,6 +46,8 @@ isminetype IsMine(const CKeyStore &keystore, const CTxDestination& dest, bool& i | |||
|
|
|||
| isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool& isInvalid, SigVersion sigversion) | |||
| { | |||
| isInvalid = false; | |||
There was a problem hiding this comment.
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.
src/test/script_standard_tests.cpp
Outdated
| keystore.AddKey(keys[0]); | ||
|
|
||
| CScript witnessScript; | ||
| witnessScript << OP_DUP << OP_HASH160 << ToByteVector(pubkeys[0].GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; |
There was a problem hiding this comment.
This variable is unused.
b84d27c to
e6eed35
Compare
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.) |
|
@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. |
|
Great okay! just making sure you weren't aware of any. |
|
@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). |
src/test/script_standard_tests.cpp
Outdated
| BOOST_CHECK(!isInvalid); | ||
|
|
||
| // Keystore has key | ||
| keystore.AddKeyPubKey(keys[0], uncompressedPubkey0); |
There was a problem hiding this comment.
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.
e6eed35 to
323dd0e
Compare
|
Thanks for the review @sipa. I had forgotten that CKey tracks the compressedness. Updated the test and removed the change to CBasicKeyStore. |
|
utACK |
src/test/script_standard_tests.cpp
Outdated
| BOOST_CHECK(solutions[4] == std::vector<unsigned char>({3})); | ||
|
|
||
| // TX_NULL_DATA | ||
| solutions.clear(); |
There was a problem hiding this comment.
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?
323dd0e to
184a07d
Compare
src/test/script_standard_tests.cpp
Outdated
| } | ||
|
|
||
| CScript expected, result; | ||
| CTxDestination dest; |
There was a problem hiding this comment.
I believe this is practically unused.
Does not test watch-only addresses.
184a07d to
7a1e873
Compare
…tions. 7a1e873 [script] Unit tests for IsMine (Jim Posen) d7afe2d [script] Unit tests for script/standard functions (Jim Posen) Pull request description: Simply adding unit test coverage. Tree-SHA512: aaf16b1b07b6d43c884a67f4fd5f83c31bf2c560f78798036d7aa37a3efe71a7ca3c82c4b3ba1f3119bcbe3b78013e64bb0020fe57ebc69aea1cb54943881959
|
|
||
| // TX_WITNESS_V0_SCRIPTHASH | ||
| s.clear(); | ||
| s << OP_0 << ToByteVector(CScriptID(redeemScript)); |
There was a problem hiding this comment.
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)
Github-Pull: bitcoin#11116 Rebased-From: d7afe2d
Does not test watch-only addresses. Github-Pull: bitcoin#11116 Rebased-From: 7a1e873
…ne functions. 7a1e873 [script] Unit tests for IsMine (Jim Posen) d7afe2d [script] Unit tests for script/standard functions (Jim Posen) Pull request description: Simply adding unit test coverage. Tree-SHA512: aaf16b1b07b6d43c884a67f4fd5f83c31bf2c560f78798036d7aa37a3efe71a7ca3c82c4b3ba1f3119bcbe3b78013e64bb0020fe57ebc69aea1cb54943881959
Bitcoin wallet PRs 3 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7687 - bitcoin/bitcoin#11116 - Excludes SegWit tests - Excludes `isInvalid` code (which is part of the SegWit support structure) - bitcoin/bitcoin#13002 - Excludes changes to `importwallet` (missing bitcoin/bitcoin#11667) - Excludes changes to `ProcessImport` (missing bitcoin/bitcoin#7551) - Third commit was rewritten to add an internal SigVersion argument (which we didn't have because it was added to support SegWit logic).
…ne functions. 7a1e873 [script] Unit tests for IsMine (Jim Posen) d7afe2d [script] Unit tests for script/standard functions (Jim Posen) Pull request description: Simply adding unit test coverage. Tree-SHA512: aaf16b1b07b6d43c884a67f4fd5f83c31bf2c560f78798036d7aa37a3efe71a7ca3c82c4b3ba1f3119bcbe3b78013e64bb0020fe57ebc69aea1cb54943881959
Simply adding unit test coverage.