Signing flow with derived accounts #990

HCastano · 2024-08-07T21:48:20Z

This PR allows signature requests to happen using the new derived account flow that was
implemented in #955. I've implemented it in such a way that the old registration and signing
flow is also valid as I didn't want to break everything.

I want to have a few follow up PRs related to this in order to keep the review queue
manageable:

After this gets merged I want to update the entropy-test-cli to support the new
derived flows and ensure everything works
Following that we can start getting rid of old code
Related to the above, at that point I want to start improving the test coverage for
both the registration and signing flows, using the tests from the old flows as a
reference to try and avoid losing coverage
Implement the message passing as described in the Tofino spec

HCastano · 2024-08-13T16:52:16Z

pallets/registry/src/lib.rs

@@ -119,6 +119,8 @@ pub mod pallet {
    pub struct RegisteredInfo<T: Config> {
        pub programs_data: BoundedVec<ProgramInstance<T>, T::MaxProgramHashes>,
        pub program_modification_account: T::AccountId,
+        /// The SCALE encoded BIP-32 `DerivationPath` used to register this account.
+        pub derivation_path: Option<Vec<u8>>,


@JesseAbram do you know what kind of runtime version bump we'd need for this? For the release it doesn't make a difference since we're bumping everything anyways, but I'm more curious in case we ever do this as a one-off thing

tbh I just click all of them and let you tell me which one it was

JesseAbram · 2024-08-13T17:16:19Z

crates/client/src/user.rs

    pub signature_verifying_key: Vec<u8>,
 }

 pub async fn get_signers_from_chain(
    api: &OnlineClient<EntropyConfig>,
    rpc: &LegacyRpcMethods<EntropyConfig>,
+    with_parent_key: bool,


ehhh maybe a get signers from chain and a get validators from chain function makes more sense

I don't disagree, but for the case of this function it is always getting the signers. The difference between both arms here is that the signers are either: queried directly or grabbed from the validator set.

JesseAbram · 2024-08-13T17:27:35Z

pallets/registry/src/lib.rs

@@ -119,6 +119,8 @@ pub mod pallet {
    pub struct RegisteredInfo<T: Config> {
        pub programs_data: BoundedVec<ProgramInstance<T>, T::MaxProgramHashes>,
        pub program_modification_account: T::AccountId,
+        /// The SCALE encoded BIP-32 `DerivationPath` used to register this account.
+        pub derivation_path: Option<Vec<u8>>,


tbh I just click all of them and let you tell me which one it was

crates/threshold-signature-server/Cargo.toml

ameba23

Great to see this working. This all happened faster than i imagined it would.

And im looking forwards to seeing the old stuff removed and hopefully everything a bit cleaner. Also looking forwards to being able to try this out with docker-compose and the test cli.

crates/client/src/client.rs

ameba23 · 2024-08-14T06:45:31Z

crates/client/src/user.rs

    // TODO #899 For now we just take the first t validators as the ones to perform signing
-    validators.sort();


Fine for now to not sort - but the reason we have this was because of query results coming back in a different order with polkadot js vs subxt. So the idea was to sort before truncating.

If the plan is that this fn should no longer be called by both the client and the TS server then we need to remove it from entropy-client.

I think it will still be called by both. Probably safer to keep the sorting

@ameba23 when I sorted the signers coming from the on-chain flow the signing test started failing. Not entirely sure why 🤷

ameba23 · 2024-08-14T07:01:02Z

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

@@ -207,22 +213,41 @@ pub async fn sign_tx(
        request_author,
    };

-    let _has_key = check_for_key(&string_verifying_key, &app_state.kv_store).await?;
+    // In the new registration flow we don't store the verifying key in the KVDB, so we only do this
+    // check if we're using the old registration flow


But we could still check here if we are currently in the signing committee. But i guess theres no point as we are about to attempt to get the parent keyshare in get_sign_context anyway

We weren't checking for inclusion in the signing committee here anyways, so maybe I'm misunderstanding your comment

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

ameba23 · 2024-08-14T07:21:47Z

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

@@ -362,6 +364,134 @@ async fn test_sign_tx_no_chain() {
    clean_tests();
 }

+#[tokio::test]
+#[serial]
+async fn signature_request_with_derived_account_works() {


Could we now remove the new registration flow test below? Is there anything in it which is not repeated here?

I want to keep them separate. We already have enough tests that conflate what their actual purpose is. What I'd want to do instead is trim down some of the code in the signing test to be the bare minimum setup we need - but I wasn't sure what the best way to do that yet so I left it in full.

Turns out this breaks the signing flow for some reason 🤷

* master: Bump serde_json from 1.0.124 to 1.0.125 in the patch-dependencies group (#1007) Signing flow with derived accounts (#990) Add `network-jumpstart` command to `entropy-test-cli` (#1004) Refactor reshare (#994) Migrate circle-ci workflow to github actions (#991) Delete old keyshare if not in next_signers (#999)

HCastano added 4 commits August 12, 2024 12:12

Store derivation_path in RegisteredInfo

f2a9f23

Bump metadata

b8a038b

Split registration flow based on existence of derivation path

e6a5ee1

Tiny up a few things related to jumpstart mocking

117d420

HCastano force-pushed the hc/new-signing-flow branch from 1a4a39c to 117d420 Compare August 12, 2024 16:15

HCastano added 12 commits August 12, 2024 12:17

Small grammatical tidyings

2f718ed

Remove extra function introduced in merge

31937ea

Fix a few typos

122bdfe

Add a few logging events

8b60957

Allow querying both new and old signers using helper

1cd8ca6

Add working test for signature request with derived account

a2b77c3

Fix Registry test compilation

170f5e0

Don't ignore old signing test

4f3fb8c

Fix Registry benchmark compilation

2f00c95

Handle BIP-32 errors instead of panicking

95416f8

Store full BIP-32 derivation path on-chain

80e315f

Add CHANGELOG entry

2ea2968

HCastano marked this pull request as ready for review August 13, 2024 16:50

HCastano requested review from JesseAbram and ameba23 August 13, 2024 16:50

HCastano commented Aug 13, 2024

View reviewed changes

Merge branch 'master' into hc/new-signing-flow

5a7757b

JesseAbram approved these changes Aug 13, 2024

View reviewed changes

ameba23 approved these changes Aug 14, 2024

View reviewed changes

Sort signers from Staking Extension pallet when signing

bac4ea8

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

Revert "Sort signers from Staking Extension pallet when signing"

a5e1dc3

Turns out this breaks the signing flow for some reason 🤷

HCastano merged commit 13a365f into master Aug 14, 2024
14 checks passed

HCastano deleted the hc/new-signing-flow branch August 14, 2024 21:58

HCastano mentioned this pull request Aug 27, 2024

Registration and signing flow follow-ups #1033

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signing flow with derived accounts #990

Signing flow with derived accounts #990

HCastano commented Aug 7, 2024 •

edited

Loading

HCastano Aug 13, 2024

JesseAbram Aug 13, 2024

JesseAbram Aug 13, 2024

HCastano Aug 13, 2024

JesseAbram Aug 13, 2024

ameba23 left a comment

ameba23 Aug 14, 2024

HCastano Aug 14, 2024

HCastano Aug 14, 2024

ameba23 Aug 14, 2024

HCastano Aug 14, 2024

ameba23 Aug 14, 2024

HCastano Aug 14, 2024

		// TODO #899 For now we just take the first t validators as the ones to perform signing
		validators.sort();

Signing flow with derived accounts #990

Signing flow with derived accounts #990

Conversation

HCastano commented Aug 7, 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

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

HCastano commented Aug 7, 2024 •

edited

Loading