wallet: rescan bc with preserving key images

[ph4r05]

• Experimental feature. Prototype implementation (just an idea).

• Enables to perform rescan_spent / key image import with an untrusted daemon. Spent check status involves RPC calls which require trusted daemon status as it leaks information (asks for particular key images being spent). The new call rescan_bc_keep_ki performs soft reset while preserving key images thus the following sequence of commands will correctly perform spent checking without need for trusted daemon:

  • refresh
  • key image sync / import
  • rescan_bc_keep_ki

• Useful to detect spent outputs (e.g., after restore) with untrusted daemon on watch_only / multisig / cold / hw-backed wallets after expensive key image sync. Downside: need to download the blockchain since restore height.

• Usage: rescan_bc keep_ki [restore_height]. Restore height defaults to 0. Ideally you wan't to perform restore from the height your wallet was generated, the refresh-from-block-height. This variable is printed by set command.

  • rescan_bc hard [restore_height]
  • rescan_bc soft [restore_height]

[moneromooo-monero]

Does it have any advantage over f26ce08 ?

[moneromooo-monero]

(apart from the height setting)

[ph4r05]

IMO this builds on top of that. It is soft (!hard) rescan_bc which on top of that preserves key images (KIs).

KIs are sometimes difficult to get, e.g., on watch-only wallets, HW wallets. So after such KI import one needs to determine which were spent. Trusted node is required for this.

rescan_bc_keep_ki call enables to determine spent status without a need for a trusted node as it just sweeps the blockchain since start height.

[ph4r05]

I mean, this can be useful after wallet restore (e.g. from a device or watch-only wallet). After restore one needs to check the spent status, this helps to avoid trusted node requirement.

Its just an idea which might be implemented in somewhat more elegant way.

[moneromooo-monero]

Oh, I thought the other one kept key images, but it doesn't. Nevermind.

[ph4r05]

I will change the PR a bit. I got an idea it can be the same rescan_bc call bust with different argument.

[ph4r05]

actually... was soft reset meant to preserve key images? If yes this PR is not needed then, just a small change to soft reset.

[moneromooo-monero]

Doing it that way wlil break if the rescan actually changes m_transfers.

[ph4r05]

Good point, if blockchain re-ogranizes then preserving key images would not work well (as mapping KI -> m_transfers_idx would not be valid).

So for doing this correctly we maybe need to check if the "new" blockchain contains the last_block_hash at last_block_height?

[ph4r05]

@moneromooo-monero you've sparked an idea of checking the m_transfers.

My idea is to hash m_transfers and to check whether it changed after the refresh (up to height as before refresh). For that purpose I've added:

uint64_t wallet2::hash_m_transfers(int64_t transfer_height, crypto::hash &hash) const;

• If hashes are different an exception is thrown saying full rescan_bc is needed (soft or hard).
• else if m_transfers.size() increases during refresh notice is shown: "New transfer received since rescan was started. Key images are incomplete".

[moneromooo-monero]

Sounds dangerous. I'd rather do three steps:

• build a temp std::map of key image to bool (spent status)
• run rescan_bc as now (full or soft, doesn't matter)
• go through the new m_transfers and set m_spent as per the map we built in the first step

That is safe and simple, and achieves what you wanted (have good spent state without asking the daemon).

[ph4r05]

I am not sure I follow 100% 😕
I assume watch-only wallet for simplicity. So what's not clear:

• key images are preserved or cleared in this process? I assume cleared otherwise we would run to the same problem.
• if cleared, then the process goes like this: key image import -> rescan_bc keep_ki -> key image import? Couldn't we instead store mapping tsxid -> key image and restore matched KIs for those transactions to avoid another key image import?
• I am not sure how std::map you mention is constructed. At this point new fresh key images were imported and we don't know whether they were spent or not. Is it from m_confirmed_txs?

Would this also work?

• Construct std::map :: tsxid -> key image before rescan from imported key images.
• Rescan_bc
• Go through m_confirmed_txs, check if known key images were spent. Change m_transfers.spent status accordingly.

Still sounds like large code change. hash(m_transfers, height) seems like safe method to determine change in the transfers, isn't it? I thought the original approach is easy to implement as minimal code change is needed. Thanks for clarification! :)

[moneromooo-monero]

You are right, for a view wallet, you'd end up with unknown key images, so you'd have to keep an output-to-key image mapping too if doing it that way (not just txid, you could have several outs with the same txid).

You can't hash m_transfers. It's not a POD type.

[ph4r05]

(not just txid, you could have several outs with the same txid).

True, I've missed that.

You can't hash m_transfers. It's not a POD type.

I had something reasonable in mind, like:

for(const transfer_details & transfer : m_transfers)
    keccak_update(&state, (const uint8_t *) transfer.m_txid.data, sizeof(transfer.m_txid.data));

Or transaction_prefix_hash should determine transaction exactly, right?

[moneromooo-monero]

Yes, but you'd also need other stuff, like output index. But that'd work I think. Just seems more complicated to me. If it's the opposite way for you, then go ahead :)

[ph4r05]

So I've iterated to the current state:

• rescan_bc [soft|hard|keep_ki] [restore_height]
• restore_height is applicable to all, defaults to 0 (as before). If is greater than refresh-from-block-height a warning is printed
• keep_ki mode checks if the m_transfers changed due to the rescan. If changed, an error is printed and the soft reset is performed to avoid inconsistency.
• m_transfers check is performed by comparison of the vector hashes. Hashes are constructed in such a way it uniquely determines the received transfers (transaction prefix hash, global output index).

[moneromooo-monero]

That seems more complicated than necessary, and seems to not help in the case whre m_transfers is changed (which is the useful case).

What I had in mind is this (untested):

diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index c08379514..ccb0333ff 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -5145,6 +5145,8 @@ void wallet2::rescan_spent()
 //----------------------------------------------------------------------------------------------------
 void wallet2::rescan_blockchain(bool hard, bool refresh)
 {
+  std::unordered_map key_image_map;
+
   if(hard)
   {
     clear();
@@ -5152,6 +5154,11 @@ void wallet2::rescan_blockchain(bool hard, bool refresh)
   }
   else
   {
+    // remember key images
+    for (const transfer_details &td: m_transfers)
+      if (td.m_key_image_known)
+        key_image_map[td.get_public_key()] = td.m_key_image;
+
     m_blockchain.clear();
     m_transfers.clear();
     m_key_images.clear();
@@ -5167,6 +5174,19 @@ void wallet2::rescan_blockchain(bool hard, bool refresh)
 
   if (refresh)
     this->refresh(false);
+
+  // restore unknown key images
+  for (size_t i = 0; i second;;
+      td.m_key_image_known = true;
+      m_key_images[it->second] = i;
+    }
+  }
 }
 //----------------------------------------------------------------------------------------------------
 bool wallet2::is_transfer_unlocked(const transfer_details& td) const
which is much simpler, and 

[ph4r05]

As I understood it, if m_transfers was changed it has to be due to blockchain reorganization
that affects your incoming transfer (change in the longest chain). And in that case, key images have to be recomputed. I also assume this event is rare. Is there another way m_transfers can change during rescan_bc so it affects key images validity?

IMO If just new transfers are added its still fine with my method (ordering of old ones and KIs stay same). If blockchain reorganizes then the key image import has to be performed again.

Problem with key_image_map in the code above is the refresh code which processes blockchain won't be able to detect our spent transactions and we would have to do it somehow manually (so the spent status, m_confirmed_txs and balance are correct).

I have an impression that "my" mechanism is relatively simple. The main idea is just to keep m_key_images so refresh can pick up spent transactions from the blockchain rescan. This strategy is not working only if blockchain reorganizes (m_transfers order changes) which is easily detected by hashing the m_transfers. The hash comparison detects any change in m_transfers that could break the strategy.

Where mine and your view deviates? :)

Use-case: restore watch-only wallet, import key images, rescan_spent with untrusted node.

[ph4r05]

There are more code changes as I've added the restore-height to the rescan_bc in the simplewallet + boilerplate for parameter processing (soft|hard|keep_ki). Besides that the patch is relatively small.

[ph4r05]

Hmm I need a minute to think it trough

[moneromooo-monero]

m_transfers will usually change because of a bug. Reorgs are supposed to be handled without any manual input.
Good point about detecting spends in view wallets.
Still way too complicated, but if you like it and others want it, it'll go in.

[moneromooo-monero]

Are you done ? Last comment says you'll think things through.

[ph4r05]

Maybe a few more days? :)

[moneromooo-monero]

Sure. Whatever you need, I just wasn't sure if you were done :)

[moneromooo-monero]

Is this ready now ?

[ph4r05]

I liked the idea this patch enables users to rescan_spent or import key images using untrusted daemon which may increase security as users just put --trusted with public node if something is not working for them (I think).

I think this PR is kind of finished for me now, I could rebase if you guys are fine with the approach and you think the PR won't make wallet more fragile or prone to user errors.

[moneromooo-monero]

Well, I don't like the way it goes about it (the hashing), but the overall idea is OK I think :)

[moneromooo-monero]

Do you want to rebase it ?

[ph4r05]

rebase finished

[fluffypony]

Reviewed