Refactor QC traits

[alxiong]

Changes include:

• Renaming:
  • trait QuorumCertificateValidation -> trait QuorumCertificate
  • struct BitvectorQuorumCertificate -> struct BitVectorQC
  • ::parti_sign() -> ::sign() (consistent with hotshot spec)
  • active_keys -> signers
• Trait refactor
  • removed associated type CheckedType as @nyospe pointed out that we don't need this.
  • removed associated type Proof, and replace with QC
  • update assemble() and check() to deal with Self::QC directly (consistent with Hotshot spec)
  • add trace() API and tests

[nyospe]

I think I remember why this is here now. It's not the Checked<T>. And it is needed.

I'm even the person who requested it, I just forgot.

And I think the entire discussion motivating this was on github.

... 90 minutes of searching later...

https://github.com/EspressoSystems/hotshot-primitives/pull/27/files/907f07d85af77fee0ba1b328a77944cdce4e64d5#r1195601782

Short version: we absolutely, non-negotiably, need the CheckedType if we want to be able to use this from HotShot.

[alxiong]

thanks for digging this comment for us! could you clarify what would the number CheckedType = U256 signify?
the number of votes inside the qc?

[nyospe]

In the case of HotShot, the total number of voted shares, I think...

[alxiong]

added it back, see if you like c9429f0 @nyospe

[alxiong]

Another question I had is if we should turn struct StakeTableEntry into a trait that for StakeTable and describe its behavior. (maybe this trait should be inside crate::stake_table)
because this struct is our imaginary struct and won't be the actual entry or stake table type. and to facilitate interfacing between QC code and StakeTable code, we just use traits following Hotshot spec's T.update, T.register etc.

cc @philippecamacho @nyospe @mrain

Will continue this work #65.

[chancharles92]

cc @dailinsubjam as this might also affect the QC integration work.

[nyospe]

It should probably be a trait that can be applied to the actual stake table, yes...

[philippecamacho]

After rerunning the build, it failed. I also have an error on my machine.

> cargo test
error[E0432]: unresolved imports `jf_relation::gadgets::ecc::non_native`, `jf_relation::gadgets::EmulationConfig`
  --> src/circuit/qc_keyagg.rs:12:15
   |
12 |         ecc::{non_native::EmulatedPointVariable, Point, SWToTEConParam},
   |               ^^^^^^^^^^ could not find `non_native` in `ecc`
13 |         EmulationConfig,
   |         ^^^^^^^^^^^^^^^ no `EmulationConfig` in `gadgets`

error[E0432]: unresolved imports `jf_primitives::merkle_tree::hasher`, `jf_primitives::reed_solomon_code`
  --> src/vid/advz.rs:27:19
   |
27 |     merkle_tree::{hasher::HasherMerkleTree, MerkleCommitment, MerkleTreeScheme},
   |                   ^^^^^^ could not find `hasher` in `merkle_tree`
...
32 |     reed_solomon_code::reed_solomon_erasure_decode_rou,
   |     ^^^^^^^^^^^^^^^^^ could not find `reed_solomon_code` in `jf_primitives`

error[E0432]: unresolved import `jf_relation::gadgets::from_emulated_field`
   --> src/circuit/qc_keyagg.rs:154:40
    |
154 |         gadgets::{ecc::SWToTEConParam, from_emulated_field},
    |                                        ^^^^^^^^^^^^^^^^^^^ no `from_emulated_field` in `gadgets`

error[E0432]: unresolved import `jf_primitives::merkle_tree::hasher`
   --> src/vid/advz.rs:540:38
    |
540 |     use jf_primitives::{merkle_tree::hasher::HasherNode, pcs::checked_fft_size};
    |                                      ^^^^^^ could not find `hasher` in `merkle_tree`

error[E0603]: struct `RescueCRHF` is private
   --> src/circuit/qc_keyagg.rs:151:40
    |
151 |     use jf_primitives::rescue::sponge::RescueCRHF;
    |                                        ^^^^^^^^^^ private struct
    |
note: the struct `RescueCRHF` is defined here
   --> /home/philippe/.cargo-nix/git/checkouts/jellyfish-aebafe050ee5960c/77f7b12/primitives/src/rescue/sponge.rs:30:1
    |
30  | pub(crate) struct RescueCRHF<F: RescueParameter> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0603]: function `checked_fft_size` is private
   --> src/vid/advz.rs:540:63
    |
540 |     use jf_primitives::{merkle_tree::hasher::HasherNode, pcs::checked_fft_size};
    |                                                               ^^^^^^^^^^^^^^^^ private function
    |
note: the function `checked_fft_size` is defined here
   --> /home/philippe/.cargo-nix/git/checkouts/jellyfish-aebafe050ee5960c/77f7b12/primitives/src/pcs/mod.rs:304:1
    |
