Fix VM.GetBlockIDAtHeight (#595)

* Fix and optimize GetBlockIDAtHeight * Add test --------- Co-authored-by: Darioush Jalali <[email protected]>
ava-labs · Jul 8, 2024 · 7684836 · 7684836
1 parent 3549307
commit 7684836
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 11 deletions.
diff --git a/plugin/evm/vm.go b/plugin/evm/vm.go
@@ -1397,20 +1397,21 @@ func (vm *VM) VerifyHeightIndex(context.Context) error {
 	return nil
 }
 
-// GetBlockAtHeight returns the canonical block at [blkHeight].
-// If [blkHeight] is less than the height of the last accepted block, this will return
-// the block accepted at that height. Otherwise, it may return a blkID that has not yet
-// been accepted.
-// Note: the engine assumes that if a block is not found at [blkHeight], then
-// [database.ErrNotFound] will be returned. This indicates that the VM has state synced
-// and does not have all historical blocks available.
-func (vm *VM) GetBlockIDAtHeight(_ context.Context, blkHeight uint64) (ids.ID, error) {
-	ethBlock := vm.blockChain.GetBlockByNumber(blkHeight)
-	if ethBlock == nil {
+// GetBlockAtHeight returns the canonical block at [height].
+// Note: the engine assumes that if a block is not found at [height], then
+// [database.ErrNotFound] will be returned. This indicates that the VM has state
+// synced and does not have all historical blocks available.
+func (vm *VM) GetBlockIDAtHeight(_ context.Context, height uint64) (ids.ID, error) {
+	lastAcceptedBlock := vm.LastAcceptedBlock()
+	if lastAcceptedBlock.Height() < height {
 		return ids.ID{}, database.ErrNotFound
 	}
 
-	return ids.ID(ethBlock.Hash()), nil
+	hash := vm.blockChain.GetCanonicalHash(height)
+	if hash == (common.Hash{}) {
+		return ids.ID{}, database.ErrNotFound
+	}
+	return ids.ID(hash), nil
 }
 
 func (vm *VM) Version(context.Context) (string, error) {

diff --git a/plugin/evm/vm_test.go b/plugin/evm/vm_test.go
@@ -2021,6 +2021,10 @@ func TestNonCanonicalAccept(t *testing.T) {
 		t.Fatalf("Expected status of built block to be %s, but found %s", choices.Processing, status)
 	}
 
+	if _, err := vm1.GetBlockIDAtHeight(context.Background(), vm1BlkA.Height()); err != database.ErrNotFound {
+		t.Fatalf("Expected unaccepted block not to be indexed by height, but found %s", err)
+	}
+
 	if err := vm1.SetPreference(context.Background(), vm1BlkA.ID()); err != nil {
 		t.Fatal(err)
 	}
@@ -2035,16 +2039,29 @@ func TestNonCanonicalAccept(t *testing.T) {
 	if status := vm2BlkA.Status(); status != choices.Processing {
 		t.Fatalf("Expected status of block on VM2 to be %s, but found %s", choices.Processing, status)
 	}
+	if _, err := vm2.GetBlockIDAtHeight(context.Background(), vm2BlkA.Height()); err != database.ErrNotFound {
+		t.Fatalf("Expected unaccepted block not to be indexed by height, but found %s", err)
+	}
 	if err := vm2.SetPreference(context.Background(), vm2BlkA.ID()); err != nil {
 		t.Fatal(err)
 	}
 
 	if err := vm1BlkA.Accept(context.Background()); err != nil {
 		t.Fatalf("VM1 failed to accept block: %s", err)
 	}
+	if blkID, err := vm1.GetBlockIDAtHeight(context.Background(), vm1BlkA.Height()); err != nil {
+		t.Fatalf("Height lookuped failed on accepted block: %s", err)
+	} else if blkID != vm1BlkA.ID() {
+		t.Fatalf("Expected accepted block to be indexed by height, but found %s", blkID)
+	}
 	if err := vm2BlkA.Accept(context.Background()); err != nil {
 		t.Fatalf("VM2 failed to accept block: %s", err)
 	}
+	if blkID, err := vm2.GetBlockIDAtHeight(context.Background(), vm2BlkA.Height()); err != nil {
+		t.Fatalf("Height lookuped failed on accepted block: %s", err)
+	} else if blkID != vm2BlkA.ID() {
+		t.Fatalf("Expected accepted block to be indexed by height, but found %s", blkID)
+	}
 
 	newHead := <-newTxPoolHeadChan1
 	if newHead.Head.Hash() != common.Hash(vm1BlkA.ID()) {
@@ -2092,6 +2109,10 @@ func TestNonCanonicalAccept(t *testing.T) {
 		t.Fatalf("Expected status of built block to be %s, but found %s", choices.Processing, status)
 	}
 
+	if _, err := vm1.GetBlockIDAtHeight(context.Background(), vm1BlkB.Height()); err != database.ErrNotFound {
+		t.Fatalf("Expected unaccepted block not to be indexed by height, but found %s", err)
+	}
+
 	if err := vm1.SetPreference(context.Background(), vm1BlkB.ID()); err != nil {
 		t.Fatal(err)
 	}
@@ -2126,10 +2147,20 @@ func TestNonCanonicalAccept(t *testing.T) {
 		t.Fatalf("Block failed verification on VM1: %s", err)
 	}
 
+	if _, err := vm1.GetBlockIDAtHeight(context.Background(), vm1BlkC.Height()); err != database.ErrNotFound {
+		t.Fatalf("Expected unaccepted block not to be indexed by height, but found %s", err)
+	}
+
 	if err := vm1BlkC.Accept(context.Background()); err != nil {
 		t.Fatalf("VM1 failed to accept block: %s", err)
 	}
 
+	if blkID, err := vm1.GetBlockIDAtHeight(context.Background(), vm1BlkC.Height()); err != nil {
+		t.Fatalf("Height lookuped failed on accepted block: %s", err)
+	} else if blkID != vm1BlkC.ID() {
+		t.Fatalf("Expected accepted block to be indexed by height, but found %s", blkID)
+	}
+
 	blkCHash := vm1BlkC.(*chain.BlockWrapper).Block.(*Block).ethBlock.Hash()
 	if b := vm1.blockChain.GetBlockByNumber(blkBHeight); b.Hash() != blkCHash {
 		t.Fatalf("expected block at %d to have hash %s but got %s", blkBHeight, blkCHash.Hex(), b.Hash().Hex())