feat(crypto): CRP-2597 Extend NiDkgTag with HighThresholdForKey variant #2445

fspreiss · 2024-11-05T13:34:14Z

Extends the NiDkgTag with a new variant NiDkgTag::HighThresholdForKey that holds a MasterPublicKeyId.

Notes:

Makes the representation for NiDkgTag explicit with #[repr(isize)] because a #[repr(inttype)] must be provided on an enum if it has a non-unit variant with a discriminant (E0732). The representation should be the same as it was before, given that isize is the default representation for enums.

Note that casting NiDkgTag with as, e.g., to i32 or even isize is no longer possible because "an as expression can be used to convert enum types to numeric types only if the enum type is unit-only or field-less" (see Enumeration casting and E0605).
The conversion impl TryFrom<i32> for NiDkgTag had to be removed, because it is no longer possible, because an i32 itself is no longer sufficient for instantiating an NiDkgTag.

This conversion was used in two places: (1) impl TryFrom<NiDkgIdProto> for NiDkgId and (2) build_tagged_transcripts_map. There, the conversion is now implemented directly.
No longer Implements EnumIter for NiDkgTag because this would require a Default implementation for MasterPublicKeyId, which does not make sense, at least in production. Instead implements EnumCount.

…nidkgtag-non-copy

…try/crypto to types

…nidkgtag-non-copy

…stefan/CRP-2597-move-masterpublickeyid-proto-to-types-ontopof-non-copy-ni-dkg-id

…-variant

…masterpublickeyid-proto-to-types

' into franzstefan/CRP-2597-extend-nidkg-tag-with-keyid-variant

…masterpublickeyid-proto-to-types

' into franzstefan/CRP-2597-extend-nidkg-tag-with-keyid-variant

eichhorl · 2024-11-07T12:38:52Z

rs/consensus/src/consensus/batch_delivery.rs

+                NiDkgTag::HighThresholdForKey(master_public_key_id) => {
+                    error!(
+                        log,
+                        "Implementation error: NiDkgTag::HighThresholdForKey({master_public_key_id}) used in SetupInitialDKG for callback ID {callback_id}",
+                    );
+                }


I think the idea here is to reuse the transcripts_for_remote_subnets field also for xnet reshares of VetKD keys. So then we would generalize this function such that it can generate the correct ConsensusResponse, depending on whether the callback ID is for a SetupInitialDKG context or a VetKD reshare context.

For now, logging an error and linking the ticket to generalize this function (CON-1416) should be fine.

I added a TODO linking to the mentioned ticket.

eichhorl · 2024-11-07T12:43:13Z

rs/consensus/src/dkg/payload_builder.rs

+            ////////////////////////////////////////////////////////////////////////////////
+            // TODO: how to behave here? The code below is copied+adapted from the HighThreshold case. However,
+            // this code currently won't be executed because we iterate over TAGS, which is a const that does not
+            // and cannot contain an NiDkgTag::HighThresholdForKey entry
+            ///////////////////////////////////////////////////////////////////////////////
+            NiDkgTag::HighThresholdForKey(master_public_key_id) => {
+                let resharing_transcript = reshared_transcripts
+                    .get(&NiDkgTag::HighThresholdForKey(master_public_key_id.clone()));
+                (
+                    resharing_transcript
+                        .map(|transcript| transcript.committee.get().clone())
+                        .unwrap_or_else(|| node_ids.clone()),
+                    resharing_transcript.cloned(),
+                )
+            }


I think the behavior is correct (maybe it could even be combined with the NiDkgTag::HighThreshold branch). We can link ticket CON-1413 above, reminding us to iterate over all vetKD keys that were requested by registry in addition to the tags in TAGS.

I added a TODO linking to the mentioned ticket.

Also, I combined the NiDkgTag::HighThreshold | NiDkgTag::HighThresholdForKey(_).

PTAL

eichhorl · 2024-11-07T12:57:16Z

rs/consensus/src/dkg/payload_builder.rs

+                        /////////////////////////////////////////
+                        // TODO: check with Consensus team
+                        NiDkgTag::HighThresholdForKey(_) => panic!("not applicable"),
+                        /////////////////////////////////////////


I think we can link CON-1417 to extend this test once we have support for vet key transcripts in registry CUPs

eichhorl · 2024-11-07T13:01:36Z

rs/consensus/src/dkg/payload_builder.rs