304 | fn checked_fft_size(degree: usize) -> Result<usize, PCSError> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0432`.
error: could not compile `hotshot-primitives` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
Some errors have detailed explanations: E0432, E0603.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `hotshot-primitives` (lib test) due to 6 previous errors

[alxiong]

After rerunning the build, it failed. I also have an error on my machine.

This is likely due to caching of cargo build in CI and failed to point to the right jellyfish version. The best way to solve it is version controlling Cargo.lock.

[nyospe]

I meant to post this comment on Friday morning... oops.

[nyospe]

I'm trying to decide how I feel about this. There's one use case for it as-is, but that's not the only way we would use it in HotShot, and this creates an implied, but unenforced, property restriction on the associated type.

I'd prefer a name that more clearly says "A precise statement about how this Certificate meets (or exceeds) the condition for validity, specific to the usage of this specific Certificate type."

Originally, I had requested some updates to QCParams. This is not strictly required, but would allow a convenient improvement. Let me illustrate.

We have the following certificates in HotShot (this is not a comprehensive list, these are the specific ones that are involved in this use case)

QuorumCertificate (certificate that a quorum of replicas has committed to accepting a sequence with a given hash/commitment, parent, and view number, contingent on eventual evidence of full availability; e.g. voted 'Yes' on the proposal)

RejectCertificate (certificate that a superminority (e.g. F+1) of replicas has seen and rejected the same proposal seen above)

ViewFailedNextViewCertificate (certificate that the previous view was proposed, but failed, and control should now progress to the appropriate proposer for the subsequent view sequence)

Now, we actually have an enumeration NextViewCertificate, which may be a few things, including a token that says "check the QuorumCertificate", because a valid QC is also sufficient evidence to allow view progress, but let's just look at this one.

ViewFailedNextViewCertificate can be implemented as follows:

struct ViewFailedNextViewQC;

impl<A> hsp::QuorumCertificate<A> for ViewFailedNextViewQC<A>
where A:... {
    type QuorumSize = (U256, U256);
    type QC = (hsp::BitVectorQC<A>::QC, hsp::BitVectorQC<A>::QC);
    // shared QCParams, except for overrides for the thresholds for Yes and No thresholds.
    // There's also a max threshold for this case, but it is derivable. It would be really nice if QCParams was composable, so that we could supply the same stake_entries/agg_sig_pp and switch in different threshold values.
    type QCVerifierParams = (hsp::BitVectorQC<A>::QCVerifierParams, hsp::BitVectorQC<A>::QCVerifierParams);
    // ..
    fn check(
        qc_vp: &Self::QCVerifierParams,
        message: &GenericArray<A::MessageUnit, Self::MessageLength>,
        qc: &Self::QC,
    ) -> Result<Self::QuorumSize, PrimitivesError> {
        let totals = (hsp::BitVectorQC<A>::check(qc_vp.0, message, qc.0)?, hsp::BitVectorQC<A>::check(qc_vp.1, message, qc.1)?);
        if totals.0 > 2 * (qc_vp.1 - 1) {
            // Yes votes over 2F threshold. Should be impossible.
            return Err(ParameterError(...));
        }
        if totals.0 + totals.1 < 2 * qc_vp.1 - 1 {
            // Total votes under 2F+1 threshold
            return Err(ParameterError(...));
        }
        Ok(totals)
    }
}

As you can see, this is not completely copacetic with the QuorumSize naming...

[alxiong]

thanks for the detailed clarification @nyospe! this is super helpful.

when I read your snippet above, I still think QuorumSize is not a bad name.
the semantic meaning of QC::check() is to check the validity of a certificate, and as you illustrated in the ViewFailedNextViewQC example, such a certificate can be a composite (tuple in this case) of several other vanilla aggregated-signature style QCs; and the validity condition depends on the status (i.e. composite quorum size) of its constituents.

Else, what about QuorumStatus or QuorumProgress, neither is clearly better than QuorumSize.

maybe a second opinion here? @dailinsubjam @philippecamacho

[philippecamacho]

QuorumSize also seems natural to me and we should have a type anyways to capture this concept. I am not sure how to express the possible multidimensions aspect like in ViewFailedNextViewQC yet still in the end we are comparing numbers (size) against some threshold.

[dailinsubjam]

The type of message inputted to signing function is &GenericArray<A::MessageUnit, Self::MessageLength>, where type MessageLength = U32; so MessageLenth is bound with the length 32, is it possible to change it to a dynamic length? In hotshot the input data could be &[u8] with length 8 or 57.

[chancharles92]

The type of message inputted to signing function is &GenericArray<A::MessageUnit, Self::MessageLength>, where type MessageLength = U32; so MessageLenth is bound with the length to 32, is it possible to change it to a dynamic length? In hotshot the input data could be &[u8] with length 8 or 57.

cc @philippecamacho for this question.

[philippecamacho]

The type of message inputted to signing function is &GenericArray<A::MessageUnit, Self::MessageLength>, where type MessageLength = U32; so MessageLenth is bound with the length to 32, is it possible to change it to a dynamic length? In hotshot the input data could be &[u8] with length 8 or 57.

We decided to only allow fixed length for the input of the signing function, meaning we expect the message to be some kind of commitment on the data.

@nyospe What is the the canonical way to compute a commitment over QC data in Hotshot?

[dailinsubjam]

The type of message inputted to signing function is &GenericArray<A::MessageUnit, Self::MessageLength>, where type MessageLength = U32; so MessageLenth is bound with the length 32, is it possible to change it to a dynamic length? In hotshot the input data could be &[u8] with length 8 or 57.

After today's discussion we plan to change the code in hotshot to make sure the commitment is of fixed size, no need to change in hotshot-primitives.

[nyospe]

There are still a few things about this that aren't very ergonomic, but it's better than it was before, and we have hacks/workarounds for use within HotShot, and maybe those can be revisited once this part has been moved to hotshot.