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

For detailed information about the code coverage, see the test coverage report.

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

in src/wallet/scriptpubkeyman.cpp:1860 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?

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.

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:1793 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])};

in src/wallet/scriptpubkeyman.cpp:1777 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    };

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

wallet: Remove IsMine from migration

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

8edcf1f29c

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

This reverts commit bbb1d51e1240da61db2ca1036f9ec91fd2f36f2d.

705d9eb0ae

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).

wallet: Remove IsMine from migration code #30328

Code Coverage

Reviews

Conflicts