Best practice feedback - part 1 (#6385)

* log error for metric
* reuse epoch e
* Better node comments
* Remove not needed breaks
* Use j over i
* Merge branch 'master' of github.com:prysmaticlabs/prysm into best-practice-pt1
* Descendent/Descendant
* Merge refs/heads/master into best-practice-pt1
* Update beacon-chain/forkchoice/protoarray/nodes.go
* Merge refs/heads/master into best-practice-pt1
* Merge refs/heads/master into best-practice-pt1

beacon-chain/blockchain/info.go

@@ -64,7 +64,7 @@ func (s *Service) TreeHandler(w http.ResponseWriter, _ *http.Request) {
 		}
 
 		if nodes[i].Slot == s.headSlot() &&
-			nodes[i].BestDescendent == ^uint64(0) {
+			nodes[i].BestDescendant == ^uint64(0) {
 			dotN = dotN.Attr("color", "green")
 		}
 

beacon-chain/blockchain/metrics.go

@@ -138,6 +138,7 @@ func reportEpochMetrics(state *stateTrie.BeaconState) {
 	for i, validator := range state.Validators() {
 		bal, err := state.BalanceAtIndex(uint64(i))
 		if err != nil {
+			log.Errorf("Could not load validator balance: %v", err)
 			continue
 		}
 		if validator.Slashed {

beacon-chain/core/helpers/validators.go

@@ -201,7 +201,7 @@ func BeaconProposerIndex(state *stateTrie.BeaconState) (uint64, error) {
 		return 0, errors.Wrap(err, "could not get active indices")
 	}
 
-	if err := UpdateProposerIndicesInCache(state, CurrentEpoch(state)); err != nil {
+	if err := UpdateProposerIndicesInCache(state, e); err != nil {
 		return 0, errors.Wrap(err, "could not update committee cache")
 	}
 

beacon-chain/forkchoice/protoarray/ffg_update_test.go

@@ -251,7 +251,7 @@ func setup(justifiedEpoch uint64, finalizedEpoch uint64) *ForkChoice {
 		JustifiedEpoch: justifiedEpoch,
 		FinalizedEpoch: finalizedEpoch,
 		BestChild:      NonExistentNode,
-		BestDescendent: NonExistentNode,
+		BestDescendant: NonExistentNode,
 		Weight:         0,
 	})
 

beacon-chain/forkchoice/protoarray/helpers.go

@@ -88,6 +88,6 @@ func copyNode(node *Node) *Node {
 		FinalizedEpoch: node.FinalizedEpoch,
 		Weight:         node.Weight,
 		BestChild:      node.BestChild,
-		BestDescendent: node.BestDescendent,
+		BestDescendant: node.BestDescendant,
 	}
 }

beacon-chain/forkchoice/protoarray/nodes.go

@@ -27,7 +27,7 @@ func (s *Store) head(ctx context.Context, justifiedRoot [32]byte) ([32]byte, err
 	}
 
 	justifiedNode := s.Nodes[justifiedIndex]
-	bestDescendantIndex := justifiedNode.BestDescendent
+	bestDescendantIndex := justifiedNode.BestDescendant
 	// If the justified node doesn't have a best descendent,
 	// the best node is itself.
 	if bestDescendantIndex == NonExistentNode {
@@ -88,7 +88,7 @@ func (s *Store) insert(ctx context.Context,
 		JustifiedEpoch: justifiedEpoch,
 		FinalizedEpoch: finalizedEpoch,
 		BestChild:      NonExistentNode,
-		BestDescendent: NonExistentNode,
+		BestDescendant: NonExistentNode,
 		Weight:         0,
 	}
 
@@ -200,15 +200,15 @@ func (s *Store) updateBestChildAndDescendant(parentIndex uint64, childIndex uint
 	}
 
 	// Define 3 variables for the 3 outcomes mentioned above. This is to
-	// set `parent.BestChild` and `parent.bestDescendent` to. These
+	// set `parent.BestChild` and `parent.bestDescendant` to. These
 	// aliases are to assist readability.
 	changeToNone := []uint64{NonExistentNode, NonExistentNode}
-	bestDescendant := child.BestDescendent
+	bestDescendant := child.BestDescendant
 	if bestDescendant == NonExistentNode {
 		bestDescendant = childIndex
 	}
 	changeToChild := []uint64{childIndex, bestDescendant}
-	noChange := []uint64{parent.BestChild, parent.BestDescendent}
+	noChange := []uint64{parent.BestChild, parent.BestDescendant}
 	newParentChild := make([]uint64, 0)
 
 	if parent.BestChild != NonExistentNode {
@@ -267,7 +267,7 @@ func (s *Store) updateBestChildAndDescendant(parentIndex uint64, childIndex uint
 
 	// Update parent with the outcome.
 	parent.BestChild = newParentChild[0]
-	parent.BestDescendent = newParentChild[1]
+	parent.BestDescendant = newParentChild[1]
 	s.Nodes[parentIndex] = parent
 
 	return nil
@@ -332,11 +332,11 @@ func (s *Store) prune(ctx context.Context, finalizedRoot [32]byte) error {
 			}
 			node.BestChild -= finalizedIndex
 		}
-		if node.BestDescendent != NonExistentNode {
-			if node.BestDescendent < finalizedIndex {
+		if node.BestDescendant != NonExistentNode {
+			if node.BestDescendant < finalizedIndex {
 				return errInvalidBestDescendantIndex
 			}
-			node.BestDescendent -= finalizedIndex
+			node.BestDescendant -= finalizedIndex
 		}
 
 		s.Nodes[i] = node
@@ -352,7 +352,7 @@ func (s *Store) prune(ctx context.Context, finalizedRoot [32]byte) error {
 // should not be viable to head.
 func (s *Store) leadsToViableHead(node *Node) (bool, error) {
 	var bestDescendentViable bool
-	bestDescendentIndex := node.BestDescendent
+	bestDescendentIndex := node.BestDescendant
 
 	// If the best descendant is not part of the leaves.
 	if bestDescendentIndex != NonExistentNode {

beacon-chain/forkchoice/protoarray/nodes_test.go

@@ -31,7 +31,7 @@ func TestStore_Head_Itself(t *testing.T) {
 
 	// Since the justified node does not have a best descendant so the best node
 	// is itself.
-	s := &Store{NodeIndices: indices, Nodes: []*Node{{Root: r, BestDescendent: NonExistentNode}}}
+	s := &Store{NodeIndices: indices, Nodes: []*Node{{Root: r, BestDescendant: NonExistentNode}}}
 	h, err := s.head(context.Background(), r)
 	if err != nil {
 		t.Fatal("Did not get wanted error")
@@ -50,7 +50,7 @@ func TestStore_Head_BestDescendant(t *testing.T) {
 
 	// Since the justified node's best descendent is at index 1 and it's root is `best`,
 	// the head should be `best`.
-	s := &Store{NodeIndices: indices, Nodes: []*Node{{Root: r, BestDescendent: 1}, {Root: best}}}
+	s := &Store{NodeIndices: indices, Nodes: []*Node{{Root: r, BestDescendant: 1}, {Root: best}}}
 	h, err := s.head(context.Background(), r)
 	if err != nil {
 		t.Fatal("Did not get wanted error")
@@ -227,14 +227,14 @@ func TestStore_UpdateBestChildAndDescendant_RemoveChild(t *testing.T) {
 	if s.Nodes[0].BestChild != NonExistentNode {
 		t.Error("Did not get correct best child index")
 	}
-	if s.Nodes[0].BestDescendent != NonExistentNode {
+	if s.Nodes[0].BestDescendant != NonExistentNode {
 		t.Error("Did not get correct best descendant index")
 	}
 }
 
 func TestStore_UpdateBestChildAndDescendant_UpdateDescendant(t *testing.T) {
 	// Make parent's best child equal to child index and child is viable.
-	s := &Store{Nodes: []*Node{{BestChild: 1}, {BestDescendent: NonExistentNode}}}
+	s := &Store{Nodes: []*Node{{BestChild: 1}, {BestDescendant: NonExistentNode}}}
 
 	if err := s.updateBestChildAndDescendant(0, 1); err != nil {
 		t.Fatal(err)
@@ -244,7 +244,7 @@ func TestStore_UpdateBestChildAndDescendant_UpdateDescendant(t *testing.T) {
 	if s.Nodes[0].BestChild != 1 {
 		t.Error("Did not get correct best child index")
 	}
-	if s.Nodes[0].BestDescendent != 1 {
+	if s.Nodes[0].BestDescendant != 1 {
 		t.Error("Did not get correct best descendant index")
 	}
 }
@@ -256,8 +256,8 @@ func TestStore_UpdateBestChildAndDescendant_ChangeChildByViability(t *testing.T)
 		JustifiedEpoch: 1,
 		FinalizedEpoch: 1,
 		Nodes: []*Node{{BestChild: 1, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode},
-			{BestDescendent: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1}}}
+			{BestDescendant: NonExistentNode},
+			{BestDescendant: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1}}}
 
 	if err := s.updateBestChildAndDescendant(0, 2); err != nil {
 		t.Fatal(err)
@@ -267,7 +267,7 @@ func TestStore_UpdateBestChildAndDescendant_ChangeChildByViability(t *testing.T)
 	if s.Nodes[0].BestChild != 2 {
 		t.Error("Did not get correct best child index")
 	}
-	if s.Nodes[0].BestDescendent != 2 {
+	if s.Nodes[0].BestDescendant != 2 {
 		t.Error("Did not get correct best descendant index")
 	}
 }
@@ -279,8 +279,8 @@ func TestStore_UpdateBestChildAndDescendant_ChangeChildByWeight(t *testing.T) {
 		JustifiedEpoch: 1,
 		FinalizedEpoch: 1,
 		Nodes: []*Node{{BestChild: 1, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1, Weight: 1}}}
+			{BestDescendant: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
+			{BestDescendant: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1, Weight: 1}}}
 
 	if err := s.updateBestChildAndDescendant(0, 2); err != nil {
 		t.Fatal(err)
@@ -290,7 +290,7 @@ func TestStore_UpdateBestChildAndDescendant_ChangeChildByWeight(t *testing.T) {
 	if s.Nodes[0].BestChild != 2 {
 		t.Error("Did not get correct best child index")
 	}
-	if s.Nodes[0].BestDescendent != 2 {
+	if s.Nodes[0].BestDescendant != 2 {
 		t.Error("Did not get correct best descendant index")
 	}
 }
@@ -301,8 +301,8 @@ func TestStore_UpdateBestChildAndDescendant_ChangeChildAtLeaf(t *testing.T) {
 		JustifiedEpoch: 1,
 		FinalizedEpoch: 1,
 		Nodes: []*Node{{BestChild: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1}}}
+			{BestDescendant: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
+			{BestDescendant: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1}}}
 
 	if err := s.updateBestChildAndDescendant(0, 2); err != nil {
 		t.Fatal(err)
@@ -312,7 +312,7 @@ func TestStore_UpdateBestChildAndDescendant_ChangeChildAtLeaf(t *testing.T) {
 	if s.Nodes[0].BestChild != 2 {
 		t.Error("Did not get correct best child index")
 	}
-	if s.Nodes[0].BestDescendent != 2 {
+	if s.Nodes[0].BestDescendant != 2 {
 		t.Error("Did not get correct best descendant index")
 	}
 }
@@ -324,8 +324,8 @@ func TestStore_UpdateBestChildAndDescendant_NoChangeByViability(t *testing.T) {
 		JustifiedEpoch: 1,
 		FinalizedEpoch: 1,
 		Nodes: []*Node{{BestChild: 1, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode}}}
+			{BestDescendant: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
+			{BestDescendant: NonExistentNode}}}
 
 	if err := s.updateBestChildAndDescendant(0, 2); err != nil {
 		t.Fatal(err)
@@ -335,7 +335,7 @@ func TestStore_UpdateBestChildAndDescendant_NoChangeByViability(t *testing.T) {
 	if s.Nodes[0].BestChild != 1 {
 		t.Error("Did not get correct best child index")
 	}
-	if s.Nodes[0].BestDescendent != 0 {
+	if s.Nodes[0].BestDescendant != 0 {
 		t.Error("Did not get correct best descendant index")
 	}
 }
@@ -347,8 +347,8 @@ func TestStore_UpdateBestChildAndDescendant_NoChangeByWeight(t *testing.T) {
 		JustifiedEpoch: 1,
 		FinalizedEpoch: 1,
 		Nodes: []*Node{{BestChild: 1, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1, Weight: 1},
-			{BestDescendent: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1}}}
+			{BestDescendant: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1, Weight: 1},
+			{BestDescendant: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1}}}
 
 	if err := s.updateBestChildAndDescendant(0, 2); err != nil {
 		t.Fatal(err)
@@ -358,7 +358,7 @@ func TestStore_UpdateBestChildAndDescendant_NoChangeByWeight(t *testing.T) {
 	if s.Nodes[0].BestChild != 1 {
 		t.Error("Did not get correct best child index")
 	}
-	if s.Nodes[0].BestDescendent != 0 {
+	if s.Nodes[0].BestDescendant != 0 {
 		t.Error("Did not get correct best descendant index")
 	}
 }
@@ -369,8 +369,8 @@ func TestStore_UpdateBestChildAndDescendant_NoChangeAtLeaf(t *testing.T) {
 		JustifiedEpoch: 1,
 		FinalizedEpoch: 1,
 		Nodes: []*Node{{BestChild: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
-			{BestDescendent: NonExistentNode}}}
+			{BestDescendant: NonExistentNode, JustifiedEpoch: 1, FinalizedEpoch: 1},
+			{BestDescendant: NonExistentNode}}}
 
 	if err := s.updateBestChildAndDescendant(0, 2); err != nil {
 		t.Fatal(err)
@@ -380,7 +380,7 @@ func TestStore_UpdateBestChildAndDescendant_NoChangeAtLeaf(t *testing.T) {
 	if s.Nodes[0].BestChild != NonExistentNode {
 		t.Error("Did not get correct best child index")
 	}
-	if s.Nodes[0].BestDescendent != 0 {
+	if s.Nodes[0].BestDescendant != 0 {
 		t.Error("Did not get correct best descendant index")
 	}
 }
@@ -419,7 +419,7 @@ func TestStore_Prune_MoreThanThreshold(t *testing.T) {
 	for i := 0; i < numOfNodes; i++ {
 		indices[indexToHash(uint64(i))] = uint64(i)
 		nodes = append(nodes, &Node{Slot: uint64(i), Root: indexToHash(uint64(i)),
-			BestDescendent: NonExistentNode, BestChild: NonExistentNode})
+			BestDescendant: NonExistentNode, BestChild: NonExistentNode})
 	}
 
 	s := &Store{Nodes: nodes, NodeIndices: indices}
@@ -445,7 +445,7 @@ func TestStore_Prune_MoreThanOnce(t *testing.T) {
 	for i := 0; i < numOfNodes; i++ {
 		indices[indexToHash(uint64(i))] = uint64(i)
 		nodes = append(nodes, &Node{Slot: uint64(i), Root: indexToHash(uint64(i)),
-			BestDescendent: NonExistentNode, BestChild: NonExistentNode})
+			BestDescendant: NonExistentNode, BestChild: NonExistentNode})
 	}
 
 	s := &Store{Nodes: nodes, NodeIndices: indices}

beacon-chain/forkchoice/protoarray/types.go

@@ -23,15 +23,15 @@ type Store struct {
 // Node defines the individual block which includes its block parent, ancestor and how much weight accounted for it.
 // This is used as an array based stateful DAG for efficient fork choice look up.
 type Node struct {
-	Slot           uint64   // slot of the block converted to the node.
+	Slot           uint64   // Slot of the block converted to the node.
 	Root           [32]byte // Root of the block converted to the node.
-	Parent         uint64   // the parent index of this node.
-	JustifiedEpoch uint64   // justified epoch of this node.
-	FinalizedEpoch uint64   // finalized epoch of this node.
-	Weight         uint64   // weight of this node.
-	BestChild      uint64   // best child index of this node.
-	BestDescendent uint64   // head index of this node.
-	Graffiti       [32]byte // graffati of the block node.
+	Parent         uint64   // Parent index of this node.
+	JustifiedEpoch uint64   // JustifiedEpoch of this node.
+	FinalizedEpoch uint64   // FinalizedEpoch of this node.
+	Weight         uint64   // Weight of this node.
+	BestChild      uint64   // BestChild index of this node.
+	BestDescendant uint64   // BestDescendant of this node.
+	Graffiti       [32]byte // Graffiti of the block node.
 }
 
 // Vote defines an individual validator's vote.

beacon-chain/main.go

@@ -131,17 +131,14 @@ func main() {
 			// the colors are ANSI codes and seen as gibberish in the log files.
 			formatter.DisableColors = ctx.String(cmd.LogFileName.Name) != ""
 			logrus.SetFormatter(formatter)
-			break
 		case "fluentd":
 			f := joonix.NewFormatter()
 			if err := joonix.DisableTimestampFormat(f); err != nil {
 				panic(err)
 			}
 			logrus.SetFormatter(f)
-			break
 		case "json":
 			logrus.SetFormatter(&logrus.JSONFormatter{})
-			break
 		default:
 			return fmt.Errorf("unknown log format %s", format)
 		}

beacon-chain/rpc/debug/forkchoice.go

@@ -24,7 +24,7 @@ func (ds *Server) GetProtoArrayForkChoice(ctx context.Context, _ *ptypes.Empty)
 			FinalizedEpoch: nodes[i].FinalizedEpoch,
 			Weight:         nodes[i].Weight,
 			BestChild:      nodes[i].BestChild,
-			BestDescendant: nodes[i].BestDescendent,
+			BestDescendant: nodes[i].BestDescendant,
 		}
 	}
 

beacon-chain/state/stateutil/arrays.go

@@ -123,11 +123,11 @@ func merkleizeTrieLeaves(layers [][][32]byte, hashLayer [][32]byte,
 	chunkBuffer.Grow(64)
 	for len(hashLayer) > 1 && i < len(layers) {
 		layer := make([][32]byte, len(hashLayer)/2, len(hashLayer)/2)
-		for i := 0; i < len(hashLayer); i += 2 {
-			chunkBuffer.Write(hashLayer[i][:])
-			chunkBuffer.Write(hashLayer[i+1][:])
+		for j := 0; j < len(hashLayer); j += 2 {
+			chunkBuffer.Write(hashLayer[j][:])
+			chunkBuffer.Write(hashLayer[j+1][:])
 			hashedChunk := hasher(chunkBuffer.Bytes())
-			layer[i/2] = hashedChunk
+			layer[j/2] = hashedChunk
 			chunkBuffer.Reset()
 		}
 		hashLayer = layer