Apply suggestions from code review

Co-authored-by: Alexander Hentschel <[email protected]>

Author: zhangchiqing

state/cluster/badger/state_test.go

@@ -43,7 +43,6 @@ func TestUnknownSnapshotReference(t *testing.T) {
 
 	// Verify that Collection() returns state.ErrUnknownSnapshotReference
 	_, err = snapshot.Collection()
-	assert.Error(t, err)
 	assert.ErrorIs(t, err, state.ErrUnknownSnapshotReference)
 
 	// Verify that Head() returns state.ErrUnknownSnapshotReference
@@ -58,7 +57,7 @@ func TestUnknownSnapshotReference(t *testing.T) {
 }
 
 // TestValidSnapshotReference verifies that AtBlockID returns a working snapshot
-// when given a valid block ID (the genesis block).
+// when given a valid block ID (which in this test is the genesis block ID).
 func TestValidSnapshotReference(t *testing.T) {
 	// Setup
 	genesis, err := unittest.ClusterBlock.Genesis()

state/cluster/invalid/snapshot.go

@@ -15,11 +15,11 @@ type Snapshot struct {
 }
 
 // NewSnapshot returns a new invalid snapshot, containing an error describing why the
-// snapshot could not be retrieved. The following are expected
-// errors when constructing an invalid Snapshot:
+// snapshot could not be retrieved. The following are typical
+// errors resulting in the construction of an invalid Snapshot:
 //   - state.ErrUnknownSnapshotReference if the reference point for the snapshot
 //     (height or block ID) does not resolve to a queriable block in the state.
-//   - generic error in case of unexpected critical internal corruption or bugs
+//   - generic error in case of unexpected state inconsistencies or bugs
 func NewSnapshot(err error) *Snapshot {
 	if errors.Is(err, state.ErrUnknownSnapshotReference) {
 		return &Snapshot{err: err}

storage/operation/children.go

@@ -10,10 +10,10 @@ import (
 	"github.com/onflow/flow-go/storage"
 )
 
-// IndexNewBlock indexes a new block and updates the parent-child relationship in the block children index.
-// This function creates an empty children index for the new block and adds the new block to the parent's children list.
+// IndexNewBlock populates the parent-child index for block, by adding the given blockID to the set of children of its parent.
 //
 // CAUTION:
+//   - This function should only be used for KNOWN BLOCKs (neither existence of the block nor its parent is verified here)
 //   - The caller must acquire the [storage.LockInsertBlock] and hold it until the database write has been committed.
 //
 // Expected error returns during normal operations:
@@ -26,10 +26,11 @@ func IndexNewBlock(lctx lockctx.Proof, rw storage.ReaderBatchWriter, blockID flo
 	return indexBlockByParent(rw, blockID, parentID)
 }
 
-// IndexNewClusterBlock indexes a new cluster block and updates the parent-child relationship in the block children index.
-// This function creates an empty children index for the new cluster block and adds the new block to the parent's children list.
+// IndexNewClusterBlock populates the parent-child index for cluster blocks, aka collections, by adding the given
+// blockID to the set of children of its parent.
 //
 // CAUTION:
+//   - This function should only be used for KNOWN BLOCKs (neither existence of the block nor its parent is verified here)
 //   - The caller must acquire the [storage.LockInsertOrFinalizeClusterBlock] and hold it until the database write has been committed.
 //
 // Expected error returns during normal operations:
@@ -57,18 +58,15 @@ func indexBlockByParent(rw storage.ReaderBatchWriter, blockID flow.Identifier, p
 			storage.ErrAlreadyExists)
 	}
 
-	// Step 2: adding the second index for the parent block
-	// if the parent block is zero, for instance root block has no parent,
-	// then no need to add index for it
-	// useful to skip for cluster root block which has no parent
+	// By convention, the parentID being [flow.ZeroID] means that the block has no parent.
+	// This is the case for genesis blocks and cluster root blocks. In this case, there
+	// is no parent, whose child we can index.
 	if parentID == flow.ZeroID {
 		return nil
 	}
 
-	// if the parent block is not zero, depending on whether the parent block has
-	// children or not, we will either update the index or insert the index:
-	// when parent block doesn't exist, we will insert the block children.
-	// when parent block exists already, we will update the block children,
+	// If the parent block is not zero, depending on whether the parent block has
+	// children or not, we will either update the index or insert the index.
 	var childrenIDs flow.IdentifierList
 	err = RetrieveBlockChildren(rw.GlobalReader(), parentID, &childrenIDs)
 	if err != nil {
@@ -98,7 +96,7 @@ func indexBlockByParent(rw storage.ReaderBatchWriter, blockID flow.Identifier, p
 
 // RetrieveBlockChildren retrieves the list of child block IDs for the specified parent block.
 //
-// Expected errors during normal operations:
+// No error returns expected during normal operations.
 // It returns [storage.ErrNotFound] if the block has no children.
 // Note, this would mean either the block does not exist or the block exists but has no children.
 // The caller has to check if the block exists by other means if needed.

storage/operation/children_test.go

@@ -13,14 +13,16 @@ import (
 	"github.com/onflow/flow-go/utils/unittest"
 )
 
-// TestOperationPair represents a pair of operations to test
+// TestOperationPair is a pair of indexing operation and corresponding lock to be held during the operation. 
+// The name is an auxiliary identifier, for debugging only. 
 type TestOperationPair struct {
 	name      string
 	indexFunc func(lockctx.Proof, storage.ReaderBatchWriter, flow.Identifier, flow.Identifier) error
 	lockType  string
 }
 
-// getTestOperationPairs returns the operation pairs to test
+// getTestOperationPairs returns a `TestOperationPair` for indexing main consensus blocks
+// and one `TestOperationPair` for indexing collector blocks (aka collections).
 func getTestOperationPairs() []TestOperationPair {
 	return []TestOperationPair{
 		{
@@ -47,7 +49,6 @@ func TestIndexAndLookupChild(t *testing.T) {
 				// retrieving children of a non-existent block should return empty list
 				var retrievedIDs flow.IdentifierList
 				err := operation.RetrieveBlockChildren(db.Reader(), nonExist, &retrievedIDs)
-				require.Error(t, err)
 				require.ErrorIs(t, err, storage.ErrNotFound)
 
 				parentID := unittest.IdentifierFixture()
@@ -68,7 +69,6 @@ func TestIndexAndLookupChild(t *testing.T) {
 
 				err = operation.RetrieveBlockChildren(db.Reader(), childID, &retrievedIDs)
 				// verify new block has no children index (returning storage.ErrNotFound)
-				require.Error(t, err)
 				require.ErrorIs(t, err, storage.ErrNotFound)
 
 				// verify indexing again would hit storage.ErrAlreadyExists error
@@ -77,7 +77,6 @@ func TestIndexAndLookupChild(t *testing.T) {
 						return opPair.indexFunc(lctx, rw, childID, parentID)
 					})
 				})
-				require.Error(t, err)
 				require.ErrorIs(t, err, storage.ErrAlreadyExists)
 			})
 		})