+                            /////////////////////////////////////////
+                            // TODO: check with Consensus team
+                            NiDkgTag::HighThresholdForKey(_) => panic!("not applicable"),
+                            /////////////////////////////////////////
                        }


We can link CON-1413 to extend this test once we can create local transcript configs for vetKeys that were requested by registry

rs/types/types/src/consensus/dkg.rs

…terns

…d-nidkg-tag-with-keyid-variant

altkdf

Thanks @fspreiss, LGTM!

altkdf · 2024-11-11T21:19:07Z

rs/crypto/src/sign/threshold_sig/store.rs

@@ -115,20 +125,21 @@ impl ThresholdSigDataStoreImpl {
    ///
    /// # Panics
    /// If `max_num_of_dkg_ids_per_tag` is smaller than 1.
-    fn new_with_max_size(max_num_of_dkg_ids_per_tag: usize) -> Self {
+    fn new_with_max_size(max_num_of_dkg_ids_per_tag_or_key: usize) -> Self {


off-topic: Seems new is the only caller of new_with_max_size. I'm wondering if we want to eventually simplify this by inlining new_with_max_size and adding a static assert on CAPACITY_FOR_TAG to avoid having the runtime panic. But this is not a concern for this PR.

Yes, sure, sounds good. Did you mean to use static_assertions for this?

Either that or just

const _: () = assert!(...);

altkdf · 2024-11-12T08:49:26Z

rs/crypto/test_utils/ni-dkg/src/initial_config/tests.rs

@@ -294,6 +294,9 @@ fn registry_with_ni_dkg_transcript(
            cup_contents.initial_ni_dkg_transcript_low_threshold =
                Some(InitialNiDkgTranscriptRecord::from(transcript()));
        }
+        NiDkgTag::HighThresholdForKey(_master_public_key_id) => {
+            unimplemented!("not supported currently")


Why is this not supported? Is this is only testing NIDKG for consensus by definition, if yes it would be better to make it explicit in the error message.

Do I understand correctly, these transcripts should go into the ChainKeyInitialization introduced with #2337? Then this should already be possible, now that the PR is merged.

TL;DR: The tests in which the registry_with_ni_dkg_transcript helper is used will never be extended so that a NiDkgTag::HighThresholdForKey is needed, which is why this is unimplemented.

Background

This helper function, registry_with_ni_dkg_transcript, is used in two tests:

should_correctly_retrieve_initial_low_threshold_ni_dkg_transcript_from_registry

should_correctly_retrieve_initial_high_threshold_ni_dkg_transcript_from_registry

Both these tests test CryptoRegistry::get_initial_dkg_transcripts, which returns a

struct DkgTranscripts { low_threshold: NiDkgTranscript, high_threshold: NiDkgTranscript, }

(I'm not sure why these tests actually live in ic-crypto-test-utils-ni-dkg. I'd say the correct place would be ic-registry-client-helpers; we should probably move the tests).

These are the two initial NiDkgTranscripts that a subnet needs to operate (low-threshold for random beacon/tape, high-threshold for state certification). They are required by the ICP protocol itself.

The NIDKG-transcript that is created for vetKeys by means of a key re-sharing request (as part of subnet creation or subnet recovery) is not needed by a subnet to operate. Because of this, such transcripts won't be returned by CryptoRegistry::get_initial_dkg_transcripts, but whoever needs them will likely fetch the CUP and then get the transcript out of that.

Because of this, I didn't see a reason

altkdf · 2024-11-12T09:32:37Z

rs/types/types/src/crypto/threshold_sig/ni_dkg.rs

@@ -31,18 +32,22 @@ mod tests;

 /// Allows to distinguish protocol executions in high and low threshold


It would be worth adding what the new variant means, i.e., what is the Key and what is its relation to an NIDKG tag.

maksymar

LGTM for management_canister_types

eichhorl · 2024-11-12T15:13:38Z

rs/types/types/src/crypto/threshold_sig/ni_dkg/id.rs

+                    1 => Ok(NiDkgTag::LowThreshold),
+                    2 => Ok(NiDkgTag::HighThreshold),


Should we return an error if the tag is LowThreshold or HighThreshold, but ni_dkg_id_proto.key_id isn't None?

eichhorl · 2024-11-12T15:14:24Z

rs/types/types/src/consensus/dkg.rs

+                            1 => Ok(NiDkgTag::LowThreshold),
+                            2 => Ok(NiDkgTag::HighThreshold),


Sawchord · 2024-11-12T16:03:59Z

rs/consensus/src/dkg/payload_builder.rs

+                        /////////////////////////////////////////////////////
+                        // TODO(CON-1417): extend this test once we have support for vetKD transcripts in registry CUPs
+                        /////////////////////////////////////////////////////
+                        NiDkgTag::HighThresholdForKey(_) => panic!("not applicable"),


Maybe instead of panicking with the not very informative "not applicable", we use unimplemented("CON-1417"). I mean, if we were to ever hit this case (which I don't believe we will), we would know much faster whats going on

Sawchord · 2024-11-12T16:20:44Z

rs/types/types/src/crypto/threshold_sig/ni_dkg/id.rs

+                    1 => Ok(NiDkgTag::LowThreshold),
+                    2 => Ok(NiDkgTag::HighThreshold),


Sawchord · 2024-11-12T16:25:46Z

rs/crypto/test_utils/ni-dkg/src/initial_config/tests.rs

@@ -294,6 +294,9 @@ fn registry_with_ni_dkg_transcript(
            cup_contents.initial_ni_dkg_transcript_low_threshold =
                Some(InitialNiDkgTranscriptRecord::from(transcript()));
        }
+        NiDkgTag::HighThresholdForKey(_master_public_key_id) => {
+            unimplemented!("not supported currently")


Do I understand correctly, these transcripts should go into the ChainKeyInitialization introduced with #2337? Then this should already be possible, now that the PR is merged.

fspreiss · 2024-11-13T16:49:10Z

rs/types/types/src/crypto/threshold_sig/ni_dkg.rs

 pub enum NiDkgTag {
    LowThreshold = 1,
    HighThreshold = 2,
+    HighThresholdForKey(MasterPublicKeyId) = 3,


@altkdf, @randombit: @Sawchord suggested to use a new type NiDkgMasterPublicKeyId here instead (i.e., using HighThresholdForKey(NiDkgMasterPublicKeyId) = 3,), similar to the IDkgMasterPublicKeyId that he introduced in #2445, so as to be explicit via the type system on which types of keys can be used with NiDkg (and which cannot) and at the same time be future proof.

This type could be a struct or an enum (or somewhat both): there is a discussion around it here: https://github.com/dfinity/ic/pull/2388/files#r1839804072

I think his suggestion makes sense, so I'll start working on integrating that. Please let me know if you'd prefer to keep it as is, or if you have other comments regarding this.

fspreiss added 19 commits October 31, 2024 00:54

feat(crypto): CRP-2597 make NiDkgTag non-Copy

a4e5d35

More fixes

808aa1d

Take NiDkgId as ref in combine_threshold_sig_shares

9cb80e8

Take NiDkgId as ref in verify_threshold_sig_combined

4461616

Take NiDkgId as ref at more places

0ae9cdc

Fmt

3a8675b

Merge remote-tracking branch 'origin' into franzstefan/CRP-2597-make-…

8c447fd

…nidkgtag-non-copy

Remove unused dkg_id param of NiDkgCspClient::load_threshold_signing_key

5e466de

Cleanup benches

b5343d6

NiDkgConfig::dkg_id now returns ref

64a7b7b

Remove obsolete dkg_id param in NiDkgCspClient::create_dealing

e0e60b9

Remove obsolete dkg_id param in NiDkgCspClient::verify_dealing

26e2b6a

Remove obsolete dkg_id param in NiDkgCspClient::verify_resharing_dealing

14fec1d

Cleanup

7339b90

refactor(crypto): CRP-2597 move MasterPublicKeyId protobuf from regis…

51c18be

…try/crypto to types

Clippy et al

d3a5dc8

Merge remote-tracking branch 'origin' into franzstefan/CRP-2597-make-…

7fda226

…nidkgtag-non-copy

Merge branch 'franzstefan/CRP-2597-make-nidkgtag-non-copy' into franz…

e98b7d8

…stefan/CRP-2597-move-masterpublickeyid-proto-to-types-ontopof-non-copy-ni-dkg-id

feat(crypto): CRP-2597-extend-nidkg-tag-with-high-threshold-for-keyid…

dbaf13a

…-variant

github-actions bot added the feat label Nov 5, 2024

fspreiss added 3 commits November 5, 2024 13:51

Fix deps

c51dc23

Remove obsolete conversion i32->NiDkgTag

fce1c63

Merge remote-tracking branch 'origin' into franzstefan/CRP-2597-move-…

d3c49ff

…masterpublickeyid-proto-to-types

fspreiss changed the base branch from franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types-ontopof-non-copy-ni-dkg-id to franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types November 6, 2024 08:38

fspreiss added 6 commits November 6, 2024 12:11

Merge branch 'franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types

ac819a5

' into franzstefan/CRP-2597-extend-nidkg-tag-with-keyid-variant

Merge remote-tracking branch 'origin' into franzstefan/CRP-2597-move-…

d43b4eb

…masterpublickeyid-proto-to-types

Merge remote-tracking branch 'origin' into franzstefan/CRP-2597-move-…

e003679

…masterpublickeyid-proto-to-types

Merge branch 'franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types

06f027f

' into franzstefan/CRP-2597-extend-nidkg-tag-with-keyid-variant

Cleanup

6a5a578

Tests

07fb339

fspreiss added 2 commits November 7, 2024 13:48

Update ThresholdSigDataStore tests

4d167a8

Merge branch 'franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types

348d4a0

' into franzstefan/CRP-2597-extend-nidkg-tag-with-keyid-variant

eichhorl reviewed Nov 7, 2024

View reviewed changes

fspreiss added 7 commits November 7, 2024 16:11

Link to CON-1416 in generate_responses_to_setup_initial_dkg_calls

61cc0d7

Link to CON-1413 in get_configs_for_local_transcripts and combine pat…

ae903fb

…terns

Link to CON-1417 in test_make_genesis_summary

dece589

Link to CON-1413 in test_create_regular_summaries

88f4281

Remove TODO on deciding about TaggedNiDkgTranscript serialization

6f9dd2a

Remove TODO for testing build_tagged_transcripts_map

d9c19c3

Add TODO to remove result_large_err clippy exception

2f3ad02

Base automatically changed from franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types to master November 9, 2024 22:08

Merge remote-tracking branch 'origin' into franzstefan/CRP-2597-exten…

dac4059

…d-nidkg-tag-with-keyid-variant

fspreiss marked this pull request as ready for review November 11, 2024 09:34

fspreiss requested review from a team as code owners November 11, 2024 09:34

github-actions bot added @consensus @crypto-team @execution @ic-interface-owners labels Nov 11, 2024

fspreiss requested review from altkdf, randombit and maksymar November 11, 2024 12:06

eichhorl approved these changes Nov 11, 2024

View reviewed changes

altkdf approved these changes Nov 12, 2024

View reviewed changes

maksymar approved these changes Nov 12, 2024

View reviewed changes

eichhorl reviewed Nov 12, 2024

View reviewed changes

Sawchord approved these changes Nov 12, 2024

View reviewed changes

fspreiss commented Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(crypto): CRP-2597 Extend NiDkgTag with HighThresholdForKey variant #2445

feat(crypto): CRP-2597 Extend NiDkgTag with HighThresholdForKey variant #2445

fspreiss commented Nov 5, 2024 •

edited

Loading

eichhorl Nov 7, 2024

fspreiss Nov 7, 2024

eichhorl Nov 7, 2024

fspreiss Nov 7, 2024

eichhorl Nov 7, 2024

fspreiss Nov 7, 2024

eichhorl Nov 7, 2024

fspreiss Nov 7, 2024

altkdf left a comment

altkdf Nov 11, 2024

fspreiss Nov 13, 2024

altkdf Nov 13, 2024

altkdf Nov 12, 2024

Sawchord Nov 12, 2024

fspreiss Nov 14, 2024

altkdf Nov 12, 2024

maksymar left a comment

eichhorl Nov 12, 2024

Sawchord Nov 12, 2024

eichhorl Nov 12, 2024

Sawchord Nov 12, 2024

Sawchord Nov 12, 2024

Sawchord Nov 12, 2024

fspreiss Nov 13, 2024

		@@ -31,18 +32,22 @@ mod tests;

		/// Allows to distinguish protocol executions in high and low threshold

		1 => Ok(NiDkgTag::LowThreshold),
		2 => Ok(NiDkgTag::HighThreshold),

feat(crypto): CRP-2597 Extend NiDkgTag with HighThresholdForKey variant #2445

Are you sure you want to change the base?

feat(crypto): CRP-2597 Extend NiDkgTag with HighThresholdForKey variant #2445

Conversation

fspreiss commented Nov 5, 2024 • edited Loading

Notes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altkdf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksymar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fspreiss commented Nov 5, 2024 •

edited

Loading