chore: fix fork_choice test cases - #119
Conversation
SoarinSkySagar
changed the title
fork_choice test cases
| /// Maximum number of distinct AttestationData entries per block (spec: chain/config.py:36). | ||
| /// Used by the import-side validation so we accept any spec-valid block from other clients. | ||
| pub(crate) const MAX_ATTESTATIONS_DATA: usize = 8; | ||
| pub(crate) const MAX_ATTESTATIONS_DATA: usize = 16; |
There was a problem hiding this comment.
| pub(crate) const MAX_ATTESTATIONS_DATA: usize = 16; | |
| pub(crate) const MAX_ATTESTATIONS_DATA: usize = 8; |
There was a problem hiding this comment.
Please do not change this
There was a problem hiding this comment.
Agreed, what's the point of changing this? Looks like spec restricts attestation count to 8?
There was a problem hiding this comment.
3 test cases have blocks for testing with 16 distinct attestation data, and the tests are expecting them to be declared valid by the client. These tests were failing because MAX_ATTESTATIONS_DATA was 8 which is too low. Changing this to 16 makes those 3 cases pass (and no new failing cases)
There was a problem hiding this comment.
Looks like spec restricts attestation count to 8?
@ArtiomTr looks like there is a mismatch between the tests and the spec then. Which should we follow?
There was a problem hiding this comment.
which test cases? maybe those test cases are just outdated? do they still exist in latest leanSpec revision?
There was a problem hiding this comment.
There was a problem hiding this comment.
maybe those test cases are just outdated?
yes. I will update the tests instead of the const
|
converting this PR to draft for now. work on this can continue after #120 is merged |
3 participants
closes #116