-
Notifications
You must be signed in to change notification settings - Fork 996
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
PeerDAS sampling clarifications #3782
Conversation
- describe sample selection - describe sample queries Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Clarify that what matters is the false positive threshold, allowing different sampling strategies as protocol compliant behavior. Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
While the technique was introduced as LossyDAS, we don't need the name in the specification itself. Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
add LossyDAS sample count generation helper function Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Importing scipy is not preferred. This is a self-contained version. Eventually an import of math and use of math.comb makes it simpler. Solving other formatting issues as well. Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
types, type conversions, and tests and look good to me, thanks! |
Co-authored-by: Pop Chunhapanya <haxx.pop@gmail.com>
Here we assume uniform random selection without replacement. If other methods are used, the target false positive threshold is the main rule to follow. Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
There is no unique naming convention for these. We follow the naming of the direct replacement scipy.stats.hypergeom.cdf function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
|
||
```python | ||
def get_extended_sample_count(allowed_failures: uint64) -> uint64: | ||
assert 0 <= allowed_failures <= NUMBER_OF_COLUMNS // 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Never seen a function docstring below the asserts. Do we do this elsewhere in the specs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we did in phase0 process_attestation
:
consensus-specs/specs/phase0/beacon-chain.md
Line 1824 in 258c2c9
assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY <= state.slot <= data.slot + SLOTS_PER_EPOCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved it, although a bit late. Anyway, if needed, here it is 195ec21
This is to clarify sample selection and sample query based on previous discussions.
Note: I think some of SHOULD might be elevated to a MUST here. I was not doing it as the baseline used SHOULD, but I think some of this behaviour should be mandated, even if we can't enforce it.