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

proactive refresh #413

Merged
merged 10 commits into from
Oct 9, 2023
Merged

proactive refresh #413

merged 10 commits into from
Oct 9, 2023

Conversation

JesseAbram
Copy link
Member

@JesseAbram JesseAbram commented Sep 27, 2023

#412

To do in subsequent PRs

  • validate endpoint
  • refactor common functions that use synedrion
  • block and partition portions of the network at a time

@vercel
Copy link

vercel bot commented Sep 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
entropy-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 6, 2023 5:37pm

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.

😎 love this

get_signer(&app_state.kv_store).await.map_err(|e| ProtocolErr::UserError(e.to_string()))?;
check_in_registration_group(&validators_info, signer.account_id())
.map_err(|e| ProtocolErr::UserError(e.to_string()))?;
// TODO: validate this endpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

is this TODO already done with the line above? Or do you mean check that this http request actually came from the chain?

Copy link
Member Author

Choose a reason for hiding this comment

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

the latter, I created a todo card on our board.....will pick it up when things calm down a bit I just wanted to leave a todo in the code too

for key in all_keys {
let sig_request_address = AccountId32::from_str(&key).map_err(ProtocolErr::StringError)?;

// TODO: check key visibility don't do private (requires user to be online)
Copy link
Contributor

Choose a reason for hiding this comment

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

To do this, get_all_keys could also return key visibility - or we make a similar function get_non_private_keys

Copy link
Member Author

Choose a reason for hiding this comment

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

oh ya, that would be a good idea......would be fun to I wrote this function a while ago but if I recall correctly it reads the raw state of the chain's db and may need to be decoded

let sig_request_address = AccountId32::from_str(&key).map_err(ProtocolErr::StringError)?;

// TODO: check key visibility don't do private (requires user to be online)
// key should always exist, figure out how to handle
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea how to handle this, but if we return an error if exists_result is false, it will at least show up in the chain logs as getting an unexpected status code.

Copy link
Member Author

Choose a reason for hiding this comment

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

mmmmm Im not sure I care about that, it is a nice to fix but doesn't....like im down to put it as a back burner todo

let deserialized_old_key: KeyShare<KeyParams> = deserialize(&old_key_share)
.ok_or_else(|| ProtocolErr::Deserialization("Failed to load KeyShare".into()))?;

let new_key_share = do_proactive_refresh(
Copy link
Contributor

Choose a reason for hiding this comment

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

as this can take a long time, it might be worth spawning a task to run this and already responding, to avoid problems with the two second deadline set in the propagation pallet.

although if there haven't been any problems with this in tests maybe we don't need to worry.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, there hasn't and Im down to put it in its own thread, once we decide if and how we tell the chain later if things are done (as the threading will effect that). Also even if the deadline is met wouldn't the process still continue and the server end until completion

@JesseAbram JesseAbram merged commit 48259be into master Oct 9, 2023
10 checks passed
@JesseAbram JesseAbram deleted the proactive-refresh branch October 9, 2023 14:35
ameba23 added a commit that referenced this pull request Oct 18, 2023
* master:
  Write a Dockerfile that can build both `entropy` and `server`. (#430)
  Workflow for automated GitHub Release drafting/publishing. (#429)
  Update `.editorconfig` to match `.rustfmt.toml` settings (#427)
  Place `demo_offence` dispatchable behind root origin check (#426)
  Ensure correct validator order by using ValidatorInfo from chain rather than from user (#425)
  changed remaining references of --ws-external to --rpc-external
  Update README.md
  proactive refresh (#413)
@JesseAbram JesseAbram mentioned this pull request Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants