core: change to one callback per batch of inserted headers + review concerns

Author: holiman

core/headerchain.go

@@ -132,7 +132,6 @@ func (hc *HeaderChain) GetBlockNumber(hash common.Hash) *uint64 {
 type headerWriteResult struct {
 	ignored    int
 	imported   int
-	status     WriteStatus
 	lastHash   common.Hash
 	lastHeader *types.Header
 }
@@ -155,12 +154,12 @@ func (hc *HeaderChain) WriteHeaders(headers []*types.Header, postWriteFn PostWri
 		return &headerWriteResult{}, consensus.ErrUnknownAncestor
 	}
 	var (
-		lastHeader         *types.Header // Last successfully imported header
-		lastNumber         uint64        // Last successfully imported number
-		lastHash           common.Hash
-		externTd           *big.Int     // TD of successfully imported chain
-		inserted           []numberHash // Ephemeral lookup of number/hash for the chain
-		firstInsertedIndex = -1         // Index of the first non-ignored header
+		lastHeader    *types.Header // Last successfully imported header
+		lastNumber    uint64        // Last successfully imported number
+		lastHash      common.Hash
+		externTd      *big.Int     // TD of successfully imported chain
+		inserted      []numberHash // Ephemeral lookup of number/hash for the chain
+		firstInserted = -1         // Index of the first non-ignored header
 	)
 	lastHash, lastNumber = headers[0].ParentHash, headers[0].Number.Uint64()-1 // Already validated above
 	batch := hc.chainDb.NewBatch()
@@ -178,7 +177,7 @@ func (hc *HeaderChain) WriteHeaders(headers []*types.Header, postWriteFn PostWri
 		}
 		hash, number := header.Hash(), header.Number.Uint64()
 		if header.ParentHash != lastHash {
-			log.Warn("Non-contiguous header insertion", "header.parent", header.ParentHash, "expected", hash, "number", number)
+			log.Warn("Non-contiguous header insertion", "parent", header.ParentHash, "expected", lastHash, "number", number)
 			break
 		}
 		externTd = new(big.Int).Add(header.Difficulty, ptd)
@@ -193,8 +192,8 @@ func (hc *HeaderChain) WriteHeaders(headers []*types.Header, postWriteFn PostWri
 			inserted = append(inserted, numberHash{number, hash})
 			hc.headerCache.Add(hash, header)
 			hc.numberCache.Add(hash, number)
-			if firstInsertedIndex < 0 {
-				firstInsertedIndex = i
+			if firstInserted < 0 {
+				firstInserted = i
 			}
 		}
 		lastHeader, lastHash, lastNumber, ptd = header, hash, number, externTd
@@ -253,7 +252,7 @@ func (hc *HeaderChain) WriteHeaders(headers []*types.Header, postWriteFn PostWri
 			// during this import batch, then we need to write that now
 			// Further down, we continue writing the staus for the ones that
 			// were not already known
-			for i := 0; i < firstInsertedIndex; i++ {
+			for i := 0; i < firstInserted; i++ {
 				hash := headers[i].Hash()
 				num := headers[i].Number.Uint64()
 				rawdb.WriteCanonicalHash(markerBatch, hash, num)
@@ -279,30 +278,23 @@ func (hc *HeaderChain) WriteHeaders(headers []*types.Header, postWriteFn PostWri
 	// Execute any post-write callback function
 	// - unless we're exiting
 	// - and unless we ignored everything
-	if postWriteFn != nil && !hc.procInterrupt() && firstInsertedIndex >= 0 {
-		// TODO: Is it really necessary to invoke this N times, instead of just
-		// invoking it for the last header?
-		// It is only used for lightchain event aggregation
-		for _, header := range headers[firstInsertedIndex:] {
-			if header.Number.Uint64() > lastNumber {
-				break
-			}
-			postWriteFn(header, status)
-		}
+	if postWriteFn != nil && !hc.procInterrupt() && len(inserted) > 0 {
+		// The postwrite function is called only for the last imported header.
+		// It is only used for lightchain event aggregation.
+		postWriteFn(lastHeader, status)
 	}
 	return &headerWriteResult{
 		ignored:    len(headers) - len(inserted),
 		imported:   len(inserted),
-		status:     status,
 		lastHash:   lastHash,
 		lastHeader: lastHeader,
 	}, nil
 }
 
 // PostWriteCallback is a callback function for inserting headers,
-// which is called after each header is inserted.
-// The reson for having it is:
-// In light-chain mode, status should be  processed and light chain events sent,
+// which is called once, with the last successfully imported header in the batch.
+// The raeson for having it is:
+// In light-chain mode, status should be processed and light chain events sent,
 // whereas in a non-light mode this is not necessary since chain events are sent after inserting blocks.
 type PostWriteCallback func(header *types.Header, status WriteStatus)
 

core/headerchain_test.go

@@ -98,43 +98,43 @@ func TestHeaderInsertion(t *testing.T) {
 	log.Root().SetHandler(log.StdoutHandler)
 
 	// Inserting 64 headers on an empty chain, expecting
-	// 64 callbacks, 64 canon-status, 0 sidestatus,
-	if err := testInsert(t, hc, chainA[:64], 64, 64, 0); err != nil {
+	// 1 callbacks, 1 canon-status, 0 sidestatus,
+	if err := testInsert(t, hc, chainA[:64], 1, 1, 0); err != nil {
 		t.Fatal(err)
 	}
 
-	// Inserting 64 inentical headers, expecting
+	// Inserting 64 identical headers, expecting
 	// 0 callbacks, 0 canon-status, 0 sidestatus,
 	if err := testInsert(t, hc, chainA[:64], 0, 0, 0); err != nil {
 		t.Fatal(err)
 	}
 	// Inserting the same some old, some new headers
-	// 32 callbacks, 32 canon, 0 side
-	if err := testInsert(t, hc, chainA[32:96], 32, 32, 0); err != nil {
+	// 1 callbacks, 1 canon, 0 side
+	if err := testInsert(t, hc, chainA[32:96], 1, 1, 0); err != nil {
 		t.Fatal(err)
 	}
 	// Inserting side blocks, but not overtaking the canon chain
-	if err := testInsert(t, hc, chainB[0:32], 32, 0, 32); err != nil {
+	if err := testInsert(t, hc, chainB[0:32], 1, 0, 1); err != nil {
 		t.Fatal(err)
 	}
 	// Inserting more side blocks, but we don't have the parent
 	if err := testInsert(t, hc, chainB[34:36], 0, 0, 0); !errors.Is(err, consensus.ErrUnknownAncestor) {
 		t.Fatal(fmt.Errorf("Expected %v, got %v", consensus.ErrUnknownAncestor, err))
 	}
 	// Inserting more sideblocks, overtaking the canon chain
-	if err := testInsert(t, hc, chainB[32:97], 65, 65, 0); err != nil {
+	if err := testInsert(t, hc, chainB[32:97], 1, 1, 0); err != nil {
 		t.Fatal(err)
 	}
 	// Inserting more A-headers, taking back the canonicality
-	if err := testInsert(t, hc, chainA[90:100], 4, 4, 0); err != nil {
+	if err := testInsert(t, hc, chainA[90:100], 1, 1, 0); err != nil {
 		t.Fatal(err)
 	}
 	// And B becomes canon again
-	if err := testInsert(t, hc, chainB[97:107], 10, 10, 0); err != nil {
+	if err := testInsert(t, hc, chainB[97:107], 1, 1, 0); err != nil {
 		t.Fatal(err)
 	}
 	// And B becomes even longer
-	if err := testInsert(t, hc, chainB[107:128], 21, 21, 0); err != nil {
+	if err := testInsert(t, hc, chainB[107:128], 1, 1, 0); err != nil {
 		t.Fatal(err)
 	}
 }