Abstract over the vector commitment scheme

[Al-Kindi-0]

Addresses #179

[irakliyk]

Not a full review - but this looks great! Thank you!

I left some comments inline - most are nits, but one will probably require some investigation.

[irakliyk]

Thank you! Looks good! Still not a full review - but I left some comments inline. Most are pretty small, but I do wonder if we should maybe make VectorCommitment trait less general as it seems like we are imposing quite a few restrictions on it anyway (see the last two comments in the PR review).

Specifically, I'm thinking that maybe we get rid of Item and Commitment associated types for now and assume that these are always digests of the associated hash function. If I'm reading the code correctly, we enforce these constraints almost everywhere anyway.

So, the trait could look something like this:

pub trait VectorCommitment<H: Hasher>: Sized {
    type Options: Default;
    type Proof: Clone + Serializable + Deserializable;
    type MultiProof: Serializable + Deserializable;
    type Error: Debug;

    fn with_options(items: Vec<H::Digest>, options: Self::Options) -> Result<Self, Self::Error>;
    fn commitment(&self) -> H::Digest;
    ...
}

I'm also wondering if it makes sense to make the H type parameter into an associated type (e.g., HashFn) - but I'm not sure if it will make things better or worse.

This does make the VectorCommitment trait quite a bit less general, but I think we can approach this refactoring as a 2 step process:

1. Introduce vector commitment scheme where we assume that items and commitments are digests. This would reduce the amount of changes in this PR.
2. Make the vector commitment scheme more general. Maybe at first transforming items into associated types and then transforming commitments themselves into associated types.

What do you think?

[Al-Kindi-0]

I would probably try to remove the hash function from the vector commitment trait altogether and substitute that with bounds in the relevant places of the form From<<H as Hasher>::Digest>. What do you think?
If we can do this then I would probably feel more inclined to not go with the last proposal which as you described makes the trait more restrictive/shallow. Having From<<H as Hasher>::Digest> bounds is as far as I can see is natural and necessary e.g., verkle trees.

[irakliyk]

I would probably try to remove the hash function from the vector commitment trait altogether and substitute that with bounds in the relevant places of the form From<<H as Hasher>::Digest>. What do you think?

Wouldn't this be roughly equivalent to imposing From<H::Digest> and Into<H::Digest> for both Commitment and Item associated types?

Overall, I don't mind trying different approaches, but my thinking was that we could simplify this PR now, and then experimenting would get simpler in the future as we'll have the core structure in place.

[Al-Kindi-0]

I would probably try to remove the hash function from the vector commitment trait altogether and substitute that with bounds in the relevant places of the form From<<H as Hasher>::Digest>. What do you think?

Wouldn't this be roughly equivalent to imposing From<H::Digest> and Into<H::Digest> for both Commitment and Item associated types?

Yes, but it would move closer to where it is actually needed.

Overall, I don't mind trying different approaches, but my thinking was that we could simplify this PR now, and then experimenting would get simpler in the future as we'll have the core structure in place.

Sure

[irakliyk]

Looks good! Thank you! I've added some more comments inline - most are pretty simple and there are a few that are thoughts for the future PRs.

Once this PR is merged, I think the goal of the next PR could be:

• Investigate making hash function an associated type of VectorCommitment trait.
• Add Item associated type to the VectorCommitment trait.

[irakliyk]

Looks good! Thank you! I left a few minor comments inline. Also, let's resolve the merge conflict.

[irakliyk]

All looks good! Thank you!