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: correct threshold for light client state update #1461

Merged
merged 7 commits into from
May 16, 2024
Merged

Conversation

mrain
Copy link
Contributor

@mrain mrain commented May 15, 2024

No issue related.

This PR:

  • Let n be the total stake. We only need n/3+1 signatures (at least 1 signature from the honest party) to update the light client contract.
  • Correct the comment/documentation for use_mock_contract option.

This PR does not:

Key places to review:

@mrain mrain marked this pull request as ready for review May 15, 2024 06:21
@mrain mrain requested a review from alxiong May 15, 2024 06:25
@sveitser
Copy link
Collaborator

Is it feasible to add a regression test for this?

@mrain
Copy link
Contributor Author

mrain commented May 15, 2024

Is it feasible to add a regression test for this?

Any suggestion for this? I don't know where to put.

@sveitser
Copy link
Collaborator

Not sure. It's kind of a "magic", it's not that obvious that the correct computation is x / 3 + 1 to the point where we even implemented it wrongly at first. So it would be better if this was computed only in one place instead of four places (or maybe two places, with the second one being a test to verify that the first computation is correct, so that if someone changed the computation it would break that test).

Would it make sense to add a quorum_threshold method to the stake table instead, or do you think it doesn't belong there?

@sveitser
Copy link
Collaborator

Also to be clear, I don't intend to delay merging of this PR. It's just a suggestion to reduce tech-debt and future problems.

@alxiong
Copy link
Contributor

alxiong commented May 16, 2024

I agree with @sveitser, single responsibility principle when possible.

@mrain
Copy link
Contributor Author

mrain commented May 16, 2024

Not sure. It's kind of a "magic", it's not that obvious that the correct computation is x / 3 + 1 to the point where we even implemented it wrongly at first. So it would be better if this was computed only in one place instead of four places (or maybe two places, with the second one being a test to verify that the first computation is correct, so that if someone changed the computation it would break that test).

Would it make sense to add a quorum_threshold method to the stake table instead, or do you think it doesn't belong there?

Good idea! Just implemented this.

@@ -79,6 +79,12 @@ pub struct StateProverConfig {
pub stake_table_capacity: usize,
}

#[inline]
/// A helper function to compute the quorum threshold given a total amount of stake.
pub fn compute_quorum_threshold(total_stake: U256) -> U256 {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should call this one_honest_threshold, not quorum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right: 0a6b8b1

@mrain mrain merged commit 0ab566c into main May 16, 2024
14 checks passed
@mrain mrain deleted the cl/lc-contract branch May 16, 2024 13:30
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.

4 participants