Merge #18067: wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition

a304a3632f0437f4d0f04589a2200e2da91624a7 Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07e89133a5fb465ad1b793362e7439f7 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92ccb5bc10c8daa106d75e1c21390461d3 wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)

Pull request description:

  Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.

  The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e8460127a40a7895519587399feff3b682 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.

  This change adds a lot of comments and allows reverting commit 4a7e43e8460127a40a7895519587399feff3b682 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible

ACKs for top commit:
  Sjors:
    re-ACK a304a3632f0437f4d0f04589a2200e2da91624a7 (rebase, slight text changes and my test)
  achow101:
    re-ACK a304a3632f0437f4d0f04589a2200e2da91624a7
  meshcollider:
    utACK a304a3632f0437f4d0f04589a2200e2da91624a7

Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e

2 files changed, 59 insertions, 8 deletions

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 4c9d88973e..0c95ab29b1 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -70,7 +70,15 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const LegacyScriptPubKeyMan&
     return true;
 }
 
-IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion)
+//! Recursively solve script and return spendable/watchonly/invalid status.
+//!
+//! @param keystore            legacy key and script store
+//! @param script              script to solve
+//! @param sigversion          script type (top-level / redeemscript / witnessscript)
+//! @param recurse_scripthash  whether to recurse into nested p2sh and p2wsh
+//!                            scripts or simply treat any script that has been
+//!                            stored in the keystore as spendable
+IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true)
 {
     IsMineResult ret = IsMineResult::NO;
 
@@ -129,7 +137,7 @@ IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& s
         CScriptID scriptID = CScriptID(uint160(vSolutions[0]));
         CScript subscript;
         if (keystore.GetCScript(scriptID, subscript)) {
-            ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::P2SH));
+            ret = std::max(ret, recurse_scripthash ? IsMineInner(keystore, subscript, IsMineSigVersion::P2SH) : IsMineResult::SPENDABLE);
         }
         break;
     }
@@ -147,7 +155,7 @@ IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& s
         CScriptID scriptID = CScriptID(hash);
         CScript subscript;
         if (keystore.GetCScript(scriptID, subscript)) {
-            ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::WITNESS_V0));
+            ret = std::max(ret, recurse_scripthash ? IsMineInner(keystore, subscript, IsMineSigVersion::WITNESS_V0) : IsMineResult::SPENDABLE);
         }
         break;
     }
@@ -476,11 +484,11 @@ std::unique_ptr<SigningProvider> LegacyScriptPubKeyMan::GetSigningProvider(const
 
 bool LegacyScriptPubKeyMan::CanProvide(const CScript& script, SignatureData& sigdata)
 {
-    if (IsMine(script) != ISMINE_NO) {
-        // If it IsMine, we can always provide in some way
-        return true;
-    } else if (HaveCScript(CScriptID(script))) {
-        // We can still provide some stuff if we have the script, but IsMine failed because we don't have keys
+    IsMineResult ismine = IsMineInner(*this, script, IsMineSigVersion::TOP, /* recurse_scripthash= */ false);
+    if (ismine == IsMineResult::SPENDABLE || ismine == IsMineResult::WATCH_ONLY) {
+        // If ismine, it means we recognize keys or script ids in the script, or
+        // are watching the script itself, and we can at least provide metadata
+        // or solving information, even if not able to sign fully.
         return true;
     } else {
         // If, given the stuff in sigdata, we could make a valid sigature, then we can provide for this script
diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp
new file mode 100644
index 0000000000..757865ea37
--- /dev/null
+++ b/src/wallet/test/scriptpubkeyman_tests.cpp
@@ -0,0 +1,43 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <key.h>
+#include <script/standard.h>
+#include <test/util/setup_common.h>
+#include <wallet/scriptpubkeyman.h>
+#include <wallet/wallet.h>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(scriptpubkeyman_tests, BasicTestingSetup)
+
+// Test LegacyScriptPubKeyMan::CanProvide behavior, making sure it returns true
+// for recognized scripts even when keys may not be available for signing.
+BOOST_AUTO_TEST_CASE(CanProvide)
+{
+    // Set up wallet and keyman variables.
+    NodeContext node;
+    std::unique_ptr<interfaces::Chain> chain = interfaces::MakeChain(node);
+    CWallet wallet(chain.get(), WalletLocation(), WalletDatabase::CreateDummy());
+    LegacyScriptPubKeyMan& keyman = *wallet.GetOrCreateLegacyScriptPubKeyMan();
+
+    // Make a 1 of 2 multisig script
+    std::vector<CKey> keys(2);
+    std::vector<CPubKey> pubkeys;
+    for (CKey& key : keys) {
+        key.MakeNewKey(true);
+        pubkeys.emplace_back(key.GetPubKey());
+    }
+    CScript multisig_script = GetScriptForMultisig(1, pubkeys);
+    CScript p2sh_script = GetScriptForDestination(ScriptHash(multisig_script));
+    SignatureData data;
+
+    // Verify the p2sh(multisig) script is not recognized until the multisig
+    // script is added to the keystore to make it solvable
+    BOOST_CHECK(!keyman.CanProvide(p2sh_script, data));
+    keyman.AddCScript(multisig_script);
+    BOOST_CHECK(keyman.CanProvide(p2sh_script, data));
+}
+
+BOOST_AUTO_TEST_SUITE_END()