Merge pull request #39 from input-output-hk/rotate

Tree checks and `toRemove` fixes

Author: kushti

src/main/scala/scorex/crypto/authds/avltree/batch/AuthenticatedTreeOps.scala

@@ -1,9 +1,8 @@
 package scorex.crypto.authds.avltree.batch
 
 import scorex.crypto.authds.{Balance, _}
-import scorex.crypto.encode.Base16
 import scorex.crypto.hash.Digest
-import scorex.utils.{ByteArray, ScryptoLogging}
+import scorex.utils.ByteArray
 
 import scala.collection.mutable.ArrayBuffer
 import scala.util.{Failure, Success, Try}
@@ -13,7 +12,7 @@ import scala.util.{Failure, Success, Try}
   * Code common to the prover and verifier of https://eprint.iacr.org/2016/994
   * (see Appendix B, "Our Algorithms")
   */
-trait AuthenticatedTreeOps[D <: Digest] extends BatchProofConstants with ScryptoLogging {
+trait AuthenticatedTreeOps[D <: Digest] extends BatchProofConstants with ToStringHelper {
 
   type ChangeHappened = Boolean
   type HeightIncreased = Boolean
@@ -39,17 +38,17 @@ trait AuthenticatedTreeOps[D <: Digest] extends BatchProofConstants with Scrypto
   protected def onNodeVisit(n: Node[D], operation: Operation, isRotate: Boolean = false): Unit = {
     n match {
       case p: ProverNodes[D] if collectChangedNodes && !n.visited && !p.isNew =>
-          if (isRotate) {
-            // during rotate operation node may stay in the tree in a different position
-            changedNodesBufferToCheck += p
-          } else if (operation.isInstanceOf[Insert] || operation.isInstanceOf[Remove]
-            || operation.isInstanceOf[InsertOrUpdate]) {
-            // during non-rotate insert and remove operations nodes on the path should not be presented in a new tree
-            changedNodesBuffer += p
-          } else if (!operation.isInstanceOf[Lookup]) {
-            // during other non-lookup operations we don't know, whether node will stay in thee tree or not
-            changedNodesBufferToCheck += p
-          }
+        if (isRotate) {
+          // during rotate operation node may stay in the tree in a different position
+          changedNodesBufferToCheck += p
+        } else if (operation.isInstanceOf[Insert] || operation.isInstanceOf[Remove]
+          || operation.isInstanceOf[InsertOrUpdate]) {
+          // during non-rotate insert and remove operations nodes on the path should not be presented in a new tree
+          changedNodesBuffer += p
+        } else if (!operation.isInstanceOf[Lookup]) {
+          // during other non-lookup operations we don't know, whether node will stay in thee tree or not
+          changedNodesBufferToCheck += p
+        }
       case _ =>
     }
     n.visited = true
@@ -382,7 +381,7 @@ trait AuthenticatedTreeOps[D <: Digest] extends BatchProofConstants with Scrypto
             if (rightChild.balance < 0) {
               // double left rotate
               // I know rightChild.left is not a leaf, because rightChild has a higher subtree on the left
-              onNodeVisit(rightChild.left, operation)
+              onNodeVisit(rightChild.left, operation, isRotate = true)
               (doubleLeftRotate(newRoot, newLeft, rightChild), true)
             } else {
               // single left rotate
@@ -409,7 +408,7 @@ trait AuthenticatedTreeOps[D <: Digest] extends BatchProofConstants with Scrypto
             if (leftChild.balance > 0) {
               // double right rotate
               // I know leftChild.right is not a leaf, because leftChild has a higher subtree on the right
-              onNodeVisit(leftChild.right, operation)
+              onNodeVisit(leftChild.right, operation, isRotate = true)
               (doubleRightRotate(r, leftChild, newRight), true)
             } else {
               // single right rotate

src/test/scala/scorex/crypto/authds/avltree/batch/BatchTestingHelpers.scala

@@ -33,7 +33,11 @@ trait BatchTestingHelpers extends ToStringHelper with Matchers {
     */
   def checkTree(prover: BatchAVLProver[D, HF], oldTop: ProverNodes[D], removedNodes: Seq[ProverNodes[D]]): Unit = {
     // check that there are no nodes in removedNodes, that are still in the tree
-    removedNodes.foreach(r => prover.contains(r) shouldBe false)
+    removedNodes.foreach{r =>
+      if(prover.contains(r)) {
+        throw new Error(s"Node $r is marked to remove while still in the tree")
+      }
+    }
 
     var removed = 0
 

src/test/scala/scorex/crypto/authds/avltree/batch/BatchingPlayground.scala

@@ -38,9 +38,9 @@ object BatchingPlayground extends App with BatchTestingHelpers with Matchers {
   //removedNodesBenchmark
   removedNodesBenchmark()
 
-  def removedNodesBenchmark(startTreeSize: Int = 1000000,
-                            toRemoveSize: Int = 3000,
-                            toInsertSize: Int = 3000): Unit = {
+  def removedNodesBenchmark(startTreeSize: Int = 10000,
+                            toRemoveSize: Int = 2000,
+                            toInsertSize: Int = 2000): Unit = {
     val iterations = 50
     var toRemoveTotal: Long = 0
     var proofGenerationTotal: Long = 0
@@ -73,7 +73,7 @@ object BatchingPlayground extends App with BatchTestingHelpers with Matchers {
       }
 
       nonModifyingProof shouldEqual proofBytes
-//      checkTree(prover, oldTop, removedNodes)
+      checkTree(prover, oldTop, removedNodes)
       toRemoveTotal += removedNodesTime
       proofGenerationTotal += proofGenerationTime
       performOperationTotal += performOperationsTime