New on-chain registration flow #955

HCastano · 2024-07-19T22:24:22Z

This PR adds a new extrinsic, Registry::register_on_chain(), which uses the new child
key derivation feature in Synedrion in order to simplify the registration process.

Since we can derive a child verification key from a parent verification key, all we need
to do for user registration is derive a user's key from the network key and store it
on-chain (alongside some program info). We don't need to reach out to the TSS to do any
cryptographic operations for us.

This PR is the first of a few that will be required to fully integrate the new
registration flow. In follow up PRs I plan to:

Improve the mechanism used to derive accounts
Remove old code related to registration (on the chain and TSS sides)
Consolidate our old tests to use the new extrinsic and ensure we don't lose any
coverage

Closes #961.

This way we can introduce it without breaking everything just yet

This lets us use the count as the derivation path for now.

This lets us change the mapping so that instead of being indexed by verifying key we're not indexing by the signature request account.

The notion of the signature_request_account is meant to be ephemeral, so we don't really need to have registration know anything about this beyond the initial extrinsic.

…ple times

HCastano · 2024-07-23T01:57:24Z

The CI is failing because Synedrion isn't able to compile to the wasm32-unknown-unknown target right now. Talking with Bogdan about this and will hopefully have it sorted soon 🤞

github-actions · 2024-08-01T01:07:32Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

HCastano · 2024-08-02T03:29:57Z

pallets/registry/src/lib.rs

+                    return Err(Error::<T>::JumpStartNotCompleted.into());
+                };
+
+            // TODO (Nando): For a `CountedStorageMap` there is the possibility that the counter


I'll swap this out with an issue after we go through the review process a bit in case there are discussions around this that pop up

ya also are we not moving from a counter to using the account or is there a possibility we would keep the counter?

There is the possibility we keep the counter, but it would probably be in the context of something like hash(account ID) ++ counter

Opened #984.

pallets/registry/Cargo.toml

ameba23

💯 looks great.

Only concern i have about using a counter as input is that anybody can know what the next entropy account public key is going to be before it is made. As soon as the network jump starts everyone can know the complete set of all the entropy account public keys there will ever be.

I can't think of a reason why this is bad - but my hunch is that concatenating something from the user (eg: signature request account id and/or program modification account id) to the counter in the derivation path feels safer.

pallets/registry/src/lib.rs

ameba23 · 2024-08-02T07:15:28Z

pallets/registry/src/lib.rs

+            let network_verifying_key =
+                if let Some(key) = <JumpStartProgress<T>>::get().verifying_key {
+                    SynedrionVerifyingKey::try_from(key.as_slice())
+                        .expect("The network verifying key must be valid.")


Did you mean to leave this expect in? I guess it should be impossible to get into a state where it is invalid

ya def remove it for proper error handling

Yes this is intentional. If this expect is hit it means there's a bug somewhere in the code (we're storing an invalid verifying key), and I would like users to log a report about it.

ameba23 · 2024-08-02T07:19:59Z

pallets/registry/src/lib.rs

+            // down the line.
+            let count = RegisteredOnChain::<T>::count();
+            let path =
+                bip32::DerivationPath::from_str(&scale_info::prelude::format!("m/0/{}", count))


im guessing the m/0 bit is like a version number that we can bump if the derivation process changes?

No, the m/.../... bit is the actual derivation path.

If you take a look at the graphic here you can see that it's sort of like a namespace. So accounts under m/0/* are all associated with each other, while those under m/1/* are distinct from m/0/* (but associated with each other).

ameba23 · 2024-08-02T07:32:10Z

pallets/registry/src/lib.rs

+            let signature_request_account = ensure_signed(origin)?;
+
+            ensure!(
+                signature_request_account.encode() != NETWORK_PARENT_KEY.encode(),


I dont think we need this check anymore, as it was to stop the network parent keyshare getting clobbered by a user keyshare in the kvdb. But here the user has no keyshare to be stored.

yaaaa but this may be one of those I don't know what this could lead to but I have a bad feeling about it not being in here

In the jumpstart PR where this was introduced I mentioned that allowing registration from the parent key could be cool.

While a bit weird, it would allow the network to have a registered account for use on other networks. Maybe it could allow for governance actions on Bitcoin or Ethereum for example. I'd be down to remove this check as long as its cool with both of you

I think leave it in make an issue and this is actually a good retreat topic

Opened #983.

ameba23 · 2024-08-02T07:38:56Z

pallets/registry/src/lib.rs

+            let child_verifying_key = BoundedVec::try_from(
+                child_verifying_key.to_encoded_point(true).as_bytes().to_vec(),
+            )
+            .expect("Synedrion must have returned a valid verifying key.");


agree that this could never happen but i still kinda don't like having expects

Same thing here, this would be a bug on the Synedrion side and I would like a bug report about it instead of "handling" it.

JesseAbram

looks good in flow, won't approve till you do negative tests

pallets/registry/src/lib.rs

JesseAbram · 2024-08-02T14:12:42Z

pallets/registry/src/lib.rs

+            let signature_request_account = ensure_signed(origin)?;
+
+            ensure!(
+                signature_request_account.encode() != NETWORK_PARENT_KEY.encode(),


yaaaa but this may be one of those I don't know what this could lead to but I have a bad feeling about it not being in here

JesseAbram · 2024-08-02T14:13:45Z

pallets/registry/src/lib.rs

+            let network_verifying_key =
+                if let Some(key) = <JumpStartProgress<T>>::get().verifying_key {
+                    SynedrionVerifyingKey::try_from(key.as_slice())
+                        .expect("The network verifying key must be valid.")


ya def remove it for proper error handling

pallets/registry/src/lib.rs

JesseAbram · 2024-08-02T14:17:02Z

pallets/registry/src/lib.rs

+                    return Err(Error::<T>::JumpStartNotCompleted.into());
+                };
+
+            // TODO (Nando): For a `CountedStorageMap` there is the possibility that the counter


ya also are we not moving from a counter to using the account or is there a possibility we would keep the counter?

JesseAbram · 2024-08-02T14:18:14Z

pallets/registry/src/lib.rs

+            // down the line.
+            let count = RegisteredOnChain::<T>::count();
+            let path =
+                bip32::DerivationPath::from_str(&scale_info::prelude::format!("m/0/{}", count))


This looks good but what path is the parent key at and we def need to block it, if the parent key is at 0 we need to start the count from 1

If we use m/0/0 it still ends up with a different private key. From this bip32 website:

true Im fine, should be a really easy test, just one assert statment in the test, probably worth it?

I don't think this is possible as long as some sort of derivation step exists in the extrinsic.

What do you think a test for this should look like though? Would we just check the m/0/0 path, or enumerate all the paths and check those?

just in the positive register test you can take the key you put as the parent network key derived off and assert there is not program data for that, one line no?

That would only check for that specific derivation path used in the test though, e.g m/0/0. Right now I don't think it's really practical to test for this, but I'll consider it further when making more changes to the actual derivation scheme

JesseAbram · 2024-08-02T14:18:34Z

pallets/registry/src/lib.rs

+            let child_verifying_key = BoundedVec::try_from(
+                child_verifying_key.to_encoded_point(true).as_bytes().to_vec(),
+            )
+            .expect("Synedrion must have returned a valid verifying key.");


pallets/registry/src/lib.rs

pallets/registry/src/benchmarking.rs

JesseAbram

add negative tests

ameba23 · 2024-08-05T06:43:43Z

@HCastano @JesseAbram - one thing that just occurred to me about this:

When we come to do the signing flow, i assume the signature request is going to specify the verifying key, and that to sign a message the threshold server will need to know the derivation path.

So we would need a way of looking up what count number a given verifying key has. Maybe CountedStorageMap gives us some easy way to get the count of a given key - but i can't see it. So maybe we need to store the count in RegisteredInfo.

JesseAbram · 2024-08-05T18:27:25Z

@HCastano @JesseAbram - one thing that just occurred to me about this:

When we come to do the signing flow, i assume the signature request is going to specify the verifying key, and that to sign a message the threshold server will need to know the derivation path.

So we would need a way of looking up what count number a given verifying key has. Maybe CountedStorageMap gives us some easy way to get the count of a given key - but i can't see it. So maybe we need to store the count in RegisteredInfo.

good point, def the second, the second is magnitudes more simple

HCastano · 2024-08-06T13:56:53Z

@ameba23

Only concern i have about using a counter as input is that anybody can know what the next entropy account public key is going to be before it is made. As soon as the network jump starts everyone can know the complete set of all the entropy account public keys there will ever be.

I can't think of a reason why this is bad - but my hunch is that concatenating something from the user (eg: signature request account id and/or program modification account id) to the counter in the derivation path feels safer.

Yeah knowing the public key of an account isn't inherently dangerous. Maybe there are some scenarios where this could be used in some way, but I don't think it's any different from having access to a vanity generator (e.g if you want an account like 0xbeef.... We should definitely incorporate some element other than the counter though.

When we come to do the signing flow, i assume the signature request is going to specify the verifying key, and that to sign a message the threshold server will need to know the derivation path.

So we would need a way of looking up what count number a given verifying key has. Maybe CountedStorageMap gives us some easy way to get the count of a given key - but i can't see it. So maybe we need to store the count in RegisteredInfo.

You're right here, and it's something I was going to deal with in the signing flow PRs.

HCastano added 21 commits July 19, 2024 17:01

Trial of using types and derivation code

e4a8ad0

Move new registration code to its own extrinsic

0ab281a

This way we can introduce it without breaking everything just yet

Use my branch of Synedrion

08bb789

Sketch out using the account ID as derivation path

c763efc

Basic test setup

fef5704

Use a CountedStorageMap for Registered struct

9515834

This lets us use the count as the derivation path for now.

Add some error handling

8b11b4c

Tidy genesis build

53a428d

Add registration struct for new flow

9ff2ea6

This lets us change the mapping so that instead of being indexed by verifying key we're not indexing by the signature request account.

Use updated RegisteredInfo struct in tests

dd0f613

Test flow and validate verifying key

9426027

Encode the signature request account in place

cdf0cfb

Update some of the variable names

8a30466

Revert registration struct to be indexed by verifying key

46c410e

The notion of the signature_request_account is meant to be ephemeral, so we don't really need to have registration know anything about this beyond the initial extrinsic.

Initialize RegisteredOnChain during genesis builds

a0a9b16

Remove check that a signature_request_account can only register once

9814934

Insert verifying key into ModifiableKeys storage struct

1d7fbdb

Remove dbg! statement

2723aa9

Add test to check that a signature request account can register multi…

0efb5f6

…ple times

Add benchmark for new registration extrisic

3d8d334

Add comments around registration flow

73a9c3f

HCastano added 2 commits July 31, 2024 17:33

Use no_std compatible version of Synedrion

57fc59a

Use core and alloc data structures in pallet

2a7b4cb

HCastano added 5 commits July 31, 2024 18:10

Use remote Synedrion reference

12fb5ec

Merge branch 'master' into hc/new-on-chain-registration-flow

11a65d9

Make derivation path in benchmarks generic instead of hardcoded

86f1b21

Use benchmark results for new extrinsic

3923688

Rename extrinsic to register_on_chain

6f02367

HCastano added 2 commits August 1, 2024 23:29

Use no_std functions for benches

21d0eda

Add entry to CHANGELOG

64d6fdb

HCastano commented Aug 2, 2024

View reviewed changes

HCastano added the Bump `spec_version` A change which affects the current runtime behaviour label Aug 2, 2024

HCastano marked this pull request as ready for review August 2, 2024 03:31

HCastano requested review from JesseAbram and ameba23 August 2, 2024 03:31

HCastano commented Aug 2, 2024

View reviewed changes

pallets/registry/Cargo.toml Outdated Show resolved Hide resolved

ameba23 approved these changes Aug 2, 2024

View reviewed changes

JesseAbram reviewed Aug 2, 2024

View reviewed changes

JesseAbram requested changes Aug 2, 2024

View reviewed changes

Add a few tests for failure conditions

ddc8f2d

HCastano requested a review from JesseAbram August 2, 2024 21:48

HCastano added 2 commits August 2, 2024 18:00

Merge branch 'master' into hc/new-on-chain-registration-flow

06fe835

Fix breaking changes from #974

7ffc56c

JesseAbram approved these changes Aug 2, 2024

View reviewed changes

Bump Synedrion to latest master

ac33b2a

This was referenced Aug 6, 2024

Allow registration from NETWORK_PARENT_KEY #983

Open

Derivation scheme used for registration #984

Open

Add issue number to TODO comment

b4f41e1

HCastano merged commit 381b310 into master Aug 6, 2024
14 checks passed

HCastano deleted the hc/new-on-chain-registration-flow branch August 6, 2024 14:57

github-actions bot locked and limited conversation to collaborators Aug 6, 2024

New on-chain registration flow #955

New on-chain registration flow #955

Conversation

HCastano commented Jul 19, 2024 • edited Loading

HCastano commented Jul 23, 2024

github-actions bot commented Aug 1, 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

ameba23 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

JesseAbram 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

JesseAbram left a comment

Choose a reason for hiding this comment

ameba23 commented Aug 5, 2024

JesseAbram commented Aug 5, 2024

HCastano commented Aug 6, 2024

HCastano commented Jul 19, 2024 •

edited

Loading

github-actions bot commented Aug 1, 2024 •

edited

Loading