Merge pull request #958 from input-output-hk/fix-get-checkpoint-rpc

Return only the child of the checkpoint block

Author: AnastasiiaL

src/main/scala/io/iohk/ethereum/jsonrpc/CheckpointingService.scala

@@ -21,8 +21,12 @@ class CheckpointingService(
 
   def getLatestBlock(req: GetLatestBlockRequest): ServiceResponse[GetLatestBlockResponse] = {
     lazy val bestBlockNum = blockchain.getBestBlockNumber()
-    lazy val blockToReturnNum = bestBlockNum - bestBlockNum % req.checkpointingInterval
-    lazy val isValidParent = req.parentCheckpoint.forall(blockchain.getBlockHeaderByHash(_).isDefined)
+    lazy val blockToReturnNum =
+      if (req.checkpointingInterval != 0)
+        bestBlockNum - bestBlockNum % req.checkpointingInterval
+      else bestBlockNum
+    lazy val isValidParent =
+      req.parentCheckpoint.forall(blockchain.getBlockHeaderByHash(_).exists(_.number < blockToReturnNum))
 
     Task {
       blockchain.getBlockByNumber(blockToReturnNum)
@@ -31,7 +35,7 @@ class CheckpointingService(
         Task.now(Right(GetLatestBlockResponse(Some(BlockInfo(b.hash, b.number)))))
 
       case Some(_) =>
-        log.debug("Parent checkpoint is not found in a local blockchain")
+        log.debug("No checkpoint candidate found for a specified parent")
         Task.now(Right(GetLatestBlockResponse(None)))
 
       case None =>
@@ -59,10 +63,10 @@ class CheckpointingService(
 }
 
 object CheckpointingService {
-  case class GetLatestBlockRequest(checkpointingInterval: Int, parentCheckpoint: Option[ByteString])
-  case class GetLatestBlockResponse(block: Option[BlockInfo])
-  case class BlockInfo(hash: ByteString, number: BigInt)
+  final case class GetLatestBlockRequest(checkpointingInterval: Int, parentCheckpoint: Option[ByteString])
+  final case class GetLatestBlockResponse(block: Option[BlockInfo])
+  final case class BlockInfo(hash: ByteString, number: BigInt)
 
-  case class PushCheckpointRequest(hash: ByteString, signatures: List[ECDSASignature])
-  case class PushCheckpointResponse()
+  final case class PushCheckpointRequest(hash: ByteString, signatures: List[ECDSASignature])
+  final case class PushCheckpointResponse()
 }

src/test/scala/io/iohk/ethereum/jsonrpc/CheckpointingServiceSpec.scala

@@ -57,7 +57,7 @@ class CheckpointingServiceSpec
       n <- Gen.choose(0, k - 1) // distance from best block to checkpointed block
     } yield (k, m, n)
 
-    val previousCheckpoint = Fixtures.Blocks.ValidBlock.block
+    val previousCheckpoint = Fixtures.Blocks.Block3125369.block
     val hash = previousCheckpoint.hash
 
     forAll(nums) { case (k, m, n) =>
@@ -70,14 +70,40 @@ class CheckpointingServiceSpec
       val expectedResponse = GetLatestBlockResponse(Some(BlockInfo(block.hash, block.number)))
 
       (blockchain.getBestBlockNumber _).expects().returning(bestBlockNum)
-      (blockchain.getBlockHeaderByHash _).expects(hash).returning(Some(previousCheckpoint.header))
+      (blockchain.getBlockHeaderByHash _).expects(hash).returning(Some(previousCheckpoint.header.copy(number = 0)))
       (blockchain.getBlockByNumber _).expects(checkpointedBlockNum).returning(Some(block))
       val result = service.getLatestBlock(request)
 
       result.runSyncUnsafe() shouldEqual Right(expectedResponse)
     }
   }
 
+  it should "not return a block that is at the same height as the passed parent checkpoint block" in new TestSetup {
+    val nums = for {
+      k <- Gen.choose[Int](1, 10) // checkpointing interval
+      m <- Gen.choose(0, 1000) // number of checkpoints in the chain
+      n <- Gen.choose(0, k - 1) // distance from best block to checkpointed block
+    } yield (k, m, n)
+
+    val previousCheckpoint = Fixtures.Blocks.ValidBlock.block
+    val hash = previousCheckpoint.hash
+
+    forAll(nums) { case (k, m, n) =>
+      val checkpointedBlockNum: BigInt = k * m
+      val bestBlockNum: BigInt = checkpointedBlockNum + n
+
+      val request = GetLatestBlockRequest(k, Some(hash))
+      val expectedResponse = GetLatestBlockResponse(None)
+
+      (blockchain.getBestBlockNumber _).expects().returning(bestBlockNum)
+      (blockchain.getBlockHeaderByHash _).expects(hash).returning(Some(previousCheckpoint.header.copy(number = bestBlockNum)))
+      (blockchain.getBlockByNumber _).expects(*).returning(Some(previousCheckpoint))
+      val result = service.getLatestBlock(request)
+
+      result.runSyncUnsafe() shouldEqual Right(expectedResponse)
+    }
+  }
+
   it should "return an empty response if the descendant is not a part of a local blockchain" in new TestSetup {
     val nums = for {
       k <- Gen.choose[Int](1, 10) // checkpointing interval