From 005f8a92ccb5bc10c8daa106d75e1c21390461d3 Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@yanofsky.org>
Date: Tue, 4 Feb 2020 17:24:48 -0500
Subject: wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition

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
https://github.com/bitcoin/bitcoin/pull/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
---
 src/script/signingprovider.h | 46 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

(limited to 'src/script/signingprovider.h')

diff --git a/src/script/signingprovider.h b/src/script/signingprovider.h
index 6ad20480a7..76f31d2f6f 100644
--- a/src/script/signingprovider.h
+++ b/src/script/signingprovider.h
@@ -66,7 +66,53 @@ protected:
     using KeyMap = std::map<CKeyID, CKey>;
     using ScriptMap = std::map<CScriptID, CScript>;
 
+    /**
+     * Map of key id to unencrypted private keys known by the signing provider.
+     * Map may be empty if the provider has another source of keys, like an
+     * encrypted store.
+     */
     KeyMap mapKeys GUARDED_BY(cs_KeyStore);
+
+    /**
+     * Map of script id to scripts known by the signing provider.
+     *
+     * This map originally just held P2SH redeemScripts, and was used by wallet
+     * code to look up script ids referenced in "OP_HASH160 <script id>
+     * OP_EQUAL" P2SH outputs. Later in 605e8473a7d it was extended to hold
+     * P2WSH witnessScripts as well, and used to look up nested scripts
+     * referenced in "OP_0 <script hash>" P2WSH outputs. Later in commits
+     * f4691ab3a9d and 248f3a76a82, it was extended once again to hold segwit
+     * "OP_0 <key or script hash>" scriptPubKeys, in order to give the wallet a
+     * way to distinguish between segwit outputs that it generated addresses for
+     * and wanted to receive payments from, and segwit outputs that it never
+     * generated addresses for, but it could spend just because of having keys.
+     * (Before segwit activation it was also important to not treat segwit
+     * outputs to arbitrary wallet keys as payments, because these could be
+     * spent by anyone without even needing to sign with the keys.)
+     *
+     * Some of the scripts stored in mapScripts are memory-only and
+     * intentionally not saved to disk. Specifically, scripts added by
+     * ImplicitlyLearnRelatedKeyScripts(pubkey) calls are not written to disk so
+     * future wallet code can have flexibility to be more selective about what
+     * transaction outputs it recognizes as payments, instead of having to treat
+     * all outputs spending to keys it knows as payments. By contrast,
+     * mapScripts entries added by AddCScript(script),
+     * LearnRelatedScripts(pubkey, type), and LearnAllRelatedScripts(pubkey)
+     * calls are saved because they are all intentionally used to receive
+     * payments.
+     *
+     * The FillableSigningProvider::mapScripts script map should not be confused
+     * with LegacyScriptPubKeyMan::setWatchOnly script set. The two collections
+     * can hold the same scripts, but they serve different purposes. The
+     * setWatchOnly script set is intended to expand the set of outputs the
+     * wallet considers payments. Every output with a script it contains is
+     * considered to belong to the wallet, regardless of whether the script is
+     * solvable or signable. By contrast, the scripts in mapScripts are only
+     * used for solving, and to restrict which outputs are considered payments
+     * by the wallet. An output with a script in mapScripts, unlike
+     * setWatchOnly, is not automatically considered to belong to the wallet if
+     * it can't be solved and signed for.
+     */
     ScriptMap mapScripts GUARDED_BY(cs_KeyStore);
 
     void ImplicitlyLearnRelatedKeyScripts(const CPubKey& pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);
-- 
cgit v1.2.3