Attestation Justified Slot Validation Per Spec (#516)

* bringing isAttestationValid up to spec and udpating tests

* Seems like a bug/typo returning Slot inside Attestation.JustifiedSlotNumber(), fixed now

* typos per @terenc3t

* change errors and messages

* using positive tests vs negative if statements to test for spec rules

* fix error typo

* remove comment

beacon-chain/types/attestation.go

@@ -94,7 +94,7 @@ func (a *Attestation) ShardBlockHash() []byte {
 
 // JustifiedSlotNumber of the attestation should be earlier than the last justified slot in crystallized state.
 func (a *Attestation) JustifiedSlotNumber() uint64 {
-	return a.data.Slot
+	return a.data.JustifiedSlot
 }
 
 // JustifiedBlockHash should be in the chain of the block being processed.

beacon-chain/types/block.go

@@ -27,6 +27,10 @@ type Block struct {
 	data *pb.BeaconBlock
 }
 
+type chainSearchService interface {
+	ContainsBlock(h [32]byte) (bool, error)
+}
+
 // NewBlock explicitly sets the data field of a block.
 // Return block with default fields if data is nil.
 func NewBlock(data *pb.BeaconBlock) *Block {
@@ -147,7 +151,10 @@ func (b *Block) isSlotValid() bool {
 // 2.) Ensure pow_chain_ref processed.
 // 3.) Ensure local time is large enough to process this block's slot.
 // 4.) Verify that the parent block's proposer's attestation is included.
-func (b *Block) IsValid(aState *ActiveState, cState *CrystallizedState, parentSlot uint64) bool {
+// IsValid is called to decide if an incoming p2p block can be processed.
+// It checks the slot against the system clock, and the validity of the included attestations.
+// Existence of the parent block and the PoW chain block is checked outside of this function because they require additional dependencies.
+func (b *Block) IsValid(chain chainSearchService, aState *ActiveState, cState *CrystallizedState, parentSlot uint64) bool {
 	_, err := b.Hash()
 	if err != nil {
 		log.Errorf("Could not hash incoming block: %v", err)
@@ -179,8 +186,8 @@ func (b *Block) IsValid(aState *ActiveState, cState *CrystallizedState, parentSl
 	}
 
 	for index, attestation := range b.Attestations() {
-		if !b.isAttestationValid(index, aState, cState, parentSlot) {
-			log.Errorf("Attestation invalid: %v", attestation)
+		if !b.isAttestationValid(index, chain, aState, cState, parentSlot) {
+			log.Debugf("attestation invalid: %v", attestation)
 			return false
 		}
 	}
@@ -191,7 +198,7 @@ func (b *Block) IsValid(aState *ActiveState, cState *CrystallizedState, parentSl
 // isAttestationValid validates an attestation in a block.
 // Attestations are cross-checked against validators in CrystallizedState.ShardAndCommitteesForSlots.
 // In addition, the signature is verified by constructing the list of parent hashes using ActiveState.RecentBlockHashes.
-func (b *Block) isAttestationValid(attestationIndex int, aState *ActiveState, cState *CrystallizedState, parentSlot uint64) bool {
+func (b *Block) isAttestationValid(attestationIndex int, chain chainSearchService, aState *ActiveState, cState *CrystallizedState, parentSlot uint64) bool {
 	// Validate attestation's slot number has is within range of incoming block number.
 	attestation := b.Attestations()[attestationIndex]
 
@@ -200,13 +207,25 @@ func (b *Block) isAttestationValid(attestationIndex int, aState *ActiveState, cS
 	}
 
 	if attestation.JustifiedSlot > cState.LastJustifiedSlot() {
-		log.Debugf("attestation's last justified slot has to match crystallied state's last justified slot. Found: %d. Want: %d",
+		log.Debugf("attestation's justified slot has to be earlier or equal to crystallized state's last justified slot. Found: %d. Want <=: %d",
 			attestation.JustifiedSlot,
 			cState.LastJustifiedSlot())
 		return false
 	}
 
-	// TODO(#468): Validate last justified block hash matches in the crystallizedState.
+	hash := [32]byte{}
+	copy(attestation.JustifiedBlockHash[:], hash[:32])
+	blockInChain, err := chain.ContainsBlock(hash)
+	if err != nil {
+		log.Errorf("unable to determine if attestation justified block is in the DB: %s", err)
+		return false
+	}
+
+	if !blockInChain {
+		log.Debugf("The attestion's justifed block hash has to be in the current chain, but was not found.  Justified block hash: %v",
+			attestation.JustifiedBlockHash)
+		return false
+	}
 
 	// Get all the block hashes up to cycle length.
 	parentHashes := aState.getSignedParentHashes(b, attestation)

beacon-chain/types/block_test.go

@@ -12,6 +12,12 @@ func init() {
 	logrus.SetLevel(logrus.DebugLevel)
 }
 
+type mockChainService struct{}
+
+func (f *mockChainService) ContainsBlock(h [32]byte) (bool, error) {
+	return true, nil
+}
+
 func TestGenesisBlock(t *testing.T) {
 	b1, err1 := NewGenesisBlock()
 	b2, err2 := NewGenesisBlock()
@@ -62,6 +68,7 @@ func TestGenesisBlock(t *testing.T) {
 
 func TestBlockValidity(t *testing.T) {
 	cState, err := NewGenesisCrystallizedState()
+
 	if err != nil {
 		t.Fatalf("failed to generate crystallized state: %v", err)
 	}
@@ -87,11 +94,13 @@ func TestBlockValidity(t *testing.T) {
 	})
 
 	parentSlot := uint64(1)
-	if !b.isAttestationValid(0, aState, cState, parentSlot) {
+	chainService := &mockChainService{}
+
+	if !b.isAttestationValid(0, chainService, aState, cState, parentSlot) {
 		t.Fatalf("failed attestation validation")
 	}
 
-	if !b.IsValid(aState, cState, parentSlot) {
+	if !b.IsValid(chainService, aState, cState, parentSlot) {
 		t.Fatalf("failed block validation")
 	}
 }