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

bugfix: broken eop memory compatibility check #4851

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Jan 15, 2025

The eop memory compatibility check was slightly broken and would try to compare memory fields with different tags.

Consider:

v0.mo

persistent actor {

  var four : [var (Nat, Text)] = [var];
  var zero = 0;

}

v1.mo

persistent actor {

  var three : [var (Nat, Text)] = [var];
  var zero = 0;

}

this upgrade should be allowed because four is dropped and three is added, while zero is preserved, but eop would fail with memory compatibility error.

The search for the next tag in the subtype would assume that the next tag found was less or equal when, in fact, it could be a higher tag in the case the field is absent on the left, which is allowed for memory compatibility.

Needs careful review.

Maybe it would be safer to adjust the code to have two cases, one for main_actor, and another for !main_actor.

@crusso crusso marked this pull request as ready for review January 16, 2025 12:32
@crusso crusso requested a review from a team as a code owner January 16, 2025 12:32
@crusso crusso changed the title bug: bug repro for dropping a field with eop bugfix: broken eop memory compatibility check Jan 16, 2025
@crusso crusso requested review from ggreif and luc-blaeser January 16, 2025 12:51
Copy link

github-actions bot commented Jan 16, 2025

Comparing from cef9238 to 6a153ab:
The produced WebAssembly code seems to be completely unchanged.

Copy link
Contributor

@luc-blaeser luc-blaeser left a comment

Choose a reason for hiding this comment

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

Thanks a lot for having found this bug and for this fix!

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