Two to one block aggregation

[einar-polygon]

This supersedes previous PR: #216

[Nashtare]

Overall looking much better. Should be soon ready to merge

[Nashtare]

These methods are incorrect, at least the way we use them.
They both expect their targeted elements to be located at the beginning of the provided slice as input. However they are both called above on the entire lhs_public_inputs / rhs_public_inputs respectively.

[Nashtare]

Additionally their documentation is a bit misleading.
from a predefined location. -> either we specify the location, passing the entire PI slice as input, or we specify that we expect the slice to start at where we want to proceed to the extraction (in which case it is the caller's responsability to do, for instance extract_block_public_values(&my_pis_slice[PV_INDEX_START..])

[einar-polygon]

That means the first 4 PIs will be considered both as 2-to-1-hash and as the first PV (i.e. trie_roots_before.state_root).

Yes. I would argue this union-like construction is similar in spirit to setting both agg_proof and base_proof to exactly one of them, and then only later we decide which one of the proof types the targets' values actually represent.

@Nashtare let me know what you think. I've changed it to more explicit offsets:

/// Extracts the two-to-one block aggregation hash from a predefined location.
pub fn extract_two_to_one_block_hash<T>(public_inputs: &[T]) -> &[T; NUM_HASH_OUT_ELTS] {
    const PV_HASH_INDEX_START: usize = 0;
    const PV_HASH_INDEX_END: usize = PV_HASH_INDEX_START + NUM_HASH_OUT_ELTS;
    public_inputs[PV_HASH_INDEX_START..PV_HASH_INDEX_END]
        .try_into()
        .expect("Public inputs vector was malformed.")
}

/// Extracts the two-to-one block aggregation hash from a predefined location.
pub fn extract_block_public_values<T>(public_inputs: &[T]) -> &[T; PublicValuesTarget::SIZE] {
    const PV_INDEX_START: usize = 0;
    const PV_INDEX_END: usize = PV_INDEX_START + PublicValuesTarget::SIZE;
    public_inputs[PV_INDEX_START..PV_INDEX_END]
        .try_into()
        .expect("Public inputs vector was malformed.")
}

[Nashtare]

This doesn't really change anything though, you're just replacing 0 by a hardcoded constant.

My point is really that we need to make it explicit in these function definitions that we expect to extract the targeted object at the start of the slice (because by construction we have len(PIs) = len(PVs) + len(VK), and we pad with dummy zeroes when we're only filling a hash instead of PVs. Here nothing indicates, from either the function names or doc, this behavior.

[hratoanina]

At this point, shouldn't we just have a general extract_public_inputs function taking a range and all PIs as input, and explain the range when calling it?

[einar-polygon]

My motivation for introducing these helper functions was to label the respective slices. If we start passing a range to a generic extract_public_inputs function, I feel we may as well just go back to slicing at the call site. That said, I don't have strong preferences regarding this issue. For now I have introduced some new rustdoc make the assumptions explicit. Let me know, what you think, @hratoanina and @Nashtare .

[hratoanina]

Hmm, yeah, I wouldn't mind just slicing directly with good comments.

Concerning the doc, I agree that the structure of the PIs should be addressed somewhere, but in my opinion it's more general than these particular functions.
No strong opinion, but maybe in the comments of the circuit structs? E.g.:

/// Data for the two-to-one block circuit, which is used to generate a
/// proof of two unrelated proofs.
/// Public inputs:
/// - Public values
/// - Cyclic verifying key
#[derive(Eq, PartialEq, Debug)]
pub struct TwoToOneBlockCircuitData<F, C, const D: usize>