Remove permissioned access type #666

JesseAbram · 2024-03-08T23:12:58Z

Closes #650

As well without permissioned a sig request key is not needed and the verifying key is used to map the user to account

…ve-permissioned

vercel · 2024-03-08T23:13:04Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
entropy-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 22, 2024 6:06pm

…ve-permissioned

crates/testing-utils/src/test_client/mod.rs

ameba23 · 2024-03-13T14:52:43Z

crates/testing-utils/src/test_client/mod.rs

@@ -145,7 +147,7 @@ pub async fn sign(
    api: &OnlineClient<EntropyConfig>,
    rpc: &LegacyRpcMethods<EntropyConfig>,
    user_keypair: sr25519::Pair,
-    signature_request_account: Option<SubxtAccountId32>,
+    signature_verifying_key: Vec<u8>,


Would be nice to have a type for this or at least make it a [u8; 33]

tbh this PR has been a lot I wanna make a follow up with this and the program modification comment

crates/threshold-signature-server/src/user/api.rs

HCastano

Took a first glance today, still trying to wrap my head around the implications of using the verifying key here

HCastano · 2024-03-20T21:34:07Z

pallets/registry/src/lib.rs

    }

    #[pallet::genesis_build]
    impl<T: Config> BuildGenesisConfig for GenesisConfig<T> {
        fn build(&self) {
            for account_info in &self.registered_accounts {
+                assert!(account_info.3.clone().len() as u32 == VERIFICATION_KEY_LENGTH);


len() shouldn't take an owned reference here, we probably don't need to clone() this

I'm also curious though, since we use a BoundedVec we're only guaranteed that we won't go past VERIFICATION_KEY_LENGTH, but not that the value is of that length, right?

Might be worth looking into enforcing that the length is as expected in the types themselves

oh I added an ensure length to the confirm_done function

HCastano · 2024-03-20T21:37:16Z

pallets/registry/src/lib.rs

+
+    #[pallet::storage]
+    #[pallet::getter(fn modifiable_keys)]
+    pub type ModifiableKeys<T: Config> = StorageMap<


I don't really understand what this is used for, can you clarify for me?

I would also like to know more about this.

It looks to me like this caps the number of entropy accounts which a single program modification account may control.

One thing to be aware of is that there is currently no check that the user owns the program modification account, so someone could activate this limit for someone else's program modification account. Although thats not really so bad since it doesn't effect their ability to use it with their existing accounts.

ya so verifying_key => prog_mod_account (and other things) but a user can have multiple vk to pma so when they log in given one pma it should easily pull up all their vk

this would be solved with #677 if we use a prog mod sig but ya def a potential

@JesseAbram I still don't understand the use case here.

Maybe as a follow up, is this something we need to introduce in this PR, or can we do it after? Because maybe having it added in isolation would help me understand the usecase a bit more

ehhh maybe but @frankiebee asked for it and it is heavily related to this so idk if that would be the best call

HCastano · 2024-03-20T21:37:58Z

pallets/registry/src/lib.rs

-        /// An account has been registered. \[who\]
-        AccountRegistered(T::AccountId),
+        /// An account has been registered. [who, signing_group, verifying_key]
+        AccountRegistering(T::AccountId, u8, VerifyingKey),


How would we be able to know the VerifyingKey before the registration process is done?

In registering this is the process where we need one representative from each subgroup to call back into the chain, this is like a checkpoint where validator x from subgroup n sends their verifying key saying im done here is what I got, the last validator triggered the registered event

pallets/registry/src/lib.rs

HCastano · 2024-03-20T21:43:55Z

pallets/registry/src/lib.rs

@@ -453,7 +473,11 @@ pub mod pallet {
                }
                registering_info.confirmations.push(signing_subgroup);
                Registering::<T>::insert(&sig_req_account, registering_info);
-                Self::deposit_event(Event::AccountRegistering(sig_req_account, signing_subgroup));
+                Self::deposit_event(Event::AccountRegistering(


Hmm, maybe we should change this to be something like a Confirmation event or something like that. From reading the events it seemed like this would only be fired in the register extrinsic

oh that is SignalRegistery, but ya im easy when it comes to naming

Yeah, let's change it since it's a bit confusing.

Something like RecievedConfirmation would be better imo

HCastano · 2024-03-20T21:51:32Z

crates/testing-utils/src/tss_server_process.rs

+    passed_verifying_key: Option<Vec<u8>>,
    // If this is true a keyshare for the user will be generated and returned
    extra_private_keys: bool,
+    // If true keyshare and verifying key is deterministic
+    deterministic_key_share: bool,


What are the expected interactions between the passed_verifying_key and deterministic_key_share flags like?

E.g, should I be allowed to pass my own verifying key and also ask for a deterministic key share?

ya, wouldn't do much tho

HCastano · 2024-03-20T21:54:40Z

crates/testing-utils/src/tss_server_process.rs

+        let signing_key = if deterministic_key_share {
+            Some(
+                SigningKey::from_bytes(
+                    &hex!("4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318")


How was this key derived? Can we derive it on the fly, or maybe put it in a const?

can put it in a const, I pulled it from the k256 docs

HCastano · 2024-03-20T21:58:11Z

crates/testing-utils/src/constants.rs

+    pub static ref DEFAULT_VERIFYING_KEY_NOT_REGISTERED: Vec<u8> = vec![10; VERIFICATION_KEY_LENGTH as usize];
+    pub static ref DAVE_VERIFYING_KEY: Vec<u8> = vec![1; VERIFICATION_KEY_LENGTH as usize];
+    // this key is associated with a constant key share generation in spawn_testing_validators
+    pub static ref EVE_VERIFYING_KEY: Vec<u8> = vec![2, 78, 59, 129, 175, 156, 34, 52, 202, 208, 157, 103, 156, 230, 3, 94, 209, 57, 35, 71, 206, 100, 206, 64, 95, 93, 205, 54, 34, 138, 37, 222, 110];


Similar question here, how was this derived?

@HCastano This is the public key corresponding to the private key given here:

entropy-core/crates/testing-utils/src/tss_server_process.rs

Line 90 in 5676471

&hex!("4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318")

The problem was that EVE is a pre-registered account used in the private mode test, but we could not know the verifying key beforehand when generating the keyshares randomly. So Jesse found how to give a static private key when generating shares with synedrion, and this is its public key.

Probably the private key should also be given as a constant here, or even generated here with lazy_static or something. But i'd be happy to have that done it a follow up.

it was the verifying key generated from the const generate from the deterministic key code above

ameba23

🥇

Great. As already discussed theres maybe still some fixes needed for the test-cli, which can be done in a follow up. And i would like to see a type for a serialized verifying key - although i see we already have a type alias for it in the registry pallet. Something with methods or trait implementations for converting to / from synedrion::ecdsa::VerifyingKey would be really cool.

If we can get this merged soon it will make me happy as this effects the stuff im doing with changing SignedMessage - so followups are good for me.

crates/test-cli/src/main.rs

ameba23 · 2024-03-21T08:41:58Z

crates/testing-utils/src/constants.rs

+    pub static ref DEFAULT_VERIFYING_KEY_NOT_REGISTERED: Vec<u8> = vec![10; VERIFICATION_KEY_LENGTH as usize];
+    pub static ref DAVE_VERIFYING_KEY: Vec<u8> = vec![1; VERIFICATION_KEY_LENGTH as usize];
+    // this key is associated with a constant key share generation in spawn_testing_validators
+    pub static ref EVE_VERIFYING_KEY: Vec<u8> = vec![2, 78, 59, 129, 175, 156, 34, 52, 202, 208, 157, 103, 156, 230, 3, 94, 209, 57, 35, 71, 206, 100, 206, 64, 95, 93, 205, 54, 34, 138, 37, 222, 110];


@HCastano This is the public key corresponding to the private key given here:

entropy-core/crates/testing-utils/src/tss_server_process.rs

Line 90 in 5676471

&hex!("4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318")

The problem was that EVE is a pre-registered account used in the private mode test, but we could not know the verifying key beforehand when generating the keyshares randomly. So Jesse found how to give a static private key when generating shares with synedrion, and this is its public key.

Probably the private key should also be given as a constant here, or even generated here with lazy_static or something. But i'd be happy to have that done it a follow up.

ameba23 · 2024-03-21T08:51:06Z

crates/testing-utils/src/test_client/mod.rs

+                query_chain(api, rpc, registered_query, block_hash).await.unwrap();
+            if registered_status.is_some() {
+                // check if the event belongs to this user
+                if event.0 == account_id {


A nitpick, but can we not do this check before doing the registered status query, so that we only do that query if the account in the event is ours?

ameba23 · 2024-03-21T08:59:44Z

crates/testing-utils/src/tss_server_process.rs

        let validator_1_threshold_keyshare: Vec<u8> =
            entropy_kvdb::kv_manager::helpers::serialize(&shares[0]).unwrap();
        let validator_2_threshold_keyshare: Vec<u8> =
            entropy_kvdb::kv_manager::helpers::serialize(&shares[1]).unwrap();
+
+        // uses the deterministic verifying key if requested
+        let verifying_key = if deterministic_key_share {


Not critical but do we need this if? If deterministic_key_share is true the public key from the keyshare should be identical to passed_verifying_key

no and maybe I should refactor this function a little (im down but in a different PR tbh we should probably merge this soon) but you can still pass a passed_verifying_key but if you set deterministic_key_share to true it will override that and use the verifying_key from the keyshare created

ameba23 · 2024-03-21T09:34:49Z

crates/threshold-signature-server/src/helpers/tests.rs

@@ -120,9 +123,11 @@ pub async fn create_clients(
 }

 pub async fn spawn_testing_validators(


Not related to this PR but i just noticed we have this function twice - this one used in unit tests and then again in testing_utils::tss_server_process used in integration tests. Im not sure what the difference is. I'll make an issue - might have been me that duplicated it, not sure.

it is, no difference to my mind I thought it might have been std and crate issues and didn't look into it tbh

Pretty sure it had something to do with not importing certain crates into the integration tests. But yeah, I also think it was in one of your PRs Peg

ameba23 · 2024-03-21T09:48:33Z

crates/threshold-signature-server/src/user/tests.rs

-    for _ in 0..10 {
+    let mut new_verifying_key = vec![];
+    // wait for registered event check that key exists in kvdb
+    for _ in 0..45 {


Im curious - does this actually fail if you only give it 10 tries as before, or did you bump that when the test wasn't working for some other reason?

lol no so the tests dev chain expects 2 validators and substate randomly choose which one should produce a block, in tests for ease we only spin up one, and they get chosen randomly (vrf really cool actually), so sometimes we get like 30 second delayed blocks and ya it could randomly fail at 10 sometimes lol

ameba23 · 2024-03-21T09:51:18Z

crates/threshold-signature-server/src/validator/tests.rs

-        "5HGjWAeFDfFCWPsjFQdVV2Msvz2XtMktvgocEZcCj68kUMaw",
+        hex::encode(DAVE_VERIFYING_KEY.to_vec()),
+        hex::encode(EVE_VERIFYING_KEY.to_vec()),
+        hex::encode(FERDIE_VERIFYING_KEY.to_vec()),


ameba23 · 2024-03-21T09:53:10Z

crates/threshold-signature-server/tests/protocol_wasm.rs

+                // check if the event belongs to this user
+                if event.0 == account_id {
+                    return event.1 .0;
+                }


This bit of code is used in a few places - we could maybe made a helper (in a follow up PR)

ameba23 · 2024-03-21T10:06:26Z

pallets/registry/src/lib.rs

+
+    #[pallet::storage]
+    #[pallet::getter(fn modifiable_keys)]
+    pub type ModifiableKeys<T: Config> = StorageMap<


I would also like to know more about this.

It looks to me like this caps the number of entropy accounts which a single program modification account may control.

One thing to be aware of is that there is currently no check that the user owns the program modification account, so someone could activate this limit for someone else's program modification account. Although thats not really so bad since it doesn't effect their ability to use it with their existing accounts.

ameba23 · 2024-03-21T10:13:14Z

crates/threshold-signature-server/src/validator/api.rs

@@ -178,11 +178,9 @@ pub async fn get_all_keys(
    while let Some(Ok((key, _account))) = iter.next().await {
        let new_key = hex::encode(key);
        let len = new_key.len();
-        let final_key = &new_key[len - 64..];
+        let final_key = &new_key[len - 66..];


Nitpick: could use entropy_shared::VERIFICATION_KEY_LENGTH * 2

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

Co-authored-by: peg <peg@magmacollective.org>

…ve-permissioned

HCastano

I'm still confused at the ModifiableKeys thing - but overall this does what I expect.

Good work on this 💪

CHANGELOG.md

pallets/registry/src/lib.rs

HCastano · 2024-03-22T01:13:38Z

pallets/registry/src/lib.rs

+
+    #[pallet::storage]
+    #[pallet::getter(fn modifiable_keys)]
+    pub type ModifiableKeys<T: Config> = StorageMap<


@JesseAbram I still don't understand the use case here.

Maybe as a follow up, is this something we need to introduce in this PR, or can we do it after? Because maybe having it added in isolation would help me understand the usecase a bit more

pallets/registry/src/lib.rs

HCastano · 2024-03-22T01:20:45Z

pallets/registry/src/benchmarking.rs

@@ -58,6 +58,7 @@ pub fn add_non_syncing_validators<T: Config>(
    <SigningGroups<T>>::insert(sig_party_number, validators.clone());
    for (c, validator) in validators.iter().enumerate() {
        <ThresholdServers<T>>::insert(validator, server_info.clone());
+        <ValidatorToSubgroup<T>>::insert(validator, 0);


Was this generally missing before this PR, or did you have to add it because of the changes introduced here?

oh ya I did wanna ping you about it but got bogged down, this was weird it was def introduced in yours and the CI didn't pick it up, after we merge this imma check to see if benchmarks are being run, cuz this should have been caught

crates/test-cli/src/main.rs

HCastano · 2024-03-22T01:43:22Z

crates/testing-utils/src/constants.rs

@@ -41,6 +42,14 @@ lazy_static! {
        .try_into()
        .unwrap(),
    ];
+    pub static ref DEFAULT_VERIFYING_KEY_NOT_REGISTERED: Vec<u8> = vec![10; VERIFICATION_KEY_LENGTH as usize];
+    pub static ref DAVE_VERIFYING_KEY: Vec<u8> = vec![1; VERIFICATION_KEY_LENGTH as usize];
+    // this key is associated with a constant key share generation in spawn_testing_validators


We'll need to update this comment now

HCastano · 2024-03-22T01:44:44Z

crates/testing-utils/src/constants.rs

+    pub static ref FERDIE_VERIFYING_KEY: Vec<u8> = vec![3; VERIFICATION_KEY_LENGTH as usize];
+    pub static ref DEFAULT_VERIFYING_KEY: Vec<u8> = vec![0; VERIFICATION_KEY_LENGTH as usize];
+    // key used to create a deterministic key share
+    pub static ref DETERMINISTIC_KEY_SHARE: [u8; 32] =  hex!("4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318");


Since you got this from the k256 docs, do you mind linking those here?

HCastano · 2024-03-22T01:45:55Z

pallets/registry/src/lib.rs

@@ -453,7 +473,11 @@ pub mod pallet {
                }
                registering_info.confirmations.push(signing_subgroup);
                Registering::<T>::insert(&sig_req_account, registering_info);
-                Self::deposit_event(Event::AccountRegistering(sig_req_account, signing_subgroup));
+                Self::deposit_event(Event::AccountRegistering(


Yeah, let's change it since it's a bit confusing.

Something like RecievedConfirmation would be better imo

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

JesseAbram added 3 commits March 7, 2024 15:55

remove permissioned

d6dea70

Merge branch 'master' of github.com:entropyxyz/entropy-core into remo…

3af0676

…ve-permissioned

have verifying key as key in registered state

989e89a

vercel bot deployed to Preview March 8, 2024 23:35 View deployment

Merge branch 'master' of github.com:entropyxyz/entropy-core into remo…

27034bd

…ve-permissioned

vercel bot had a problem deploying to Preview March 12, 2024 15:31 Failure

tss crate compile

8e4d33a

vercel bot had a problem deploying to Preview March 12, 2024 20:28 Failure

compiling

c626a6b

ameba23 reviewed Mar 13, 2024

View reviewed changes

crates/testing-utils/src/test_client/mod.rs Outdated Show resolved Hide resolved

ameba23 reviewed Mar 13, 2024

View reviewed changes

crates/testing-utils/src/test_client/mod.rs Outdated Show resolved Hide resolved

ameba23 reviewed Mar 13, 2024

View reviewed changes

vercel bot deployed to Preview March 13, 2024 15:02 View deployment

fix one broken test

65c067e

vercel bot deployed to Preview March 13, 2024 19:37 View deployment

fix another test

fe01aff

vercel bot deployed to Preview March 13, 2024 21:02 View deployment

ameba23 reviewed Mar 14, 2024

View reviewed changes

crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved

fix more tests

0a8cbda

vercel bot deployed to Preview March 14, 2024 23:04 View deployment

ameba23 mentioned this pull request Mar 15, 2024

Removing permissioned access mode makes rate limiting harder #672

Closed

sign tx test

45bbf9c

vercel bot deployed to Preview March 15, 2024 15:46 View deployment

more tests

6139f91

vercel bot deployed to Preview March 15, 2024 19:53 View deployment

fix all tests

27c508b

vercel bot deployed to Preview March 18, 2024 19:54 View deployment

at an event listener for registered

66bd7a4

vercel bot deployed to Preview March 18, 2024 22:13 View deployment

HCastano reviewed Mar 20, 2024

View reviewed changes

ameba23 approved these changes Mar 21, 2024

View reviewed changes

JesseAbram and others added 6 commits March 21, 2024 09:24

clean

a9c480f

Update pallets/registry/src/lib.rs

d2af298

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

fix

7cece5c

Update crates/test-cli/src/main.rs

cb7bbe1

Co-authored-by: peg <peg@magmacollective.org>

fixes

da9239c

Merge branch 'master' of github.com:entropyxyz/entropy-core into remo…

5cc4890

…ve-permissioned

vercel bot deployed to Preview March 21, 2024 14:18 View deployment

clean

65b5e9e

vercel bot deployed to Preview March 21, 2024 15:46 View deployment

HCastano approved these changes Mar 22, 2024

View reviewed changes

JesseAbram and others added 11 commits March 22, 2024 11:07

clean

4977541

Apply suggestions from code review

8bb7a94

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

clean

18981a7

clean

24013c7

fix

635da54

fix

9370430

fix

4faee12

fmt

bff4fbf

Update pallets/registry/src/lib.rs

b30d652

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

lint

c15ebf7

fix wasm

9421b70

vercel bot deployed to Preview March 22, 2024 17:13 View deployment

fix

9741afc

vercel bot deployed to Preview March 22, 2024 18:06 View deployment

JesseAbram merged commit da40bd7 into master Mar 22, 2024
10 checks passed

JesseAbram deleted the remove-permissioned branch March 22, 2024 18:25

HCastano changed the title ~~Remove permissioned~~ Remove permissioned access type Mar 22, 2024

ameba23 mentioned this pull request Apr 19, 2024

Improve test CLI following removing permissioned mode #770

Merged

		@@ -120,9 +123,11 @@ pub async fn create_clients(
		}

		pub async fn spawn_testing_validators(

Remove permissioned access type #666

Remove permissioned access type #666

Conversation

JesseAbram commented Mar 8, 2024

vercel bot commented Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano 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

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

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

ameba23 Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameba23 left a comment

Choose a reason for hiding this comment

ameba23 Mar 21, 2024 • edited Loading

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

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

HCastano 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

vercel bot commented Mar 8, 2024 •

edited

Loading

ameba23 Mar 21, 2024 •

edited

Loading

ameba23 Mar 21, 2024 •

edited

Loading