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.

Sign up for GitHub

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

Jump to bottom

Wallet no history sync #444

Merged
merged 2 commits into from
Dec 16, 2019
Merged

Wallet no history sync #444

merged 2 commits into from
Dec 16, 2019

Conversation

chris-belcher
Copy link
Contributor

@chris-belcher chris-belcher commented Nov 11, 2019 •
edited
Loading

No-history is a method for synchronizing a wallet by scanning the UTXO
set. It can be useful for checking whether seed phrase backups have
money on them before committing the time and effort required to
rescanning the blockchain. No-history sync is compatible with pruning.
The sync method cannot tell which empty addresses have been used, so
cannot guarentee avoidance of address reuse. For this reason no-history
sync disables wallet address generation and can only be used with
wallet-tool and for sending coinjoins without change addresses.

This PR can be easily tested by setting blockchain_source = bitcoin-rpc-no-history, importing a seed phrase to create a new wallet file (with $ python3 wallet-tool.py recover) then running $ python3 wallet-tool.py --recoversync -g 500 newwalletfile.jmdat and checking that all your money appears. Make sure to increase the gap limit to an appropriate value so that all the addresses which might have coin on them are scanned.

@chris-belcher chris-belcher force-pushed the wallet-no-history-sync branch 3 times, most recently from 538b6d5 to 1325e24 Compare November 11, 2019 10:30
@chris-belcher
Copy link
Contributor Author

All tests are passing, the travis test is failing for some other reason.

@undeath
Copy link
Contributor

undeath commented Nov 11, 2019

tests are failing because flake8 has complaints

@chris-belcher
Copy link
Contributor Author

I pushed an edit to fix one of flake8's complaints. The other errors are because of the file jmclient/jmclient/wallet_service.py which is not touched in this PR.

@undeath
Copy link
Contributor

undeath commented Nov 11, 2019

Did you base this off the wrong branch maybe? Because clearly there are changes to that file and relating to the flake8 errors.

@chris-belcher
Copy link
Contributor Author

Yes you're right. I didn't rebase off the wrong branch I just forgot my own code :p

@chris-belcher
Copy link
Contributor Author

Fixed now, thanks for the help

@chris-belcher chris-belcher force-pushed the wallet-no-history-sync branch 2 times, most recently from dfa678a to 470f1f4 Compare November 12, 2019 15:05
@chris-belcher chris-belcher force-pushed the wallet-no-history-sync branch 3 times, most recently from 1b86587 to ae7bd90 Compare November 28, 2019 23:09
@AdamISZ
Copy link
Member

AdamISZ commented Dec 10, 2019

As of ae7bd90 tests seem to show the functionality is correct, including recovering from seed then specifying -g <number above max index> --recoversync after setting *no-history in config.

There are a few other tests we should try of course.

@chris-belcher
Copy link
Contributor Author

I've dealt with some of the review comments, waiting for more discussion on the others. Commit left unsquished while the review is in progress.

@chris-belcher
Copy link
Contributor Author

Added edits to fix all the review points.

AdamISZ
AdamISZ reviewed Dec 16, 2019
View reviewed changes
jmclient/jmclient/wallet_service.py Outdated
@@ -334,7 +335,8 @@ def sync_wallet(self, fast=True):
# before startup
self.old_txs = [x['txid'] for x in self.bci.list_transactions(100)
if "txid" in x]
self.bci.post_sync_wallet_callback(self.wallet)
if self.bci.__class__ == BitcoinCoreNoHistoryInterface:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe isinstance() is more technically correct, since it'd handle the case that self.bci was an instance of a child class of BitcoinCoreNoHistoryInterface. This is of course just theoretical for now, so I wouldn't be upset if you don't change it :)

@AdamISZ
Copy link
Member

AdamISZ commented Dec 16, 2019

Apart from that one (very minor) comment, everything else seems clear. I'll just do another quick test but this seems fine.

@AdamISZ
Copy link
Member

AdamISZ commented Dec 16, 2019

OK tested an old and a brand new wallet; balance on old recovered exactly correctly. New behaved as expected without any errors (displaying 0 of course). Both utxo set scans take a little over 30s here but probably that's on the quick side because of hardware.

Anyway everything is good afaict so tACK, feel free to merge.

