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

BlockProcessing testing #559

Merged
merged 70 commits into from Nov 12, 2019

Conversation

@pscott
Copy link
Contributor

pscott commented Oct 9, 2019

Issue Addressed

Closes #354 , closes #355 , closes #356 , closes #357 , closes #358 , closes #359

Additional Info

Decided to merge every branch I had created for each individual issues into one big PR, as originally proposed by #323 .

Removed all tests trying to insert MaxOperations + 1 per block, as they were really long, and added about 1-2 minutes of testing on my computer.

Proposed Changes

Deposits

  • Valid: Insert multiple valid deposits
  • Invalid: Insert block and increment deposit_count
  • Invalid: Insert block and decrement deposit_count
  • Invalid: Insert block with incorrect merkle proof
  • Invalid: Insert block with incorrect public key
  • Invalid: Insert block with incorrect block signature
  • Invalid: Insert block with invalid public key (i.e. not a valid BLS point)

Additional Info


Exits

  • Valid: Insert n deposits
  • Invalid: Validator Unknown
  • Invalid: Already Exited
  • Invalid: Already Initiated
  • Invalid: Future Epoch
  • Invalid: TooYoungToLeave
  • Invalid: Not Active
  • Invalid: BadSignature

Attestations

  • Valid: Insert Attestations
  • Invalid: BadParentCrossLinkStartEpoch
  • Invalid: BadParentCrossLinkEndEpoch
  • Invalid: NoCommitteeForShard
  • Invalid: BadParentCrossLinkDataRoot
  • Invalid: BadTargetTooLow
  • Invalid: BadTargetTooHigh
  • Invalid: WrongJustifiedCheckpoint
  • Invalid: BadIndexedAttestation(BadSignature)
  • Invalid: BadShard
  • Invalid: IncludedTooEarly
  • Invalid: IncludedTooLate (see additional info)
  • Invalid: BadTargetEpoch (see additional info)
  • Invalid: CustodyBitfieldHasSetBits
  • Invalid: BadCustodyBitfieldLength (see additional info)
  • Invalid: BadAggregationBitfieldLength (see additional info)
  • Invalid: CustodyBitfieldNotSubset
  • Invalid: BadSignature

Additional Info

  • BadCustodyBitfieldLength is not called in the code, I nevertheless added a test invalid_attestation_bad_custody_bitfield_len (the error returned is BeaconStateError::InvalidBitfield)
  • BadAggregationBitfieldLength is not called in the code, I nevertheless added a test invalid_attestation_bad_aggregation_bitfield_len (the error returned is BeaconStateError::InvalidBitfield)
  • I've removed the UnknownValidator variant in the enum as it is never returned.

-> Need help / ideas / reviews:

  • BadTargetEpoch is hard to reach : if target epoch is too low, we trigger BadTargetTooLow. If it is too high, we trigger NoCommitteeForShard, or EpochTooHigh.
  • IncludedTooLate triggers NoCommitteeForShard even with 2048 validators, because we are setting the target epoch to be earlier than the current epoch.

AttesterSlashings

  • Valid: Insert valid attester slashing
  • Invalid: NotSlashable
  • Invalid: IndexedAttestation1Invalid
  • Invalid: IndexedAttestation1Invalid

Additional Info

  • Removed AttestationDataIdentical enum variant as it was never called in the code.

ProposerSlashings

  • Valid: Insert proposer slashing
  • Invalid: ProposerUnknown
  • Invalid: ProposalsIdentical
  • Invalid: ProposalEpochMismatch
  • Invalid: ProposerNotSlashable
  • Invalid: BadProposal1Signature
  • Invalid: BadProposal2Signature
pscott added 30 commits Sep 22, 2019
pscott added 4 commits Oct 10, 2019
@paulhauner

This comment has been minimized.

Copy link
Member

paulhauner commented Oct 13, 2019

This looks like an awesome effort, thank you!

Could you please confirm this is ready for review? If so, I'll review mid-late next week (I'm still traveling sorry). I think @michaelsproul was keen to have a peek too.

@pscott

This comment has been minimized.

Copy link
Contributor Author

pscott commented Oct 13, 2019

@paulhauner Yup, everything should be good to go.

@paulhauner @michaelsproul Please let me know what needs to be changed / what I could've done better, as I'm still learning (and have a lot to learn from you guys!!). I'd also like to point out the BadTargetEpoch and IncludedTooLate tests (see original PR post). They're not getting properly tested.

@paulhauner

This comment has been minimized.

Copy link
Member

paulhauner commented Oct 28, 2019

I just triggered CI :)

@pscott

This comment has been minimized.

Copy link
Contributor Author

pscott commented Oct 29, 2019

Fixed cargo fmt as my rustfmt was not up to date when I ran cargo fmt before creating this PR.

Re: test failures -> the failing test for both test_dev and test_release are coming from signatures_minimal::deposit.
However here's what's for the test:

https://github.com/pscott/lighthouse/blob/8bc94297c2b244f47dd919605b8a47a5080bbf3b/eth2/state_processing/tests/tests.rs#L178-L191

There's an explicit TODO for fixing the test. Should I do anything about it? Not sure what to do here. Thanks!

@paulhauner

This comment has been minimized.

Copy link
Member

paulhauner commented Oct 30, 2019

There's an explicit TODO for fixing the test. Should I do anything about it? Not sure what to do here. Thanks!

Oh, so it looks like we expect that test to panic (#[should_panic]), but it's not panic-ing! So, I would guess that we have indirectly fixed that test.

I would remove the // TODO and the #[should_panic] and count this one as a win :)

It looks like there's another cargofmt fail in there too, sorry.

pscott added 3 commits Oct 31, 2019
…n insert_deposits
@pscott

This comment has been minimized.

Copy link
Contributor Author

pscott commented Oct 31, 2019

Updated rustfmt and ran it again.
Found out where the deposit test bug was coming from! Fixed it (it should indeed panic, but I had previously modified the insert_deposits method, it is now fixed).
Had to merge conflicts in cargo.toml, I kept everything on master and simply added the dependency to merkle_proof that was needed for generating proofs for the tests.

@paulhauner

This comment has been minimized.

Copy link
Member

paulhauner commented Nov 5, 2019

CI triggered :)

@paulhauner

This comment has been minimized.

Copy link
Member

paulhauner commented Nov 5, 2019

It failed CI, sorry :(

Seems to be related to deposits.

@pscott

This comment has been minimized.

Copy link
Contributor Author

pscott commented Nov 5, 2019

Argh! Adapted previous change to work with all test cases. Ran tests on my computer and cargo fmt, this push should be the last! :)

@paulhauner

This comment has been minimized.

Copy link
Member

paulhauner commented Nov 6, 2019

CI triggered :)

Copy link
Member

paulhauner left a comment

LGTM!

@paulhauner paulhauner merged commit c7b3a7a into sigp:master Nov 12, 2019
4 checks passed
4 checks passed
ci/gitlab/trigger-ci-for-559 Pipeline passed on GitLab
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.