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

proof-producer: introduce command chain #258

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oclaw
Copy link
Contributor

@oclaw oclaw commented Jan 10, 2025

Implemented resource subscription mechanism based on boost::signals + command_chain for achieving smart resource management. Now each step of the executing command keeps its own copy of shared pointer to the data it uses

Rewrote all prover command to the new syntax. Now each command is located into its own file and represented as queue of executing steps (each step releases references to the resources after popping from the queue)

New approach allows:

  • keep different parts of proof-producer independently
  • lazy-initialize large objects (LpcScheme, AssignmentTable)
  • release these resources automatically based on the steps added to the command

Related changes in crypto3:

  • common_data is now placed into shared_ptr as far as it is used as an independent data part in most cases
  • Added reset of KZG scheme in unit tests

Also added ResultCode types for identifying the exact reason of failure externally

@oclaw oclaw requested review from makxenov, x-mass and akokoshn January 10, 2025 11:13
class resources_provider: public resource_provider<Resources>... {};

template <typename T, typename... Resources>
concept ProvidesAll = (std::is_base_of_v<resource_provider<Resources>, T> && ...);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It still might be used, will remove if there will be no need for it

@oclaw oclaw requested a review from makxenov January 14, 2025 13:50
@akokoshn akokoshn requested a review from martun January 17, 2025 08:20
@oclaw oclaw requested a review from akokoshn January 17, 2025 10:31
@oclaw oclaw force-pushed the feature/proof-producer-cmd-refactor branch from cc366de to 7f4d86c Compare January 20, 2025 18:23
Copy link

github-actions bot commented Jan 20, 2025

Clang Test Results

  174 files  ± 0    174 suites  ±0   16m 15s ⏱️ +15s
8 545 tests +97  8 539 ✅ +97  6 💤 ±0  0 ❌ ±0 
8 611 runs  +97  8 605 ✅ +97  6 💤 ±0  0 ❌ ±0 

Results for commit 7cdb98d. ± Comparison against base commit 331cae7.

This pull request removes 6 and adds 103 tests. Note that renamed tests count towards both.
pack_imploder_test_suite ‑ be_to_be_1
pack_imploder_test_suite ‑ be_to_be_2
pack_imploder_test_suite ‑ be_to_le_1
pack_imploder_test_suite ‑ be_to_le_2
pack_imploder_test_suite ‑ be_to_le_3
pack_imploder_test_suite ‑ bubb_to_lulb_4
lpc_math_polynomial_suite ‑ lpc_basic_skipping_layers_test
lpc_math_polynomial_suite ‑ lpc_dfs_basic_test
lpc_params_test_suite ‑ lpc_batches_num_3_test
lpc_params_test_suite ‑ lpc_different_hash_types_test
pack_equal_test_suite ‑ bubb_to_bubb_1
pack_equal_test_suite ‑ bubb_to_bubb_2
pack_equal_test_suite ‑ bubb_to_bubb_3
pack_equal_test_suite ‑ bubb_to_bulb_1
pack_equal_test_suite ‑ bubb_to_bulb_2
pack_equal_test_suite ‑ bubb_to_lubb_1
…

♻️ This comment has been updated with latest results.

@oclaw oclaw marked this pull request as ready for review January 20, 2025 18:34
Copy link

Copy link

github-actions bot commented Jan 20, 2025

Gcc Test Results

  174 files  ±0    174 suites  ±0   17m 18s ⏱️ -3s
8 545 tests ±0  8 539 ✅ ±0  6 💤 ±0  0 ❌ ±0 
8 611 runs  ±0  8 605 ✅ ±0  6 💤 ±0  0 ❌ ±0 

Results for commit 7cdb98d. ± Comparison against base commit 331cae7.

♻️ This comment has been updated with latest results.

@oclaw oclaw force-pushed the feature/proof-producer-cmd-refactor branch from 52888e8 to 34eea3c Compare January 22, 2025 09:49
@makxenov
Copy link
Collaborator

General question. We have 2 similar terms: proof producer (as top-level directory, Nix derivation name, name of binaries) and proof generator (namespace for source files inside proof-producer/, name of libraries). Do these terms have any differences in the meaning? If no, let's unify it to avoid possible confusions.
@akokoshn @oclaw @x-mass

@oclaw oclaw force-pushed the feature/proof-producer-cmd-refactor branch 2 times, most recently from cf44b50 to fe3d57b Compare January 27, 2025 14:05
@oclaw oclaw force-pushed the feature/proof-producer-cmd-refactor branch from d029fd5 to e0408e9 Compare January 27, 2025 16:11
@oclaw oclaw requested a review from makxenov January 27, 2025 16:13
proof-producer/bin/proof-producer/CMakeLists.txt Outdated Show resolved Hide resolved
auto const res = steps_.front()->execute();
if (!res.succeeded())
{
BOOST_LOG_TRIVIAL(error) << "command failed on stage " << stage << " of " << total_stages << ": " << res.error_message();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice to have print here name of the stage together with number

@akokoshn
Copy link
Collaborator

General question. We have 2 similar terms: proof producer (as top-level directory, Nix derivation name, name of binaries) and proof generator (namespace for source files inside proof-producer/, name of libraries). Do these terms have any differences in the meaning? If no, let's unify it to avoid possible confusions. @akokoshn @oclaw @x-mass

@martun , do you remember why was used proof_generator namespace but not proof_producer?

Copy link
Collaborator

@akokoshn akokoshn left a comment

Choose a reason for hiding this comment

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

generally look good.
Let's wait for @martun review

@oclaw oclaw force-pushed the feature/proof-producer-cmd-refactor branch from 2be08a1 to c3ed5de Compare January 29, 2025 11:37
// returns reference to the pushed step (non-owning, ownership is guaranteed by the step queue)
template <typename Step, typename... Args>
requires (std::derived_from<Step, command_step> && std::constructible_from<Step, Args...>)
Step& add_step(Args&&... args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Such a complex way of adding the next step, why not accept a command_step here, and construct it outside?

Copy link
Contributor Author

@oclaw oclaw Jan 29, 2025

Choose a reason for hiding this comment

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

It is complicated only here (not in usage) and the pattern is much similar to make_shared . IMO such approach reduces possible resource leaks caused by unneeded copies of command_step instances outside the queue

};

inline constexpr const char* result_code_to_string(ResultCode rc) {
switch (rc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the project is throwing exceptions. You will still need to have a "catch block" at the start of each command to report those exceptions if there will be any. Maybe here too you could just throw standard std::exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last time I saw entire proof-producer is not throwing exceptions, maybe your take is more relevant to the crypto3 internals

I, actually, do not mind switching to an exception-based mechanism of error handling, but it is a topic for a separate discussion
cc @akokoshn

boost::filesystem::path aggregated_challenge_file;
};

AggregatedChallengeCommand(const Args& args): args_(args) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use this "Args" structure somewhere else? Can't we just accept the arguments here, and delete the "Args" structure from everywhere?

If we use the Args, I would suggest to mark somehow which arguments are inputs, which are the outputs, and when printing the error messages from the "execute" function of command_step.hpp, print the name of the command and the parameters it was called with as a part of the error message. This way will use the Args in some way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate args structs will help us to split command line options in future. I thought to do it during the initial refactoring too but amount of LoC made me to reconsider :)


template <typename CurveType, typename HashType>
struct AggregatedFriProofGenerator: public command_step
{
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 move the scope { to the previous line.

circuit_reader,
table_reader, // for table
table_reader, // for table description
public_preprocessor, // for public data
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks a bit strange that we pass the same object twice, because it contains inside it 2 object that class "Prover" will need to access. Why not extract those objects (the public data and LPC scheme) and pass them to this class? The benefit of creating class "resources::resource_provider" is to load the resource from a file only when required? Is it worth it writing so much code for that purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the part I am not proud of too :(

I think it could be improved with some another factory methods or some kind of dependency injection but it will bring much more complications to an already complicated logic

add_step<Prover>(
circuit_reader,
table_reader,
table_reader,
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplicate?

@oclaw oclaw force-pushed the feature/proof-producer-cmd-refactor branch 3 times, most recently from 2b21cbd to e4b6aa3 Compare January 30, 2025 13:50
@oclaw oclaw force-pushed the feature/proof-producer-cmd-refactor branch from e4b6aa3 to b60a6b0 Compare January 30, 2025 17:48
@oclaw oclaw requested review from makxenov and akokoshn January 30, 2025 17:49
@@ -139,7 +139,7 @@ namespace nil {
// clang-format on
po::options_description cmdline_options("nil; Proof Producer");
cmdline_options.add(generic).add(config);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: file still has trailing whitespaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually wanted to include 'clang-format' run to this PR but it would turn the whole diff to a mess
I suggest to create a precommit hook with implicit call of it in further development :)

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

Successfully merging this pull request may close these issues.

4 participants