Skip to content
This repository was archived by the owner on May 13, 2022. It is now read-only.

Conversation

@AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented Sep 17, 2016

Current wallet sync is not smart enough about the situation where a wallet was previously in sync and is now restarting, with index_cache and wallet imports intact.

In practice this was not too important until recently, when spying activity vastly increased the size of used address lists, but it is now a major practical problem.

This adds a --fast option to all the scripts that sync the wallet before beginning. Under the hood it uses rpc listaddressgroupings to get all of the relevant address data in one call.

Since it is slightly tricky for the code to automatically recognize whether this faster sync can happen successfully, it was decided to leave it as an option which is turned off by default.

Also important is that an attempt to use --fast which is unsuccessful will simply return an incomplete wallet (not showing all coins), but should not create any safety issue, i.e. you should be able to re-run without --fast and get the correct result.

Experiments on regtest and mainnet show it working well, although I only currently have wallets with of the order of a few thousand addresses; I'd like to see the effect for those with 10k+ addresses.

Next comment will explain how easily and safely to test this.

@AdamISZ
Copy link
Member Author

AdamISZ commented Sep 17, 2016

If you want to test this PR with me, do something like:

First, for avoidance of confusion with running code, make a copy of your whole joinmarket directory somewhere else (or do a new git clone, maybe simpler).

In the new directory, copy your "live" wallet.json into the new wallets/ directory. Also copy your existing joinmarket.cfg into the new directory (which must be configured for Core, this PR is only for Core wallets).

Although it isn't strictly necessary, shut down any running yield-generator while running the test.

Then pull the PR:

git fetch origin refs/pull/612/head:pull_612

where origin should be replaced with whatever your name is for this repo (origin is the default). A branch is automatically created with name pull_612 in your local repo. Make sure to also git checkout pull_612 so as to run this code.

Then do:

python wallet-tool.py --fast wallet.json (or whatever your wallet's called). Note the returned balance. Then go back to your "live"/"normal" joinmarket folder and do a python wallet-tool.py wallet.json to compare if the amounts are the same. I realise this last instruction might not always be practical for people with 10k+ addresses! If so, find a way to validate whether the balance is correct (perhaps you noted it down from before). You can check if the indices of the addresses match what you see in index_cache inside the wallet.json.

Let me know what you find. In my experience, if index_cache is there, and you haven't changed your Core instance, then --fast works, gives the right output and happened in a few seconds I guess. I don't know if it'll always be that fast, hence reports most welcome.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 82.39% when pulling ffcd304 on AdamISZ:newsync into 8fed0da on JoinMarket-Org:develop.

@AdamISZ
Copy link
Member Author

AdamISZ commented Sep 20, 2016

Please test if you get a chance. I've done all kinds of tests myself but can't find a problem, whereas the only other tester so far reports an incorrect balance.

@chris-belcher
Copy link
Collaborator

chris-belcher commented Sep 21, 2016

Havent had a chance to test yet but thinking it might be more appropriate to put the option in joinmarket.cfg instead of in the command line of each script.

@AdamISZ
Copy link
Member Author

AdamISZ commented Sep 21, 2016

@chris-belcher i see where you're coming from, the problem is users really should explicitly choose it and then not use it when it isn't appropriate or doesn't work. I thought about it for a while and decided, annoying as it is to put it all over the place, it really must be an explicit choice, otherwise they will get wrong balance reports and will not know why.

@AdamISZ
Copy link
Member Author

AdamISZ commented Sep 21, 2016

@mustyoshi reported problems but it seems it was because the index_cache in his copied directory was not in sync (because yg was still running). When brought into sync this method worked.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 82.541% when pulling 6210225 on AdamISZ:newsync into 93dfefc on JoinMarket-Org:develop.

@CohibAA
Copy link
Contributor

CohibAA commented Sep 28, 2016

tACK

@AdamISZ AdamISZ merged commit 6210225 into JoinMarket-Org:develop Sep 28, 2016
AdamISZ added a commit that referenced this pull request Sep 28, 2016
6210225 add fast sync option for Core wallets (Adam Gibson)
@AdamISZ
Copy link
Member Author

AdamISZ commented Sep 28, 2016

@CohibAA thanks, I've merged it now after 3 people reported it working, plus the fact it doesn't alter anything if not used. But, can you let us know if it gave you much speedup? So far I've had one person saying it reduced it down to a minute or two, and another saying it was reduced only from 27 to 16(!) minutes; this actually might not be that strange if the time is dominated by address generation, but I'm sure we can do better than that, even for > 10k addresses as this person had.

@CohibAA
Copy link
Contributor

CohibAA commented Sep 28, 2016

@AdamISZ I have approximately 3500 addresses in the wallet I tested, and this gave about 30% improvement on time. I don't have any wallets with higher address count to test/compare though.

Standard = 1 minute, 50 seconds

Fast mode = 1 minute, 15 seconds

@HamishMacEwan
Copy link

Hi,

jimmy:~/bin/joinmarket-AdamISZ$ git checkout pull_612
Switched to branch 'pull_612'
jimmy:~/bin/joinmarket-AdamISZ$ python wallet-tool.py --fast wallet.json
2016-09-19 07:14:45,363 [MainThread  ] [DEBUG]  hello joinmarket
2016-09-19 07:14:45,548 [MainThread  ] [DEBUG]  Joinmarket directory is: /home/macewanh/bin/joinmarket-AdamISZ
Enter wallet decryption passphrase: 
2016-09-19 07:14:49,534 [MainThread  ] [DEBUG]  rpc: listaddressgroupings []
2016-09-19 07:14:50,447 [MainThread  ] [DEBUG]  Fast sync in progress. Got this many used addresses: 9463
total balance = correct btc

Run time, was 07:19:24 to 07:22:23

A vast improvement on the couple of hours it otherwise takes.

I was a bit puzzled by the 9463 figure given these figures:
"index_cache": [[255, 10044], [148, 12860], [185, 9837], [120, 7554], [124, 5658]]

@AdamISZ
Copy link
Member Author

AdamISZ commented Oct 18, 2016

@HamishMacEwan the 9463 is how many addresses have actually been used in transactions; the index_cache is the actual indices, so you can see there are vast gaps of unused addresses that were requested to initialize transactions, but never used ("gaps"), caused by the spy bot.

@CohibAA yeah for small-ish wallets there generally isn't much difference, I was originally a bit disappointed that some people didn't report big (enough) speed ups but it seems to depend on various factors, because part of the time is taken by simply address generation, and another part is taken by a large number of rpc requests (in the non-fast version).
The more I look into this stuff (wallet sync/index/indexcache) the more I think it needs to be redesigned, I think the original model worked fine within its limits, but when people's wallets "blew up" it started creaking and the "fixes" I made improved certain scenarios but made others worse in some cases.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants