cmd/utils: report the correct invalid block number in block import

[gballet]

When the block import fails, the error displays the number of the first block past the import batch, not the number of the failing block. This PR fixes this problem by identifying which blocks fails and reporting its number.

[holiman]

This code can now panic on invalid array index, -1 or too large. If you just check that so we don't panic here, then it's fine by me

[gballet]

just double-checked, it really can not be -1 (in the current state of the code) since there is no return between the moment the iterator is initalized with -1 and the moment the index is set to a >= 0 value. This is just FYI, not using an array index is the better, future-proof approach.

[holiman]

Well, here's a panic though:

func (bc *BlockChain) InsertChain(chain types.Blocks) (int, error) {
	// Sanity check that we have something meaningful to import
	if len(chain) == 0 {
		return 0, nil
	}

[holiman]

And, incidentally, this should not return 0, but i or i-1:

	for i := 1; i < len(chain); i++ {
		block, prev := chain[i], chain[i-1]
		if block.NumberU64() != prev.NumberU64()+1 || block.ParentHash() != prev.Hash() {
			log.Error("Non contiguous block insert",
				"number", block.Number(),
				"hash", block.Hash(),
				"parent", block.ParentHash(),
				"prevnumber", prev.Number(),
				"prevhash", prev.Hash(),
			)
			return 0, fmt.Errorf("non contiguous insert: item %d is #%d [%x..], item %d is #%d [%x..] (parent [%x..])", i-1, prev.NumberU64(),
				prev.Hash().Bytes()[:4], i, block.NumberU64(), block.Hash().Bytes()[:4], block.ParentHash().Bytes()[:4])

[holiman]

Also, perhaps better to show both the index of the block in the sequence, and the block number ( if we have it), possibly aswell as the current "last good" (?).

[gballet]

Well, here's a panic though:

func (bc *BlockChain) InsertChain(chain types.Blocks) (int, error) {
	// Sanity check that we have something meaningful to import
	if len(chain) == 0 {
		return 0, nil
	}

thought so at first too, but the error would be nil, so it would not try to access that branch, and therefore not panic.

And, incidentally, this should not return 0, but i or i-1:

True, I'll fix that

[gballet]

And, incidentally, this should not return 0, but i or i-1:

So actually, it's subtle but returning 0 here is correct imo: since the code hasn't tried to insert any block, no individual single block is responsible for the failure, it's the sequence as a whole that is incorrect. But it's also possible to consider i to be incorrect, also it's harder to argue in the case i=1 because maybe the first block is the incorrect one. What do you think?

Also, perhaps better to show both the index of the block in the sequence, and the block number ( if we have it), possibly aswell as the current "last good" (?).

That is already done inside the error message that is returned.

[holiman]

LGTM

[rjl493456442]

I think we can simplify it a bit.

diff --git a/cmd/utils/cmd.go b/cmd/utils/cmd.go
index e1bafc53c3..37b6d222a5 100644
--- a/cmd/utils/cmd.go
+++ b/cmd/utils/cmd.go
@@ -211,8 +211,14 @@ func ImportChain(chain *core.BlockChain, fn string) error {
                        log.Info("Skipping batch as all blocks present", "batch", batch, "first", blocks[0].Hash(), "last", blocks[i-1].Hash())
                        continue
                }
-               if _, err := chain.InsertChain(missing); err != nil {
-                       return fmt.Errorf("invalid block %d: %v", n, err)
+               if failIndex, err := chain.InsertChain(missing); err != nil {
+                       var failNumber uint64
+                       if failIndex == -1 {
+                               failNumber = missing[0].NumberU64()
+                       } else {
+                               failNumber = missing[failIndex].NumberU64()
+                       }
+                       return fmt.Errorf("invalid block %d: %v", failNumber, err)
                }
        }

[holiman]

I'd change @rjl493456442 's condition-check into

+                       if failIndex > 0 && failIndex < len(missing) {
+                               failNumber = missing[failIndex].NumberU64()
+                       } else {
+                               failNumber = missing[0].NumberU64()
+                       }

[mohammadfarari1360]

Hello dear

[rjl493456442]

Good to go!