Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: Fast rescan with BIP157 block filters #15845

Open
wants to merge 6 commits into
base: master
from

Conversation

@MarcoFalke
Copy link
Member

MarcoFalke commented Apr 18, 2019

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

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Apr 18, 2019

Concept ACK

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Apr 18, 2019

Concept ACK

for (const auto& i : CBasicKeyStore::GetAllWatchedScriptPubKeys()) {
ret.emplace(i.begin(), i.end());
}
for (const auto& bs : CBasicKeyStore::GetAllBareScripts()) {

This comment has been minimized.

Copy link
@sipa

sipa Apr 18, 2019

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 18, 2019

Author Member

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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 18, 2019

Nice! That was quick.
Concept ACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 18, 2019

Some rough benchmarks

{
"start_height": 0,
"stop_height": 572152
}

real 4m23.596s

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletFastRescan branch 3 times, most recently from a0aa855 to 106e7c9 Apr 19, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Apr 19, 2019

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, 77e593b) Reference (master, 56376f3)
Lines +0.0064 % 87.5552 %
Functions -0.0143 % 84.6451 %
Branches +0.0017 % 51.5534 %

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

Copy link
Contributor

jimpo left a comment

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.
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/interfaces/chain.cpp Outdated Show resolved Hide resolved
@@ -263,6 +264,19 @@ class ChainImpl : public Chain
return std::move(result);
}
std::unique_ptr<Chain::Lock> assumeLocked() override { return MakeUnique<LockImpl>(); }
bool filterMatchesAny(const uint256& hash, const GCSFilter::ElementSet& filter_set) override

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 19, 2019

Contributor

Maybe make the BlockFilterType a parameter here?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 22, 2019

Author Member

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

src/wallet/wallet.h Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Apr 20, 2019

@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

This comment has been minimized.

Copy link
Member

instagibbs commented Apr 20, 2019

and the obligatory concept ACK!

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Apr 20, 2019

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

This comment has been minimized.

Copy link
Member

sipa commented Apr 20, 2019

@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

This comment has been minimized.

Copy link
Member

sipa commented Apr 20, 2019

@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 MarcoFalke force-pushed the MarcoFalke:1904-walletFastRescan branch 5 times, most recently from 3ddbb2a to abcddec Apr 22, 2019
@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented May 11, 2019

@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 MarcoFalke force-pushed the MarcoFalke:1904-walletFastRescan branch from abcddec to 474f9b9 May 17, 2019
@DrahtBot DrahtBot removed the Needs rebase label May 17, 2019
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletFastRescan branch from 474f9b9 to 5c9bbcf May 24, 2019
Copy link
Contributor

mzumsande left a comment

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.

src/test/util.cpp Outdated Show resolved Hide resolved
src/wallet/test/wallet_blockfilter_tests.cpp Outdated Show resolved Hide resolved
src/interfaces/chain.h Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
MarcoFalke added a commit that referenced this pull request Nov 4, 2019
fa0a731 test: Add RegTestingSetup to setup_common (MarcoFalke)
fa54b3e test: move-only ComputeFilter to src/test/lib/blockfilter (MarcoFalke)

Pull request description:

  The default chain for `TestingSetup` is the main chain. However, any test that wants to mine blocks on demand needs to switch to regtest. This is done manually and in-line right now.

  Fix that by creating an explicit `RegTestingSetup` and use it where appropriate.

  Also, add a move-only commit to move `ComputeFilter` into the newly created unit test library.

  Both commits are part of #15845, but split up because they are useful on their own.

ACKs for top commit:
  practicalswift:
    ACK fa0a731 -- diff looks correct

Tree-SHA512: 02b9765580b355ed8d1be555f8ae11fa6e3d575f5cb177bbdda0319378837e29de5555c126c477dc8a1e8a5be47335afdcff152cf2dea2fbdd1a988ddde3689b
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletFastRescan branch 2 times, most recently from 4771b1c to 8edf3c6 Nov 4, 2019
MarcoFalke added a commit that referenced this pull request Nov 4, 2019
fa07b8b test: Reset global args between test suites (MarcoFalke)

Pull request description:

  Ideally there wouldn't be any globals in Bitcoin Core. However, as we still have globals, they need to be reset between runs of test cases. One way to do this is to run each suite in a different process. `make check` does that. However, `./src/test/test_bitcoin` when run manually or on appveyor is a single process, where all globals are preserved between test cases.

  This leads to hard to debug issues such as #15845 (review).

  Fix that by resetting the global arg for each test suite. Note that this wont reset the arg between test cases, as the constructor/destructor is not called for them.

  Addendum: This is not a general fix, only for `-segwitheight`. I don't know if clearing all args can be done with today's argsmanager.  Nor do I know if it makes sense. Maybe we want datadir set to a temp path to not risk accidentally corrupting the default data dir?

