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

Multi threading on Lelantus-sigma prover and sigma/lelantus batch verification #1058

Merged
merged 16 commits into from
Mar 18, 2022

Conversation

levonpetrosyan93
Copy link
Contributor

  1. Adding simple thread pool,
  2. Applying multithrading on Lelantus-sigma prover
  3. Applying multithrading on batch verification during sync/reindex.

@levonpetrosyan93 levonpetrosyan93 changed the title Multithreading on Lelantus-sigma prover and sigma/lelantus batch verification Multi threading on Lelantus-sigma prover and sigma/lelantus batch verification Aug 1, 2021
@lgtm-com
Copy link

lgtm-com bot commented Aug 1, 2021

This pull request fixes 3 alerts when merging e26a7fd into dbce40b - view on LGTM.com

fixed alerts:

  • 3 for Unused static function

@a-bezrukov
Copy link
Contributor

Thread sanitizer does not report anything new for the batching.

@reubenyap reubenyap added this to the v0.14.9.1 Maintenance Release milestone Sep 26, 2021
@reubenyap
Copy link
Member

Could you post the before/after benchmarks here?

@levonpetrosyan93
Copy link
Contributor Author

levonpetrosyan93 commented Dec 19, 2021

Benchmarking on Lelantus for creation of multiple sigma proofs
Running on i7-11700K 3.60GHz × 16

number of coins 1, parallel proof time 1624, regular proof time 1616
number of coins 2, parallel proof time 1672, regular proof time 3155
number of coins 8, parallel proof time 2046, regular proof time 12598
number of coins 12 parallel proof time 2602 regular proof time 18833
number of coins 16 parallel proof time 3078 regular proof time 24985
number of coins 20 parallel proof time 4719 regular proof time 31159

@levonpetrosyan93 levonpetrosyan93 requested review from levoncrypto and removed request for a-bezrukov December 27, 2021 12:48
std::list<std::thread> threadsToJoin;

