wallet: immediate lookahead table expansion + set_subaddresss_lookahead RPC endpoint #9953
Conversation
|
Why was the original PR closed instead of continuing there ? |
|
merge conflicts, op unavailable (is what i thought. idc where it continues. Was requested that someone continue it) |
|
a review on if the logic is in the correct place / manner and is not duplicating some other function would be useful. e.g. we have a so if someone who knows what they're looking at can give feedback on if this needs, or would be better as a |
There was a problem hiding this comment.
or would be better as a
should_expandthen call toexpand_subaddresses
@plowsof I thought this might be the case too, but actually that would create labels for all subaddresses in the lookahead range, which would clutter UI's / isn't necessary. @benevanoff's decision not to do that looks correct to me. My suggestion is minor (but worth doing imo), the PR looks good as is and this was tricky to get right. Thank you @benevanoff for this code. And thank you @nahuhh for following up.
There was a problem hiding this comment.
Sorry, @plowsof's comment here made me realize a bug in my code (and a separate bug in the original code as well). These changes include a fix and test cases that capture the bugs.
The fix matches the logic of set_address_lookahead to expand_subaddresses. So repeating @plowsof 's test:
starting with a
1:10lookahead. sending to above expanded , then decreasing idx until i find the max idx
With this fix, the table will look like this:
| lookahead | max idx |
|---|---|
| 1:10 | 9 |
| 18 | |
| 27 | |
| 1:20 | 46 |
| 1:1000 | 1045 |
It's a little counter-intuitive that the lookahead only reaches the max idx + lookahead - 1 in each step.
I think that may make sense to change in both expand_subaddresses and set_subaddress_lookahead in a separate PR. But for now, this matches the logic of set_subaddress_lookahead to expand_subaddresses.
|
confirming the above behaviour. thanks for going through my ramblings and debugging further @j-berman . for those testing: do note that an output within the expanded area will be detected in the pool but it will not further expand until at least 1 block confirmation. closes #7364 can be added to this one |
|
before anyone asks for squash, i'm waiting for 9971 merge (ci fix) |
|
sanity checking @nahuhh comment here about wallet-rpc and persistance #9954 (comment) (i thought this was a simple case of not calling open with cli
|
|
RPC persistence should probably be in this PR imo. Try this patch: diff --git a/src/wallet/wallet_rpc_server.cpp b/src/wallet/wallet_rpc_server.cpp
index 1ed8eb9fc..4f349e4bd 100644
--- a/src/wallet/wallet_rpc_server.cpp
+++ b/src/wallet/wallet_rpc_server.cpp
@@ -707,11 +707,24 @@ namespace tools
bool wallet_rpc_server::on_set_subaddr_lookahead(const wallet_rpc::COMMAND_RPC_SET_SUBADDR_LOOKAHEAD::request& req, wallet_rpc::COMMAND_RPC_SET_SUBADDR_LOOKAHEAD::response& res, epee::json_rpc::error& er, const connection_context *ctx)
{
if (!m_wallet) return not_open(er);
- try {
- m_wallet->set_subaddress_lookahead(req.major_idx, req.minor_idx);
+ CHECK_IF_BACKGROUND_SYNCING();
+ const std::string wallet_file = m_wallet->get_wallet_file();
+ if (wallet_file == "" || m_wallet->verify_password(req.password))
+ {
+ try
+ {
+ m_wallet->set_subaddress_lookahead(req.major_idx, req.minor_idx);
+ m_wallet->rewrite(wallet_file, req.password);
+ }
+ catch (const std::exception& e) {
+ handle_rpc_exception(std::current_exception(), er, WALLET_RPC_ERROR_CODE_UNKNOWN_ERROR);
+ return false;
+ }
}
- catch (const std::exception& e) {
- handle_rpc_exception(std::current_exception(), er, WALLET_RPC_ERROR_CODE_UNKNOWN_ERROR);
+ else
+ {
+ er.code = WALLET_RPC_ERROR_CODE_INVALID_PASSWORD;
+ er.message = "Invalid password.";
return false;
}
return true;
diff --git a/src/wallet/wallet_rpc_server_commands_defs.h b/src/wallet/wallet_rpc_server_commands_defs.h
index 0871d6369..d0273d162 100644
--- a/src/wallet/wallet_rpc_server_commands_defs.h
+++ b/src/wallet/wallet_rpc_server_commands_defs.h
@@ -186,9 +186,11 @@ namespace wallet_rpc
{
struct request_t
{
+ std::string password;
uint64_t major_idx;
uint64_t minor_idx;
BEGIN_KV_SERIALIZE_MAP()
+ KV_SERIALIZE(password)
KV_SERIALIZE(major_idx)
KV_SERIALIZE(minor_idx)
END_KV_SERIALIZE_MAP()
diff --git a/utils/python-rpc/framework/wallet.py b/utils/python-rpc/framework/wallet.py
index 88cece2bc..d0a516c1f 100644
--- a/utils/python-rpc/framework/wallet.py
+++ b/utils/python-rpc/framework/wallet.py
@@ -334,11 +334,12 @@ class Wallet(object):
}
return self.rpc.send_json_rpc_request(generate_from_keys)
- def set_subaddress_lookahead(self, major_idx: int, minor_idx: int):
+ def set_subaddress_lookahead(self, major_idx: int, minor_idx: int, password = ""):
lookahead = {
'method': 'set_subaddress_lookahead',
'jsonrpc': '2.0',
'params' : {
+ 'password': password,
'major_idx': major_idx,
'minor_idx': minor_idx
},
|
|
patch works , thank you! wallet cli shows the new lookahead value 👍 |
| // Expand the subaddresses map so that outputs received to the higher lookaheads will be identified in the scan loop | ||
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not use this->expand_subaddresses()? (Before/conditional on setting m_subaddress_lookahead_major and m_subaddress_lookahead_minor above)
There was a problem hiding this comment.
expand_subaddresses expects a user to have received to a specific subaddr index, and then expands the subaddress map (and adds subaddress labels) from there. UI's populate subaddresses based on the labels that are set. So we don't want to add any new labels from setting this lookahead. We could have expand_subaddresses reuse this logic from here, but I think this approach is reasonable.
There was a problem hiding this comment.
I will need to double-check, but I think a lot of the wallet2 balance calls will break if the subaddress map is populated, but not the subaddress labels. See: #9934 (comment). A lot of higher-level calling code iterates through the subaddresses wallet2 provides using the get_num_subaddresses() and get_num_subaddress_accounts() calls, which query m_subaddress_labels, not m_subaddresses.
There was a problem hiding this comment.
expand_subaddresses already does expand the subaddresses map past the subaddress labels the same way that this new logic does (see the new unit tests), so the wallet2 balance calls would be currently broken if true
There was a problem hiding this comment.
Okay good point. On first receive during refresh, should_expand will return true for that receive address index if the subaddress lookahead was already set previously, in which case the subaddress labels will be filled up to that index. So basically the account/subaddress counts will be "wrong" until the first receive, but those would have 0 balance in them anyways. You also can't lookup the subaddress by index until it's first receive, which is funky, but that's already the case for lookahead addresses. I still don't like the code duplication though...
|
only changes in the rebase are to apply this patch from @j-berman |
| @@ -182,6 +182,29 @@ namespace wallet_rpc | |||
| typedef epee::misc_utils::struct_init<response_t> response; | |||
| }; | |||
|
|
|||
| struct COMMAND_RPC_SET_SUBADDR_LOOKAHEAD | |||
There was a problem hiding this comment.
The RPC minor version should be bumped.
|
Here's an alternative approach (apply diff to current HEAD): diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 23a0880e6..ca8343c51 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -1597,39 +1597,62 @@ 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)
+ m_subaddress_labels.resize(index.major + 1, {"Untitled account"});
+ auto &subaddr_labels_in_account = m_subaddress_labels[index.major];
+ if (subaddr_labels_in_account.size() <= index.minor)
+ subaddr_labels_in_account.resize(index.minor + 1);
+ get_account_tags(); //trigger m_account_tags integrity checks
+
+ // compile all indices present in subaddress scanning map, as well as find highest major index
+ std::unordered_set<cryptonote::subaddress_index> all_indices;
+ std::uint32_t max_major_index = 0;
+ for (const auto &p : m_subaddresses)
{
- // 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)
+ all_indices.insert(p.second);
+ max_major_index = std::max(max_major_index, p.second.major);
+ }
+
+ // find lowest "missing" minor index in map, for all major indices
+ // this is an optimization which allows us to skip re-generating pubkeys that we already have
+ std::vector<std::uint32_t> missing_minor_index(max_major_index + 1);
+ for (std::uint32_t major = 0; major < missing_minor_index.size(); ++major)
+ {
+ std::uint32_t &minor = missing_minor_index[major];
+ while (minor < std::numeric_limits<std::uint32_t>::max())
{
- 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;
- }
+ if (!all_indices.count({major, minor}))
+ break;
+ ++minor;
}
- m_subaddress_labels.resize(index.major + 1, {"Untitled account"});
- m_subaddress_labels[index.major].resize(index.minor + 1);
- get_account_tags();
}
- else if (m_subaddress_labels[index.major].size() <= index.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)
{
- // add new subaddresses
- const uint32_t end = get_subaddress_clamped_sum(index.minor, m_subaddress_lookahead_minor);
- 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);
- for (; index2.minor < end; ++index2.minor)
+ 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 = (major < missing_minor_index.size()) ? missing_minor_index.at(major) : 0;
+ 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[index2.minor - begin];
- m_subaddresses[D] = index2;
+ 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 +1980,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
- 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;
- }
- }
+ expand_subaddresses({0, 0});
}
//----------------------------------------------------------------------------------------------------
/*!
diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h
index 47b1753d7..41f1dffaa 100644
--- a/src/wallet/wallet2.h
+++ b/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;As compared to current |
|
Here's a functional test that tests live transfers to subaddresses at high lookaheads, IDK, might be worth adding?: diff --git a/tests/functional_tests/transfer.py b/tests/functional_tests/transfer.py
index 0df870e9c..f1fb6e51d 100755
--- a/tests/functional_tests/transfer.py
+++ b/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()
|
|
I'm fine with de-duplicating logic, calling Looks like there are a couple problems with the updated
|
|
Force pushed to remove extra line on EOF and to rerun tests (passing now) |
replaces #8981
no changeschanges from original:
#include <boost/optional/optional_io.hpp>resolves #8980
resolves #8954
resolves #7364