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

No conflicts as of last run.

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:1724 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:1800 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:1799 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

tests: Test migration of additional P2WSH scripts 82d974323c

wallet: Remove IsMine from migration

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

a5a81cf7bc

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

This reverts commit b231f4d556876ae70305e8710e31d53525ded8ae.

97893b124a

achow101 force-pushed on Nov 20, 2024

wallet: Remove IsMine from migration code #30328

Code Coverage & Benchmarks

Reviews

Conflicts