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

Fix TSS AccountId keys in chainspec #993

Merged
merged 4 commits into from
Aug 8, 2024
Merged

Fix TSS AccountId keys in chainspec #993

merged 4 commits into from
Aug 8, 2024

Conversation

JesseAbram
Copy link
Member

@JesseAbram JesseAbram commented Aug 8, 2024

Keys were generated incorrectly with subkey when we needed to use hpke

@HCastano HCastano changed the title Fix tss keys in chainspec Fix TSS AccountID keys in chainspec Aug 8, 2024
@HCastano HCastano changed the title Fix TSS AccountID keys in chainspec Fix TSS AccountId keys in chainspec Aug 8, 2024
Copy link
Contributor

@ameba23 ameba23 left a comment

Choose a reason for hiding this comment

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

Im approving but i haven't actually checked the keys. Should be from this fn right?

pub fn get_signer_and_x25519_secret_from_mnemonic(

Copy link
Collaborator

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

I need a bit more clarity on where the accounts are coming from

CHANGELOG.md Outdated Show resolved Hide resolved
/// Mnemonic: "beef dutch panic monkey black glad audit twice humor gossip wealth drive"
pub static ref DAVE: sp_runtime::AccountId32 =
super::hex!["12bf1c8e365c20cc2af606e3814b98b192857d85d182dac5e33fb90d0380ca75"].into();
super::hex!["0a9054ef6b6b8ad0dd2c89895b2515583f2fbf1edced68e7328ae456d86b9402"].into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I thought we were just supposed to be using subkey for these, not the HPKE stuff.

This is because an operator would be passing this in using the --mnemonic flag, which ideally they get by running subkey generate.

In this case then, the public key here should be 0x12bf1c8e365c20cc2af606e3814b98b192857d85d182dac5e33fb90d0380ca75 iiuc.

❮ ./target/release/entropy key inspect "beef dutch panic monkey black glad audit twice humor gossip wealth drive"
Secret phrase:       beef dutch panic monkey black glad audit twice humor gossip wealth drive
  Network ID:        substrate
  Secret seed:       0xb7bb3269a4a184529445a7fc91b03a87c17be3309cfa23adbcccb0f0fbc7f087
  Public key (hex):  0x12bf1c8e365c20cc2af606e3814b98b192857d85d182dac5e33fb90d0380ca75
  Account ID:        0x12bf1c8e365c20cc2af606e3814b98b192857d85d182dac5e33fb90d0380ca75
  Public key (SS58): 5CVHVSDm8643E1JWesWUjt7GZR6zMQa5oytg9TkBTMJ8T3cq
  SS58 Address:      5CVHVSDm8643E1JWesWUjt7GZR6zMQa5oytg9TkBTMJ8T3cq

Copy link
Member Author

Choose a reason for hiding this comment

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

so you can get the phrase and pass it in then on spin up the tss server will generate the keys from the seed and spit it out in the .production file under account_id

Copy link
Contributor

Choose a reason for hiding this comment

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

The auditors didn't like us deriving the encryption keys from signing keys, so now both signing keypair and encryption keys are derived from the mnemonic, using a different derivation process to that of subkey.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay maybe I need to follow up with that issue again 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

that is cool gonna merge in the meantime since it is blocking

@@ -75,15 +75,15 @@ pub mod tss_account_id {
pub static ref CHARLIE: sp_runtime::AccountId32 =
Copy link
Collaborator

Choose a reason for hiding this comment

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

How come Alice, Bob, and Charlie don't need to change?

Copy link
Member Author

Choose a reason for hiding this comment

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

they were done correctly by @ameba23 when he added hpke, I added these ones later for the four node images

/// Mnemonic: "impact federal dish number fun crisp various wedding radio immense whisper glue"
pub static ref EVE: sp_runtime::AccountId32 =
super::hex!["f2b4113735e988f662fe45e97b39770e804ebcd893ad0ab7cd8b7c5b5dcfff22"].into();
super::hex!["ac0d9030598f1722ff7c6a2a3043fa65903448dcc7a23011ec06c1c31cdad120"].into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar thing here

❮ ./target/release/entropy key inspect "impact federal dish number fun crisp various wedding radio immense whisper glue"
Secret phrase:       impact federal dish number fun crisp various wedding radio immense whisper glue
  Network ID:        substrate
  Secret seed:       0x0b22c9142215fbb5de12d6f372e3a0819887945209c98405787a68c10a9ed4fb
  Public key (hex):  0xf2b4113735e988f662fe45e97b39770e804ebcd893ad0ab7cd8b7c5b5dcfff22
  Account ID:        0xf2b4113735e988f662fe45e97b39770e804ebcd893ad0ab7cd8b7c5b5dcfff22
  Public key (SS58): 5HYvwGKUxxCLWnhhGXbonLdgQ3wTLopUVyMvjAAo4o2j3XQi
  SS58 Address:      5HYvwGKUxxCLWnhhGXbonLdgQ3wTLopUVyMvjAAo4o2j3XQi

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
@JesseAbram
Copy link
Member Author

to answer both comments, yes I got these keys from actually running the TSS server and grabbing the output of what is generated

@JesseAbram JesseAbram merged commit 1a6faf8 into master Aug 8, 2024
14 checks passed
@JesseAbram JesseAbram deleted the fix-tss-keys branch August 8, 2024 17:02
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants