rpc: Control if rescan updates/notifies existing transactions on importmulti #16050

promag commented at 8:26 PM on May 19, 2019: member

This PR adds a new option rescanUpdate in importmulti RPC method to optionally do not rescan transactions that already exist in the wallet.

This is very important for large wallets when importing a single address with a old timestamp, which in this moment triggers a rescan to the whole wallet, replaying several already known wallet events (via notify-wallet).

Continues the work done in #11484.

Changed the option from update to update_existing, added test and release note.

promag force-pushed on May 19, 2019

fanquake added the label RPC/REST/ZMQ on May 19, 2019

DrahtBot commented at 10:44 PM on May 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:

#14942 (wallet: Make scan / abort status private to CWallet by Empact)

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.

in test/functional/feature_notifications.py:81 in 9c014295c8 outdated

  76 | @@ -66,6 +77,10 @@ def run_test(self):
  77 |              txids_rpc = list(map(lambda t: t['txid'], self.nodes[1].listtransactions("*", block_count)))
  78 |              assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir)))
  79 |  
  80 | +            self.log.info("test -walletnotify with importmulti")
  81 | +            self.test_importmulti(updateexisting=True, count=11)

jonatack commented at 11:19 AM on May 20, 2019:

suggested clarification

-            self.test_importmulti(updateexisting=True, count=11)
+            self.test_importmulti(updateexisting=True, count=block_count+1)

jonatack commented at 11:36 AM on May 20, 2019:

and maybe the 1 in each self.test_importmulti call and inside the function with a NEW_ADDRESSES_COUNT constant. Ideally the number could vary randomly with each test run.

promag commented at 1:21 PM on May 20, 2019:

Let's keep this simple for now.

in test/functional/feature_notifications.py:43 in 9c014295c8 outdated

  38 | +        os.mkdir(self.walletnotify_dir)
  39 | +
  40 | +        addr = self.nodes[0].getnewaddress()
  41 | +        self.nodes[0].generatetoaddress(1, addr)
  42 | +
  43 | +        self.nodes[1].importmulti([{"scriptPubKey": {"address": addr}, "timestamp": "now"}], {"updateexisting": updateexisting})

jonatack commented at 11:22 AM on May 20, 2019:

logging suggestion

         self.nodes[0].generatetoaddress(1, addr)

+        self.log.info('test -walletnotify with importmulti "updateexisting": {}'.format(updateexisting))
         self.nodes[1].importmulti([{"scriptPubKey": {"address": addr}, "timestamp": "now"}], {"updateexisting": updateexisting})

             assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir)))

-            self.log.info("test -walletnotify with importmulti")

promag commented at 1:13 PM on May 20, 2019:

I don't think it has to be so verbose.

jonatack commented at 11:47 AM on May 20, 2019: member

Concept ACK. Clean implementation. RPC arg updateexisting could be more readable; could we use snakecase instead (update_existing) as per doc/developer-notes.md::L908 RPC interface guidelines:

- Argument naming: use snake case `fee_delta` (and not, e.g. camel case `feeDelta`)

in doc/release-notes-16050.md:5 in 9c014295c8 outdated

   0 | @@ -0,0 +1,5 @@
   1 | +RPC changes
   2 | +-----------
   3 | +The RPC `importmulti` has a new option `updateexisting` to control whether
   4 | +rescan updates existing transactions. When `false` only new wallet transactions
   5 | +are notified to `-walletnotify` script.

jonatack commented at 11:51 AM on May 20, 2019:

nits: comma after false and "the" after "notified to"

in test/functional/feature_notifications.py:44 in 9c014295c8 outdated

  39 | +
  40 | +        addr = self.nodes[0].getnewaddress()
  41 | +        self.nodes[0].generatetoaddress(1, addr)
  42 | +
  43 | +        self.nodes[1].importmulti([{"scriptPubKey": {"address": addr}, "timestamp": "now"}], {"updateexisting": updateexisting})
  44 | +        wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == count, timeout=10)

jonatack commented at 11:52 AM on May 20, 2019:

Comment suggestion

+        # Wait at most 10 seconds for expected number of files predicate to be true.
         wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == count, timeout=10)

promag commented at 1:14 PM on May 20, 2019:

Code is pretty much self explanatory?

jonatack commented at 1:38 PM on May 20, 2019:

True. My thought was to highlight for reviewers unfamiliar with wait_until that it can be a form of test assertion but it's definitely optional.

promag force-pushed on May 20, 2019

rpc: Control if rescan updates existing transactions on importmulti 91fc5084a3

qa: Test wallet notifications with importmulti 8ddd05c15c

doc: Add relase note for updateexisting option on importmulti f91d18ab40

promag force-pushed on May 20, 2019

promag commented at 1:27 PM on May 20, 2019: member

@jonatack thanks for the review, followed some of your suggestions.

in test/functional/feature_notifications.py:36 in f91d18ab40

  32 | @@ -32,6 +33,16 @@ def setup_network(self):
  33 |                              "-walletnotify=echo > {}".format(os.path.join(self.walletnotify_dir, '%s'))]]
  34 |          super().setup_network()
  35 |  
  36 | +    def test_importmulti(self, update_existing, expected_notifications):

jonatack commented at 1:41 PM on May 20, 2019:

expected_notifications: good idea :+1:

jb55 commented at 5:20 PM on May 20, 2019: member

Concept ACK. It's not really clear what "update" means in this context. Should it be called notify_existing?

promag renamed this:
~~rpc: Control if rescan updates existing transactions on importmulti~~
rpc: Control if rescan updates/notifies existing transactions on importmulti
on May 21, 2019

promag commented at 10:04 AM on May 21, 2019: member

Updated OP from original by @pedrobranco.

jonasschnelli commented at 10:21 AM on May 22, 2019: contributor

Looks good. The release notes text may need to a bit more precise (fUpdate is not only about calling the wallet notify endpoint?!).

promag commented at 10:25 AM on May 22, 2019: member

@jonasschnelli right, that's why the RPC option isn't notify_existing. Alternatively we could split fUpdate in order to control only the notification.

sipa commented at 6:39 PM on May 22, 2019: member

I have a feeling this is a bit of a hack (or at least an "expert option" that shouldn't be needed for normal operation), to work around the fact that we always rescan for all addresses.

In a more optimal implementation (which I hope we'll more easily have with a native descriptor design), importing a new (set of) addresses/descriptors/scripts should only scan for transactions interacting with what is imported.

promag commented at 6:49 PM on May 22, 2019: member

@sipa agree, always thought rescan could take into account a "wallet subset". I'm still going to leave this open while I look into that.

luke-jr commented at 10:11 PM on August 19, 2019: member

I agree this is a kind of ugly hack. Maybe we should just never update existing on rescans? Would that break any use case?

promag commented at 10:20 AM on August 25, 2019: member

Would that break any use case? @luke-jr safe bet is yes.

promag commented at 9:45 AM on September 15, 2019: member

Let's revisit this issue later then.

promag closed this on Sep 15, 2019

promag deleted the branch on Sep 15, 2019

DrahtBot locked this on Dec 16, 2021