-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[seraphis] ringct: adjust multiexp #8679
Conversation
src/ringct/multiexp.cc
Outdated
rct::key res; | ||
ge_p3_tobytes(res.bytes, &result); | ||
const ge_p3 result_p3 = pippenger_p3(std::move(data), cache, cache_size, c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since pippenger_p3
takes const&
, you can just pass it as is
3d01747
to
8efdb10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like both straus
and pippenger
retain the exact same functionality . Would be nice to have some performance tests comparing the _p3
versions to their counterparts to see the efficiency difference. Besides that, just one minor warning thing.
You can check this perf test: https://github.com/monero-project/monero/blob/master/tests/performance_tests/ge_tobytes.h It's not significant, but still nice to shave off easy wins like this (iirc sarang was a big fan of avoiding serialization/deserialization ops). |
8efdb10
to
1d0142f
Compare
1d0142f
to
b986421
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointing out for other reviewers there's a nice unit test helping make sure pippenger_cached_data
retains behavior here:
monero/tests/unit_tests/multiexp.cpp
Lines 235 to 255 in 50aa0e8
TEST(multiexp, pippenger_cached) | |
{ | |
static constexpr size_t N = 256; | |
std::vector<rct::MultiexpData> P(N); | |
for (size_t n = 0; n < N; ++n) | |
{ | |
P[n].scalar = rct::zero(); | |
ASSERT_TRUE(ge_frombytes_vartime(&P[n].point, rct::scalarmultBase(rct::skGen()).bytes) == 0); | |
} | |
std::shared_ptr<rct::pippenger_cached_data> cache = rct::pippenger_init_cache(P); | |
for (size_t n = 0; n < N/16; ++n) | |
{ | |
std::vector<rct::MultiexpData> data; | |
size_t sz = 1 + crypto::rand<size_t>() % (N-1); | |
for (size_t s = 0; s < sz; ++s) | |
{ | |
data.push_back({rct::skGen(), P[s].point}); | |
} | |
ASSERT_TRUE(basic(data) == pippenger(data, cache)); | |
} | |
} |
This is a PR in my 'upstreaming seraphis_lib project', the changes here are not used anywhere yet.
pippenger_cached_data
to use a vector ofge_cached
directly. This makespippenger_cached_data
reusable (I use it in a multiexp utility in theseraphis_lib
).ge_p3
versions of the multiexp functions for efficiency when desired.