{
std::unique_lock<std::mutex> lock(task_queue_mutex);
Copy link
Contributor

Choose a reason for hiding this comment

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

use std::scoped_lock here (if we have C++17, if no then std::lock_guard)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


void ThreadProc() {
for (;;) {
std::packaged_task<Result()> job;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line

Copy link
Contributor

Choose a reason for hiding this comment

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

and also remove brackets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
break;
}
job = std::move(task_queue.front());
Copy link
Contributor

Choose a reason for hiding this comment

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

make this line
std::packaged_task<Result()> job (std::move(task_queue.front()));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

std::packaged_task<Result()> packagedTask(std::move(task));
std::future<Result> ret = packagedTask.get_future();

std::unique_lock<std::mutex> lock(task_queue_mutex);
Copy link
Contributor

Choose a reason for hiding this comment

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

use std::scoped_lock here (if we have C++17, if no then std::lock_guard)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


lelantus::SigmaExtendedVerifier sigmaVerifier(params->get_g(), params->get_sigma_h(), params->get_sigma_n(),
params->get_sigma_m());
lelantus::SigmaExtendedVerifier sigmaVerifier(params->get_g(), params->get_sigma_h(), params->get_sigma_n(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can move this outside of loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

parallelTasks.reserve(threadsMaxCount);
ParallelOpThreadPool<void> threadPool(threadsMaxCount);

std::vector<std::vector<GroupElement>> C_;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can store only one array, and clear() it at the start of each iteration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are passing all values by reference, storing and clearing will not work.

Yk[i].resize(params->get_sigma_m());
a[i].resize(params->get_sigma_n() * params->get_sigma_m());

auto& sigma_i = sigma[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

const auto&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, we need to fill i-th vector

levoncrypto
levoncrypto previously approved these changes Jan 16, 2022
@lgtm-com
Copy link

lgtm-com bot commented Feb 21, 2022

This pull request introduces 2 alerts when merging a75b339 into d73a892 - view on LGTM.com

new alerts:

  • 2 for Unused static function

firstcryptoman
firstcryptoman previously approved these changes Feb 23, 2022
Copy link
Collaborator

@firstcryptoman firstcryptoman left a comment

Choose a reason for hiding this comment

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

It looks good for me.

for (auto& th : parallelTasks) {
try {
th.get();
} catch (std::exception& except) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if exception other than std::exception is thrown from inside prover.sigma_commit() it'll be rethrown on th.get(). Imagine situation where the first task in the list results in exception which is not supposed to abort the program. We're getting out of LelantusProver::generate_sigma_proofs while all other tasks are still running and referencing local variables. This will result in segfault. If sigma_commit() can throw you need to wrap it into catch

Copy link
Contributor

Choose a reason for hiding this comment

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

also in case of client shutdown (even graceful!) all the threads are cancelled (boost::thread_group::interrupt_all()). In this case th.get() will throw boost exception, call frame will cease to exist and you'll get corrupted data or segfault

isFail = true;
} catch (...) {
LogPrintf("Lelantus proof creation failed.");
isFail = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

look for docs there
https://www.boost.org/doc/libs/1_54_0/doc/html/thread/thread_management.html#thread.thread_management.tutorial.interruption

you can't use catch(...) here (without rethrowing at least). This this catch boost::thread_interrupted and then throw std::runtime_error. Meaning every time you gracefully shutdown there will be runtime error

please use the method from zerocoin code to block thread interruption while in function

@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2022

This pull request introduces 1 alert when merging 9ae34d9 into 9f2c3b9 - view on LGTM.com

new alerts:

  • 1 for Commented-out code

@reubenyap reubenyap requested a review from psolstice March 7, 2022 07:12
std::size_t threadsMaxCount = std::min((unsigned int)sigmaProofs.size(), boost::thread::hardware_concurrency());
std::vector<boost::future<bool>> parallelTasks;
parallelTasks.reserve(threadsMaxCount);
ParallelOpThreadPool<bool> threadPool(threadsMaxCount);
Copy link
Contributor

Choose a reason for hiding this comment

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

DoNotDisturb here? and everywhere where ParallelOpThreadPool is used

Copy link
Contributor Author

@levonpetrosyan93 levonpetrosyan93 Mar 11, 2022

Choose a reason for hiding this comment

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

We added multithreading in Lelantus-sigma roof creation code and batch proving code.
1 Added DoNotDisturb here for proof creation, I think we need it inside the function which is called in thread. https://github.com/firoorg/firo/pull/1058/files#diff-35b15f4d3b021c6dc41983092fa0d3666801234fe9f9ce790b41c65297a68483R32
2 Fir batching I don't think we need don't disturb as shutdown waits until batch verification finishes, https://github.com/firoorg/firo/blob/78e29d68b7354be702965fdd4f033709750776e5/src/init.cpp#L258

@lgtm-com
Copy link

lgtm-com bot commented Mar 14, 2022

This pull request introduces 3 alerts when merging 7e19959 into 78e29d6 - view on LGTM.com

new alerts:

  • 2 for Unused static function
  • 1 for Commented-out code

auto params = sigma::Params::get_default();
sigma::SigmaPlusVerifier<Scalar, GroupElement> sigmaVerifier(params->get_g(), params->get_h(), params->get_n(), params->get_m());
parallelTasks.emplace_back(threadPool.PostTask([=]() {
DoNotDisturb dnd;
Copy link
Contributor

Choose a reason for hiding this comment

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

dnd should be in the same function frame that gets std::future result, not in lambda

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@lgtm-com
Copy link

lgtm-com bot commented Mar 16, 2022

This pull request introduces 1 alert when merging 90b5b59 into 78e29d6 - view on LGTM.com

new alerts:

  • 1 for Commented-out code

@reubenyap reubenyap merged commit 904984e into master Mar 18, 2022
@reubenyap reubenyap deleted the multithreading branch March 18, 2022 00:24
levonpetrosyan93 added a commit that referenced this pull request May 17, 2023
…ification (#1058)

* Multi-core optimization to lelantus-sigma proof creation

* Multithreading applied on batch verification

* Using another thread pool implementation

* Review comments applied

* Exeption handling fixed

* Fixed bug in threadpool, using boost thread

* Exception handling refactored

* Fixed crash on shutdown

* Review comment applied

* Moved DoNotDisturb into right place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants