Skip to content

Commit

Permalink
Remove remaining instances of proto.clone() (prysmaticlabs#4806)
Browse files Browse the repository at this point in the history
* prysm-4757 remove proto.Clone() in favor of existing getters.Copy* methods
* prysm-4757 added a bunch of copy methods, and broke some tests
* squash commits
 fix tests and getter implementations
 remove usage of CopySignedBeaconBlock from ReceiveBlockNoVerify
* correctly copy Deposit proof and remove proto.clone() again
* Merge branch 'master' into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
* Fix for comments, inline possible function calls
* Merge branch 'master' of https://github.com/prysmaticlabs/Prysm into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
* updated with feedback from review
* Merge branch 'master' into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
* Merge branch 'master' into prysm-4757-no-proto-clone
  • Loading branch information
garyschulteog authored and cryptomental committed Feb 28, 2020
1 parent b1b7fec commit 59a174d
Show file tree
Hide file tree
Showing 17 changed files with 328 additions and 119 deletions.
5 changes: 2 additions & 3 deletions beacon-chain/blockchain/head.go
Expand Up @@ -3,10 +3,9 @@ package blockchain
import (
"context"

"github.com/gogo/protobuf/proto"
"github.com/pkg/errors"
ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
"github.com/prysmaticlabs/prysm/beacon-chain/state"
stateTrie "github.com/prysmaticlabs/prysm/beacon-chain/state"
"github.com/prysmaticlabs/prysm/shared/bytesutil"
"github.com/prysmaticlabs/prysm/shared/featureconfig"
"go.opencensus.io/trace"
Expand Down Expand Up @@ -100,7 +99,7 @@ func (s *Service) saveHead(ctx context.Context, headRoot [32]byte) error {
// Cache the new head info.
s.headSlot = newHead.Block.Slot
s.canonicalRoots[newHead.Block.Slot] = headRoot[:]
s.headBlock = proto.Clone(newHead).(*ethpb.SignedBeaconBlock)
s.headBlock = stateTrie.CopySignedBeaconBlock(newHead)
s.headState = headState

// Save the new head root to DB.
Expand Down
4 changes: 2 additions & 2 deletions beacon-chain/blockchain/process_attestation.go
Expand Up @@ -5,11 +5,11 @@ import (
"fmt"
"time"

"github.com/gogo/protobuf/proto"
"github.com/pkg/errors"
ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
"github.com/prysmaticlabs/prysm/beacon-chain/flags"
stateTrie "github.com/prysmaticlabs/prysm/beacon-chain/state"
"github.com/prysmaticlabs/prysm/shared/bytesutil"
"github.com/prysmaticlabs/prysm/shared/featureconfig"
"go.opencensus.io/trace"
Expand Down Expand Up @@ -72,7 +72,7 @@ func (s *Service) onAttestation(ctx context.Context, a *ethpb.Attestation) ([]ui
ctx, span := trace.StartSpan(ctx, "blockchain.onAttestation")
defer span.End()

tgt := proto.Clone(a.Data.Target).(*ethpb.Checkpoint)
tgt := stateTrie.CopyCheckpoint(a.Data.Target)
tgtSlot := helpers.StartSlot(tgt.Epoch)

if helpers.SlotToEpoch(a.Data.Slot) != a.Data.Target.Epoch {
Expand Down
8 changes: 4 additions & 4 deletions beacon-chain/blockchain/receive_block.go
Expand Up @@ -5,7 +5,6 @@ import (
"context"
"encoding/hex"

"github.com/gogo/protobuf/proto"
"github.com/pkg/errors"
ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
"github.com/prysmaticlabs/go-ssz"
Expand All @@ -14,6 +13,7 @@ import (
"github.com/prysmaticlabs/prysm/beacon-chain/core/feed"
statefeed "github.com/prysmaticlabs/prysm/beacon-chain/core/feed/state"
"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
stateTrie "github.com/prysmaticlabs/prysm/beacon-chain/state"
"github.com/prysmaticlabs/prysm/shared/featureconfig"
"github.com/prysmaticlabs/prysm/shared/traceutil"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -66,7 +66,7 @@ func (s *Service) ReceiveBlock(ctx context.Context, block *ethpb.SignedBeaconBlo
func (s *Service) ReceiveBlockNoPubsub(ctx context.Context, block *ethpb.SignedBeaconBlock) error {
ctx, span := trace.StartSpan(ctx, "beacon-chain.blockchain.ReceiveBlockNoPubsub")
defer span.End()
blockCopy := proto.Clone(block).(*ethpb.SignedBeaconBlock)
blockCopy := stateTrie.CopySignedBeaconBlock(block)

// Apply state transition on the new block.
postState, err := s.onBlock(ctx, blockCopy)
Expand Down Expand Up @@ -130,7 +130,7 @@ func (s *Service) ReceiveBlockNoPubsub(ctx context.Context, block *ethpb.SignedB
func (s *Service) ReceiveBlockNoPubsubForkchoice(ctx context.Context, block *ethpb.SignedBeaconBlock) error {
ctx, span := trace.StartSpan(ctx, "beacon-chain.blockchain.ReceiveBlockNoForkchoice")
defer span.End()
blockCopy := proto.Clone(block).(*ethpb.SignedBeaconBlock)
blockCopy := stateTrie.CopySignedBeaconBlock(block)

// Apply state transition on the new block.
_, err := s.onBlock(ctx, blockCopy)
Expand Down Expand Up @@ -183,7 +183,7 @@ func (s *Service) ReceiveBlockNoPubsubForkchoice(ctx context.Context, block *eth
func (s *Service) ReceiveBlockNoVerify(ctx context.Context, block *ethpb.SignedBeaconBlock) error {
ctx, span := trace.StartSpan(ctx, "beacon-chain.blockchain.ReceiveBlockNoVerify")
defer span.End()
blockCopy := proto.Clone(block).(*ethpb.SignedBeaconBlock)
blockCopy := stateTrie.CopySignedBeaconBlock(block)

// Apply state transition on the incoming newly received blockCopy without verifying its BLS contents.
if err := s.onBlockInitialSyncStateTransition(ctx, blockCopy); err != nil {
Expand Down
19 changes: 9 additions & 10 deletions beacon-chain/blockchain/service.go
Expand Up @@ -10,7 +10,6 @@ import (
"sync"
"time"

"github.com/gogo/protobuf/proto"
"github.com/pkg/errors"
ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
"github.com/prysmaticlabs/go-ssz"
Expand Down Expand Up @@ -150,10 +149,10 @@ func (s *Service) Start() {
}

// Resume fork choice.
s.justifiedCheckpt = proto.Clone(justifiedCheckpoint).(*ethpb.Checkpoint)
s.bestJustifiedCheckpt = proto.Clone(justifiedCheckpoint).(*ethpb.Checkpoint)
s.finalizedCheckpt = proto.Clone(finalizedCheckpoint).(*ethpb.Checkpoint)
s.prevFinalizedCheckpt = proto.Clone(finalizedCheckpoint).(*ethpb.Checkpoint)
s.justifiedCheckpt = stateTrie.CopyCheckpoint(justifiedCheckpoint)
s.bestJustifiedCheckpt = stateTrie.CopyCheckpoint(justifiedCheckpoint)
s.finalizedCheckpt = stateTrie.CopyCheckpoint(finalizedCheckpoint)
s.prevFinalizedCheckpt = stateTrie.CopyCheckpoint(finalizedCheckpoint)
s.resumeForkChoice(justifiedCheckpoint, finalizedCheckpoint)

if finalizedCheckpoint.Epoch > 1 {
Expand Down Expand Up @@ -290,7 +289,7 @@ func (s *Service) saveHeadNoDB(ctx context.Context, b *ethpb.SignedBeaconBlock,

s.canonicalRoots[b.Block.Slot] = r[:]

s.headBlock = proto.Clone(b).(*ethpb.SignedBeaconBlock)
s.headBlock = stateTrie.CopySignedBeaconBlock(b)

headState, err := s.beaconDB.State(ctx, r)
if err != nil {
Expand Down Expand Up @@ -347,10 +346,10 @@ func (s *Service) saveGenesisData(ctx context.Context, genesisState *stateTrie.B
genesisCheckpoint := &ethpb.Checkpoint{Root: genesisBlkRoot[:]}

// Add the genesis block to the fork choice store.
s.justifiedCheckpt = proto.Clone(genesisCheckpoint).(*ethpb.Checkpoint)
s.bestJustifiedCheckpt = proto.Clone(genesisCheckpoint).(*ethpb.Checkpoint)
s.finalizedCheckpt = proto.Clone(genesisCheckpoint).(*ethpb.Checkpoint)
s.prevFinalizedCheckpt = proto.Clone(genesisCheckpoint).(*ethpb.Checkpoint)
s.justifiedCheckpt = stateTrie.CopyCheckpoint(genesisCheckpoint)
s.bestJustifiedCheckpt = stateTrie.CopyCheckpoint(genesisCheckpoint)
s.finalizedCheckpt = stateTrie.CopyCheckpoint(genesisCheckpoint)
s.prevFinalizedCheckpt = stateTrie.CopyCheckpoint(genesisCheckpoint)

if err := s.forkChoiceStore.ProcessBlock(ctx,
genesisBlk.Block.Slot,
Expand Down
1 change: 0 additions & 1 deletion beacon-chain/cache/BUILD.bazel
Expand Up @@ -18,7 +18,6 @@ go_library(
"//shared/hashutil:go_default_library",
"//shared/params:go_default_library",
"//shared/sliceutil:go_default_library",
"@com_github_gogo_protobuf//proto:go_default_library",
"@com_github_hashicorp_golang_lru//:go_default_library",
"@com_github_prometheus_client_golang//prometheus:go_default_library",
"@com_github_prometheus_client_golang//prometheus/promauto:go_default_library",
Expand Down
3 changes: 1 addition & 2 deletions beacon-chain/cache/checkpoint_state.go
Expand Up @@ -4,7 +4,6 @@ import (
"errors"
"sync"

"github.com/gogo/protobuf/proto"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
Expand Down Expand Up @@ -103,7 +102,7 @@ func (c *CheckpointStateCache) AddCheckpointState(cp *CheckpointState) error {
c.lock.Lock()
defer c.lock.Unlock()
if err := c.cache.AddIfNotPresent(&CheckpointState{
Checkpoint: proto.Clone(cp.Checkpoint).(*ethpb.Checkpoint),
Checkpoint: stateTrie.CopyCheckpoint(cp.Checkpoint),
State: cp.State.Copy(),
}); err != nil {
return err
Expand Down
1 change: 0 additions & 1 deletion beacon-chain/core/helpers/BUILD.bazel
Expand Up @@ -31,7 +31,6 @@ go_library(
"//shared/params:go_default_library",
"//shared/roughtime:go_default_library",
"//shared/sliceutil:go_default_library",
"@com_github_gogo_protobuf//proto:go_default_library",
"@com_github_pkg_errors//:go_default_library",
"@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library",
"@com_github_prysmaticlabs_go_bitfield//:go_default_library",
Expand Down
5 changes: 2 additions & 3 deletions beacon-chain/core/helpers/attestation.go
Expand Up @@ -3,7 +3,6 @@ package helpers
import (
"encoding/binary"

"github.com/gogo/protobuf/proto"
"github.com/pkg/errors"
ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
"github.com/prysmaticlabs/go-ssz"
Expand Down Expand Up @@ -79,8 +78,8 @@ func AggregateAttestation(a1 *ethpb.Attestation, a2 *ethpb.Attestation) (*ethpb.
return nil, ErrAttestationAggregationBitsOverlap
}

baseAtt := proto.Clone(a1).(*ethpb.Attestation)
newAtt := proto.Clone(a2).(*ethpb.Attestation)
baseAtt := stateTrie.CopyAttestation(a1)
newAtt := stateTrie.CopyAttestation(a2)
if newAtt.AggregationBits.Count() > baseAtt.AggregationBits.Count() {
baseAtt, newAtt = newAtt, baseAtt
}
Expand Down
Expand Up @@ -183,7 +183,7 @@ func TestBatchAttestations_Single(t *testing.T) {
}

if !reflect.DeepEqual(wanted, s.pool.ForkchoiceAttestations()) {
t.Error("Did not aggregation and save for batch")
t.Error("Did not aggregate and save for batch")
}
}

Expand Down
14 changes: 13 additions & 1 deletion beacon-chain/p2p/peers/status_test.go
Expand Up @@ -341,31 +341,43 @@ func TestTrimmedOrderedPeers(t *testing.T) {

expectedTarget := uint64(2)
maxPeers := 3

mockroot2 := [32]byte{}
mockroot3 := [32]byte{}
mockroot4 := [32]byte{}
mockroot5 := [32]byte{}
copy(mockroot2[:], "two")
copy(mockroot3[:], "three")
copy(mockroot4[:], "four")
copy(mockroot5[:], "five")
// Peer 1
pid1 := addPeer(t, p, peers.PeerConnected)
p.SetChainState(pid1, &pb.Status{
FinalizedEpoch: 3,
FinalizedRoot: mockroot3[:],
})
// Peer 2
pid2 := addPeer(t, p, peers.PeerConnected)
p.SetChainState(pid2, &pb.Status{
FinalizedEpoch: 4,
FinalizedRoot: mockroot4[:],
})
// Peer 3
pid3 := addPeer(t, p, peers.PeerConnected)
p.SetChainState(pid3, &pb.Status{
FinalizedEpoch: 5,
FinalizedRoot: mockroot5[:],
})
// Peer 4
pid4 := addPeer(t, p, peers.PeerConnected)
p.SetChainState(pid4, &pb.Status{
FinalizedEpoch: 2,
FinalizedRoot: mockroot2[:],
})
// Peer 5
pid5 := addPeer(t, p, peers.PeerConnected)
p.SetChainState(pid5, &pb.Status{
FinalizedEpoch: 2,
FinalizedRoot: mockroot2[:],
})

_, target, pids := p.BestFinalized(maxPeers, 0)
Expand Down
1 change: 0 additions & 1 deletion beacon-chain/rpc/validator/BUILD.bazel
Expand Up @@ -42,7 +42,6 @@ go_library(
"//shared/slotutil:go_default_library",
"//shared/traceutil:go_default_library",
"//shared/trieutil:go_default_library",
"@com_github_gogo_protobuf//proto:go_default_library",
"@com_github_gogo_protobuf//types:go_default_library",
"@com_github_pkg_errors//:go_default_library",
"@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library",
Expand Down
4 changes: 2 additions & 2 deletions beacon-chain/rpc/validator/attester.go
Expand Up @@ -4,14 +4,14 @@ import (
"context"
"time"

"github.com/gogo/protobuf/proto"
ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
"github.com/prysmaticlabs/go-ssz"
"github.com/prysmaticlabs/prysm/beacon-chain/cache"
"github.com/prysmaticlabs/prysm/beacon-chain/core/feed"
statefeed "github.com/prysmaticlabs/prysm/beacon-chain/core/feed/state"
"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
"github.com/prysmaticlabs/prysm/beacon-chain/core/state"
stateTrie "github.com/prysmaticlabs/prysm/beacon-chain/state"
"github.com/prysmaticlabs/prysm/shared/bls"
"github.com/prysmaticlabs/prysm/shared/bytesutil"
"github.com/prysmaticlabs/prysm/shared/params"
Expand Down Expand Up @@ -134,7 +134,7 @@ func (vs *Server) ProposeAttestation(ctx context.Context, att *ethpb.Attestation

go func() {
ctx = trace.NewContext(context.Background(), trace.FromContext(ctx))
attCopy := proto.Clone(att).(*ethpb.Attestation)
attCopy := stateTrie.CopyAttestation(att)
if err := vs.AttPool.SaveUnaggregatedAttestation(attCopy); err != nil {
log.WithError(err).Error("Could not handle attestation in operations service")
return
Expand Down
1 change: 1 addition & 0 deletions beacon-chain/state/BUILD.bazel
Expand Up @@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
go_library(
name = "go_default_library",
srcs = [
"cloners.go",
"getters.go",
"setters.go",
"types.go",
Expand Down

0 comments on commit 59a174d

Please sign in to comment.