wallet: faster value conveyance via five various velocity advances #8046
Conversation
src/wallet/ringdb.cpp
Outdated
| MDB_val key, data; | ||
| std::string key_ciphertext = encrypt(key_image, chacha_key, 0); | ||
| std::string key_ciphertext = encrypt(key_images[i], chacha_key, 0); |
There was a problem hiding this comment.
This change is unnecessary, key_image can be left here.
src/wallet/ringdb.cpp
Outdated
| std::vector<std::vector<uint64_t>> all_outs; | ||
| if (!get_rings(chacha_key, std::vector<crypto::key_image>(1, key_image), all_outs)) | ||
| return false; | ||
| outs = all_outs.front(); |
There was a problem hiding this comment.
outs = std::move(all_outs.front()); should be faster
src/ringct/rctSigs.cpp
Outdated
| { | ||
| const rct::key I = rct::identity(); | ||
| const size_t n_scalars = ring_size; | ||
| return rct::clsag{rct::keyV(n_scalars, I), I, I, I};; |
moneromooo-monero
force-pushed
the
v
branch
2 times, most recently
from
e5c67d3 to
7c0f2f8
Compare
|
@moneromooo-monero the description of each commit says |
There was a problem hiding this comment.
All commits LGTM except bd74083. I can't see how that one should be an improvement
@SChernykh AFAICT the improvements look like they could be significant when constructing a tx with many inputs, guessing that's the test case
| @@ -7763,16 +7780,18 @@ bool wallet2::tx_add_fake_output(std::vector<std::vector<tools::wallet2::get_out | |||
| if (std::find(outs.back().begin(), outs.back().end(), item) != outs.back().end()) // don't add duplicates | |||
| return false; | |||
| // check the keys are valid | |||
| if (!rct::isInMainSubgroup(rct::pk2rct(output_public_key))) | |||
| if (valid_public_keys_cache.find(output_public_key) == valid_public_keys_cache.end() && !rct::isInMainSubgroup(rct::pk2rct(output_public_key))) | |||
There was a problem hiding this comment.
The only circumstance I can think of where this valid_public_keys_cache is an improvement is if there are duplicate fake outs selected in a single tx, and imo duplicates should be prevented in the first place.
Are there other circumstances I'm missing where the wallet would attempt to re-add the same fake out such that valid_public_keys_cache would be an improvement? I can't repro it when trying a normal transfer
Alternative suggestion that would take a fair amount of effort and require a migration: if you were to also store the output public key and mask in ringdb, then you can assume the values coming from ringdb are valid and just do a simple equality check of the values stored in ringdb against the values returned by the daemon, rather than the more expensive rct::isInMainSubgroup checks (since to get into ringdb in the first place, the rct::isInMainSubgroup checks must have already been performed)... Probably not worth the effort since ringdb-based tx's should be rare in practice anyway
There was a problem hiding this comment.
All commits LGTM except bd74083
It does say 5.9 second -> 5.2 seconds, so there is an improvement.
There was a problem hiding this comment.
I meant if that commit's improvement is because the sample test case is selecting duplicate fake outs in a single tx, then I would think the commit probably shouldn't be included because I think duplicate outs should actually be prevented instead..
Unless there is some other way that this would yield an improvement I'm not seeing
There was a problem hiding this comment.
IIRC this is because this function ends up called several times when selecting rings. I'll try it to make sure.
There was a problem hiding this comment.
Doesn't seem to. Possibly I made several txes in a row to average, I don't remember. In that case, the timing would be unrepresentative, unless one cancels a tx, or most of the outputs the wallet requests are unusable, in which case the wallets asks for more (I think this includes the previous set). Probably down to averaging.
There was a problem hiding this comment.
Actually, I quite possibly tested on a short chain on which I mined a few hundred blocks first, so that'd mean fake out sets with overlapping outputs as you suggested. Doesn't seem enough to cause 5.9 -> 5.2 though, but who knows.
There was a problem hiding this comment.
I didn't consider reusing across attempts could come into play here, fair enough. Your call on it. Approving as is
5.9 second -> 5.2 seconds on a test case
5.2 seconds -> 4.1 seconds on a test case
4.1 seconds -> 3.3 seconds on a test case
3.3 seconds -> 2.8 seconds on a test case
2.8 seconds -> 2.6 seconds on a test case
|
I know of no reason why the monero revolution should ever be forgot. |
No description provided.