FRI Implementation: frontend + CPU Backend #795
Conversation
…ees are given from the user.
…h non-random parameters
| ICICLE_LOG_ERROR << "Number of queries must be > 0"; | ||
| return eIcicleError::INVALID_ARGUMENT; | ||
| } | ||
| if (__builtin_expect(this->m_folding_factor != 2, 0)) { |
There was a problem hiding this comment.
I think you need to check this in the constructor, it's not even an argument of this method so it's strange to fail here for that.
| CpuFriBackend(const size_t folding_factor, const size_t stopping_degree, std::vector<MerkleTree> merkle_trees) | ||
| : FriBackend<S, F>(folding_factor, stopping_degree, std::move(merkle_trees)), | ||
| m_nof_fri_rounds(this->m_merkle_trees.size()), | ||
| m_log_input_size(this->m_merkle_trees.size() + std::log2(static_cast<double>(stopping_degree + 1))), |
There was a problem hiding this comment.
Why is this the log_size? number of trees is probably the number of rounds, so does it assume folding factor is 2?
| const F* input_data, | ||
| FriProof<F>& fri_proof /*out*/) override |
There was a problem hiding this comment.
I think it is probably better to get the size of input_data here as another parameter. You assume it is valid but maybe it's not. If not, you need to identify and log an error.
| * @return eIcicleError Error code indicating success or failure. | ||
| */ | ||
| eIcicleError commit_fold_phase( | ||
| const F* input_data, FriTranscript<F>& transcript, const FriConfig& fri_config, FriProof<F>& fri_proof) |
There was a problem hiding this comment.
I think you should accept the size and not assume it it valid when you take a pointer like that
|
|
||
| eIcicleError proof_of_work(FriTranscript<F>& transcript, const size_t pow_bits, FriProof<F>& fri_proof) | ||
| { | ||
| for (uint64_t nonce = 0; nonce < UINT64_MAX; nonce++) { |
There was a problem hiding this comment.
nonce is only 64b? Why don't you use the pow API here?
| std::vector<std::unique_ptr<F[]>> m_rounds_evals; | ||
|
|
||
| // Holds MerkleTree for each round. m_merkle_trees[i] is the tree for round i. | ||
| std::vector<MerkleTree>& m_merkle_trees; |
There was a problem hiding this comment.
I think it's a bad idea to store a reference to another object like that since you don't control its lifetime and it may be released too soon (before the FriRounds object). In such cases you need to either pass by value, move or use a smart pointer. Even if this is not exposed to users, it still creates coupling and assumptions for code using this class and changes in other places may break this code.
| virtual eIcicleError get_proof( | ||
| const FriConfig& fri_config, | ||
| const FriTranscriptConfig<F>& fri_transcript_config, | ||
| const F* input_data, |
There was a problem hiding this comment.
I think you need to add input_size here
| * @param folding_factor The factor by which the codeword is folded each round. | ||
| * @param stopping_degree Stopping degree threshold for the final polynomial. | ||
| */ | ||
| FriBackend(const size_t folding_factor, const size_t stopping_degree, std::vector<MerkleTree> merkle_trees) |
There was a problem hiding this comment.
You probably need to check the values are valid here. Since it's a constructor it may be problematic to fail here. Why are those not part of the config?
| : m_transcript_config(transcript_config), m_prev_alpha(F::zero()), m_pow_nonce(0) | ||
| { | ||
| m_entry_0.clear(); | ||
| m_entry_0.reserve(1024); // pre-allocate some space |
There was a problem hiding this comment.
don't hardcode values like that. Define it somewhere above
| */ | ||
| eIcicleError init(const size_t nof_queries, const size_t nof_fri_rounds, const size_t final_poly_size) | ||
| { | ||
| if (nof_queries <= 0 || nof_fri_rounds <= 0) { |
| * @param round_idx Index of the round (FRI round). | ||
| * @return Reference to the Merkle proof at the specified position. | ||
| */ | ||
| MerkleProof& get_query_proof(const size_t query_idx, const size_t round_idx) |
There was a problem hiding this comment.
Is it not const by design?
There was a problem hiding this comment.
Each query has a proof per round for two values? Does it assume bit reverse vectors?
| */ | ||
| std::pair<const std::byte*, std::size_t> get_merkle_tree_root(const size_t round_idx) const | ||
| { | ||
| return m_query_proofs[0][round_idx].get_root(); |
There was a problem hiding this comment.
this [0] deserves a comment.
| auto run = [log_input_size, input_size, folding_factor, stopping_degree, output_store_min_layer, nof_queries, | ||
| pow_bits, &scalars](const std::string& dev_type, bool measure) { | ||
| Device dev = {dev_type, 0}; | ||
| icicle_set_device(dev); |
There was a problem hiding this comment.
need ICICLE_CHECK around icicle APIs in tests so it fails if this fails
| ICICLE_CHECK(ntt_init_domain(scalar_t::omega(log_input_size), init_domain_config)); | ||
|
|
||
| // ===== Prover side ====== | ||
| uint64_t merkle_tree_arity = 2; // TODO SHANIE (future) - add support for other arities |
There was a problem hiding this comment.
For now this is not supported right? Do you check it?
| Hash hash = Keccak256::create(sizeof(TypeParam)); // hash element -> 32B | ||
| Hash compress = Keccak256::create(merkle_tree_arity * hash.output_size()); // hash every 64B to 32B | ||
|
|
||
| Fri prover_fri = create_fri<scalar_t, TypeParam>( |
There was a problem hiding this comment.
I see you only test the case of hash+compress. Do you currently expose the other API too? It's fine if not, but if you do then you must test it as well
| ICICLE_CHECK(ntt_release_domain<scalar_t>()); | ||
| }; | ||
|
|
||
| run(IcicleTestBase::reference_device(), 0 /*nof_queries*/, 2 /*folding_factor*/, log_input_size /*log_domain_size*/); |
There was a problem hiding this comment.
(1) Please comment why each case should fail
(2) I think there are more invalid cases like tree arity > 2
| } | ||
| for (size_t log_input_size = 16; log_input_size <= 24; log_input_size += 4) { | ||
| const size_t input_size = 1 << log_input_size; | ||
| const size_t folding_factor = 2; // TODO SHANIE (future) - add support for other folding factors |
There was a problem hiding this comment.
I think you assume this when you compute the input size in the cpu backend. I commented there that it is a mistake. Also if you don't support then missing test to assert it fails.
Anyway I think you need to pass the logsize/size of data to the fri prover and not implicitly assume things like that and compute it. If you are wrong you may have segmentation faults or issues like that.
Update: I saw later that you take it in the create_fri() and use it to compute #rounds. and later you recompute the size but I think it's bug prone and better store the size for later.
| eIcicleError get_proof( | ||
| const FriConfig& fri_config, | ||
| const FriTranscriptConfig<F>& fri_transcript_config, | ||
| const F* input_data, |
There was a problem hiding this comment.
I actually now see you pass it to create_fri() but do you use it? I may be confused here. I commented about it multiple times
| * @param fri_proof Reference to a FriProof object (output). | ||
| * @return An eIcicleError indicating success or failure. | ||
| */ | ||
| eIcicleError get_proof( |
There was a problem hiding this comment.
So we expose only this constructor for now?
| const size_t folding_factor, const size_t stopping_degree, std::vector<MerkleTree> merkle_trees) | ||
| { | ||
| std::shared_ptr<FriBackend<S, F>> backend; | ||
| ICICLE_CHECK(FriDispatcher::execute(folding_factor, stopping_degree, std::move(merkle_trees), backend)); |
There was a problem hiding this comment.
I don't this this move does anything. It probably copies
| Hash merkle_tree_compress_hash, | ||
| const uint64_t output_store_min_layer) | ||
| { | ||
| const size_t log_input_size = static_cast<size_t>(std::log2(static_cast<double>(input_size))); |
There was a problem hiding this comment.
size may not be a power of two but you don't check this
No description provided.