wallet: refactor subaddress expansion & add to transfer test

src/wallet/wallet2.cpp

@@ -1597,39 +1597,60 @@ bool wallet2::should_expand(const cryptonote::subaddress_index &index) const
 //----------------------------------------------------------------------------------------------------
 void wallet2::expand_subaddresses(const cryptonote::subaddress_index& index)
 {
-  hw::device &hwdev = m_account.get_device();
+  // check if index will overflow container (usually only applicable on 32-bit systems)
+  if constexpr (sizeof(std::size_t) <= sizeof(std::uint32_t))
+  {
+    static constexpr std::uint32_t max_idx = static_cast<std::uint32_t>(std::numeric_limits<std::size_t>::max());
+    const bool cannot_label_index = index.major == max_idx || index.minor == max_idx;
+    THROW_WALLET_EXCEPTION_IF(cannot_label_index, error::wallet_internal_error, "subaddress index out of range");
+  }
+
+  // resize subaddress labels just big enough that `m_subaddress_labels[index.major][index.minor]` is present
   if (m_subaddress_labels.size() <= index.major)
-  {
-    // add new accounts
-    cryptonote::subaddress_index index2;
-    const uint32_t major_end = get_subaddress_clamped_sum(index.major, m_subaddress_lookahead_major);
-    for (index2.major = m_subaddress_labels.size(); index2.major < major_end; ++index2.major)
-    {
-      const uint32_t end = get_subaddress_clamped_sum((index2.major == index.major ? index.minor : 0), m_subaddress_lookahead_minor);
-      const std::vector<crypto::public_key> pkeys = hwdev.get_subaddress_spend_public_keys(m_account.get_keys(), index2.major, 0, end);
-      for (index2.minor = 0; index2.minor < end; ++index2.minor)
-      {
-         const crypto::public_key &D = pkeys[index2.minor];
-         m_subaddresses[D] = index2;
-      }
-    }
     m_subaddress_labels.resize(index.major + 1, {"Untitled account"});
-    m_subaddress_labels[index.major].resize(index.minor + 1);
+  auto &subaddr_labels_in_account = m_subaddress_labels[index.major];
-    get_account_tags();
+  if (subaddr_labels_in_account.size() <= index.minor)
-  }
+    subaddr_labels_in_account.resize(index.minor + 1);
-  else if (m_subaddress_labels[index.major].size() <= index.minor)
+  get_account_tags(); //trigger m_account_tags integrity checks
+
+  // compile all indices present in subaddress scanning map, as well as every major index
+  std::unordered_set<cryptonote::subaddress_index> all_indices;
+  std::unordered_map<std::uint32_t, std::uint32_t> lowest_missing_minor;
+  for (const auto &p : m_subaddresses)
   {
-    // add new subaddresses
+    all_indices.insert(p.second);
-    const uint32_t end = get_subaddress_clamped_sum(index.minor, m_subaddress_lookahead_minor);
+    lowest_missing_minor[p.second.major];
-    const uint32_t begin = m_subaddress_labels[index.major].size();
+  }
-    cryptonote::subaddress_index index2 = {index.major, begin};
+
-    const std::vector<crypto::public_key> pkeys = hwdev.get_subaddress_spend_public_keys(m_account.get_keys(), index2.major, index2.minor, end);
+  // find lowest "missing" minor index in map, for all major indices
-    for (; index2.minor < end; ++index2.minor)
+  // this is an optimization which allows us to skip re-generating pubkeys that we already have
-    {
+  for (auto &p : lowest_missing_minor)
-       const crypto::public_key &D = pkeys[index2.minor - begin];
+  {
-       m_subaddresses[D] = index2;
+    const std::uint32_t major = p.first;
+    std::uint32_t &minor = p.second;
+    while (all_indices.count({major, minor}) && minor < std::numeric_limits<std::uint32_t>::max())
+      ++minor;
+  }
+
+  // resize subaddress scanning map to highest historical received subaddress index plus lookahead
+  hw::device &hwdev = m_account.get_device();
+  const std::uint32_t major_base = std::max<std::uint32_t>(m_subaddress_labels.size(), 1) - 1;
+  const std::uint32_t major_end = get_subaddress_clamped_sum(major_base, m_subaddress_lookahead_major);
+  for (std::uint32_t major = 0; major < major_end; ++major)
+  {
+    const std::size_t n_minor_labels = (major < m_subaddress_labels.size()) ? m_subaddress_labels.at(major).size() : 0;
+    const std::uint32_t minor_base = std::max<std::uint32_t>(n_minor_labels, 1) - 1;
+    const std::uint32_t minor_end = get_subaddress_clamped_sum(minor_base, m_subaddress_lookahead_minor);
+    const std::uint32_t minor_begin = lowest_missing_minor.count(major) ? lowest_missing_minor.at(major) : 0;
+    if (minor_begin >= minor_end)
+      continue;
+    const std::vector<crypto::public_key> pkeys
+      = hwdev.get_subaddress_spend_public_keys(m_account.get_keys(), major, minor_begin, minor_end);
+    for (std::uint32_t minor = minor_begin; minor < minor_end; ++minor)
+    {
+      const crypto::public_key &D = pkeys.at(minor - minor_begin);
+      m_subaddresses[D] = {major, minor};
     }
-    m_subaddress_labels[index.major].resize(index.minor + 1);
   }
 }
 //----------------------------------------------------------------------------------------------------
@@ -1957,32 +1978,7 @@ void wallet2::set_subaddress_lookahead(size_t major, size_t minor)
   if (old_major_lookahead >= major && old_minor_lookahead >= minor)
     return;
 
-  // Expand the subaddresses map so that outputs received to the higher lookaheads will be identified in the scan loop
+  expand_subaddresses({0, 0});
-  hw::device &hwdev = m_account.get_device();
-  cryptonote::subaddress_index index2;
-  const uint32_t max_major_idx = this->get_num_subaddress_accounts() > 0 ? (this->get_num_subaddress_accounts() - 1) : 0;
-  const uint32_t major_end = get_subaddress_clamped_sum(max_major_idx, major);
-  for (index2.major = 0; index2.major < major_end; ++index2.major)
-  {
-    // The existing minor addresses already set for this account
-    const uint32_t n_minor_subaddrs = this->get_num_subaddresses(index2.major);
-
-    // The subaddress lookahead is expected to expand from the max index in expand_subaddresses
-    const uint32_t max_minor_idx = n_minor_subaddrs > 0 ? (n_minor_subaddrs - 1) : 0;
-    const uint32_t begin = (n_minor_subaddrs || index2.major < old_major_lookahead) ? get_subaddress_clamped_sum(max_minor_idx, old_minor_lookahead) : 0;
-    // The expected new n minor subaddresses allocated for this account
-    const uint32_t end = get_subaddress_clamped_sum(max_minor_idx, minor);
-
-    if (begin >= end)
-      continue;
-
-    const std::vector<crypto::public_key> pkeys = hwdev.get_subaddress_spend_public_keys(m_account.get_keys(), index2.major, begin, end);
-    for (index2.minor = begin; index2.minor < end; ++index2.minor)
-    {
-      const crypto::public_key &D = pkeys.at(index2.minor - begin);
-      m_subaddresses[D] = index2;
-    }
-  }
 }
 //----------------------------------------------------------------------------------------------------
 /*!

src/wallet/wallet2.h

@@ -1124,6 +1124,13 @@ private:
     size_t get_num_subaddress_accounts() const { return m_subaddress_labels.size(); }
     size_t get_num_subaddresses(uint32_t index_major) const { return index_major < m_subaddress_labels.size() ? m_subaddress_labels[index_major].size() : 0; }
     void add_subaddress(uint32_t index_major, const std::string& label); // throws when index is out of bound
+    /**
+     * brief: expand subaddress labels up to `index`, and scanning map to highest labeled index plus current lookahead
+     * param: index -
+     *
+     * All calls to `expand_subaddresses()` will *always* expand the subaddress map if the lookahead
+     * values have been increased since the last call.
+     */
     void expand_subaddresses(const cryptonote::subaddress_index& index);
     void create_one_off_subaddress(const cryptonote::subaddress_index& index);
     std::string get_subaddress_label(const cryptonote::subaddress_index& index) const;

