-
Notifications
You must be signed in to change notification settings - Fork 270
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
chore: extract merge from UC and simplify #4343
Changes from all commits
4d6f380
1216323
2cd1307
b2f6a4f
5c466ca
f32cea9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,31 +12,28 @@ namespace bb { | |
/** | ||
* @brief Prover class for the Goblin ECC op queue transcript merge protocol | ||
* | ||
* @tparam Flavor | ||
*/ | ||
template <typename Flavor> class MergeProver_ { | ||
using FF = typename Flavor::FF; | ||
using Polynomial = typename Flavor::Polynomial; | ||
using CommitmentKey = typename Flavor::CommitmentKey; | ||
using Commitment = typename Flavor::Commitment; | ||
using PCS = typename Flavor::PCS; | ||
using Curve = typename Flavor::Curve; | ||
using OpeningClaim = ProverOpeningClaim<Curve>; | ||
using OpeningPair = bb::OpeningPair<Curve>; | ||
class MergeProver { | ||
using Curve = curve::BN254; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't still be taken from the flavor, even though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No because essentially the only data being extracted from the Flavor here was curve data. We'll only ever run the merge protocol on bn254 so there's no need for templating (similar to how the Translator is not templated). |
||
using FF = Curve::ScalarField; | ||
using Polynomial = polynomial; | ||
using CommitmentKey = bb::CommitmentKey<Curve>; | ||
using Commitment = Curve::AffineElement; | ||
using PCS = bb::KZG<Curve>; | ||
using OpeningClaim = typename bb::ProverOpeningClaim<Curve>; | ||
using Transcript = BaseTranscript; | ||
|
||
public: | ||
std::shared_ptr<Transcript> transcript; | ||
std::shared_ptr<ECCOpQueue> op_queue; | ||
std::shared_ptr<CommitmentKey> pcs_commitment_key; | ||
|
||
explicit MergeProver_(const std::shared_ptr<CommitmentKey>&, | ||
const std::shared_ptr<ECCOpQueue>&, | ||
const std::shared_ptr<Transcript>& transcript = std::make_shared<Transcript>()); | ||
BBERG_PROFILE HonkProof& construct_proof(); | ||
explicit MergeProver(const std::shared_ptr<ECCOpQueue>&); | ||
|
||
BBERG_PROFILE HonkProof construct_proof(); | ||
|
||
private: | ||
HonkProof proof; | ||
std::shared_ptr<ECCOpQueue> op_queue; | ||
std::shared_ptr<CommitmentKey> pcs_commitment_key; | ||
static constexpr size_t NUM_WIRES = GoblinUltraFlavor::NUM_WIRES; | ||
}; | ||
|
||
} // namespace bb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the plan to eventually do this for all Provers/Verifiers or is there sth specific to the Merge ones that motivated this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the Merge classes it didn't make any sense for the Composer to be involved, this was just leftover from when the merge protocol was baked into the main Ultra protocol. More generally, it makes sense for classes to be constructed directly via their constructors when possible. The only argument for having methods like create_prover/verifier on the Composer is that the composer is doing some behind the scenes work that's required for the construction of both of these classes. I suspect that we could do what I've done here for most of the other classes currently constructed by the composer and I think we probably should.