From 4d463c4a855b4846eabcf7eac1aabc1ed6e82393 Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Mon, 17 Aug 2020 18:21:10 -0700 Subject: [PATCH] Don't ban blocks for context deadlines (#7040) * Don't ban blocks for context deadlines * Don't ban blocks for context deadlines * Add test * Merge branch 'master' into dont-ban-ctx-deadline2 * fmt --- beacon-chain/sync/pending_blocks_queue.go | 4 ++-- beacon-chain/sync/subscriber_beacon_blocks.go | 2 +- beacon-chain/sync/validate_aggregate_proof_test.go | 2 +- .../sync/validate_beacon_attestation_test.go | 2 +- beacon-chain/sync/validate_beacon_blocks.go | 13 ++++++++----- beacon-chain/sync/validate_beacon_blocks_test.go | 13 +++++++++++++ 6 files changed, 26 insertions(+), 10 deletions(-) diff --git a/beacon-chain/sync/pending_blocks_queue.go b/beacon-chain/sync/pending_blocks_queue.go index adab59c305..d36165c018 100644 --- a/beacon-chain/sync/pending_blocks_queue.go +++ b/beacon-chain/sync/pending_blocks_queue.go @@ -90,7 +90,7 @@ func (s *Service) processPendingBlocks(ctx context.Context) error { if parentIsBad || blockIsBad { // Set block as bad if its parent block is bad too. if parentIsBad { - s.setBadBlock(blkRoot) + s.setBadBlock(ctx, blkRoot) } // Remove block from queue. s.pendingQueueLock.Lock() @@ -142,7 +142,7 @@ func (s *Service) processPendingBlocks(ctx context.Context) error { if err := s.chain.ReceiveBlock(ctx, b, blkRoot); err != nil { log.Debugf("Could not process block from slot %d: %v", b.Block.Slot, err) - s.setBadBlock(blkRoot) + s.setBadBlock(ctx, blkRoot) traceutil.AnnotateError(span, err) } diff --git a/beacon-chain/sync/subscriber_beacon_blocks.go b/beacon-chain/sync/subscriber_beacon_blocks.go index d7e1442f28..04d4384d95 100644 --- a/beacon-chain/sync/subscriber_beacon_blocks.go +++ b/beacon-chain/sync/subscriber_beacon_blocks.go @@ -32,7 +32,7 @@ func (s *Service) beaconBlockSubscriber(ctx context.Context, msg proto.Message) if err := s.chain.ReceiveBlock(ctx, signed, root); err != nil { interop.WriteBlockToDisk(signed, true /*failed*/) - s.setBadBlock(root) + s.setBadBlock(ctx, root) return err } diff --git a/beacon-chain/sync/validate_aggregate_proof_test.go b/beacon-chain/sync/validate_aggregate_proof_test.go index a38355e33a..69aef1f085 100644 --- a/beacon-chain/sync/validate_aggregate_proof_test.go +++ b/beacon-chain/sync/validate_aggregate_proof_test.go @@ -586,7 +586,7 @@ func TestValidateAggregateAndProof_BadBlock(t *testing.T) { err = r.initCaches() require.NoError(t, err) // Set beacon block as bad. - r.setBadBlock(root) + r.setBadBlock(context.Background(), root) buf := new(bytes.Buffer) _, err = p.Encoding().EncodeGossip(buf, signedAggregateAndProof) require.NoError(t, err) diff --git a/beacon-chain/sync/validate_beacon_attestation_test.go b/beacon-chain/sync/validate_beacon_attestation_test.go index 6bd9c7299b..c973682b7b 100644 --- a/beacon-chain/sync/validate_beacon_attestation_test.go +++ b/beacon-chain/sync/validate_beacon_attestation_test.go @@ -52,7 +52,7 @@ func TestService_validateCommitteeIndexBeaconAttestation(t *testing.T) { require.NoError(t, err) invalidRoot := [32]byte{'A', 'B', 'C', 'D'} - s.setBadBlock(invalidRoot) + s.setBadBlock(ctx, invalidRoot) digest, err := s.forkDigest() require.NoError(t, err) diff --git a/beacon-chain/sync/validate_beacon_blocks.go b/beacon-chain/sync/validate_beacon_blocks.go index b3264ab594..69e6e3e601 100644 --- a/beacon-chain/sync/validate_beacon_blocks.go +++ b/beacon-chain/sync/validate_beacon_blocks.go @@ -81,7 +81,7 @@ func (s *Service) validateBeaconBlockPubSub(ctx context.Context, pid peer.ID, ms // Check if parent is a bad block and then reject the block. if s.hasBadBlock(bytesutil.ToBytes32(blk.Block.ParentRoot)) { log.Debugf("Received block with root %#x that has an invalid parent %#x", blockRoot, blk.Block.ParentRoot) - s.setBadBlock(blockRoot) + s.setBadBlock(ctx, blockRoot) return pubsub.ValidationReject } @@ -118,7 +118,7 @@ func (s *Service) validateBeaconBlockPubSub(ctx context.Context, pid peer.ID, ms if err := s.chain.VerifyBlkDescendant(ctx, bytesutil.ToBytes32(blk.Block.ParentRoot)); err != nil { log.WithError(err).Warn("Rejecting block") - s.setBadBlock(blockRoot) + s.setBadBlock(ctx, blockRoot) return pubsub.ValidationReject } @@ -136,7 +136,7 @@ func (s *Service) validateBeaconBlockPubSub(ctx context.Context, pid peer.ID, ms if err := blocks.VerifyBlockSignature(parentState, blk); err != nil { log.WithError(err).WithField("blockSlot", blk.Block.Slot).Warn("Could not verify block signature") - s.setBadBlock(blockRoot) + s.setBadBlock(ctx, blockRoot) return pubsub.ValidationReject } @@ -152,7 +152,7 @@ func (s *Service) validateBeaconBlockPubSub(ctx context.Context, pid peer.ID, ms } if blk.Block.ProposerIndex != idx { log.WithError(err).WithField("blockSlot", blk.Block.Slot).Warn("Incorrect proposer index") - s.setBadBlock(blockRoot) + s.setBadBlock(ctx, blockRoot) return pubsub.ValidationReject } @@ -186,9 +186,12 @@ func (s *Service) hasBadBlock(root [32]byte) bool { } // Set bad block in the cache. -func (s *Service) setBadBlock(root [32]byte) { +func (s *Service) setBadBlock(ctx context.Context, root [32]byte) { s.badBlockLock.Lock() defer s.badBlockLock.Unlock() + if ctx.Err() != nil { // Do not mark block as bad if it was due to context error. + return + } s.badBlockCache.Add(string(root[:]), true) } diff --git a/beacon-chain/sync/validate_beacon_blocks_test.go b/beacon-chain/sync/validate_beacon_blocks_test.go index ab2cc60f2f..6b353e77be 100644 --- a/beacon-chain/sync/validate_beacon_blocks_test.go +++ b/beacon-chain/sync/validate_beacon_blocks_test.go @@ -712,3 +712,16 @@ func TestValidateBeaconBlockPubSub_InvalidParentBlock(t *testing.T) { result = r.validateBeaconBlockPubSub(ctx, "", m) == pubsub.ValidationAccept assert.Equal(t, false, result) } + +func TestService_setBadBlock_DoesntSetWithContextErr(t *testing.T) { + s := Service{} + require.NoError(t, s.initCaches()) + + root := [32]byte{'b', 'a', 'd'} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + s.setBadBlock(ctx, root) + if s.hasBadBlock(root) { + t.Error("Set bad root with cancelled context") + } +}