Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.0.0] Header.checkPow method #968

Merged
merged 21 commits into from
Sep 2, 2024
Merged

[6.0.0] Header.checkPow method #968

merged 21 commits into from
Sep 2, 2024

Conversation

kushti
Copy link
Member

@kushti kushti commented Apr 27, 2024

In this PR, a new method added to check Ergo header's proof-of-work

{
     val h = CONTEXT.headers(0)
      h.checkPow
 }

@kushti kushti changed the base branch from develop to v6.0.0 April 27, 2024 18:18
@kushti kushti added this to the v6.0 milestone May 1, 2024
@kushti kushti changed the title [6.0] Header.checkPow method [6.0.0] Header.checkPow method May 1, 2024
@kushti kushti changed the base branch from v6.0.0 to i969 June 5, 2024 11:16
@kushti kushti requested a review from aslesarenko July 26, 2024 15:23
@kushti
Copy link
Member Author

kushti commented Aug 1, 2024

JS test was failing due to #1023 , fixed now

@@ -1313,6 +1313,9 @@ case class MethodCall(
addCost(MethodCall.costKind) // MethodCall overhead
method.costKind match {
case fixed: FixedCost =>
if (method.sinceVersion > 0 && E.context.currentErgoTreeVersion < method.sinceVersion) {
syntax.error(s"Method ${method.name} is not supported in tree version ${E.context.currentErgoTreeVersion}")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be the first code fragment versioned by tree version (rather than activatedVersion). Maybe use activatedVersion instead? dApps usually don't care about versions, and hence requiring a minimal tree version may require dApp upgrades.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible, however, there are some questions to resolve then :

  • NewFeature implementation by you was expecting this:
    override def isSupportedIn(vc: VersionContext): Boolean =
      vc.activatedVersion >= sinceVersion && vc.ergoTreeVersion >= sinceVersion

why ?

  • what does tree version mean at all ? How tree of version v+1 is different from tree version v?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rollback versioning by tree and changed NewFeature definition. The question about versioning semantics still remains.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "vc.activatedVersion >= sinceVersion && vc.ergoTreeVersion >= sinceVersion" was an intermediate version, which was fixed in the in my original more recent PRs.
The tree version is more about bytecode format, see for example switch from v0, to v1, there size has become required. However, there is also a relation between tree version and block version, s.t. treeVersion <= activatedScriptVersion.

@kushti
Copy link
Member Author

kushti commented Aug 12, 2024

@aslesarenko comments addressed, please make another pass

@kushti kushti requested a review from aslesarenko August 23, 2024 20:35
Copy link
Member

@aslesarenko aslesarenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, also see comment about tree version.

@kushti kushti changed the base branch from i969 to v6.0.0 September 2, 2024 17:13
@kushti kushti merged commit 2cd57e1 into v6.0.0 Sep 2, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants