Minimise modules and publish them separately (part 1) #912

aslesarenko · 2023-08-21T21:18:36Z

Work towards #902, please read for motivation background.
In this PR:

core-lib and graph-ir modules merged into core and sc respectively
some package renaming
code moved to better packages
code cleanup

… special.sigma -> sigma

# Conflicts: # sdk/js/src/main/scala/org/ergoplatform/sdk/js/Header.scala # sdk/js/src/main/scala/org/ergoplatform/sdk/js/Isos.scala # sdk/js/src/main/scala/org/ergoplatform/sdk/js/PreHeader.scala # sdk/shared/src/test/scala/org/ergoplatform/sdk/multisig/SigningSpec.scala

kushti · 2023-09-10T21:48:45Z

sdk/shared/src/test/scala/org/ergoplatform/sdk/multisig/SigningSpec.scala

Empty file added

kushti · 2023-09-10T21:54:18Z

interpreter/shared/src/main/scala/sigmastate/eval/CostingDataContext.scala


  /** Override default logging which outputs stack trace to the console. */
  override protected def logError(t: Throwable): Unit = {}
 }
+object AvlTreeVerifier {
+  def apply(tree: AvlTree, proof: Coll[Byte]): AvlTreeVerifier = {


Better to make this method private (so apply is not a best name), and do a comment that digest is not updated!

turned constructor of AvlTreeVerifier into private, added ScalaDoc for apply method, which serves as constructor (this is standard pattern in Scala, not sure what you mean by "not the best name").
It is created simply as AvlTreeVerifier(tree, proof).

I suppose that apply() is used around Scala ecosystem to create an object in consistent state. Which is not the case here - the state would be consistent only for lookup proofs.

createVerifier was a dirty hack already, but now you are going further with it by endorsing a method used everywhere to create an object with inconsistent state in most cases.

Instead of hiding dangerous behavior silently under apply(), you need to limit access to dirty code and provide a warning(better, via naming also), or do a builder.

Please explain how createVerifier is a dirty hack?
Give an example of when "object with inconsistent state mostly" is created?
I don't understand your hints, please clarify.

Note, for each AvlTree operation, a new AvlTreeVerifier is created and not reused across operations. The role of createVerifier method is to do costing of creation itself.

When you are calling def apply(tree: AvlTree, proof: Coll[Byte]): AvlTreeVerifier, you are expecting from resulting tree to have digest AFTER proof application (otherwise, proof argument does not make sense), which is not the case here, as resulting tree has digest BEFORE the proof. Thus the state is inconsistent and everywhere updateDigest is called after apply to restore consistency. That is what I am referring as a dirty hack to.

AvlTree.digest is always connected with the proof in such a way, that when the verifier reconstructs the tree from the proof, the root node should have the avlTree.digest as the label.
This is checked in the Verifier constructor.
With this in mind I don't see any hacks you mentioned.

AvlTree.digest is always connected with the proof in such a way, that when the verifier reconstructs the tree from the proof, the root node should have the avlTree.digest as the label. This is checked in the Verifier constructor. With this in mind I don't see any hacks you mentioned.

What check in the constructor are you talking about ?

What check in the constructor are you talking about ?

https://github.com/input-output-hk/scrypto/blob/d925f9357d3a3f2ee2b3f1be5d7b82f1eb8f8b54/shared/src/main/scala/scorex/crypto/authds/avltree/batch/BatchAVLVerifier.scala#L224

you are expecting from resulting tree to have digest AFTER proof application

as far as I understand, after Verifier construction, the digest indeed corespoinds to the digest of the tree reconstructed from the proof. At least what I can learn from require(startingDigest startsWith root.label) in the computation of reconstructedTree field (which is done as part of constructor see topNode)

resulting tree has digest BEFORE the proof

this is wrong

Starting digest is indeed corresponding to the proof's tree. However, my point was not about that, rather, about e relation between tree and proof.

I think as a quick workaround you can call it treeBeforeProof, and update scaladoc accordingly.

But, it is not tree before proof, my point is that it is actually the tree AFTER proof.
Clarified this point in the ScalaDoc.

kushti · 2023-09-10T21:55:23Z

interpreter/shared/src/main/scala/sigmastate/eval/CostingDataContext.scala

-import scalan.RType
-import scalan.util.Extensions.BigIntegerOps
+import sigma.data.OverloadHack.Overloaded1
+import sigma.data.{CollOverArrayBuilder, RType}


CollOverArrayBuilder import not used

kushti · 2023-09-10T21:56:41Z

interpreter/shared/src/main/scala/sigmastate/eval/CostingDataContext.scala

@@ -494,7 +492,7 @@ class CostingSigmaDslBuilder extends SigmaDslBuilder { dsl =>
  implicit val validationSettings: SigmaValidationSettings = ValidationRules.currentSettings

  // manual fix
-  override val Colls: CollBuilder = new CollOverArrayBuilder
+  override val Colls: CollBuilder = sigma.Colls


comment is confusing, what does "manual fix" mean?

removed here and in other places

kushti · 2023-09-10T21:59:02Z

interpreter/shared/src/main/scala/sigmastate/eval/Evaluation.scala

    case _: Box => BoxRType

-    case _: AvlTreeData => AvlTreeDataRType
+    case _: AvlTreeData => AvlTreeDataRType // TODO remove this RType


Please describe in the comment why to remove

kushti · 2023-09-10T22:31:57Z

interpreter/shared/src/main/scala/sigmastate/sigmastate.scala

 package object sigmastate {
  import CheckingSigmaBuilder._

+  /** RType descriptors for predefined types used in AOTC-based interpreter. */
+  def rtypeToClassTag = ???


unused method with meaningless definition

added clarifying scaladocs

kushti · 2023-09-10T22:35:14Z

core/shared/src/main/scala/sigma/kiama/rewriting/Rewriter.scala

 package rewriting

-import scalan.reflection.{Platform, RClass, RConstructor}
+import sigma.reflection.{Platform, RClass, RConstructor}
+//import sigma.kiama.==>


commented out code

kushti · 2023-09-12T08:37:15Z

interpreter/shared/src/main/scala/sigmastate/eval/CostingDataContext.scala


  /** Override default logging which outputs stack trace to the console. */
  override protected def logError(t: Throwable): Unit = {}
 }
+object AvlTreeVerifier {
+  def apply(tree: AvlTree, proof: Coll[Byte]): AvlTreeVerifier = {


I suppose that apply() is used around Scala ecosystem to create an object in consistent state. Which is not the case here - the state would be consistent only for lookup proofs.

createVerifier was a dirty hack already, but now you are going further with it by endorsing a method used everywhere to create an object with inconsistent state in most cases.

Instead of hiding dangerous behavior silently under apply(), you need to limit access to dirty code and provide a warning(better, via naming also), or do a builder.

kushti

Please rename "sc" to "smartContracts", as sc is not descriptive

aslesarenko · 2023-09-12T14:36:08Z

Please rename "sc" to "smartContracts"

Please rename "sc" to "smartContracts", as sc is not descriptive

Agree, but I plan to rename it into compiler to reflect what is inside it.

minimize-modules: core-lib module removed

913baec

aslesarenko marked this pull request as draft August 21, 2023 21:18

aslesarenko added 26 commits August 22, 2023 00:04

minimize-modules: graphir module removed

83c38cd

minimize-modules: move code around

cbbd0f3

minimize-modules: move downcastCollType to CollType

0e979b6

minimize-modules: rename sigmastate -> sigma in common

d9638b4

minimize-modules: replaced special.collection -> sigma.collection and…

9d4d53c

… special.sigma -> sigma

minimize-modules: collections moved to sigma package

3fce4d7

minimize-modules: use fully qualified java.util.

8ed3f81

minimize-modules: TupleType moved to separate file

55f1c1b

minimize-modules: reflection moved to sigma

7f11c9a

minimize-modules: util moved to sigma

a756e43

minimize-modules: move from scalan to sigma.core

3bbc1e4

minimize-modules: fix packages

6e7ab0f

minimize-modules: move from sigmastate.basics -> crypto

b4e344e

minimize-modules: move ExactIntegral, ExactOrdering

dec171b

minimize-modules: CollType moved

f1d0fc7

minimize-modules: RType descriptors moved to sigma package

87682bb

minimize-modules: CollType and TupleType moved to RType.scala

86c83e0

minimize-modules: CollsTests, CollGens moved to sigma package

c5d4022

minimize-modules: renamed package sigma.core -> sigma.data

ca26fb4

minimize-modules: move CollsOverArrays.scala to sigma.data

ec37de7

minimize-modules: introduced sigma.Colls

fdeda45

minimize-modules: Removed CoreLibReflection.scala

9676d88

minimize-modules: common module renamed to core

c01113a

minimize-modules: fix file tests + renamed ReflectionData

7d57628

minimize-modules: AvlTreeVerifier moved out of core

3eba2d1

minimize-modules: RType implicits moved from SType

71e67c4

aslesarenko marked this pull request as ready for review August 30, 2023 13:10

minimize-modules: fixes after merge

008d909

aslesarenko requested a review from kushti August 31, 2023 08:43

aslesarenko changed the title ~~Minimise modules and publish them separately~~ Minimise modules and publish them separately (part 1) Aug 31, 2023

aslesarenko changed the base branch from todo-v5.x to v5.0.12-RC September 5, 2023 08:36

kushti requested changes Sep 10, 2023

View reviewed changes

minimize-modules: addressed review comments

a13a03c

aslesarenko requested a review from kushti September 11, 2023 10:12

kushti requested changes Sep 12, 2023

View reviewed changes

aslesarenko requested a review from kushti September 12, 2023 08:57

kushti approved these changes Sep 12, 2023

View reviewed changes

aslesarenko merged commit f9c067d into v5.0.12-RC Sep 12, 2023
3 of 4 checks passed

aslesarenko mentioned this pull request Sep 26, 2023

Release Candidate v5.0.12 #921

Merged

aslesarenko deleted the minimize-modules branch May 1, 2024 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimise modules and publish them separately (part 1) #912

Minimise modules and publish them separately (part 1) #912

aslesarenko commented Aug 21, 2023 •

edited

Loading

kushti Sep 10, 2023

aslesarenko Sep 11, 2023

kushti Sep 10, 2023

aslesarenko Sep 11, 2023

kushti Sep 12, 2023 •

edited

Loading

aslesarenko Sep 12, 2023 •

edited

Loading

kushti Sep 12, 2023 •

edited

Loading

aslesarenko Sep 12, 2023

kushti Sep 12, 2023

aslesarenko Sep 12, 2023

kushti Sep 12, 2023

aslesarenko Sep 12, 2023

kushti Sep 10, 2023

aslesarenko Sep 11, 2023

kushti Sep 10, 2023

aslesarenko Sep 11, 2023

kushti Sep 10, 2023

aslesarenko Sep 11, 2023

kushti Sep 10, 2023

aslesarenko Sep 11, 2023

kushti Sep 10, 2023

aslesarenko Sep 11, 2023

kushti Sep 12, 2023 •

edited

Loading

kushti left a comment

aslesarenko commented Sep 12, 2023

Minimise modules and publish them separately (part 1) #912

Minimise modules and publish them separately (part 1) #912

Conversation

aslesarenko commented Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kushti Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

aslesarenko Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

kushti Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kushti Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

kushti left a comment

Choose a reason for hiding this comment

aslesarenko commented Sep 12, 2023

aslesarenko commented Aug 21, 2023 •

edited

Loading

kushti Sep 12, 2023 •

edited

Loading

aslesarenko Sep 12, 2023 •

edited

Loading

kushti Sep 12, 2023 •

edited

Loading

kushti Sep 12, 2023 •

edited

Loading