ACKs for top commit:
  laanwj:
    ACK fa07b8b
  practicalswift:
    ACK fa07b8b
  mzumsande:
    ACK fa07b8b, I also tested that this fixes the issue in #15845.

Tree-SHA512: 1e30b06f0d2829144a61cc1bc9bdd6a694cbd911afff83dd3ad2a3f15b577fd30acdf9f1469f8cb724d0642ad5d297364fd5a8a2a9c8619a7a71fa9ae2837cdc
src/test/util.cpp Show resolved Hide resolved
src/test/util.cpp Outdated Show resolved Hide resolved
src/test/util.cpp Outdated Show resolved Hide resolved
src/index/blockfilterindex.h Outdated Show resolved Hide resolved
//! * first : whether the block filter for this block could be found.
//! * second: whether any of the elements match the block via a BIP 157
//! block filter.
virtual std::pair<bool, bool> filterMatchesAny(const uint256& hash, const GCSFilter::ElementSet& filter_set) = 0;

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 5, 2019

Contributor

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 5, 2019

Author Member

I think this can be done as a follow-up

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletFastRescan branch from 8edf3c6 to 09212f7 Nov 5, 2019
@DrahtBot DrahtBot removed the Needs rebase label Nov 5, 2019
MarcoFalke added 3 commits Apr 18, 2019
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletFastRescan branch from 09212f7 to ce61001 Nov 5, 2019
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletFastRescan branch from ce61001 to 7b024b8 Nov 5, 2019
MarcoFalke added 2 commits Sep 26, 2019
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletFastRescan branch from 7b024b8 to faee7b6 Nov 5, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Nov 5, 2019

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

const BlockFilterIndex* block_filter_index = GetBlockFilterIndex(BlockFilterType::BASIC);
if (!block_filter_index) return {};
BlockFilter filter;
const CBlockIndex* index;

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 6, 2019

This could lock even smaller block of code:

        {
            LOCK(cs_main);
            index = LookupBlockIndex(hash);
        }
        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):

const CBlockIndex* const index = [&]{ 
    LOCK(cs_main);
    return LookupBlockIndex(hash);
}();
if (!index) return {};
@@ -248,6 +249,20 @@ class ChainImpl : public Chain
// LockImpl to Lock pointer
return std::move(result);
}
Optional<bool> filterMatchesAny(const uint256& hash, const GCSFilter::ElementSet& filter_set) override
{
const BlockFilterIndex* block_filter_index = GetBlockFilterIndex(BlockFilterType::BASIC);

This comment has been minimized.

Copy link
@Talkless

Talkless Nov 6, 2019

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

src/test/util.cpp Show resolved Hide resolved
CTxIn MineBlock(const CScript& coinbase_scriptPubKey);
/** Returns the generated coins (output at index 0 in the coinbase tx), or nothing if the block was invalid */

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 6, 2019

Member

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.

ScanResult result;

WalletLogPrintf("Rescan started from block %s...\n", start_block.ToString());

uint256 block_hash = start_block;
const auto filter_set = GetLegacyScriptPubKeyMan()->GetAllRelevantScriptPubKeys();

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 6, 2019

Member

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

This comment has been minimized.

Copy link
Member

jnewbery commented Nov 6, 2019

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

sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 7, 2019
fa0a731 test: Add RegTestingSetup to setup_common (MarcoFalke)
fa54b3e test: move-only ComputeFilter to src/test/lib/blockfilter (MarcoFalke)

Pull request description:

  The default chain for `TestingSetup` is the main chain. However, any test that wants to mine blocks on demand needs to switch to regtest. This is done manually and in-line right now.

  Fix that by creating an explicit `RegTestingSetup` and use it where appropriate.

  Also, add a move-only commit to move `ComputeFilter` into the newly created unit test library.

  Both commits are part of bitcoin#15845, but split up because they are useful on their own.

ACKs for top commit:
  practicalswift:
    ACK fa0a731 -- diff looks correct

Tree-SHA512: 02b9765580b355ed8d1be555f8ae11fa6e3d575f5cb177bbdda0319378837e29de5555c126c477dc8a1e8a5be47335afdcff152cf2dea2fbdd1a988ddde3689b
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 7, 2019

Needs rebase
{
LOCK(cs_KeyStore);
GCSFilter::ElementSet ret;
for (const auto& k : mapKeys) {

This comment has been minimized.

Copy link
@luke-jr

luke-jr Nov 11, 2019

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Nov 11, 2019

Contributor

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.

This comment has been minimized.

Copy link
@achow101

achow101 Nov 12, 2019

Member

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.

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 14, 2019

Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.