tests/functional_tests/transfer.py

@@ -86,6 +86,7 @@ class TransferTest():
         self.check_subtract_fee_from_outputs()
         self.check_background_sync()
         self.check_background_sync_reorg_recovery()
+        self.check_subaddress_lookahead()
 
     def reset(self):
         print('Resetting blockchain')
@@ -1531,5 +1532,47 @@ class TransferTest():
         self.wallet[0].close_wallet()
         self.wallet[0].restore_deterministic_wallet(seed = seeds[0])
 
+    def check_subaddress_lookahead(self):
+        daemon = Daemon()
+
+        print('Testing transfers to subaddresses with large lookahead')
+
+        # From wallet 1 to wallet 0 at subaddress (0, 999)
+
+        address_0_999 = '8BQKgTSSqJjP14AKnZUBwnXWj46MuNmLvHfPTpmry52DbfNjjHVvHUk4mczU8nj8yZ57zBhksTJ8kM5xKeJXw55kCMVqyG7' # this is the address for address 999 of the main account in the test wallet
+        try:  # assert address_1_999 is not in the current pubkey table
+            self.wallet[0].get_address_index(address_0_999)
+            assert False # address should not already be loaded
+        except Exception as e:
+            assert str(e) ==  "{'error': {'code': -2, 'message': \"Address doesn't belong to the wallet\"}, 'id': '0', 'jsonrpc': '2.0'}"
+        # update the lookahead and assert the high index address is now in the table
+        self.wallet[0].set_subaddress_lookahead(50, 1000)
+        res = self.wallet[0].get_address_index(address_0_999)
+        assert res['index']['major'] == 0
+        assert res['index']['minor'] == 999
+
+        dst = {'address': address_0_999, 'amount': 454545454545}
+
+        self.wallet[1].refresh()
+        assert self.wallet[1].get_balance().balance > dst['amount']
+        self.wallet[1].transfer([dst])
+        daemon.generateblocks('46r4nYSevkfBUMhuykdK3gQ98XDqDTYW1hNLaXNvjpsJaSbNtdXh1sKMsdVgqkaihChAzEy29zEDPMR3NHQvGoZCLGwTerK', 1)
+        self.wallet[0].refresh()
+
+        res = self.wallet[0].get_balance()
+        balance_info_0_999 = None
+        for balance_info in res['per_subaddress']:
+            if balance_info['account_index'] == 0 and balance_info['address_index'] == 999:
+                balance_info_0_999 = balance_info
+                break
+        assert balance_info_0_999 is not None, "balance info for address (0, 999) not found"
+        assert balance_info_0_999['address'] == address_0_999
+        assert balance_info_0_999['balance'] == dst['amount']
+        assert balance_info_0_999['unlocked_balance'] == 0
+        assert balance_info_0_999['label'] == ''
+        assert balance_info_0_999['num_unspent_outputs'] == 1
+        assert balance_info_0_999['blocks_to_unlock'] == 9
+        assert balance_info_0_999['time_to_unlock'] == 0
+
 if __name__ == '__main__':
     TransferTest().run_test()