espresso-sequencer
espresso-sequencer copied to clipboard
fix: correct threshold for light client state update
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_contractoption.
This PR does not:
Key places to review:
Is it feasible to add a regression test for this?
Is it feasible to add a regression test for this?
Any suggestion for this? I don't know where to put.
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?
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.
I agree with @sveitser, single responsibility principle when possible.
Not sure. It's kind of a "magic", it's not that obvious that the correct computation is
x / 3 + 1to 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_thresholdmethod to the stake table instead, or do you think it doesn't belong there?
Good idea! Just implemented this.