Skip to content
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

New STM registration procedure #433

Merged
merged 80 commits into from
Oct 11, 2022
Merged

New STM registration procedure #433

merged 80 commits into from
Oct 11, 2022

Conversation

iquerejeta
Copy link
Contributor

@iquerejeta iquerejeta commented Aug 18, 2022

Content

This PR implements the new registration procedure and closes #301 and closes #384 . When a registration procedure is initialised, we have to give as input the cardano stake distribution, which is a set of pairs (PoolId, Stake). This would be the stake distribution of all SPOs, and not only the mithril nodes. Then, when an SPO wants to register as a Mithril node, it must provide:

  • The operational certificate
  • The cold verification key
  • KES signature of the Mithril key
  • The KES period (we need to figure out how we are going to get this)
  • The Mithril key

To this end, this PR introduces the OpCert structure, which contains the KES verification key and a signature using the cold key. There certainly is more fields, but we can complete than once we know which fields a OpCert has. The registration procedure takes as input the raw cbor bytes of the OpCert and parses them to extract the required data. Then, it can verify the validity of OpCert, check that the hash of the cold VK (i.e. the PoolId) is indeed in the stake distribution of cardano, verify the KES signature of the mithril key, and finally verify the validity of the Mithril key.

Wrappers over mithril-core

To avoid making mithril-core Cardano specific, while maintaining its usage in the mithril project, we write wrappers over certain structures to enforce Cardano specific requirements. In particular:

  • StmInitializeWrapper: With this wrapper we enforce the usage of a KES key and period during the setup. This allows the initialiser to produce a KES signature of the mithril verification key.
  • KeyRegWrapper: The registration wrapper enforces an initialisation with cardano's stake distribution (poolID and stake). Similarly, it enforces that when a party is registering it needs to submit the poolID, the OpCert and the KES sig and period. This allows the registrar to check that the registering party is indeed who it claims to be.

A few things remaining in this PR:

  • Introduce a KesSignature of the Mithril key in the StmInitializer. We therefore need to access the bytes of the KES signing key (probably through the mithril server cli)
  • Implement parsing of the OpCert
  • Handle KES deserialisation with serde
  • Compute PoolID from cold verification key

Aggregator/Signer Hybrid Registration implementation

  • Use new key registration functions that use KES Secret Key and Operational Certificate
  • Implement a KES Period requester inside the Chain Observer
  • Implement a hybrid mode: Certified and Legacy to allow for smooth transition (no breaking changes)
  • Adapt Aggregator key registration
  • Adapt Signer key registration
  • Adapt Signer individual signatures registration
  • Update crypto tests helpers from Common to handle hybrid modes (and add a test_only feature for them)
  • Update devnet
  • Update test lab and make it able to verify that Signers from both modes are able to contribute to signatures
  • Add Verified Signer badge on the Explorer for certified signers

Pre-submit checklist

  • Branch
    • Tests are provided
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Rollback test only CI modifications
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update documentation website

Issue(s)

Closes #301, #384 and #455

@github-actions
Copy link

github-actions bot commented Aug 18, 2022

Unit Test Results

    7 files  ±0    24 suites  ±0   2m 6s ⏱️ -13s
340 tests +3  340 ✔️ +3  0 💤 ±0  0 ±0 
341 runs  +3  341 ✔️ +3  0 💤 ±0  0 ±0 

Results for commit d49cdb4. ± Comparison against base commit 329d6fa.

This pull request removes 2 and adds 5 tests. Note that renamed tests count towards both.
crypto_helper::conversions::tests ‑ test_stake_signers_from_into
src/stm.rs - stm::StmSigner<D> ‑ new_epoch (line 405)
chain_observer::cli_observer::tests ‑ test_get_current_kes_period
crypto_helper::cardano::cold_key::tests ‑ test_generate_deterministic_genesis_keypair
crypto_helper::cardano::key_certification::test ‑ test_vector_key_reg
crypto_helper::cardano::opcert::tests ‑ test_vector_opcert
entities::signer::tests ‑ test_stake_signers_from_into

♻️ This comment has been updated with latest results.

@iquerejeta iquerejeta force-pushed the mock_certification branch 2 times, most recently from be47ea5 to 941fef2 Compare September 5, 2022 09:05
@jpraynaud jpraynaud added the breaking-change 🔥 Contains a breaking change label Sep 5, 2022
@jpraynaud jpraynaud force-pushed the mock_certification branch 3 times, most recently from 91cd855 to e6dcad3 Compare September 15, 2022 15:34
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

@iquerejeta here are the 3 modifications we talked about while pairing 👍

mithril-core/src/error.rs Outdated Show resolved Hide resolved
@jpraynaud jpraynaud force-pushed the mock_certification branch 9 times, most recently from 2d59fef to 3828b56 Compare September 26, 2022 16:51
@jpraynaud jpraynaud force-pushed the mock_certification branch 6 times, most recently from b047f75 to 83eeb95 Compare September 28, 2022 10:41
jpraynaud and others added 19 commits October 10, 2022 16:12
Add a 'Verified Signer' badge for certified signers.
Following update of arguments precedence in #511
To avoid 'warning: very complex type used. Consider factoring parts into  definitions' clippy warning.
The KES Period previously used when registering to Aggregator would have evolved when verified by Signer.
Instead store the KES Period with the other signer material at first.
In a different commit, the KES Period will be enforced within a valid range given an epoch.
And some other enahncements seen during peer reviewing.
This help gracefully handle the operational certificate information returned by cardano cli,
that can be on multiple lines (number of lines may vary).
Now, we scan the output and retrieve the JSON part from the first occurrence of '{'.
And replace its name with 'PartyIdNonExisting'.
Indeed, this error does not belong to the core libray that is Cardano agnostic.
This flag is required for test only functions that are used outside of 'mithril-common'.
It should be activated exclusively on the 'dev-dependencies' configuration block of the crates.
Each type reflects now the native type it encodes.
To decide at a higher level if key certification is enabled.
use blake2::{digest::consts::U32, Blake2b};
use ed25519_dalek;
use mithril::key_reg::KeyReg;
use crate::crypto_helper::cardano::{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Parameter names are getting longer since we have a lot of resembling parameters. However, it sometimes gets confusing to track the code and decide which is which. I don't think we should change the names, but maybe the parameter documentation should give a little more detail.

Copy link
Collaborator

@curiecrypt curiecrypt left a comment

Choose a reason for hiding this comment

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

The PR satisfies the requirements given here and #301, #384 (note that I did not review the code related to #455). So it can be merged with the main branch.

Please see the suggestions.

jpraynaud and others added 4 commits October 10, 2022 17:48
Replace the 'rust:buster' with 'ubuntu:latest' for building image.
This allows to not rely on libssl1.1 anymore.
Co-authored-by: curiecrypt <36852463+curiecrypt@users.noreply.github.com>
Use 'ubuntu:22.04' instead of 'ubuntu:latest'.
@jpraynaud jpraynaud merged commit 978f15f into main Oct 11, 2022
@jpraynaud jpraynaud deleted the mock_certification branch October 11, 2022 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mock "certification" of mithril keys Change the initialisation and registration functions
4 participants