Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Introduce trait StakeTableScheme and migrate previous implementations #66

Merged
merged 20 commits into from
Jul 27, 2023

Conversation

alxiong
Copy link
Collaborator

@alxiong alxiong commented Jun 25, 2023

Resolve #65.
(a follow-up of, and should be merged after, #64)

Major changes include:

  • introduce trait StakeTableScheme
  • rename SnapshotVersion from pending/frozen/active to head/epoch_start/last_epoch_start for better readability
  • most old APIs are migrated under the trait impl, notable additions include: len(), contains_key(), deregister() (left unimplemented and future work); notable changes include: lookup() now returns (Amount, Proof) instead of just MerkleProof as before.
  • replace struct StakeTableEntry<V> with generic API from trait StakeTableScheme
    • implement an Iterator for PersistentMerkleNode, implement IntoIterator for StakeTableScheme
    • Make current stake_table implementation generic over K instead of fixated on EncodedPublicKey

@alxiong alxiong requested a review from mrain June 25, 2023 04:10
@alxiong alxiong self-assigned this Jun 25, 2023
@alxiong alxiong changed the title Add trait StakeTableScheme Introduce trait StakeTableScheme and migrate previous implementations Jun 25, 2023
@alxiong alxiong marked this pull request as draft July 4, 2023 12:12
@alxiong
Copy link
Collaborator Author

alxiong commented Jul 4, 2023

Q: should we make struct StakeTable generic over <K, V>?

currently our stake table implementation uses concrete types (EncodedPublicKey, U256) for its key-value, where the encoded pk just contains the seralized bytes (of whatever actual pk being used)

This decision forces me to write:

/// Public parameters for [`BitVectorQC`].
/// generic over the stake table type `ST` and aggregated signature type `A`
#[derive(Serialize, Deserialize, PartialEq, Debug)]
pub struct QCParams<A: AggregateableSignatureSchemes, ST: StakeTableScheme> {
    // pub stake_entries: Vec<StakeTableEntry<V>>,
    pub stake_table: Arc<ST>,
    pub threshold: U256,
    pub agg_sig_pp: A::PublicParameter,
}

impl<A, ST> QuorumCertificate<A> for BitVectorQC<A, ST>
where
    A: AggregateableSignatureSchemes + Serialize + for<'a> Deserialize<'a>,
    ST: StakeTableScheme<Amount = U256>,
    ST::Key: From<A::VerificationKey>,
    A::VerificationKey: From<ST::Key>,

instead of

impl<A, ST> QuorumCertificate<A> for BitVectorQC<A, ST>
where
    A: AggregateableSignatureSchemes + Serialize + for<'a> Deserialize<'a>,
    ST: StakeTableScheme<Key = A::VerificationKey>,
    ST::Amount: Add<Output = Self>,

Apart from having cleaner trait bound, the main advantages of the latter (if we have generic struct StakeTable<K, V>) is avoiding lots of internal serde.
For example, insideQC::check(), we will be calling A::multi_sig_verify on the aggregated signature scheme which accepts list of public keys of type &[A::VerificationKey], which may not be of the same type as &[ST::Key] (which currently is concretized as struct EncodedPublicKey).
To deal with this, we would have to implement two-way conversion (thus the bound A::VerificationKey: From<ST::Key>), and this (arguably unnecessary) serde is being done each time people run QC::check().

In short, I would propose that we make turn our Stake Table generic struct StakeTable<K, V>, wdyt? @mrain
(if agreed, I would work on a separate PR that merge into this PR)

@mrain
Copy link
Contributor

mrain commented Jul 5, 2023

@alxiong Good idea on generalizing K. However doing this for V may introduce complicacy for other interfaces, e.g.

    pub fn update(
        &mut self,
        key: &EncodedPublicKey,
        delta: U256,
        negative: bool,
    ) -> Result<U256, StakeTableError>;

@alxiong
Copy link
Collaborator Author

alxiong commented Jul 5, 2023

right, all I need is to add trait bound related to Add, AddAssign, Sub, SubAssign to V, then the code should mostly work, correct?

@mrain
Copy link
Contributor

mrain commented Jul 5, 2023

Yep. Also that's the only place with all troubles.

alxiong and others added 2 commits July 21, 2023 19:23
* refactor: make stake table impl generic over key type

* fix err

* feat: QC key aggregate gadget (#74)

* update to the newest interface

* refactor keyagg gadget

* update doc

* update test name

* public inputs

* remove cargo.lock

* refactor qc gadgets

* refactor qc gadgets

* remove branch from jellyfish deps

* updated comments

* more comments

---------

Co-authored-by: Chengyu Lin <[email protected]>
@alxiong
Copy link
Collaborator Author

alxiong commented Jul 24, 2023

Here's a problem:

previously, we had this VerKey -> ark_bn254::Fq conversion:

.map(|i| EncodedPublicKey(to_bytes!(&FieldType::from(i)).unwrap()))

but this code has the wrong assumption that bls_on_bn254::VerKey can be serialized into 32 bytes without any loss/truncation.

VerKey is of type G2Affine which has x, y coordinate where each is a quadratic extension field, thus at least even with compressed serialized bytes, we need 64 bytes.
And lossy conversion would result in failed deserialization back into the correct VerKey.


at the current stage of this PR, I introduce this IntoField<FieldType> trait bound, as a reflection of the old code (thus the wrong assumption that it can be converted to a FieldType w/o loss),

And I think I might be better refactoring it to IntoFields<[FieldType; const N]> and then change:

let input = [
FieldType::from(0),
<FieldType as CanonicalDeserialize>::deserialize_compressed(&key.0[..])
.unwrap(),
u256_to_field(value),
];
let init = Digest::evaluate(input).map_err(|_| StakeTableError::RescueError)?[0];

and
/// Hash algorithm used in Merkle tree, using a RATE-3 rescue
pub(crate) type Digest = FixedLengthRescueCRHF<FieldType, TREE_BRANCH, 1>;

into maybe type Digest<N> = FixedLengthRescueCRHF<FieldType, TREE_BRANCH + N-1, 1>,
I'm not sure if I could make the compiler happy, but this seems a necessary change.


There's another almost hacky way to avoid deserialization error. that is to define and manually implement serialization of struct StakeTable as the its commitment, meaning I don't serialize the whole tree (thus avoiding serializing the Key type), instead I only serialize the commitment.

^^ the problem with this approach is given a serialized QCparam, you can't just directly reconstruct the stake table. instead you can only check for consistency against another stake table you download from other means. <-- which also seem a little strange.

@mrain let's chat and see which one do we prefer and what to do next.

@alxiong
Copy link
Collaborator Author

alxiong commented Jul 24, 2023

After discussion with @mrain, we decide to go with: IntoFields<[FieldType; 2]>.
While we acknowledge that this looks hardcoded, we prefer it over using VariableLengthRescueCRHF which will automatically append an 1, and ends up costing 2 rescue hash instead of 1 for the Digest::evaluate().

@alxiong alxiong marked this pull request as ready for review July 25, 2023 14:53
src/stake_table/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, with some minor comments

@alxiong alxiong merged commit 35d6e8c into main Jul 27, 2023
@alxiong alxiong deleted the feat/trait-st branch July 27, 2023 15:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add trait for Stake Table
2 participants