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

Fix fast 1-weso parameter approximation, for massive speedup #205

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xearl4
Copy link
Contributor

@xearl4 xearl4 commented Aug 27, 2024

The "fast" 1-Wesolowski prover inadvertently mixed up parameters in the call to ApproximateParameter.

ApproximateParameter's signature has L followed by K:

void ApproximateParameters(uint64_t T, uint32_t& L, uint32_t& K)

However, the 1-Weso calls used K followed by L:

ApproximateParameters(num_iterations, k, l);

While this typo does not affect the correctness of the created 1-Weso proof, this causes 1-Weso proving to be much slower than necessary. In isolated testing, the 1-Weso proof generation is up to 10x slower.

Funnily enough, the "slow" VDF impl (in src/provers_slow.h) uses a separate 1-Weso prover implementation which doesn't suffer from this bug, thus making the "slow" VDF impl faster than the "fast" overall.

To be clear: the fixed bug only affects the performance (not the correctness!) of computing 1-Weso proofs (thus, blueboxing) using the "fast" implementation. Regular software timelords (N-Weso), hardware (ASIC) timelords, and the "slow" bluebox implementation are unaffected.

The speedup can be observed/tested by running src/1weso_test.cpp:

$ hyperfine --warmup 5 "./1weso_test.orig" "./1weso_test.fixed"
Benchmark 1: ./1weso_test.orig
  Time (mean ± σ):     17.250 s ±  1.153 s    [User: 23.946 s, System: 0.037 s]
  Range (min … max):   15.582 s … 18.372 s    10 runs

Benchmark 2: ./1weso_test.fixed
  Time (mean ± σ):      8.746 s ±  0.010 s    [User: 14.651 s, System: 0.029 s]
  Range (min … max):    8.733 s …  8.765 s    10 runs

Summary
  ./1weso_test.fixed ran
    1.97 ± 0.13 times faster than ./1weso_test.orig

1weso_test also confirms that 1-Weso proof correctness is unaffected by this change. The prover still generates the exact same proofs, it just does so much faster.

The "fast" 1-Wesolowski prover inadvertently mixed up parameters in the
call to ApproximateParameter.

ApproximateParameter's signature has L followed by K:

    void ApproximateParameters(uint64_t T, uint32_t& L, uint32_t& K)

However, the 1-Weso calls used K followed by L:

    ApproximateParameters(num_iterations, k, l);

While this typo does not affect the correctness of the created 1-Weso
proof, this causes 1-Weso proving to be _much_ slower than necessary. In
isolated testing, the 1-Weso proof generation is up to 10x slower.

Funnily enough, the "slow" VDF impl (in `src/provers_slow.h`) uses a
separate 1-Weso prover implementation which _doesn't_ suffer from this
bug, thus making the "slow" VDF impl faster than the "fast" overall.

To be clear: the fixed bug _only_ affects the _performance_ (not the
correctness!) of computing 1-Weso proofs (thus, blueboxing) using the
"fast" implementation. Regular software timelords (N-Weso), hardware
(ASIC) timelords, and the "slow" bluebox implementation are unaffected.

The speedup can be observed/tested by running `src/1weso_test.cpp`:

    $ hyperfine --warmup 5 "./1weso_test.orig" "./1weso_test.fixed"
    Benchmark 1: ./1weso_test.orig
      Time (mean ± σ):     17.250 s ±  1.153 s    [User: 23.946 s, System: 0.037 s]
      Range (min … max):   15.582 s … 18.372 s    10 runs

    Benchmark 2: ./1weso_test.fixed
      Time (mean ± σ):      8.746 s ±  0.010 s    [User: 14.651 s, System: 0.029 s]
      Range (min … max):    8.733 s …  8.765 s    10 runs

    Summary
      ./1weso_test.fixed ran
        1.97 ± 0.13 times faster than ./1weso_test.orig
Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

Many thanks!

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.

2 participants