test: add `ismine` test for descriptor ScriptPubKeyMan #25942

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:add_desc_ismine_test changing 1 files +281 −16

w0xlt commented at 6:14 AM on August 27, 2022: contributor

Currently src/wallet/test/ismine_tests.cpp has tests for the legacy ScriptPubKeyMan only. This PR adds tests for the descriptor ScriptPubKeyMan.
w0xlt force-pushed on Aug 27, 2022
DrahtBot added the label Tests on Aug 27, 2022
w0xlt force-pushed on Aug 27, 2022
in src/wallet/test/ismine_tests.cpp:32 in c51a5c044d outdated
```
  27 | +    if (success) {
  28 | +        BOOST_CHECK(parsed_desc);
  29 | +    } else {
  30 | +        BOOST_CHECK(!parsed_desc);
  31 | +        return nullptr;
  32 | +    }
```
furszy commented at 12:45 PM on August 27, 2022:

nit:

BOOST_CHECK(success && parsed_desc); if (!success) return nullptr;
w0xlt commented at 9:53 PM on August 27, 2022:

Done in 1b77db265317a6470d0914b520f04eb64b3c0942. But I changed it to BOOST_CHECK(success == (parsed_desc != nullptr)); since success can be false if Parse function is expected to fail.

in src/wallet/test/ismine_tests.cpp:44 in c51a5c044d outdated

  39 | +
  40 | +    auto spk_manager = keystore.AddWalletDescriptor(w_desc, keys, "", false);
  41 | +
  42 | +    BOOST_CHECK(spk_manager != nullptr);
  43 | +
  44 | +    return spk_manager;

furszy commented at 12:51 PM on August 27, 2022:

as this shouldn't fail, maybe could inline it:

return Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false));

w0xlt commented at 9:53 PM on August 27, 2022:

Done in 1b77db265317a6470d0914b520f04eb64b3c0942.

furszy commented at 1:00 PM on August 27, 2022: member

Concept ACK, good catch. Strange that this wasn't implemented before.
w0xlt force-pushed on Aug 27, 2022
w0xlt commented at 9:54 PM on August 27, 2022: contributor

#25942#pullrequestreview-1087678588 addressed in 1b77db265317a6470d0914b520f04eb64b3c0942.

Tests for combo() and tr() have also been added.
w0xlt force-pushed on Aug 27, 2022
test: add `ismine` test for descriptor scriptpubkeyman 1b77db2653
w0xlt force-pushed on Aug 27, 2022
in src/wallet/test/ismine_tests.cpp:234 in 1b77db2653
```
 230 | +        CWallet keystore(chain.get(), "", m_args, CreateDummyWalletDatabase());
 231 | +        std::string desc_str = "sh(sh(" + EncodeSecret(keys[0]) + "))";
 232 | +
 233 | +        auto spk_manager = CreateDescriptor(keystore, desc_str, false);
 234 | +        BOOST_CHECK_EQUAL(spk_manager, nullptr);
 235 | +    }
```
ishaanam commented at 6:22 PM on August 28, 2022:

Is ismine_tests.cpp the right place for this test? It doesn't seem to be testing IsMine. There are a few other places where it seems like IsMine isn't being tested, namely wherever the script is invalid and CreateDescriptor returns a nullptr.
w0xlt commented at 8:22 PM on August 28, 2022:

Right. This happens in the cases below:

P2PKH inside P2SH inside P2SH (invalid)

P2PKH inside P2SH inside P2WSH (invalid)

P2WPKH inside P2WSH (invalid)

P2PKH inside P2WSH inside P2WSH (invalid)

P2WPKH uncompressed (invalid)

P2WSH multisig with uncompressed key (invalid)

These cases cannot be reproduced for descriptor ScriptPubKeyMan, so I added them just for consistency and to make this difference explicit. I can remove them if the reviewers think this is not necessary.
ishaanam commented at 6:51 PM on August 28, 2022: contributor

Concept ACK
furszy approved
furszy commented at 1:14 PM on September 3, 2022: member

ACK 1b77db26 with a non-blocking comment.

Instead of continue expanding the long ismine_standard test case, what do you think about creating another test case ismine_standard_descriptors?
DrahtBot commented at 4:48 AM on September 23, 2022: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK furszy, ishaanam, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
ishaanam commented at 12:21 AM on November 30, 2022: contributor

ACK 1b77db265317a6470d0914b520f04eb64b3c0942
maflcko assigned achow101 on Nov 30, 2022
achow101 commented at 4:10 PM on November 30, 2022: member

ACK 1b77db265317a6470d0914b520f04eb64b3c0942
achow101 merged this on Nov 30, 2022
achow101 closed this on Nov 30, 2022
w0xlt deleted the branch on Dec 1, 2022
sidhujag referenced this in commit b5c59dee0f on Dec 1, 2022
bitcoin locked this on Dec 1, 2023

Type	Reviewers
ACK	furszy, ishaanam, achow101

Contributors

Labels

test: add `ismine` test for descriptor ScriptPubKeyMan #25942

Reviews

Conflicts