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

Generate mnemonic internally in entropy-tss #1128

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ameba23
Copy link
Contributor

@ameba23 ameba23 commented Oct 22, 2024

Addresses #1015 by not allowing mnemonics to be passed in (except in development / testing) via the command line or an environment variable. Instead they are always randomly generated internally.

However i think this does not completely resolve the issue - see #1127

@ameba23
Copy link
Contributor Author

ameba23 commented Oct 22, 2024

Im pretty sure failing CI is unrelated to this PR - its the good ol' reshare test

Copy link
Member

@JesseAbram JesseAbram left a comment

Choose a reason for hiding this comment

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

Also tagging @vitropy cuz will have an effect on devops flow

@vitropy
Copy link
Contributor

vitropy commented Oct 22, 2024

Also tagging @vitropy cuz will have an effect on devops flow

@JesseAbram, please tag teams, not individuals: @entropyxyz/system-reliability-engineers, not me. Thanks!

Copy link
Contributor

@vitropy vitropy left a comment

Choose a reason for hiding this comment

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

If I understand correctly, this will not just "have an impact on devops flow," it will break the possibility of deterministically bringing up a specific TSS server with a prior knowledge of a given private key. This is not really feasible from a devops perpsective at all because we will not be able to replace a broken/failed/otherwise unusable TSS instance without replacing the TSS instance with a new key. If that key or account needs to be communicated to any other part of the system, we can not make use of this change, period.

If this understand is correct, then this will simply break automation, full stop. Please either correct my understanding or do not merge this.

@JesseAbram
Copy link
Member

If I understand correctly, this will not just "have an impact on devops flow," it will break the possibility of deterministically bringing up a specific TSS server with a prior knowledge of a given private key. This is not really feasible from a devops perpsective at all because we will not be able to replace a broken/failed/otherwise unusable TSS instance without replacing the TSS instance with a new key. If that key or account needs to be communicated to any other part of the system, we can not make use of this change, period.

If this understand is correct, then this will simply break automation, full stop. Please either correct my understanding or do not merge this.

yes it will but also using TDX would break this flow anyways, as well we can automate around this, by having the last step of a spin up process run a script to fund the TSS and inform the chain what the TSS is. Giving access to the kvdb or the TSS accounts outside of TDX removes the security of running TDX

@vitropy
Copy link
Contributor

vitropy commented Oct 22, 2024

yes it will but also using TDX would break this flow anyways, as well we can automate around this, by having the last step of a spin up process run a script to fund the TSS and inform the chain what the TSS is.

Okay, but that then sounds like it has nothing to do with devops because it's something the TSS will do for itself by itself and speak to the chain on its own behalf, in which case I have no objections.

@JesseAbram
Copy link
Member

yes it will but also using TDX would break this flow anyways, as well we can automate around this, by having the last step of a spin up process run a script to fund the TSS and inform the chain what the TSS is.

Okay, but that then sounds like it has nothing to do with devops because it's something the TSS will do for itself by itself and speak to the chain on its own behalf, in which case I have no objections.

mmmm will probably have to run a script and need access to a secure funded account but other than that ya

@ameba23
Copy link
Contributor Author

ameba23 commented Oct 23, 2024

So maybe this is not useful without the other part where the TSS informs the chain of its public keys. With things as they currently are this would need to happen before the validate extrinsic gets submitted by the node operator, as the node operator needs to know these public keys for that extrinsic.

The obvious place to do this would be the attestation pallet's request_attestation extrinsic, as this needs to anyway be called by the TSS on launch and before validate, as validate needs to include a quote. The problem is it doesn't give us the x25519 public key, and i think @HCastano would object to us putting it in there as he's quite keen on not mixing up the logic of the attestation pallet and staking extension pallet. Also, currently the node operator has no way of getting the quote from the TSS node which they will need for the validate extrinsic (which is a separate issue i guess).

So my proposal would be that the TS server has a GET route which returns its account ID and x25519 public keys as JSON. Not sure if we could just put this in the existing /healthz route or we need a new one. The operator can call this route, then fund the TSS account. Then we need yet another route where they tell the TSS that it has been funded and should request an attestation from the chain, get the nonce, and respond the the HTTP request with a quote that can be used in the validate extrinsic. This would all be much simpler if we had free transactions.

@entropyxyz/system-reliability-engineers - i feel like it is tricky to design TS launch/attestation process accommodating for both TSS nodes which are hard-coded at genesis as well as dynamically joining ones. Ideally i would like that we remove TS server details from the testnet chainspec and have them all join after launching the chain.

@ameba23
Copy link
Contributor Author

ameba23 commented Oct 23, 2024

I have added a GET route for retrieving the account ID and x25519 public key.

@ameba23
Copy link
Contributor Author

ameba23 commented Nov 5, 2024

@vitropy i am re-requesting a review as this is currently blocked by requested changes. I know this is going to break deployment scripts, but if the plan is to have the next release run on TDX hardware, then the deployment scripts are anyway going to need some changes.

@ameba23 ameba23 requested a review from vitropy November 5, 2024 10:45
@ameba23 ameba23 added this to the v0.4.0 milestone Nov 13, 2024
@ameba23 ameba23 self-assigned this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants