wallet: Fast rescan with BIP157 block filters

MarcoFalke commented at 6:40 pm on April 18, 2019: member

Use BIP157 block filters if they are available to speed up rescans

gmaxwell commented at 6:44 pm on April 18, 2019: contributor

Concept ACK

in src/wallet/wallet.cpp:1755 in ffff46a547 outdated

1747@@ -1748,6 +1748,21 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
1748     return startTime;
1749 }
1750 
1751+GCSFilter::ElementSet CWallet::GetScriptPubKeysForBlockFilter()
1752+{
1753+    GCSFilter::ElementSet ret;
1754+    for (const auto& i : CBasicKeyStore::GetAllWatchedScriptPubKeys()) {
1755+        ret.emplace(i.begin(), i.end());

sipa commented at 6:53 pm on April 18, 2019:

This will miss P2PKH outputs I think.

MarcoFalke commented at 6:58 pm on April 18, 2019:

Could you please elaborate in which code path we import a P2PKH address or a public key without adding it to setWatchOnly?

MarcoFalke commented at 12:56 pm on September 25, 2019:

I was wrong. Fixed.

jnewbery commented at 6:53 pm on April 18, 2019: member

Concept ACK

in src/wallet/wallet.cpp:1757 in ffff46a547 outdated

1752+{
1753+    GCSFilter::ElementSet ret;
1754+    for (const auto& i : CBasicKeyStore::GetAllWatchedScriptPubKeys()) {
1755+        ret.emplace(i.begin(), i.end());
1756+    }
1757+    for (const auto& bs : CBasicKeyStore::GetAllBareScripts()) {

sipa commented at 6:54 pm on April 18, 2019:

Just because we know about a script does not imply we’re watching it I believe.

MarcoFalke commented at 6:56 pm on April 18, 2019:

Sure, but I’d rather err on the safe side of scanning a block too much than missing a block (and thus lose coins)

jonasschnelli commented at 6:58 pm on April 18, 2019: contributor

Nice! That was quick. Concept ACK

DrahtBot added the label Utils/log/libs on Apr 18, 2019

DrahtBot added the label UTXO Db and Indexes on Apr 18, 2019

DrahtBot added the label Wallet on Apr 18, 2019

jonasschnelli commented at 8:14 pm on April 18, 2019: contributor

Some rough benchmarks

{ “start_height”: 0, “stop_height”: 572152 }

real 4m23.596s

A simple wallet,… filtered out roughly 2900 blocks and scanned them.

MarcoFalke removed the label Utils/log/libs on Apr 18, 2019

MarcoFalke force-pushed on Apr 19, 2019

DrahtBot commented at 10:20 pm on April 19, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17384 (test: Create new test library by MarcoFalke)
#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#16442 (Serve BIP 157 compact filters by jimpo)

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.

Coverage

Coverage	Change (pull 15845, 77e593bc79216175fa4c2fd05278bf1b7e4c7146)	Reference (master, 56376f336548b53cf31e98a58dfb4db22cede6e5)
Lines	+0.0064 %	87.5552 %
Functions	-0.0143 %	84.6451 %
Branches	+0.0017 %	51.5534 %

Updated at: 2019-04-19T22:20:19.826705.

in src/wallet/wallet.cpp:1823 in 2a884d347e outdated

1819@@ -1806,6 +1820,8 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1820                 WalletLogPrintf("Still rescanning. At block %d. Progress=%f\n", *block_height, progress_current);
1821             }
1822 
1823+            if (chain().filterMatchesAny(block_hash, filter_set)) {

jimpo commented at 11:03 pm on April 19, 2019:

result.last_scanned_block and result.last_scanned_height should both be updated even if the filter doesn’t match.

jimpo commented at 11:31 pm on April 19, 2019:

Oh, there’s another optimization possible, which is to not call filterMatchesAny if the filter index doesn’t exist or doesn’t contain any entry for any of the previously queried block hashes. If filterMatchesAny indicates whether the filter was found in addition to whether it matches, it would be pretty easy to check this.

MarcoFalke commented at 2:00 pm on April 22, 2019:

Indeed, last_scanned_height is reported by rpc as the stop_height, so should be updated.

in src/interfaces/chain.cpp:276 in 106e7c9ab6 outdated

274-        if (!block_filter_index->LookupFilter(index, filter)) return true;
275+        {
276+            LOCK(cs_main);
277+            const CBlockIndex* index = LookupBlockIndex(hash);
278+            if (!index) return true;
279+            if (!block_filter_index->LookupFilter(index, filter)) return true;

jimpo commented at 11:10 pm on April 19, 2019:

You don’t need the lock for the filter lookup, just the block index lookup.

MarcoFalke commented at 2:00 pm on April 22, 2019:

CBlockIndex::nDataPos is protected by cs_main, but we never read that in LookupFilter, so yeah, don’t need the lock

in src/interfaces/chain.cpp:267 in 106e7c9ab6 outdated

263@@ -263,6 +264,19 @@ class ChainImpl : public Chain
264         return std::move(result);
265     }
266     std::unique_ptr<Chain::Lock> assumeLocked() override { return MakeUnique<LockImpl>(); }
267+    bool filterMatchesAny(const uint256& hash, const GCSFilter::ElementSet& filter_set) override

jimpo commented at 11:11 pm on April 19, 2019:

Maybe make the BlockFilterType a parameter here?

MarcoFalke commented at 2:01 pm on April 22, 2019:

Will there ever be a filter different from the basic filter?

in src/wallet/wallet.h:681 in 106e7c9ab6 outdated

677@@ -677,6 +678,9 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
678      * Should be called with non-zero block_hash and posInBlock if this is for a transaction that is included in a block. */
679     void SyncTransaction(const CTransactionRef& tx, const uint256& block_hash, int posInBlock = 0, bool update_tx = true) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
680 
681+    /** Return all scriptPubKeys to be used in a BIP 157 block filter */

jimpo commented at 11:12 pm on April 19, 2019:

I’d note in the comment that it’s a basic (type) block filter (in cases an other types are added).

MarcoFalke commented at 2:02 pm on April 22, 2019:

Mentioned “basic”

in src/init.cpp:967 in 106e7c9ab6 outdated

985@@ -986,9 +986,6 @@ bool AppInitParameterInteraction()
986     if (gArgs.GetArg("-prune", 0)) {
987         if (gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX))
988             return InitError(_("Prune mode is incompatible with -txindex."));
989-        if (!g_enabled_filter_types.empty()) {

jimpo commented at 11:24 pm on April 19, 2019:

This is problematic for a few reasons.

You can turn on the filter index and it will build in the background using the blocks on disk. This wouldn’t work in pruned mode. It might be safe though to allow pruning if the block filter index tip is already within chain tip height - MIN_BLOCKS_TO_KEEP. But then we couldn’t prune until the block filter index is in sync (it has its own sort of initial sync logic). Not prohibitively difficult though.
There still needs to be logic to fetch blocks from the network if the filters match, which I don’t see here. Doing that in a blocking way would probably still be quite slow, especially with the default keypool size as large as it is.

MarcoFalke commented at 2:03 pm on April 22, 2019:

Hmm, right

jimpo commented at 11:28 pm on April 19, 2019: contributor

Yay, Concept ACK.

Comments:

I noticed there are lots of false positive hits because DEFAULT_KEYPOOL_SIZE is 1,000, with is multiplied by like 2 or 4 (I forget) with all of the scriptPubKey variants added for a key. Why is the default keypool size so large?
The scanning might be faster using the method to batch fetch filters, LookupFilterRange. It’s probably not worth the additional complexity in the wallet and chain interface code though.

instagibbs commented at 7:06 pm on April 20, 2019: member

@jimpo

Why is the default keypool size so large?

IIRC this makes it much less likely typical usage will hit a case when an encrypted wallet runs out of viable keys and starts to miss funds during a sync(since it currently doesn’t pause a sync and ask the user for password).

instagibbs commented at 7:07 pm on April 20, 2019: member

and the obligatory concept ACK!

gmaxwell commented at 8:00 pm on April 20, 2019: contributor

Why is the default keypool size so large?

It isn’t. It’s small… the whole concept of ‘gap’ scanning is fairly broken and easily results in funds loss… e.g. when a user hands out multiple addresses to people who haven’t used them yet. The brokenness can be largely addressed by setting the lookahead pretty far.

In some prelim testing this appears to be slower than just rescanning the block files directly with a specialized parser. :(

sipa commented at 8:36 pm on April 20, 2019: member

@gmaxwell What specialized parser are you comparing to? Does it include the time to match all outputs/inputs to a set of 6000 sPKs?

sipa commented at 8:42 pm on April 20, 2019: member

@jimpo 2 keypools (external and change), each 1000 keys, each key 3 sPKs (p2pkh, p2sh-p2wpkh, native p2wpkh), so I expect 6000 entries to match against.

MarcoFalke force-pushed on Apr 22, 2019

MarcoFalke force-pushed on Apr 23, 2019

MarcoFalke force-pushed on Apr 24, 2019

DrahtBot added the label Needs rebase on Apr 27, 2019

gmaxwell commented at 7:56 pm on May 11, 2019: contributor

@gmaxwell modified copy of the old scanner from bitcoin talk, doesn’t check the inputs. I think the main issue is that rehashing all addresses for each block sequentially is pegging a single core and limiting the speed.

MarcoFalke force-pushed on May 17, 2019

DrahtBot removed the label Needs rebase on May 17, 2019

MarcoFalke force-pushed on May 24, 2019

DrahtBot added the label Needs rebase on Jul 12, 2019

MarcoFalke force-pushed on Jul 12, 2019

DrahtBot removed the label Needs rebase on Jul 12, 2019

MarcoFalke force-pushed on Jul 12, 2019

Sjors commented at 3:31 pm on July 13, 2019: member

Concept ACK. @achow101 how much does this get in the way of #16341?

scantxoutset could be a more light-touch place to try this.

achow101 commented at 4:39 pm on July 13, 2019: member

Concept ACK. @achow101 how much does this get in the way of #16341?

I don’t think it really will. The only wallet change looks to be easily portable to the SPKManager model.

MarcoFalke force-pushed on Jul 23, 2019

Sjors commented at 2:46 pm on August 6, 2019: member

I’m unable to compile on macOS:

0interfaces/chain.cpp:258:48: error: constexpr variable 'NO_FILTER' must be initialized by a constant expression
1        constexpr static std::pair<bool, bool> NO_FILTER{/* exists */ false, /* matches */ true};
2                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3interfaces/chain.cpp:258:48: note: non-constexpr constructor 'pair<bool, bool, false>' cannot be used in a constant expression
4/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/utility:446:5: note: declared here
5    pair(_U1&& __u1, _U2&& __u2)
6    ^
71 error generated.
8make[2]: *** [interfaces/libbitcoin_server_a-chain.o] Error 1

MarcoFalke commented at 5:27 pm on August 6, 2019: member

I have pulled out the test changes to #16465, so you might want to review that first.

DrahtBot added the label Needs rebase on Aug 15, 2019

MarcoFalke force-pushed on Sep 19, 2019

DrahtBot removed the label Needs rebase on Sep 19, 2019

MarcoFalke force-pushed on Sep 19, 2019

MarcoFalke commented at 3:41 pm on September 24, 2019: member

I wrote some more dev doc and unit tests, but they require #16956, so please review that first.

MarcoFalke force-pushed on Sep 25, 2019

fanquake referenced this in commit fdfaeb67de on Sep 26, 2019

MarcoFalke force-pushed on Sep 26, 2019

sidhujag referenced this in commit 38fce4ad10 on Sep 26, 2019

Sjors commented at 8:27 pm on September 27, 2019: member

Can you make the log mention that it’s using the filters? I rescanned a mainnet wallet from genesis. On a 2019 MacBook Pro running macOS 10.14.6, configured without debug, using QT:

before: 38 minutes
after: 8 minutes 20 seconds

In case you want to tweak the progress bar, it seems a bit pessimistic in the beginning:

02019-09-27T19:36:19Z [TestRescan] Rescan started from block 000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f...
12019-09-27T19:37:19Z [TestRescan] Still rescanning. At block 103799. Progress=0.000526
22019-09-27T19:38:19Z [TestRescan] Still rescanning. At block 186382. Progress=0.009899
32019-09-27T19:39:19Z [TestRescan] Still rescanning. At block 276838. Progress=0.065700
42019-09-27T19:40:19Z [TestRescan] Still rescanning. At block 356644. Progress=0.151432
52019-09-27T19:41:19Z [TestRescan] Still rescanning. At block 421003. Progress=0.314152
62019-09-27T19:42:19Z [TestRescan] Still rescanning. At block 476729. Progress=0.529299
72019-09-27T19:43:19Z [TestRescan] Still rescanning. At block 529573. Progress=0.714625
82019-09-27T19:44:19Z [TestRescan] Still rescanning. At block 579080. Progress=0.919178
92019-09-27T19:44:40Z [TestRescan] Rescan completed in          500403ms

laanwj added the label Resource usage on Sep 30, 2019

MarcoFalke force-pushed on Oct 11, 2019

MarcoFalke commented at 8:24 pm on October 11, 2019: member

Thanks, force pushed to add logging

DrahtBot added the label Needs rebase on Oct 23, 2019

in src/test/util.cpp:63 in faff768d67 outdated

54@@ -55,6 +55,23 @@ CTxIn generatetoaddress(const std::string& address)
55 CTxIn MineBlock(const CScript& coinbase_scriptPubKey)
56 {
57     auto block = PrepareBlock(coinbase_scriptPubKey);
58+    return *MineBlock(block);
59+}
60+
61+int GetWitnessCommitmentIndex(const CBlock& block);

mzumsande commented at 0:29 am on November 4, 2019:

This gives me a “redundant redeclaration” compiler warning.

MarcoFalke commented at 1:08 pm on November 5, 2019:

Thanks. Not sure how that got there. Removed

in src/wallet/test/wallet_blockfilter_tests.cpp:33 in faff768d67 outdated

28+
29+BOOST_FIXTURE_TEST_SUITE(wallet_blockfilter_tests, RegTestingSetup)
30+
31+BOOST_AUTO_TEST_CASE(element_set_matches)
32+{
33+    LogInstance().m_print_to_console = 1; // DEBUG TODO

mzumsande commented at 0:30 am on November 4, 2019:

just a reminder to remove at some point

MarcoFalke commented at 1:09 pm on November 5, 2019:

Removed

mzumsande commented at 1:54 am on November 4, 2019: member

The new test wallet_blockfilter_tests fails for me reliably with AcceptBlock FAILED (unexpected-witness, ContextualCheckBlock : unexpected witness data found (code 16)) at height 112 when I execute the entire suite with test_bitcoin (like in the current AppVeyor run), but succeeds when run in isolation. I tried for a bit, but haven’t been able to find out why.

in src/interfaces/chain.h:137 in faff768d67 outdated

129@@ -129,6 +130,12 @@ class Chain
130     //! unlocked when the returned interface is freed.
131     virtual std::unique_ptr<Lock> lock(bool try_lock = false) = 0;
132 
133+    //! Returns a pair:
134+    //!  * first : whether the block filter for this block could be found.
135+    //!  * second: whether any of the elements match the block via a BIP 157
136+    //!            block filter.
137+    virtual std::pair<bool, bool> filterMatchesAny(const uint256& hash, const GCSFilter::ElementSet& filter_set) = 0;

luke-jr commented at 1:04 pm on November 4, 2019:

Optional<bool> might make more sense?

jkczyz commented at 2:05 am on November 5, 2019:

This is quite a complicated interface which asks a lot from the caller. This is an indication that the calling code should be refactored to use better abstractions.

For instance, a BlockScanner abstraction could be used to return blocks and whose implementation may vary depending on whether block filters exist. The logic around filters could be encapsulated there rather than having the details exposed in the wallet. I think this would benefit long-term code health.

MarcoFalke commented at 1:41 pm on November 5, 2019:

Thanks, switched to Optional<>

MarcoFalke commented at 1:42 pm on November 5, 2019:

I think this can be done as a follow-up

in src/wallet/wallet.cpp:1618 in faff768d67 outdated

2088+        // Only query the filter index if it exists
2089+        if (filter_result.first /* exists */) filter_result = chain().filterMatchesAny(block_hash, filter_set);
2090+
2091         CBlock block;
2092-        if (chain().findBlock(block_hash, &block) && !block.IsNull()) {
2093+        if (!filter_result.second /* matches */) {

luke-jr commented at 1:07 pm on November 4, 2019:

This doesn’t check filter_result.first

MarcoFalke commented at 1:41 pm on November 5, 2019:

fixed

MarcoFalke referenced this in commit 5933c6d924 on Nov 4, 2019

MarcoFalke force-pushed on Nov 4, 2019

MarcoFalke referenced this in commit 33b155f287 on Nov 4, 2019

in src/test/util.cpp:63 in 04e729eef2 outdated

56@@ -57,6 +57,23 @@ CTxIn generatetoaddress(const std::string& address)
57 CTxIn MineBlock(const CScript& coinbase_scriptPubKey)
58 {
59     auto block = PrepareBlock(coinbase_scriptPubKey);
60+    return *MineBlock(block);
61+}
62+
63+int GetWitnessCommitmentIndex(const CBlock& block);
64+
65+Optional<CTxIn> MineBlock(std::shared_ptr<CBlock>& block)

jkczyz commented at 11:59 pm on November 4, 2019:

Is Optional needed? This function always returns a block.

MarcoFalke commented at 1:19 pm on November 5, 2019:

Added docs

jnewbery commented at 4:37 pm on November 6, 2019:

Which docs? I still don’t understand why you’re using an optional here. The TODO implies that you plan to implement something in a later PR. Can you just return a CTxIn until then?

in src/test/util.cpp:67 in 04e729eef2 outdated

64+
65+Optional<CTxIn> MineBlock(std::shared_ptr<CBlock>& block)
66+{
67+    const auto idx = GetWitnessCommitmentIndex(*block);
68+    if (idx != -1) {
69+        // Refresh, just in case

jkczyz commented at 11:59 pm on November 4, 2019:

Could you clarify this comment a bit. In case what?

MarcoFalke commented at 1:19 pm on November 5, 2019:

Added docs. Thx

in src/test/util.cpp:75 in 04e729eef2 outdated

71+        tx_cb.vout.erase(tx_cb.vout.begin() + idx);
72+        block->vtx[0] = MakeTransactionRef(std::move(tx_cb));
73+        LOCK(cs_main);
74+        GenerateCoinbaseCommitment(*block, ChainActive().Tip()->pprev, Params().GetConsensus());
75+    }
76+    block->hashMerkleRoot = BlockMerkleRoot(*block);

jkczyz commented at 0:03 am on November 5, 2019:

Could you update the commit message to explain this change?

MarcoFalke commented at 1:19 pm on November 5, 2019:

Added docs

in src/index/blockfilterindex.h:18 in 2499fd09a5 outdated

14@@ -15,7 +15,7 @@
15  * blocks by height. An index is constructed for each supported filter type with its own database
16  * (ie. filter data for different types are stored in separate databases).
17  *
18- * This index is used to serve BIP 157 net requests.
19+ * This index can be used to serve BIP 157 net requests or a more efficient wallet rescan.

jkczyz commented at 0:08 am on November 5, 2019:

grammar nit: s/or a more efficient/or for a more efficient

MarcoFalke commented at 1:41 pm on November 5, 2019:

Reworded

MarcoFalke force-pushed on Nov 5, 2019

DrahtBot removed the label Needs rebase on Nov 5, 2019

test: Add blockfilter variant to wallet_import_rescan.py faf7cc8a42

doc: Fixup txindex->index fa80d61b3e

test: Add test/util to test_bitcoin eeee0c1c65

MarcoFalke force-pushed on Nov 5, 2019

test: Split MineBlock into two parts fa9992182c

MarcoFalke force-pushed on Nov 5, 2019

wallet: Add method to get element set for blockfilters fa99419124

wallet: Rescan with blockfilters faee7b6581

MarcoFalke force-pushed on Nov 5, 2019

MarcoFalke commented at 1:43 pm on November 5, 2019: member

Thanks for the review @mzumsande , @luke-jr , @jkczyz . Addressed all your feedback in the latest push

in src/test/util.h:26 in faee7b6581

20@@ -19,8 +21,10 @@ extern const std::string ADDRESS_BCRT1_UNSPENDABLE;
21 
22 // Lower-level utils //
23 
24-/** Returns the generated coin */
25+/** Returns the generated coin (output at index 0 in the coinbase tx) */
26 CTxIn MineBlock(const CScript& coinbase_scriptPubKey);
27+/** Returns the generated coins (output at index 0 in the coinbase tx), or nothing if the block was invalid */

jnewbery commented at 4:38 pm on November 6, 2019:

s/coins/coin/

I also don’t like the (mis)use of the word coin here, which is generally understood to mean a utxo. These functions return a CTxIn that points to the first coinbase output.

in src/interfaces/chain.cpp:257 in faee7b6581

248@@ -248,6 +249,20 @@ class ChainImpl : public Chain
249         // LockImpl to Lock pointer
250         return std::move(result);
251     }
252+    Optional<bool> filterMatchesAny(const uint256& hash, const GCSFilter::ElementSet& filter_set) override
253+    {
254+        const BlockFilterIndex* block_filter_index = GetBlockFilterIndex(BlockFilterType::BASIC);
255+        if (!block_filter_index) return {};
256+        BlockFilter filter;
257+        const CBlockIndex* index;

Talkless commented at 5:32 pm on November 6, 2019:

This could lock even smaller block of code:

0        {
1            LOCK(cs_main);
2            index = LookupBlockIndex(hash);
3        }
4        if (!index) return {};

Or maybe you would care to use ES.28: Use lambdas for complex initialization, especially of const variables guideline (note const for index pointer itself):

0const CBlockIndex* const index = [&]{ 
1    LOCK(cs_main);
2    return LookupBlockIndex(hash);
3}();
4if (!index) return {};

in src/interfaces/chain.cpp:254 in faee7b6581

248@@ -248,6 +249,20 @@ class ChainImpl : public Chain
249         // LockImpl to Lock pointer
250         return std::move(result);
251     }
252+    Optional<bool> filterMatchesAny(const uint256& hash, const GCSFilter::ElementSet& filter_set) override
253+    {
254+        const BlockFilterIndex* block_filter_index = GetBlockFilterIndex(BlockFilterType::BASIC);

Talkless commented at 5:34 pm on November 6, 2019:

Looks like the pointer itself could be even const too: const BlockFilterIndex* const block_filter_index = …

in src/wallet/wallet.cpp:1583 in faee7b6581

1581     ScanResult result;
1582 
1583     WalletLogPrintf("Rescan started from block %s...\n", start_block.ToString());
1584 
1585+    uint256 block_hash = start_block;
1586+    const auto filter_set = GetLegacyScriptPubKeyMan()->GetAllRelevantScriptPubKeys();

jnewbery commented at 6:50 pm on November 6, 2019:

Setting the filter_set outside the rescan loop means that this doesn’t get updated when the keypool is topped up. That means that any payments to an address beyond the first 1000 will be missed on rescan.

jnewbery commented at 6:53 pm on November 6, 2019: member

I think as well as fixing the bug here: #15845 (review), it’d be useful to add a test that would catch cases like this, where a rescan exhausts the keypool.

sidhujag referenced this in commit dda5c932c1 on Nov 7, 2019

DrahtBot added the label Needs rebase on Nov 7, 2019

DrahtBot commented at 2:28 pm on November 7, 2019: member

in src/wallet/scriptpubkeyman.cpp:827 in faee7b6581

819@@ -820,6 +820,31 @@ bool LegacyScriptPubKeyMan::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyO
820     return GetWatchPubKey(address, vchPubKeyOut);
821 }
822 
823+GCSFilter::ElementSet LegacyScriptPubKeyMan::GetAllRelevantScriptPubKeys() const
824+{
825+    LOCK(cs_KeyStore);
826+    GCSFilter::ElementSet ret;
827+    for (const auto& k : mapKeys) {

luke-jr commented at 4:13 pm on November 11, 2019:

What about mapCryptedKeys? I don’t think mapKeys is used on encrypted wallets?

ryanofsky commented at 6:18 pm on November 11, 2019:

What about mapCryptedKeys? I don’t think mapKeys is used on encrypted wallets?

Yes, this should just loop over mapCryptedKeys as well. mapKeys should be empty in encryped wallets.

achow101 commented at 2:38 am on November 12, 2019:

This function in general does not match everything that IsMine would match.In particular it doesn’t enumerate all possible combinations of multisig policies with all combinations of pubkeys or all of the weird nested scripts that are possible. There probably needs to be a release note that if you have some weird and non-standard script stuff, the fast rescan won’t see those things.

Sjors commented at 11:52 am on November 14, 2019:

Such a warning makes sense; people with such wallets will presumably understand. And descriptor wallets won’t have this problem.

luke-jr commented at 1:52 pm on November 14, 2019:

IsMine itself won’t test weird combinations. I don’t see why fast rescan shouldn’t be able to check for everything IsMine does… users shouldn’t be exposed to internal implementation details like this.

MarcoFalke closed this on May 7, 2020

MarcoFalke deleted the branch on May 7, 2020

MarcoFalke added the label Up for grabs on May 7, 2020

jnewbery commented at 1:04 am on May 8, 2020: member

@MarcoFalke - do you mind documenting the problems with this approach, so people who find this PR in future know why you closed it?

MarcoFalke commented at 11:20 am on May 8, 2020: member

I don’t think there are any problems with this approach, once this and this is addressed.

As pointed out here multisig should work just fine, but I haven’t had the time or interest to look into this, so I closed it.

pstratem referenced this in commit b59f89b1fd on May 31, 2020

pstratem referenced this in commit 9daf59ddce on Jun 6, 2020

MarcoFalke removed the label Up for grabs on Jun 30, 2020

sidhujag referenced this in commit 8cc45f92f2 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

MarcoFalke added the label Up for grabs on Mar 21, 2022

fanquake removed the label Up for grabs on Aug 30, 2022

fanquake removed the label Needs rebase on Aug 30, 2022

wallet: Fast rescan with BIP157 block filters #15845

Conflicts

Coverage