@chris-belcher
Implement no-history synchronization
cbf69c6 
No-history is a method for synchronizing a wallet by scanning the UTXO
set. It can be useful for checking whether seed phrase backups have
money on them before committing the time and effort required to
rescanning the blockchain. No-history sync is compatible with pruning.
The sync method cannot tell which empty addresses have been used, so
cannot guarentee avoidance of address reuse. For this reason no-history
sync disables wallet address generation and can only be used with
wallet-tool and for sending transactions without change addresses.
@chris-belcher
Add test code for no-history sync
f5e27c3
chris-belcher added a commit that referenced this pull request Dec 16, 2019
@chris-belcher
Merge #444: Wallet no history sync
3c48fd3 
f5e27c3 Add test code for no-history sync (chris-belcher)
cbf69c6 Implement no-history synchronization (chris-belcher)

Tree-SHA512: 681efc9cf0a37da8438f8401caaed13fdae40d57e4457a20a1258fdc80483ce1b2d366d222eee63da81056e3c8487d13ce7fb1fadb9ae7bd007bff7e54a94c1f
@chris-belcher chris-belcher merged commit f5e27c3 into master Dec 16, 2019
@chris-belcher chris-belcher deleted the wallet-no-history-sync branch December 16, 2019 13:10
@kristapsk kristapsk mentioned this pull request Jan 3, 2020
Merged
AdamISZ added a commit that referenced this pull request Apr 29, 2020
@AdamISZ
Update no-history-sync code
e6d7dd6 
This updates the new functionality in jmclient.wallet_utils
in the no-history-sync PR #444 to be compatible
with the python-bitcointx refactoring.
AdamISZ added a commit that referenced this pull request May 1, 2020
@AdamISZ
Update no-history-sync code
0213118 
This updates the new functionality in jmclient.wallet_utils
in the no-history-sync PR #444 to be compatible
with the python-bitcointx refactoring.
AdamISZ added a commit that referenced this pull request May 4, 2020
@AdamISZ
Update no-history-sync code
1a493dc 
This updates the new functionality in jmclient.wallet_utils
in the no-history-sync PR #444 to be compatible
with the python-bitcointx refactoring.
AdamISZ added a commit that referenced this pull request May 11, 2020
@AdamISZ
Update no-history-sync code
01aa36b 
This updates the new functionality in jmclient.wallet_utils
in the no-history-sync PR #444 to be compatible
with the python-bitcointx refactoring.
AdamISZ added a commit that referenced this pull request Jun 6, 2020
@AdamISZ
Update no-history-sync code
647e318 
This updates the new functionality in jmclient.wallet_utils
in the no-history-sync PR #444 to be compatible
with the python-bitcointx refactoring.
AdamISZ added a commit that referenced this pull request Jul 4, 2020
@AdamISZ
Update no-history-sync code
40c3c7d 
This updates the new functionality in jmclient.wallet_utils
in the no-history-sync PR #444 to be compatible
with the python-bitcointx refactoring.
AdamISZ added a commit that referenced this pull request Jul 6, 2020
@AdamISZ
Adds libsecp256k1 installation and addresses reviews
03a1359 
Update no-history-sync code:
This updates the new functionality in jmclient.wallet_utils
in the no-history-sync PR #444 to be compatible
with the python-bitcointx refactoring.

Remove all future/py2 compatibility code remaining:
This is in line with #525 and corrects erroneous
addition of more compatibility code.

Addresses all flake8 complaints (ununsed imports etc)

Addresses review of @dgpv

Addresses review of @kristapsk
This was referenced Dec 10, 2020
Closed
Merged
kristapsk added a commit that referenced this pull request Dec 25, 2020
@kristapsk
Merge #764: Drop support for pre-0.17 Bitcoin Core
2f8bbe8 
81982c7 Drop support for pre-0.17 Bitcoin Core (Kristaps Kaupe)

Pull request description:

  Simplifies code, resolves #452.

  I would like also drop official support for versions older than 0.18 (still support 0.17 at first, but put 0.18 as a requirement in docs), as 0.18 is minimum required for [wallet no history sync](#444). In future that would allow [some more optimizations](#462). Asked it [here](#452 (comment)), want to hear some comments about that. In any case, these would be changes on top of this anyway.

  Tested by running test suite with 0.17.1 (where 4 of  tests of `jmclient/test/test_core_nohistory_sync.py` fail as expected) and 0.20.1.

Top commit has no ACKs.

Tree-SHA512: 240a0dc0887465ecc5545dffa3759e9b03cc41fc54a759c6757328949d4b40621e029b7f3acfe6d456b659d3d62a9d1df1c6a39594dd2c94ee66b71a267a65bb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristapsk kristapsk kristapsk left review comments

@AdamISZ AdamISZ AdamISZ left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chris-belcher @undeath @AdamISZ @kristapsk