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

Speed up proof generation #103

Merged
merged 51 commits into from
Feb 24, 2023
Merged

Speed up proof generation #103

merged 51 commits into from
Feb 24, 2023

Conversation

fasmat
Copy link
Member

@fasmat fasmat commented Feb 10, 2023

Motivation

Part of #99. This change replaces the current proof generation with a new method using AES that should significantly speed up the process

Changes

  • A new proving.Generate function has been added that implements the new algorithm for proof generation.
  • A new verifying.VerifyNew function has been added that implements the new algorithm for proof verification.
  • The old Prover has been marked as deprecated but not modified/removed to allow for benchmark comparisons
  • The parameters for K1/K2, N, B have been chosen in a way that they work fast and reliably in unit tests. They are not optimised for security and performance in an actual network. For these parameters see: [Merged by Bors] - Integrate new PoST using AES go-spacemesh#4071

Optional:

@fasmat fasmat self-assigned this Feb 10, 2023
@fasmat fasmat marked this pull request as ready for review February 20, 2023 16:29
@fasmat fasmat requested a review from poszu February 20, 2023 16:29
@fasmat fasmat requested a review from pigmej February 20, 2023 19:44
Copy link
Member

@pigmej pigmej left a comment

Choose a reason for hiding this comment

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

From logic perspective, from speedup perspective yes. But I did not checked the code ;-)

@pigmej
Copy link
Member

pigmej commented Feb 22, 2023

Looked a bit more, @fasmat what's the reason for keeping the old implementation that we know it's broken?

@fasmat
Copy link
Member Author

fasmat commented Feb 22, 2023

I already needed it multiple times for reference, like benchmarking and comparison with the current version. I wouldn't have been able to fix the system tests with the new version so quickly if the old version would not have been around any more.

The old algorithms are all marked as deprecated. If any code still uses them we will get a warning in the IDE and in our CI.

We can remove them at any point. Why does it have to be now, when we might still need them for reference?

@pigmej
Copy link
Member

pigmej commented Feb 22, 2023

not only slow, @fasmat it's also broken and missing few items that would make it possible to generate proof correctly.

@fasmat
Copy link
Member Author

fasmat commented Feb 22, 2023

Again, that's not the point. We are in a transitional period the "new code" is also already decided to be replaced with a new implementation in Rust.

It doesn't hurt to have them around for reference until we have the version we want to use for Genesis.

@fasmat fasmat force-pushed the 99-speed-up-proof-generation branch from 707876d to 058b29f Compare February 22, 2023 13:03
proving/proving.go Show resolved Hide resolved
proving/worker.go Show resolved Hide resolved
proving/worker.go Outdated Show resolved Hide resolved
proving/worker.go Outdated Show resolved Hide resolved
proving/worker.go Show resolved Hide resolved
verifying/verifying.go Show resolved Hide resolved
verifying/verifying.go Outdated Show resolved Hide resolved
verifying/verifying.go Show resolved Hide resolved
@fasmat fasmat merged commit b36de2e into develop Feb 24, 2023
@fasmat fasmat deleted the 99-speed-up-proof-generation branch February 24, 2023 17:13
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Feb 24, 2023
## Motivation
Integrates the changes in post from spacemeshos/post#103

## Changes
New algorithms for generating and verifying proofs have been implemented. This PR integrates those.

## Test Plan
- Existing Tests need to pass.
- New tests where applicable

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [x] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
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.

3 participants