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

Jumpstart network #918

Merged
merged 27 commits into from
Jul 11, 2024
Merged

Jumpstart network #918

merged 27 commits into from
Jul 11, 2024

Conversation

JesseAbram
Copy link
Member

Jumpstarts the network with a global key

  • Uses same pathways but hardsets global key to 0x000 address

@JesseAbram JesseAbram marked this pull request as ready for review July 2, 2024 19:52
@JesseAbram JesseAbram requested a review from HCastano July 2, 2024 19:52
@JesseAbram JesseAbram self-assigned this Jul 2, 2024
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.

There's a couple more things I need to look at, in particular the use of the zero-address around the different calls

pallets/registry/src/lib.rs Outdated Show resolved Hide resolved
})]
pub fn jump_start_network(origin: OriginFor<T>) -> DispatchResult {
let _who = ensure_signed(origin)?;
let network_account = H256::zero();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to avoid the zero address here because it has a well-known private key.

I say may instead of must since I don't have a full idea of where this address is actually "used", or if it's more of just a placeholder.

Copy link
Member Author

Choose a reason for hiding this comment

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

should be easily fixed with a guard on register, good find

pallets/registry/src/lib.rs Outdated Show resolved Hide resolved
pallets/registry/src/lib.rs Outdated Show resolved Hide resolved
pallets/registry/src/lib.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved
crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved
crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved
crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved
Co-authored-by: Hernando Castano <[email protected]>
@JesseAbram JesseAbram added the Bump `transaction_version` A change which affects how existing extrinsics are created (e.g parameter changes) label Jul 9, 2024
@JesseAbram JesseAbram requested a review from HCastano July 9, 2024 22:03
@ameba23
Copy link
Contributor

ameba23 commented Jul 10, 2024

Ive had a quick look at this and it looks great. Since it is a step towards having a network parent key but not fully implemented, Im trying to figure out what exactly the scope of this PR is.

This is what i am getting, please correct if wrong:

  • This adds a 'parent' DKG for the network - but does not yet remove per-user DKG.
  • The registry pallet tracks whether a network parent key exists (Done), does not exist (Ready) or DKG is in progress (InProgress), but there are not yet restrictions on users signing or registering when the network parent key does not exist.
  • The network DKG is activated by submitting a jump_start_network extrinsic - but there are currently no restrictions as to who can submit this or how big the network should be (Jump Start network requirements #923).
  • The network can only be jump started if it is either not yet jump started, or jump starting has been in-progress for at least 50 blocks.
  • The network is considered successfully jump started on getting register confirmations from all but one of the TSS nodes who participated in the network DKG (i am not totally clear on this, and i feel like it should be 100%).
  • Signature requests for the parent key account will result in an error.

@JesseAbram @HCastano

@JesseAbram
Copy link
Member Author

Ive had a quick look at this and it looks great. Since it is a step towards having a network parent key but not fully implemented, Im trying to figure out what exactly the scope of this PR is.

This is what i am getting, please correct if wrong:

  • This adds a 'parent' DKG for the network - but does not yet remove per-user DKG.

yes

  • The registry pallet tracks whether a network parent key exists (Done), does not exist (Ready) or DKG is in progress (InProgress), but there are not yet restrictions on users signing or registering when the network parent key does not exist.

also yes because we don't use it yet.....and as we move the registery flow to the new flow this, is where having a block before jumpstart should be addressed

  • The network DKG is activated by submitting a jump_start_network extrinsic - but there are currently no restrictions as to who can submit this or how big the network should be (Jump Start network requirements #923).
  • The network can only be jump started if it is either not yet jump started, or jump starting has been in-progress for at least 50 blocks.
  • The network is considered successfully jump started on getting register confirmations from all but one of the TSS nodes who participated in the network DKG (i am not totally clear on this, and i feel like it should be 100%).

No it is all of them...... If you are referring to this line https://github.com/entropyxyz/entropy-core/pull/918/files#diff-1e0acd338c23affee5840b8efb06bd46be87c49819841bdcf3ce804631a40b92R340 it is because the last confirmation sets it to done as if there were 4 out of 5 done, this one is the 5th

  • Signature requests for the parent key account will result in an error.

@JesseAbram @HCastano

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.

Getting close!

Couple things:

  • It's unclear to me why we need to substitute the parent verifying key in the KVDB with 0x00...
  • I'm still slightly worried about the use of the zero address here, but we could kick this down the road to at least get things moving (maybe we open an issue for right now)
  • We need to rename comments, errors, etc. to parent

crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved
crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved
crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved
crates/threshold-signature-server/src/user/errors.rs Outdated Show resolved Hide resolved
pallets/registry/src/lib.rs Outdated Show resolved Hide resolved
pallets/registry/src/lib.rs Outdated Show resolved Hide resolved
crates/threshold-signature-server/src/user/tests.rs Outdated Show resolved Hide resolved
Comment on lines +382 to +385
ensure!(
encoded_sig_req_account != NETWORK_PARENT_KEY.encode(),
Error::<T>::NoRegisteringFromParentKey
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be useful to have. E.g we can have the Entropy network control an account on other chains like this.

Obvs this wouldn't work if the parent key address is the zero-address though.

crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved
crates/threshold-signature-server/src/user/api.rs Outdated Show resolved Hide resolved
Co-authored-by: Hernando Castano <[email protected]>
@JesseAbram
Copy link
Member Author

Getting close!

Couple things:

  • It's unclear to me why we need to substitute the parent verifying key in the KVDB with 0x00...

In theory we don't and we can change it, but when we keep it a set amount, we can and it is easier to make it a global constant and have if parent key do x code easier (which we can also add extra security on it too)

  • I'm still slightly worried about the use of the zero address here, but we could kick this down the road to at least get things moving (maybe we open an issue for right now)

Global constant, I know your impulse, I think it is fine we can make it any random key tbh

  • We need to rename comments, errors, etc. to parent

@JesseAbram JesseAbram requested a review from HCastano July 10, 2024 17:12
@HCastano HCastano added this to the t-of-N Redesign milestone Jul 11, 2024
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.

Few things we need to follow up on, but looks good otherwise!

crates/threshold-signature-server/src/user/errors.rs Outdated Show resolved Hide resolved
data.block_number,
)
.await?;
let verifying_key = key_share.verifying_key().to_encoded_point(true).as_bytes().to_vec();
let string_verifying_key = hex::encode(verifying_key.clone()).to_string();
let string_verifying_key = if sig_request_account == NETWORK_PARENT_KEY.encode() {
hex::encode(*NETWORK_PARENT_KEY)
Copy link
Collaborator

Choose a reason for hiding this comment

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

One suggestion: instead of overwriting the parent's verifying key to be 0x00 we could
have a separate "forbidden key" entry, PARENT_VERIFYING_KEY, which points to the parent
verifying key.

For those checks you mention, we could achieve them the same way, but doing an extra
lookup.

That way we keep the verifying key entries consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure at this point tho im in favour of opening an issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah an issue is fine

Copy link
Member Author

Choose a reason for hiding this comment

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

CHANGELOG.md Outdated Show resolved Hide resolved
@JesseAbram JesseAbram mentioned this pull request Jul 11, 2024
@JesseAbram JesseAbram merged commit 314849f into master Jul 11, 2024
13 checks passed
@JesseAbram JesseAbram deleted the jumpstart-network branch July 11, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bump `transaction_version` A change which affects how existing extrinsics are created (e.g parameter changes)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants