wallet rescan: Add error for SPV mode and excessive depth in pruned mode #532

pinheadmz · 2018-07-13T21:39:28Z

In SPV mode, rescan doesn't actually do anything. If a user creates a watch-only wallet by importing an xpub while in SPV mode, that wallet will not necessarily be in sync with the chain. Unlike fullnode, a rescan will not sync the wallet. This PR adds an error that redirects the user to reset which, in SPV mode at least, should not take too long and will discover any wallet transactions that occurred before the import point.

This issue also applies to pruned nodes, if historical transactions for the imported address were confirmed before the prune height. However a pruned node can not reset. We could expand the error to cover pruned nodes as well.

We might also want to add a note in the docs that importing keys into a pruned node may not correctly recover those keys' blockchain history.

Or... is it possible to enable reset on a pruned node?

tuxcanfly · 2018-07-16T15:23:25Z

Needs a rebase to fix tests.

pinheadmz · 2018-07-16T15:59:13Z

Rebased. Tests successful on local. Think I will want to add one more commit to check prune height against rescan requested height, but would need to import chain into wallet/http.js (See #536)

codecov-io · 2018-07-16T16:01:32Z

Codecov Report

Merging #532 into master will increase coverage by 0.17%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
+ Coverage   53.33%   53.51%   +0.17%     
==========================================
  Files         104      104              
  Lines       27676    27696      +20     
  Branches     4738     4740       +2     
==========================================
+ Hits        14761    14821      +60     
+ Misses      12915    12875      -40

Impacted Files	Coverage Δ
lib/protocol/networks.js	`100% <ø> (ø)`	⬆️
lib/node/http.js	`39.64% <ø> (+0.5%)`	⬆️
lib/wallet/nodeclient.js	`79.41% <100%> (+3.65%)`	⬆️
lib/wallet/plugin.js	`97.05% <100%> (+0.18%)`	⬆️
lib/wallet/walletdb.js	`75.66% <100%> (+0.19%)`	⬆️
lib/wallet/nullclient.js	`82.5% <100%> (+0.92%)`	⬆️
lib/wallet/http.js	`44.39% <14.28%> (-0.02%)`	⬇️
lib/blockchain/chain.js	`60.74% <0%> (+0.18%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54cdd16...5c2d9b5. Read the comment docs.

tuxcanfly · 2018-07-20T06:40:10Z

I saw reset was getting timed out, presumably because the operation takes longer than bclients timeout.

pinheadmz · 2018-07-20T07:20:58Z

I think I noticed that too. The command times out but the reset continues until it's done in the background.

pinheadmz · 2018-11-05T01:08:21Z

Adding to this conversation: There should be an error if a pruned node is asked to rescan beyond it's prune depth. I have a tx in my wallet that is stored as unconfirmed even though it was confirmed 1,000 blocks ago.

What happened, I think, is that I executed rescan with a height deeper than my prune depth. The wallet clears all confirmations shallower than the given height (without any checks), but on a pruned node, the block confirming my tx was no longer on disk, so it did not get reconfirmed during the rescan.

pinheadmz · 2018-11-08T19:52:54Z

UPDATE: This PR now adds an important error if wallet rescan is attempted on a pruned node with a depth greater than the number of blocks on disk.

A new test file was added prune-test.js -- I'm sure we have more tests we can do on pruned nodes. I would also add an spv-test.js file, but it's pretty tricky generating blocks on a SPV node!

This PR also updates the regtest block parameters around pruning to make testing more practical:

--- a/lib/protocol/networks.js
+++ b/lib/protocol/networks.js
@@ -775,8 +775,8 @@ regtest.block = {
   bip65hash: null,
   bip66height: 1251,
   bip66hash: null,
-  pruneAfterHeight: 1000,
-  keepBlocks: 10000,
+  pruneAfterHeight: 500,
+  keepBlocks: 288,
   maxTipAge: 0xffffffff,
   slowHeight: 0
 };

nodech · 2018-11-13T13:25:36Z

lib/wallet/http.js

      const valid = Validator.fromRequest(req);
      const height = valid.u32('height');

+      // Pruned node can only rescan the blocks it has on disk
+      if (this.options.node.rpc.client.node.chain.options.prune) {


There are two modes in wallet:

Wallet as a plugin, in which case lib/wallet/plugin is being called that initializes the wallet

Wallet is run as a separate server, in which case lib/wallet/node is the main file that does all that.

Even if in the first case this "can" be a way to get information about the node that wallet is communicating with, this data wont be there for Wallet that is running as a separate service.

Also wallet as a separate service sometimes might change the node it is communicating with. It might be fullnode one time and pruned another time or spv.

So I think, when establishing connection we should communicate with node and add admin endpoint where we will grab the meta information we are interested about. (We need to think this through though, what else is interesting and can be used in the future).

pinheadmz · 2018-12-19T21:58:34Z

rebased on master, testing against bmocha

pinheadmz · 2018-12-20T08:20:48Z

@nodar-chkuaselidze @braydonf Did some updates:

bcoin node getInfo() now also returns chain properties spv and prune (booleans).
All wallet node clients (nodeclient, nullclient and client in lib/wallet) have new method getInfo() that returns the bcoin client getInfo chain properties
All wallet types (plugin, independent node, and walletDB with no node & nullclient) check getinfo during open() and store the flags in a new object this.chain

tuxcanfly

Tested and reviewed.

lib/node/http.js

lib/wallet/http.js

lib/wallet/node.js

test/prune-test.js

lib/protocol/networks.js

lib/wallet/plugin.js

pinheadmz · 2019-01-02T20:36:43Z

Tests are failing on timeouts generating blocks again, hm seems like sometimes it does sometimes it doesn't...

pinheadmz · 2019-01-02T23:18:12Z

NICE! Little rebase-on-master fixed the testing issue

pinheadmz · 2019-01-08T00:28:14Z

Adding a TODO here that my tests should not be using bclient rpc generatetoaddress calls as discussed this morning because it makes the unit test too obtuse, will fix soon.

pinheadmz · 2019-02-06T23:51:34Z

@tuxcanfly @braydonf

Ok this PR got a little long, but I think it's in great shape now. Going to put a recap in this comment:

Motivation: Wallet can not rescan an SPV chain at all. Pruned nodes can only rescan the limited number of blocks on disk. In the case of a pruned node, rescanning beyond the depth of the chain stored on disk means transactions will be marked as "unconfirmed" forever.

Solution: Wallet asks chain what its properties are and throws an error in either case described.

Method:

There are three ways wallet connects to chain: nullclient (testing), nodeclient (plugin), and client (standalone server). Each of these three objects were updated to get chain properties from whatever chain is about to be rescanned.
To support this as well as inform the user, new properties spv and prune were added to the chain object in the http getInfo() call.
Testing. Created a new test module prune-test.js that tests a too-deep rescan command. I also changed the regtest params for pruning, and added this note in the draft CHANGELOG:

In the old configuration, a pruned node on Regtest would start pruning at height
1000 and then keep the last 1000 blocks on disk (pruning anything older). This
has been changed to facilitate testing. The new parameters are
pruneAfterHeight: 500, keepBlocks: 288.

pinheadmz · 2019-04-05T19:30:32Z

Closing this for #749 and #751

pinheadmz mentioned this pull request Jul 16, 2018

wallet rpc: this.chain undefined (listsinceblock) #536

Closed

pinheadmz force-pushed the rescanspv branch from 9dd8247 to c7abc9e Compare July 16, 2018 15:57

pinheadmz mentioned this pull request Jul 18, 2018

WIP: writing multisig wallet guide bcoin-org/bcoin-org.github.io#96

Open

tuxcanfly mentioned this pull request Oct 28, 2018

refactor indexers #424

Closed

3 tasks

pinheadmz force-pushed the rescanspv branch from c7abc9e to 7754d6e Compare November 8, 2018 19:36

pinheadmz changed the title ~~wallet-http: add error for rescan attempt in SPV mode~~ wallet-http: rescan: Add error for SPV mode and excessive depth in pruned mode Nov 8, 2018

pinheadmz force-pushed the rescanspv branch from 7754d6e to ed3033f Compare November 8, 2018 19:45

nodech reviewed Nov 13, 2018

View reviewed changes

pinheadmz force-pushed the rescanspv branch from 5e1ba39 to c503af4 Compare December 19, 2018 21:57

pinheadmz changed the title ~~wallet-http: rescan: Add error for SPV mode and excessive depth in pruned mode~~ wallet rescan: Add error for SPV mode and excessive depth in pruned mode Dec 20, 2018

tuxcanfly requested changes Jan 2, 2019

View reviewed changes

lib/node/http.js Show resolved Hide resolved

lib/wallet/http.js Outdated Show resolved Hide resolved

lib/wallet/node.js Outdated Show resolved Hide resolved

test/prune-test.js Outdated Show resolved Hide resolved

lib/protocol/networks.js Show resolved Hide resolved

pinheadmz commented Jan 2, 2019

View reviewed changes

lib/wallet/plugin.js Outdated Show resolved Hide resolved

pinheadmz added 8 commits January 2, 2019 15:15

wallet http: prevent rescan in spv

c4d2882

wallet http: prevent excessive rescan in prune

09aeeca

test: add pruned node tests

f5d82e6

prune-test: increase timeout

65a31a4

smaller client requests for timeout

123aa30

expose SPV and prune in info API

098d597

pass chain info to wallet

bd360c1

wallet http: get spv and prune

ef6d0bf

pinheadmz added 4 commits January 2, 2019 15:15

wdb check pruneheight

4f696d4

better names and messages

bc0dabd

revert to chain.options.spv

ea9d2a0

test getInfo for prune

3297a9b

pinheadmz force-pushed the rescanspv branch from fcc4325 to 3297a9b Compare January 2, 2019 23:15

pinheadmz mentioned this pull request Jan 21, 2019

SPV node w/ WalletDB + Full node fail to connect #567

Closed

pinheadmz added enhancement Improving a current feature logs / errors Better feedback to user or prevent user error tests Has tests for code change is just an addtional test labels Jan 23, 2019

pinheadmz mentioned this pull request Jan 24, 2019

Best way to monitor HD wallet balance using only xpub (watch mode) #673

Closed

braydonf added the wallet Wallet related label Feb 6, 2019

pinheadmz added 4 commits February 6, 2019 14:26

generate directly with miner

04daf2b

remove bclient from test

68f7f84

update changelog

b28e25b

correct test for getblock

5c2d9b5

This was referenced Apr 2, 2019

Automatically set walletdb.spv according to client #621

Open

walletDB: get spv and prune settings from node #749

Open

pinheadmz closed this Apr 5, 2019

pinheadmz mentioned this pull request Apr 5, 2019

Prevent wallet from rescanning beyond prune depth #751

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet rescan: Add error for SPV mode and excessive depth in pruned mode #532

wallet rescan: Add error for SPV mode and excessive depth in pruned mode #532

pinheadmz commented Jul 13, 2018 •

edited

Loading

tuxcanfly commented Jul 16, 2018

pinheadmz commented Jul 16, 2018 •

edited

Loading

codecov-io commented Jul 16, 2018 •

edited

Loading

tuxcanfly commented Jul 20, 2018

pinheadmz commented Jul 20, 2018 •

edited

Loading

pinheadmz commented Nov 5, 2018

pinheadmz commented Nov 8, 2018

nodech Nov 13, 2018

pinheadmz commented Dec 19, 2018

pinheadmz commented Dec 20, 2018

tuxcanfly left a comment

pinheadmz commented Jan 2, 2019

pinheadmz commented Jan 2, 2019

pinheadmz commented Jan 8, 2019

pinheadmz commented Feb 6, 2019

pinheadmz commented Apr 5, 2019 •

edited

Loading

wallet rescan: Add error for SPV mode and excessive depth in pruned mode #532

wallet rescan: Add error for SPV mode and excessive depth in pruned mode #532

Conversation

pinheadmz commented Jul 13, 2018 • edited Loading

tuxcanfly commented Jul 16, 2018

pinheadmz commented Jul 16, 2018 • edited Loading

codecov-io commented Jul 16, 2018 • edited Loading

Codecov Report

tuxcanfly commented Jul 20, 2018

pinheadmz commented Jul 20, 2018 • edited Loading

pinheadmz commented Nov 5, 2018

pinheadmz commented Nov 8, 2018

nodech Nov 13, 2018

Choose a reason for hiding this comment

pinheadmz commented Dec 19, 2018

pinheadmz commented Dec 20, 2018

tuxcanfly left a comment

Choose a reason for hiding this comment

pinheadmz commented Jan 2, 2019

pinheadmz commented Jan 2, 2019

pinheadmz commented Jan 8, 2019

pinheadmz commented Feb 6, 2019

pinheadmz commented Apr 5, 2019 • edited Loading

pinheadmz commented Jul 13, 2018 •

edited

Loading

pinheadmz commented Jul 16, 2018 •

edited

Loading

codecov-io commented Jul 16, 2018 •

edited

Loading

pinheadmz commented Jul 20, 2018 •

edited

Loading

pinheadmz commented Apr 5, 2019 •

edited

Loading