[6.0] Lazy default for Option.getOrElse and Coll.getOrElse #1008
Conversation
kushti
changed the title
| ValUse(1, SPair(SCollectionType(SInt), SPair(SInt, SInt))), | ||
| 2.toByte | ||
|
|
||
| if(!VersionContext.current.isV6SoftForkActivated) { |
There was a problem hiding this comment.
Protocol change like this can be tested using changeFeature (see other tests) in combination with newVersionedResults (also see code). In this case both old and new behaviour will be tested.
In principle this test case can be moved to LSV6.
There was a problem hiding this comment.
changedFeature is not enough here, you need for versioned verifyCases, as in 6.0 costing trace would be different in some cases due to difference in default evaluation (at the same time, it wouldnt provide higher cost, so the change is secure costing-wise).
Seems like *Feature machinery is overly complex, but still not enough to describe protocol changes, thus I would recommend to decide what it is actually testing (or should test), and then follow simpler approaches
There was a problem hiding this comment.
Please see other examples where changedFeature is used, there are test cases where cost trace depend on the version and you can pass different traces to the constructor of Expected.
The perceived complexity is due to learning curve.
There was a problem hiding this comment.
changedFeature is tied to V4/V5 switch, see checkExpected in ChangedFeature / checkVerify functions in SigmaDslTesting. So it cant be used in V6 testing without modifications it seems. So it is definitely not about learning curve, changedFeature is just not ready for V6
There was a problem hiding this comment.
@aslesarenko see #1024 for example
| existingFeature({ (x: Option[Long]) => x.getOrElse(1L) }, | ||
| "{ (x: Option[Long]) => x.getOrElse(1L) }", | ||
| FuncValue(Vector((1, SOption(SLong))), OptionGetOrElse(ValUse(1, SOption(SLong)), LongConstant(1L))))) | ||
| if (!VersionContext.current.isV6SoftForkActivated) { |
There was a problem hiding this comment.
Same as above, this if is not necessary when changeFeature + newVersionedResults is used.
|
|
||
| if (VersionContext.current.isV6SoftForkActivated) { | ||
| forAll { x: Option[Long] => | ||
| Seq(getOrElse).map(_.checkEquality(x)) |
There was a problem hiding this comment.
Check equality is not enough, as it doesn't involve all interpreter components.
There was a problem hiding this comment.
Please specify what is needed
There was a problem hiding this comment.
verifyCases will exercise all the components.
sc/shared/src/test/scala/sigmastate/utxo/BasicOpsSpecification.scala
Outdated
Show resolved
Hide resolved
|
@aslesarenko comments addressed, please make another pass |
|
@aslesarenko after all I've found some V6 related modifications in #995 , tweaked them further and made verifyCases tests. Please make another pass |
In this PR , lazy default evaluation for Option.getOrElse and Coll.getOrElse is implemented