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

More tests related to ErgoTree versioning #953

Merged
merged 5 commits into from
Apr 3, 2024
Merged

Conversation

aslesarenko
Copy link
Member

@aslesarenko aslesarenko commented Mar 3, 2024

In this PR:

  • added property("Pay2SHAddress.script should create ErgoTree v0")
  • added property("using default VersionContext still creates ErgoTree v0")
  • Rollback changes in Pay2SHAddress behaviour introduced in v5.0.14-RC

@aslesarenko aslesarenko changed the title v5.0.14-RC: More tests related to ErgoTree versioning Mar 3, 2024
@aslesarenko aslesarenko marked this pull request as ready for review March 3, 2024 12:00
@aslesarenko aslesarenko requested a review from kushti March 3, 2024 12:00
Copy link
Member

@kushti kushti left a comment

Choose a reason for hiding this comment

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

Still, the issue with embedded script version is not fixed. With ongoing efforts to update compilers around to produce v1 trees, and 6.0 development going on, this will lead to disaster with users having money locked in boxes with non-deserializeable trees.

How do you plan to fix the issue?

@aslesarenko
Copy link
Member Author

aslesarenko commented Mar 31, 2024

Still, the issue with embedded script version is not fixed.

@kushti , embedded where? I rolled back changes in the P2SH, so they will be done (once decided how) in a separate PR.
I also rolled back changes in VersionContext._defaultContext to make the target v5.0.14-RC pure refactoring without changes in behaviour.

I suggest to merge this PR, and then do another round of review for #943, where all the version related issues will be fixed with merging this PR.

@kushti
Copy link
Member

kushti commented Apr 1, 2024

@aslesarenko the term embedded was already used in this context https://discord.com/channels/668903786361651200/669989266478202917/1214289447877615776

I can accept this PR but 5.0.14 will be blocked before issues mentioned by you in https://discord.com/channels/668903786361651200/669989266478202917/1214353034499330048 resolved

So what is your plan to fix the issue? (have to ask this again)

@aslesarenko
Copy link
Member Author

aslesarenko commented Apr 1, 2024

So what is your plan to fix the issue?

@kushti, The Plan:

  1. Merge this PR to rollback any version related changes in v5.0.14 (those changes shouldn't be there in a first place)
  2. Finalize, merge and release v5.0.14 without any version related changes (it will be pure refactoring PR)
  3. List all the version related changes along with the analysis of related implications for dApps.
  4. Decide (maybe via voting) on which changes will be implemented
  5. Implement the accepted changes

@kushti
Copy link
Member

kushti commented Apr 2, 2024

@aslesarenko shouldn't P2SH be disabled at this point while a solution to fix their issues is not proposed? Preparing for it in the node ergoplatform/ergo#2140

@aslesarenko
Copy link
Member Author

aslesarenko commented Apr 2, 2024

@aslesarenko shouldn't P2SH be disabled at this point while a solution to fix their issues is not proposed? Preparing for it in the node ergoplatform/ergo#2140

@kushti I'm not sure about disabling. We cannot prevent users to create p2sh-like addresses.
If we decide to embed version into a new p2sh encoding, then we can do it via new address type code (the current p2sh will become legacy p2sh encoding).

A suggest to merge this PR and continue discussion in a new issue I created #959

@aslesarenko aslesarenko merged commit 155a08d into v5.0.14-RC Apr 3, 2024
4 checks passed
@aslesarenko aslesarenko deleted the p2sh-version branch May 1, 2024 15:27
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