wallet: Remove IsMine from migration code

achow101 commented at 4:40 pm on June 24, 2024: member

The legacy wallet IsMine code will be removed with the legacy wallet, but is still partially needed for migration. Instead of using IsMine directly in migration, equivalent checks are performed by migration.

Builds on #26596

DrahtBot commented at 4:40 pm on June 24, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30328.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31423 (wallet: migration, don’t create spendable wallet from a watch-only legacy wallet by furszy)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Jun 24, 2024

achow101 force-pushed on Jul 1, 2024

achow101 marked this as a draft on Jul 8, 2024

achow101 force-pushed on Jul 11, 2024

achow101 marked this as ready for review on Jul 11, 2024

achow101 commented at 7:30 pm on July 11, 2024: member

Ready for review now that #26596 has been merged.

in src/wallet/scriptpubkeyman.cpp:1728 in d5d994c02b outdated

1725-            // Add ScriptHash for scripts that are not already P2SH
1726-            if (!script.IsPayToScriptHash()) {
1727+        std::vector<std::vector<unsigned char>> sols;
1728+        TxoutType type = Solver(script, sols);
1729+        switch (type) {
1730+        // We don't care aboue these types because they are not spendable

brunoerg commented at 6:01 pm on July 18, 2024:

0        // We don't care about these types because they are not spendable

achow101 commented at 11:17 am on October 16, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1861 in 905e22b469 outdated

1868+            return true;
1869+        }
1870+        case TxoutType::MULTISIG:
1871+        {
1872+            if (ctx == ScriptContext::P2WSH) {
1873+                std::vector<valtype> keys(sols.begin() + 1, sols.begin() + sols.size() - 1);

brunoerg commented at 8:41 pm on July 18, 2024:

In 905e22b469a1e09df9ff0e98bf989a55642f301e “wallet: Remove IsMine from migration”: Just a question, but could it be a vector of CPubKey then use IsCompressed?

achow101 commented at 11:16 am on October 16, 2024:

HaveKeys uses std::vector<valtype>

in src/wallet/scriptpubkeyman.cpp:1725 in 905e22b469 outdated

1720@@ -1721,37 +1721,157 @@ std::unordered_set<CScript, SaltedSipHasher> LegacyDataSPKM::GetScriptPubKeys()
1721     // For all keys, if they have segwit scripts, those scripts will end up in mapScripts
1722     for (const auto& script_pair : mapScripts) {
1723         const CScript& script = script_pair.second;
1724-        if (IsMine(script) == ISMINE_SPENDABLE) {

brunoerg commented at 9:09 pm on July 18, 2024:

In 905e22b469a1e09df9ff0e98bf989a55642f301e “wallet: Remove IsMine from migration”: Maybe it’s worth updating the documentation?

0// For every script in mapScript, only the ISMINE_SPENDABLE ones are being tracked.

achow101 commented at 11:17 am on October 16, 2024:

Updated the comment.

DrahtBot added the label CI failed on Jul 22, 2024

DrahtBot commented at 11:32 am on July 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27341718423

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Jul 22, 2024

in src/wallet/scriptpubkeyman.cpp:1794 in 905e22b469 outdated

1802+        const CScript& script = script_pair.second;
1803+        std::vector<std::vector<unsigned char>> sols;
1804+        TxoutType type = Solver(script, sols);
1805+        if (type == TxoutType::WITNESS_V0_SCRIPTHASH) {
1806+            uint160 hash;
1807+            CRIPEMD160().Write(sols[0].data(), sols[0].size()).Finalize(hash.begin());

theStack commented at 10:37 am on July 24, 2024:

nit: can use the RIPEMD160 helper here

0            uint160 hash{RIPEMD160(sols[0])};

achow101 commented at 11:22 am on October 16, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1778 in 905e22b469 outdated

1786+                    if (key.size() != 33) {
1787+                        allowed = false;
1788+                        break;
1789+                    }
1790+                }
1791+                if (allowed) {

theStack commented at 10:40 am on July 24, 2024:

nit: to deduplicate code, could introduce an all_keys_compressed helper that is used here and below in the is_valid_script lambda, e.g. something like:

0    const auto& all_keys_compressed = [](const std::vector<valtype>& keys) -> bool {
1        return std::all_of(keys.cbegin(), keys.cend(),
2               [](const auto& key) { return key.size() == 33; });
3    };

achow101 commented at 11:22 am on October 16, 2024:

Done

theStack commented at 10:42 am on July 24, 2024: contributor

Concept ACK, left two code-deduplication nits below

hodlinator commented at 12:21 pm on July 26, 2024: contributor

Commit message in d5d994c02bb54db395da457724ec45539f1c10a8 incorrectly states: “This reverts commit bbb1d51e1240da61db2ca1036f9ec91fd2f36f2d.”

I believe that commit ended up being merged as b231f4d556876ae70305e8710e31d53525ded8ae.

DrahtBot added the label CI failed on Aug 3, 2024

DrahtBot commented at 3:00 pm on August 3, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27341718760

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

achow101 force-pushed on Sep 3, 2024

achow101 force-pushed on Sep 10, 2024

DrahtBot removed the label CI failed on Sep 11, 2024

hodlinator commented at 9:22 am on September 12, 2024: contributor

#30328#pullrequestreview-2201704324 still applies to 705d9eb0aef8831891e1cce80c33615440547e90 instead of d5d994c02bb54db395da457724ec45539f1c10a8.

(Clarified linked comment a bit).

achow101 force-pushed on Oct 16, 2024

laanwj requested review from laanwj on Oct 24, 2024

in src/wallet/scriptpubkeyman.cpp:1735 in 4a1009f90e outdated

1730     for (const auto& script_pair : mapScripts) {
1731         const CScript& script = script_pair.second;
1732-        if (IsMine(script) == ISMINE_SPENDABLE) {
1733-            // Add ScriptHash for scripts that are not already P2SH
1734-            if (!script.IsPayToScriptHash()) {
1735+        std::vector<std::vector<unsigned char>> sols;

murchandamus commented at 4:23 pm on November 7, 2024:

What does “sols” stand for beside solar days on Mars? Is this intended to refer to “solvers”? If so, abbreviating that just seems unnecessarily confusing.

0        std::vector<std::vector<unsigned char>> solvers;

achow101 commented at 10:52 pm on November 15, 2024:

solutions

rkrux commented at 11:04 am on November 18, 2024:

0- std::vector<std::vector<unsigned char>> sols;
1+ std::vector<std::vector<unsigned char>> solutions;

Agree, would be easier to read.

achow101 commented at 5:32 pm on November 19, 2024:

Renamed

in src/wallet/scriptpubkeyman.cpp:1734 in 4a1009f90e outdated

1726+    // Insert every P2PK, P2PKH, and P2WPKH script. Multisigs are only included if we know
1727+    // all of the keys (matches legacy wallet IsMine behavior).
1728     // The watchonly ones will be in setWatchOnly which we deal with later
1729     // For all keys, if they have segwit scripts, those scripts will end up in mapScripts
1730     for (const auto& script_pair : mapScripts) {
1731         const CScript& script = script_pair.second;

furszy commented at 6:55 pm on November 13, 2024:

nit: if you move this to use an structured binding, could use the script id directly instead of calculating it on every ´ScriptHash(script)´ call.

e.g.

0for (const auto& [id, script] : mapScripts) {
1   // ... stuff ...
2   spks.insert(GetScriptForDestination(ScriptHash(id))); 
3  // ^^ this does not perform the extra Hash160(script) call.
4}

achow101 commented at 2:16 am on November 16, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1741 in 4a1009f90e outdated

1740+        case TxoutType::NULL_DATA:
1741+        case TxoutType::WITNESS_UNKNOWN:
1742+        case TxoutType::WITNESS_V1_TAPROOT:
1743+        case TxoutType::SCRIPTHASH:
1744+        case TxoutType::ANCHOR:
1745+        // These are only spendable if the witness scripts is also spendable as a scriptPubKey

murchandamus commented at 8:40 pm on November 13, 2024:

0        // These are only spendable if the witness script is also spendable as a scriptPubKey

achow101 commented at 2:17 am on November 16, 2024:

This comment has been reworded

in src/wallet/scriptpubkeyman.cpp:1769 in 4a1009f90e outdated

1762+        {
1763+            CKeyID key_id{uint160(sols[0])};
1764+            CPubKey pubkey;
1765+            if (GetPubKey(key_id, pubkey) && pubkey.IsCompressed() && HaveKey(key_id)) {
1766                 spks.insert(script);
1767+                spks.insert(GetScriptForDestination(ScriptHash(script)));

murchandamus commented at 9:40 pm on November 13, 2024:

Could we maybe give a few pointers what’s going on here?

0            if (GetPubKey(key_id, pubkey) && pubkey.IsCompressed() && HaveKey(key_id)) {
1                // Insert the output script for a P2WPKH output
2                spks.insert(script);
3                // Insert the output script for a P2SH-P2WPKH output
4                spks.insert(GetScriptForDestination(ScriptHash(script)));

Again, mostly guessing. :shrug:

achow101 commented at 2:17 am on November 16, 2024:

I’ve added some additional comments.

in src/wallet/scriptpubkeyman.cpp:1765 in 4a1009f90e outdated

1777-                    spks.insert(ms_spk);
1778+            break;
1779+        }
1780+        case TxoutType::MULTISIG:
1781+        {
1782+            // Multisigs are only spendable if we have all of their keys

murchandamus commented at 9:43 pm on November 13, 2024:

Are we talking about private keys here? Or public keys? E.g. clearly we would need all three public keys, but only two private keys to spend a 2-of-3 bare multisig.

This is talking about bare multisig, right? Would something like this make sense?

0            // Bare multisig outputs are only spendable if we have [public|private|public and private] keys that appear in the output script

achow101 commented at 10:55 pm on November 15, 2024:

Generally “keys” refers to private keys and public keys are typically specified as such or as “pubkey”.

In this instance, multisigs are only spendable if we have all of their private keys, regardless of the context the multisig appears in.

achow101 commented at 2:17 am on November 16, 2024:

I’ve reworded this comment.

achow101 force-pushed on Nov 14, 2024

achow101 commented at 9:16 pm on November 14, 2024: member

Commit message in d5d994c incorrectly states: “This reverts commit bbb1d51.”

I believe that commit ended up being merged as b231f4d.

Indeed, fixed.

in src/wallet/scriptpubkeyman.cpp:1791 in 4a1009f90e outdated

1803+    for (const auto& script_pair : mapScripts) {
1804+        const CScript& script = script_pair.second;
1805+        std::vector<std::vector<unsigned char>> sols;
1806+        TxoutType type = Solver(script, sols);
1807+        if (type == TxoutType::WITNESS_V0_SCRIPTHASH) {
1808+            uint160 hash{RIPEMD160(sols[0])};

furszy commented at 4:48 pm on November 15, 2024:

Note for reviewers: This is because we store scripts (witness programs, witness scripts and redeem scripts) in a map whose key is a CScriptID -> which is a Hash160 of the script -> which is a RIPEMD160(SHA256(script)). As P2WSH are in the form of OP_0 <SHA256(witness_script)>, the SHA256 part is already there and we only need to perform the RIPEMD160 calculation to obtain the CScriptID.

murchandamus commented at 7:04 pm on November 15, 2024:

Could this please be documented in the code?

achow101 commented at 2:16 am on November 16, 2024:

I’ve added a comment.

in src/wallet/scriptpubkeyman.cpp:1801 in 4a1009f90e outdated

1813+                !witness_script.IsPayToScriptHash() &&
1814+                !witness_script.IsWitnessProgram(wit_ver, wit_prog) &&
1815+                spks.count(witness_script) > 0) {
1816+                spks.insert(script);
1817+                spks.insert(GetScriptForDestination(ScriptHash(script)));
1818+            }

furszy commented at 5:47 pm on November 15, 2024:

Is this being tested? It seems we have P2WSH(multisig) tests inside ismine_tests.cpp and wallet_migration.py but these type of script should be covered inside the first loop already. Because if the wallet owns the P2WSH, it will also contain the inner multisig script, which will be handled by the first loop.

achow101 commented at 2:16 am on November 16, 2024:

I’ve added additional tests to wallet_migration.py.

furszy commented at 5:47 pm on November 15, 2024: member

Half way through it. Small comments so far.

in src/wallet/scriptpubkeyman.cpp:1797 in f01e56aaf4 outdated

1809+            CScript witness_script;
1810+            int wit_ver = -1;
1811+            std::vector<unsigned char> wit_prog;
1812+            if (GetCScript(CScriptID(hash), witness_script) &&
1813+                !witness_script.IsPayToScriptHash() &&
1814+                !witness_script.IsWitnessProgram(wit_ver, wit_prog) &&

furszy commented at 6:13 pm on November 15, 2024:

How can we end up with a P2SH or a witness program inside the witness script?

achow101 commented at 10:46 pm on November 15, 2024:

Using importaddress, you can import arbitrary scripts that can appear as output scripts. It is thus possible to import a P2WSH script which contains the hash of a P2SH script or a witness program. The user can then use importaddress again to import those particular scripts.

Most of the edge cases revolve around the user doing something insane, but because they were allowed, we have to handle them.

in src/wallet/scriptpubkeyman.cpp:1741 in 4a1009f90e outdated

1716@@ -1717,42 +1717,160 @@ std::unordered_set<CScript, SaltedSipHasher> LegacyDataSPKM::GetScriptPubKeys()
1717         spks.insert(GetScriptForDestination(PKHash(pub)));
1718     }
1719 
1720-    // For every script in mapScript, only the ISMINE_SPENDABLE ones are being tracked.
1721+    const auto& all_keys_compressed = [](const std::vector<valtype>& keys) -> bool {
1722+        return std::all_of(keys.cbegin(), keys.cend(),
1723+               [](const auto& key) { return key.size() == 33; });
1724+    };

murchandamus commented at 6:31 pm on November 15, 2024:

I was first confused by the snake_case apparently being used as a function below. A comment would have helped me figure this out a bit faster. Maybe something in the vein of:

0    // Assigns an unnamed lambda function which checks that all keys are compressed
1    const auto& func_all_keys_compressed = [](const std::vector<valtype>& keys) -> bool {
2        return std::all_of(keys.cbegin(), keys.cend(),
3               [](const auto& key) { return key.size() == 33; });
4    };

achow101 commented at 2:17 am on November 16, 2024:

Added a comment

in src/wallet/scriptpubkeyman.cpp:1725 in 4a1009f90e outdated

1721+    const auto& all_keys_compressed = [](const std::vector<valtype>& keys) -> bool {
1722+        return std::all_of(keys.cbegin(), keys.cend(),
1723+               [](const auto& key) { return key.size() == 33; });
1724+    };
1725+
1726+    // Insert every P2PK, P2PKH, and P2WPKH script. Multisigs are only included if we know

murchandamus commented at 6:33 pm on November 15, 2024:

Could this comment please be more specific? What sorts of scripts are being stored in this map? Input scripts, output scripts, redeemscripts, witness programs, witness scripts, all of the above, a subset of the above, etc.?

achow101 commented at 10:57 pm on November 15, 2024:

All of the above. All types of scripts are stored in mapScripts, without contexts. Thus all scripts in mapScripts can appear in any context. That is the insanity of legacy IsMine.

achow101 commented at 2:17 am on November 16, 2024:

Reworded the comment.

in src/wallet/scriptpubkeyman.cpp:1738 in 4a1009f90e outdated

1737+        switch (type) {
1738+        // We don't care about these types because they are not spendable
1739+        case TxoutType::NONSTANDARD:
1740+        case TxoutType::NULL_DATA:
1741+        case TxoutType::WITNESS_UNKNOWN:
1742+        case TxoutType::WITNESS_V1_TAPROOT:

murchandamus commented at 6:36 pm on November 15, 2024:

0        // The legacy wallet never had support for P2TR, therefore it’s among the unspendable scripts here
1        case TxoutType::WITNESS_V1_TAPROOT:

achow101 commented at 2:18 am on November 16, 2024:

Added a comment

in src/wallet/scriptpubkeyman.cpp:1742 in 4a1009f90e outdated

1741+        case TxoutType::WITNESS_UNKNOWN:
1742+        case TxoutType::WITNESS_V1_TAPROOT:
1743+        case TxoutType::SCRIPTHASH:
1744+        case TxoutType::ANCHOR:
1745+        // These are only spendable if the witness scripts is also spendable as a scriptPubKey
1746+        // We will check these later after spks has been updated with spks from scripts.

murchandamus commented at 6:37 pm on November 15, 2024:

This comment is confusing, because once spks refers to the variable, and once “spks” seems to refer to output scripts. If I understand this right:

0        // We will check these later after `spks` has been updated with scriptPubKeys from the processed scripts.

achow101 commented at 2:18 am on November 16, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1746 in 4a1009f90e outdated

1745+        // These are only spendable if the witness scripts is also spendable as a scriptPubKey
1746+        // We will check these later after spks has been updated with spks from scripts.
1747+        case TxoutType::WITNESS_V0_SCRIPTHASH:
1748+            break;
1749+        // These scriptPubKeys have already been handled by dealing with the keys
1750+        // However if these scripts are here, then the P2SH nested spk will be spendable if these are also scriptPubKeys

murchandamus commented at 6:49 pm on November 15, 2024:

These two lines of comments are hard to understand. What is a “P2SH nested spk”? It would make it easier to understand if references to variables and references to abstract concepts were clearly differentiated.

After staring at this for a while, is the following roughly what is meant here?

0        // P2PK and P2PKH output scripts can already be handled after the the wallet’s keys have been processed above.
1        // If we encounter any output scripts that match the P2PKH output script pattern, these are here to handle P2SH-P2WPKH output scripts

Otherwise, please consider my comment evidence that it was hard to understand what is going on here.

achow101 commented at 11:07 pm on November 15, 2024:

If a P2PK or P2PKH script exists in mapScripts, then the corresponding P2SH-P2PK and P2SH-P2PKH output script is spendable.

achow101 commented at 2:18 am on November 16, 2024:

Reworded the comment.

in src/wallet/scriptpubkeyman.cpp:1772 in 4a1009f90e outdated

1784+            if (!HaveKeys(keys, *this)) {
1785+                break;
1786+            }
1787+            // Multisigs are always spendable inside of P2SH scripts
1788+            spks.insert(GetScriptForDestination(ScriptHash(script)));
1789+            // We need to have the P2WSH script for the P2WSH to be spendable.

murchandamus commented at 6:56 pm on November 15, 2024:

Did you mean?

0            // We need to have the witness script to consider the P2WSH output script to be spendable.

achow101 commented at 2:18 am on November 16, 2024:

Reworded the comment.

in src/wallet/scriptpubkeyman.cpp:1801 in 4a1009f90e outdated

1818+            }
1819         }
1820     }
1821 
1822+    enum class ScriptContext {
1823+        TOP,

murchandamus commented at 7:05 pm on November 15, 2024:

What is a “TOP” ScriptContext?

achow101 commented at 11:08 pm on November 15, 2024:

Output script level.

in src/wallet/scriptpubkeyman.cpp:1826 in 4a1009f90e outdated

1842+        case TxoutType::WITNESS_V0_KEYHASH:
1843+            return ctx != ScriptContext::P2WSH;
1844+        case TxoutType::PUBKEYHASH:
1845+            if (ctx == ScriptContext::P2WSH) {
1846+                CPubKey pubkey;
1847+                if (GetPubKey(CKeyID(uint160(sols[0])), pubkey) && !pubkey.IsCompressed()) {

murchandamus commented at 7:08 pm on November 15, 2024:

What is our expectation to be happening when uint160(sols[0]) is called here? Could we maybe pull apart these three conversations in a conditional statement into two lines where the first line provides some insight on what we are converting here?

Would I be right in guessing that a CKeyID is the hash of a public key?

achow101 commented at 11:11 pm on November 15, 2024:

It’s basically just casting types.

sols[0] contains a vector of bytes which represents the hash of a pubkey. This is cast to the uint160 type, which is then cast to the CKeyID type (which is really just a wrapper around uint160) which is the type that GetPubKey can take. There is no direct conversion from std::vector<> to CKeyID hence the uint160 in between.

It doesn’t make sense to pull this apart since the intermediate types are never used.

in src/wallet/scriptpubkeyman.cpp:1852 in 4a1009f90e outdated

1860+            return true;
1861+        }
1862+        case TxoutType::WITNESS_V0_SCRIPTHASH:
1863+        {
1864+            if (ctx == ScriptContext::P2WSH) return false;
1865+            CScriptID script_id{RIPEMD160(sols[0])};

murchandamus commented at 7:13 pm on November 15, 2024:

Wouldn’t all of this be way easier and more readable, if we had explicit types for our various output script components instead of treating them as if they were just numbers and strings?

achow101 commented at 0:24 am on November 16, 2024:

It is possible that the solver could be rewritten to pull out all of the components of scripts into structs with proper typing and member names, but that is a much larger refactor that is out of scope for this PR.

murchandamus commented at 7:15 pm on November 15, 2024: contributor

This section of the code is fairly arcane and hard to read. It would help me (and probably other reviewers) if there were more commentary on why things are being done and what is actually happening, especially because a lot of the involved variables are never explicitly denominated.

furszy commented at 7:24 pm on November 15, 2024: member

There is a bug in the second loop. Before adding the P2WSH program to the spks set, it is necessary to verify that the underlying witness script does not contain any uncompressed keys, as these are prohibited under segwit rules. Crafted a test exercising the behavior https://github.com/furszy/bitcoin-core/commit/3a0d127343320cc00f13cef96fad7f3b1bd3335a.

achow101 force-pushed on Nov 16, 2024

achow101 commented at 2:19 am on November 16, 2024: member

There is a bug in the second loop. Before adding the P2WSH program to the spks set, it is necessary to verify that the underlying witness script does not contain any uncompressed keys, as these are prohibited under segwit rules.

I’ve added a test to wallet_migration.py so that it will persist once legacy wallets are removed.

in src/wallet/scriptpubkeyman.cpp:1889 in fa155991ff outdated

1904+                !witness_script.IsWitnessProgram(wit_ver, wit_prog) && // Witness programs are not allowed inside of P2WSH
1905+                // We only allow scripts that we would consider spendable as an output script.
1906+                // Note that while this would exclude P2WSH-multisigs, we are already handling those in the first loop.
1907+                spks.count(witness_script) > 0 &&
1908+                // Pubkeys must be compressed.
1909+                is_valid_script(script, ScriptContext::P2WSH)) {

theStack commented at 9:12 pm on November 17, 2024:

This call tests for P2WSH inside P2WSH and always returns false, hence the whole if condition is not fulfilled and the two lines below will never be executed (can be verified easily by putting an assert(false) in the body and running unit and wallet functional tests – they still pass without crash). I assume this was meant to be

0                is_valid_script(witness_script, ScriptContext::P2WSH)) {

It’s surprising though that the tests still passed. Either the tests are insufficient, or we really don’t need this “Iterate again for all the P2WSH scripts” loop at all. :thinking:

achow101 commented at 5:31 pm on November 19, 2024:

Hmm, the test that is supposed to fail here is passing because the P2WSH scriptPubKey also ends up in the setWatchOnly so it is added when we iterate those.

I think if the wallet was used normally, this loop probably isn’t necessary. However, I am hesitant to remove it as in theory, if a P2WSH were in mapScripts and not in setWatchOnly, we would miss it but IsMine() would’ve still allowed it. I think the only way to get that though is through direct modification of the wallet file or with importwallet shenanigans.

Fixed it, but will continue thinking on a test.

achow101 commented at 11:26 pm on November 20, 2024:

I was able to contrive a test that requires this loop, which also revealed a bug.

The situation is if a sh(wsh(pkh())) is imported with importmulti, the wsh(pkh()) will only be in mapScripts. However, this wsh(pkh()) is still ISMINE_SPENDABLE and coins sent to it will be seen by the wallet. This loop is where that would be detected.

in src/wallet/scriptpubkeyman.cpp:1720 in fa155991ff outdated

1716@@ -1717,42 +1717,187 @@ std::unordered_set<CScript, SaltedSipHasher> LegacyDataSPKM::GetScriptPubKeys()
1717         spks.insert(GetScriptForDestination(PKHash(pub)));
1718     }
1719 
1720-    // For every script in mapScript, only the ISMINE_SPENDABLE ones are being tracked.
1721-    // The watchonly ones will be in setWatchOnly which we deal with later
1722-    // For all keys, if they have segwit scripts, those scripts will end up in mapScripts
1723+    // Lamba helper to check that all keys found by the solver are compressed

rkrux commented at 10:03 am on November 18, 2024:

Lambda

achow101 commented at 5:31 pm on November 19, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1806 in fa155991ff outdated

1821+        std::vector<valtype> sols;
1822+        TxoutType spk_type = Solver(script, sols);
1823+
1824+        CKeyID keyID;
1825+        switch (spk_type) {
1826+        // Scripts with no nesting (arbitraty, unknown scripts) are always valid.

rkrux commented at 10:15 am on November 18, 2024:

arbitrary

achow101 commented at 5:32 pm on November 19, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1741 in fa155991ff outdated

1809+            break;
1810+        }
1811         }
1812     }
1813 
1814+    enum class ScriptContext {

rkrux commented at 10:18 am on November 18, 2024:

We can’t reuse IsMineSigVersion because it will be removed soon? Can we add few comments here like done in IsMineSigVersion? https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/scriptpubkeyman.cpp#L56-L67

achow101 commented at 5:32 pm on November 19, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1723 in fa155991ff outdated

1721-    // The watchonly ones will be in setWatchOnly which we deal with later
1722-    // For all keys, if they have segwit scripts, those scripts will end up in mapScripts
1723+    // Lamba helper to check that all keys found by the solver are compressed
1724+    const auto& all_keys_compressed = [](const std::vector<valtype>& keys) -> bool {
1725+        return std::all_of(keys.cbegin(), keys.cend(),
1726+               [](const auto& key) { return key.size() == 33; });

rkrux commented at 11:01 am on November 18, 2024:

0- { return key.size() == 33; });
1+ { return key.size() == CPubKey::COMPRESSED_SIZE; });

There’s another usage down below as well on line 1816: https://github.com/bitcoin/bitcoin/commit/fa155991ff30bc001229e1d71bf44e8782f02d17#diff-55ce403d9c3440b4a38a261b7d452f84dd9a1d09b2a5ef1b4d3ac0c3567b9106R1816

rkrux commented at 11:45 am on November 18, 2024:

There must be a reason for making IsCompressedPublicKey() here static, which I dont know about, otherwise, it could be used within all_of here, thereby getting rid of some logic duplication: https://github.com/bitcoin/bitcoin/blob/28.x/src/script/interpreter.cpp#L85

achow101 commented at 5:32 pm on November 19, 2024:

Done

rkrux commented at 12:05 pm on November 18, 2024: none

Preliminary review fa155991ff30bc001229e1d71bf44e8782f02d17

I feel e6342ffdf0030f6350a07f325ce603fddfff1bd8 commit could be a separate PR, these functional tests are not dependent on the changes in the next two commits and don’t need to be tied to these changes. I see the comment history now.

achow101 force-pushed on Nov 19, 2024

achow101 force-pushed on Nov 20, 2024

in src/wallet/scriptpubkeyman.cpp:1732 in 97893b124a outdated

1735+    // as redeemScripts and witnessScripts.
1736+    //
1737+    // mapScripts contains redeemScripts and witnessScripts. It may also contain output scripts which,
1738+    // in addition to being treated as output scripts, are also treated as redeemScripts and witnessScripts.
1739+    // All scripts in mapScripts are treated as redeemScripts, unless that script is also a P2SH.
1740+    // A script is only treated as a witnessScript if there its corresponding P2WSH output script is in the map.

laanwj commented at 2:57 pm on November 21, 2024:

comment: -there

achow101 commented at 7:13 pm on December 5, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1778 in 97893b124a outdated

1794+        }
1795+        // Multisig scripts are spendable if they are inside of a P2SH or P2WSH, and we have all of the private keys.
1796+        // Bare multisigs are never considered spendable
1797+        case TxoutType::MULTISIG:
1798+        {
1799+            std::vector<std::vector<unsigned char>> keys(solutions.begin() + 1, solutions.begin() + solutions.size() - 1);

laanwj commented at 3:17 pm on November 21, 2024:

Maybe add an assertion on solutions.size() >= 2,as an internal check against future bugs in Solve (and maybe also in other places before the solutions vector is indexed).

achow101 commented at 7:13 pm on December 5, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1754 in 97893b124a outdated

1757+        case TxoutType::SCRIPTHASH:
1758+            break;
1759+        // Any P2PK or P2PKH scripts found in mapScripts can be spent as P2SH-P2PK or P2SH-P2PKH respectively,
1760+        // if we have the private key.
1761+        // Since all private keys were iterated earlier and their corresponding P2PK and P2PKH scripts inserted
1762+        // to "spks", we can simply check whether this P2PK or P2PKH script is in "spk" to determine spendability.

laanwj commented at 10:54 am on November 22, 2024:

comment: in “spks”

achow101 commented at 7:13 pm on December 5, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1757 in 97893b124a outdated

1760+        // if we have the private key.
1761+        // Since all private keys were iterated earlier and their corresponding P2PK and P2PKH scripts inserted
1762+        // to "spks", we can simply check whether this P2PK or P2PKH script is in "spk" to determine spendability.
1763+        case TxoutType::PUBKEY:
1764+        case TxoutType::PUBKEYHASH:
1765+            if (spks.count(script) > 0) {

laanwj commented at 10:58 am on November 22, 2024:

Doing both inserting into and reading from spks here makes it somewhat harder to be confident that the behavior is independent from the specific order of processing mapScripts, but the comment above helps.

laanwj approved

laanwj commented at 11:18 am on November 22, 2024: member

Must admit that i’m not versed enough in the subtle specifics with regard to validity and spendability of scripts, to be sure all of this is correct. But where i’ve checked, the behavior matches the IsMine-based implementation. Overall changes LGTM.

in src/wallet/scriptpubkeyman.cpp:1890 in a5a81cf7bc outdated

1907+            std::vector<unsigned char> wit_prog;
1908+            CScriptID script_id{RIPEMD160(solutions[0])};
1909+            // For a P2WSH output script to be spendable, we must know and inspect its witnessScript.
1910+            if (GetCScript(script_id, witness_script) &&
1911+                !witness_script.IsPayToScriptHash() && // P2SH inside of P2WSH is not allowed
1912+                !witness_script.IsWitnessProgram(wit_ver, wit_prog) && // Witness programs are not allowed inside of P2WSH

theStack commented at 4:44 pm on November 25, 2024:

It seems that these conditions are not needed, as the checks are done in is_valid_script below anyway (see Solver cases TxoutType::SCRIPTHASH and TxoutType::WITNESS_V...)?

Tests still pass without them.

achow101 commented at 7:13 pm on December 5, 2024:

Indeed they are redundant, removed.

furszy commented at 6:08 pm on November 27, 2024: member

Found a few ways to crash migration while reviewing these PR that also occur in master: #31374, #31378. I think we should focus on those before moving forward with this final IsMine killing refactoring. They also include further test coverage that would be nice to have here.

achow101 force-pushed on Dec 5, 2024

in test/functional/wallet_migration.py:1018 in 82d974323c outdated

1013@@ -1012,6 +1014,140 @@ def check_comments():
1014 
1015         wallet.unloadwallet()
1016 
1017+    def test_p2wsh(self):
1018+        self.log.info("Test that P2WSH output scripts are migrated")

murchandamus commented at 8:04 pm on December 6, 2024:

Maybe: “test that non-multisig P2WSH output scripts are migrated”

achow101 commented at 5:39 pm on December 7, 2024:

Done

in test/functional/wallet_migration.py:1036 in 82d974323c outdated

1031+
1032+        def_wallet.sendtoaddress(wsh_pkh_addr, 5)
1033+        self.generate(self.nodes[0], 6)
1034+        assert_equal(wallet.getbalances()['mine']['trusted'], 5)
1035+
1036+        wallet.migratewallet()

murchandamus commented at 8:07 pm on December 6, 2024:

Is there any way that you convince the test reader some more here that something happened with the wallet that might cause doing the same thing below is actually not just a no-op?

achow101 commented at 4:35 pm on December 7, 2024:

We generally assume that test readers have a basic understanding of how RPCs work, such as that unless stated otherwise in their help docs, all RPCs throw when they fail.

achow101 commented at 5:41 pm on December 7, 2024:

Since this PR needed to be rebased for #31248 which added a migrate_and_get_rpc helper function that replaced all migratewallet calls, I’ve added a check to that function to verify that the wallet is legacy before migration, and descriptors after.

in test/functional/wallet_migration.py:1186 in 82d974323c outdated

1122+        assert "embedded" in comp_wsh_pkh_addr_info
1123+
1124+        for addr in invalid_addrs:
1125+            addr_info = wallet.getaddressinfo(addr)
1126+            assert_equal(addr_info["ismine"], False)
1127+            assert_equal(addr_info["iswatchonly"], False)

murchandamus commented at 8:21 pm on December 6, 2024:

You could clarify the role of “embedded” here. My understanding is that the presence of it indicates that the imported data is present, which qualifies the two false results above.

achow101 commented at 5:41 pm on December 7, 2024:

Added addtional comments.

in test/functional/wallet_migration.py:1212 in 82d974323c outdated

1143+
1144+        for addr in invalid_addrs:
1145+            addr_info = wallet.getaddressinfo(addr)
1146+            assert_equal(addr_info["ismine"], False)
1147+            assert_equal(addr_info["iswatchonly"], False)
1148+            assert "embedded" not in addr_info

murchandamus commented at 8:24 pm on December 6, 2024:

Please highlight that only this line differs and what that indicates.

achow101 commented at 5:41 pm on December 7, 2024:

Added additional comments.

in test/functional/wallet_migration.py:1047 in 82d974323c outdated

1042+        assert_equal(addr_info["solvable"], True)
1043+
1044+        wallet.unloadwallet()
1045+
1046+    def test_disallowed_p2wsh(self):
1047+        self.log.info("Test that P2WSH output scripts with invalid witnessScripts are not migrated")

murchandamus commented at 8:34 pm on December 6, 2024:

Why is it important to test that an invalid script that we can import for historical reasons is not migrated? Are we testing that these artifacts do not cause the migration to fail rather than them not being migrated?

achow101 commented at 5:42 pm on December 7, 2024:

Because invalid scripts can exist in the legacy wallet, but can cause issues (crashes, loading failure, etc.) in both migration and descriptor wallets. So we need to test that migration doesn’t fail, and that those scripts do not end up in the descriptor wallet.

in src/wallet/scriptpubkeyman.cpp:1748 in 5aba0cd226 outdated

1751+        case TxoutType::WITNESS_V1_TAPROOT:
1752+        // These are only spendable if the witness script is also spendable as a scriptPubKey
1753+        // We will check these later after "spks" has been updated with the computed output scripts from mapScripts.
1754+        case TxoutType::WITNESS_V0_SCRIPTHASH:
1755+        // For P2SH to be spendable, we need to have the redeemScript, and if we have it, it will be handled when
1756+        // this loop gets to it. A P2SH script itself cannot be nested in anything, so we can skip them.

murchandamus commented at 9:28 pm on December 6, 2024:

Perhaps rephrase this:

P2SH outputs are tracked in legacy wallets per their redeemscripts. Redeemscripts only are used in the input script, and do not directly appear in spks. We will later generate the corresponding spks from the redeemscripts and can skip P2SH outputs here.

achow101 commented at 5:43 pm on December 7, 2024:

No, that’s not correct. I’ve rephrased the comment to be clearer.

in src/wallet/scriptpubkeyman.cpp:1769 in 5aba0cd226 outdated

1776+            CKeyID key_id{uint160(solutions[0])};
1777+            CPubKey pubkey;
1778+            if (GetPubKey(key_id, pubkey) && pubkey.IsCompressed() && HaveKey(key_id)) {
1779                 spks.insert(script);
1780+                // Also insert P2SH-P2WPKH output script
1781+                spks.insert(GetScriptForDestination(ScriptHash(script)));

murchandamus commented at 10:15 pm on December 6, 2024:

Why are we not generating the relevant spks for all keys directly in the initial loop? It would just mean that we would now perhaps recognize and be able to spend more output script types than before, so they should simply not appear in our history, but it might make the code simpler?

achow101 commented at 5:44 pm on December 7, 2024:

Done

in src/wallet/scriptpubkeyman.cpp:1777 in 5aba0cd226 outdated

1794+        }
1795+        // Multisig scripts are spendable if they are inside of a P2SH or P2WSH, and we have all of the private keys.
1796+        // Bare multisigs are never considered spendable
1797+        case TxoutType::MULTISIG:
1798+        {
1799+            Assert(solutions.size() >= 2);

murchandamus commented at 10:21 pm on December 6, 2024:

Errr, can a multisig be 0 of 0? If so, wouldn’t that only have a solution size of 1?

achow101 commented at 5:38 pm on December 7, 2024:

The Solver returns TxoutType::NONSTANDARD for 0-of-n, so this code cannot be reached in that case.

in src/wallet/scriptpubkeyman.cpp:1781 in 5aba0cd226 outdated

1798+        {
1799+            Assert(solutions.size() >= 2);
1800+            std::vector<std::vector<unsigned char>> keys(solutions.begin() + 1, solutions.begin() + solutions.size() - 1);
1801+            if (!HaveKeys(keys, *this)) {
1802+                break;
1803             }

murchandamus commented at 10:27 pm on December 6, 2024:

At this point, the witness program, witness script, or redeemscript is in our mapScript, and we might have enough keys for the quorum but just not all of the keys.

Why does this require all the keys to be present in the wallet rather than just the threshold?

achow101 commented at 5:39 pm on December 7, 2024:

To maintain strict compatibility with LegacyScriptPubKeyMan::IsMine(). I don’t think we should change this behavior in this PR.

in src/wallet/scriptpubkeyman.cpp:1879 in 5aba0cd226 outdated

1896+            return true;
1897+        }
1898+        }
1899+        assert(false);
1900+    };
1901+    // Iterate again for all the P2WSH scripts

murchandamus commented at 10:48 pm on December 6, 2024:

I’m pretty lost at this point. We already handled multisig and even really weird stuff like putting a PKH into a SH. What exactly are we looking for here that we then not also skip?

achow101 commented at 5:46 pm on December 7, 2024:

This was to handle pkh inside of wsh, which was a previously unhandled case. Furthermore, I thought that it would be possible for other (nonstandard) scripts to be included in here, however further analysis shows that this is not possible as nonstandard scripts are always ISMINE_NO.

I’ve removed this loop and added wsh handling for pkh to the loop above.

achow101 force-pushed on Dec 7, 2024

DrahtBot added the label Needs rebase on Dec 9, 2024

test: Extra verification that migratewallet migrates 1eb23bc852

achow101 force-pushed on Dec 9, 2024

DrahtBot removed the label Needs rebase on Dec 9, 2024

in test/functional/wallet_migration.py:1122 in 883030c414 outdated

1117+        # importmulti and wrap the wsh(pkh()) inside of a sh(). This will insert the sh(wsh(pkh())) into
1118+        # setWatchOnly but not the wsh(pkh()).
1119+        # Furthermore, migration should not migrate the wsh(pkh()) if the key is uncompressed.
1120+        comp_eckey = ECKey()
1121+        comp_eckey.generate(compressed=True)
1122+        comp_pubkey = comp_eckey.get_pubkey().get_bytes().hex()

theStack commented at 3:13 pm on December 10, 2024:

in commit 883030c414f6008eb07b43774d8da7fe3e24bef2: could use the generate_keypair helper both here and for the uncompressed variant below

0        comp_eckey_wif, comp_pubkey = generate_keypair(compressed=True, wif=True)

then the ECKey and byte_to_base58 imports are not needed anymore

in src/wallet/scriptpubkeyman.cpp:1811 in 19dc8a3368 outdated

1842+        }
1843+        // Multisig in any nesting level is valid, but inside of P2WSH, all pubkeys must be compressed.
1844+        case TxoutType::MULTISIG:
1845+        {
1846+            if (ctx == ScriptContext::P2WSH) {
1847+                std::vector<std::vector<unsigned char>> keys(solutions.begin() + 1, solutions.begin() + solutions.size() - 1);

theStack commented at 4:21 pm on December 10, 2024:

in commit 19dc8a3368714bff001b967ea1e90b322196c0d8: same as in the MULTISIG Solver case below (in the mapScripts loop), could also add an assertion about the solutions size here (or maybe even introduce some get_multisig_keys_from_solutions lambda to further deduplicate)

0                Assert(solutions.size() >= 2);
1                std::vector<std::vector<unsigned char>> keys(solutions.begin() + 1, solutions.begin() + solutions.size() - 1);

in src/wallet/scriptpubkeyman.cpp:1888 in 19dc8a3368 outdated

1914+            // Insert P2SH-Multisig
1915+            spks.insert(GetScriptForDestination(ScriptHash(script)));
1916+            // P2WSH-Multisig output scripts are spendable if the P2WSH output script is also in mapScripts,
1917+            // and all keys are compressed
1918+            CScript ms_wsh = GetScriptForDestination(WitnessV0ScriptHash(script));
1919+            if (HaveCScript(CScriptID(ms_wsh)) && all_keys_compressed(keys)) {

theStack commented at 4:24 pm on December 10, 2024:

nit: could use is_valid_script here to not repeat the logic (as also done in the PUBKEY/PUBKEYHASH cases above):

0            if (HaveCScript(CScriptID(ms_wsh)) && is_valid_script(script, ScriptContext::P2WSH)) {

theStack commented at 4:27 pm on December 10, 2024: contributor

LGTM, the PR is in a much clearer state now with all the newly introduced comments and the P2WSH scripts loop removed.

tests: Test migration of additional P2WSH scripts dedb15caad

wallet: Remove IsMine from migration

As IsMine will be removed, the relevant components of IsMine are inlined
into the migration functions.

afb4514465

Revert "wallet: Move LegacyScriptPubKeyMan::IsMine to LegacyDataSPKM"

This reverts commit b231f4d556876ae70305e8710e31d53525ded8ae.

d25c0fb11f

achow101 force-pushed on Dec 10, 2024

theStack referenced this in commit 2a1bb045c8 on Dec 10, 2024

sipa commented at 6:59 pm on December 12, 2024: member

Very concepty comment (after IRL discussion with @achow101 and @furszy)

Since this code is effectively introducing an analogue of the existing IsMine() logic, it would be useful to add tests that can help convince reviewers of equivalence between the two (especially since the existing IsMine semDamn miners digging into the floor above us!antics are notoriously unintuitive (sorry)).

I would suggest a fuzz test that constructs a legacy SPKM (populated with some keys, redeemscripts, watchscripts), then generates the set of scriptPubKeys to watch using the migration code, checks that all the scripts in that set are IsMine() according to the old code, and then also generate a number of other scriptPubKeys, and test that IsMine() is true for them if and only if they’re in the set. A complication is that IsMine may return true for things we cannot actually migrate, but I believe that’s fixable by in the test ignoring IsMine() output for anything that cannot be InferDescriptor’d to a valid/roundtrippable descriptor.

I’m happy to help brainstorming how such a fuzz could work.

achow101 commented at 10:08 pm on December 13, 2024: member

Following our discussion yesterday on additional edge cases, I have concluded that this approach is no longer viable as it has a high likelihood of missing further edge cases. I have opened #31495 which uses the suggested alternative approach of filtering the upper bound of scripts with IsMine().

achow101 closed this on Dec 13, 2024

wallet: Remove IsMine from migration code #30328

Code Coverage & Benchmarks

Reviews

Conflicts