Skip to content
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

bip-340: recreate batch verify speedup graph w/ latest libsecp256k1 #1096

Closed
wants to merge 2 commits into from
Closed

bip-340: recreate batch verify speedup graph w/ latest libsecp256k1 #1096

wants to merge 2 commits into from

Conversation

jonasnick
Copy link
Contributor

Since creating that graph there have been significant improvements in
libsecp256k1 to the speed of certain operations, such as making use of the
endomorphism by default and the safegcd algorithm. This new graph shows the
batch verification speedup in light of these improvements.

There are also some minor improvements to the graph itself. In particular,
the (1,1) datapoint was removed because it is not part of the estimation for the
fitted line.

This commit also contains the Makefile, shell script and gnuplot file that were
used to create the graph.

CC @sipa @real-or-random

@sipa
Copy link
Member

sipa commented Apr 2, 2021

Trying to run your benchmark code I get:

$ make
cat raw_output.txt | grep -v "schnorrsig_batch_verify_1" | awk 'match($0,                         /schnorrsig_batch_verify_(.*):.*avg (.*)us /, a) {print a[1] " " a[2]}' >                         batch.dat                                                                         awk: line 1: syntax error at or near ,

Since creating that graph there have been significant improvements in
libsecp256k1 to the speed of certain operations, such as making use of the
endomorphism by default and the safegcd algorithm. This new graph shows the
batch verification speedup in light of these improvements.

There are also some minor improvements to the graph itself. In particular,
the (1,1) datapoint was removed because it is not part of the estimation for the
fitted line.

This commit also contains the Makefile, shell script and gnuplot file that were
used to create the graph.
@jonasnick
Copy link
Contributor Author

@sipa Hm, I can't reproduce this. awk --version is GNU Awk 5.1.0 on my end.

@sipa
Copy link
Member

sipa commented Apr 2, 2021

@jonasnick Ok, apparently I was using "mawk" instead of "gawk".

I redid the benchmarks myself (Intel i7-7820HQ, clock fixed at 2.6 Ghz) with higher granularity and higher iteration count, and got this graph:

speedup-batch

I'm not sure a logarithmic interpolation is what we need...

@jonasnick jonasnick marked this pull request as draft April 2, 2021 17:04
@jonasnick
Copy link
Contributor Author

I'll rerun the experiment with similar granularity. That many data points makes the fitted line useless anyway.

@sipa
Copy link
Member

sipa commented Apr 2, 2021

New graph with much higher scratch space:

speedup-batch

@jonasnick
Copy link
Contributor Author

Before creating a new graph, we should take into account 1) sipa#219 and 2) new estimated optimal pippenger threshold/windows.

@sipa
Copy link
Member

sipa commented Apr 13, 2021

@jonasnick Also, the optimal pippenger thresholds/windows may be affected by the use of 128-bit randomizers?

@0racl3z
Copy link

0racl3z commented Apr 13, 2021 via email

@luke-jr
Copy link
Member

luke-jr commented Apr 22, 2021

What is the status here? It's marked as a Draft...

@sipa
Copy link
Member

sipa commented Apr 22, 2021

I think this is not ready for merge.

@jonasnick
Copy link
Contributor Author

@luke-jr I changed this to draft after we realized that this PR isn't satisfactory, requires more work and has a dependency on another potential BIP-340 update. If you prefer, we can (temporarily) close this PR and move the discussion elsewhere.

@0racl3z
Copy link

0racl3z commented Apr 23, 2021 via email

@gmaxwell
Copy link
Contributor

An alternative might just be removing the speedup details from the BIP entirely (or just for now).

@0racl3z
Copy link

0racl3z commented Apr 27, 2021 via email

@real-or-random
Copy link
Contributor

An alternative might just be removing the speedup details from the BIP entirely (or just for now).

I like that idea. This BIP a spec, which should be valid for years. So at some point the numbers will be outdated anyway... (Yeah, we have also other "motivational" sections which will be outdated at some point but I still don't think we need the speedup numbers.)

@gmaxwell
Copy link
Contributor

gmaxwell commented May 5, 2021

Should be sufficient for the purpose of the BIP to just say that batch validation is a substantial speedup.

Copy link

@0racl3z 0racl3z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

@jonasnick
Copy link
Contributor Author

Given that there are apparently many factors that significantly affect the speedup I agree that this plot is not very useful to a reader. We don't even mention that the graph only applies to a specific implementation. And for it's limited usefulness the graph draws too much attention in the BIP.

I'd suggest to add the graph to the libsecp doc/ dir and link to it from this BIP to prove that batching does in fact lead to a substantial speedup.

@real-or-random
Copy link
Contributor

I'd suggest to add the graph to the libsecp doc/ dir and link to it from this BIP to prove that batching does in fact lead to a substantial speedup.

That's a good idea.

@0racl3z
Copy link

0racl3z commented May 14, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants