Skip to content

Conversation

@rymnc
Copy link
Member

@rymnc rymnc commented Apr 21, 2025

[Link to related issue(s) here, if any]

  • none

[Short description of the changes.]

  • we zero out the dirty memory when reallocating the heap. this is only detectable after a reset.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
    • we need to do this
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@rymnc rymnc self-assigned this Apr 21, 2025
@rymnc rymnc changed the title fix(memory_instance): zero out heap memory when reallocating fix(memory_instance): zero out heap memory when reallocating after reset Apr 21, 2025
@rymnc rymnc force-pushed the fix/patch-vm-memory-with-test branch from 49161d4 to 08f5902 Compare April 21, 2025 17:24
@rymnc rymnc force-pushed the fix/patch-vm-memory-with-test branch from cf064fe to b03c9b2 Compare April 21, 2025 18:06
Comment on lines 329 to 335
macro_rules! generate_memory_instance_grow_by_heap_tests {
($($size:expr),*) => {
$(
generate_memory_instance_grow_by_heap_test!($size);
)*
};
}
Copy link
Member

@MitchTurner MitchTurner Apr 21, 2025

Choose a reason for hiding this comment

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

nit: Can we name these macros:
generate_tests__memory_instance__grow_heap_by_after_reset__does_not_retain_dirty_memory_size
and
generate_test__memory_instance__grow_heap_by_after_reset__does_not_retain_dirty_memory_size
or something that makes it abundantly clear that these macros are generating tests. It's just not the expected format as a reader.

I wish there was a way to do this with prop_test, but you need to know the sizes at compile time, so I don't think that's supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish there was a way to do this with prop_test, but you need to know the sizes at compile time, so I don't think that's supported

exactly why I went with this approach

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in 868938d

@rymnc rymnc requested a review from MitchTurner April 21, 2025 22:30
@rymnc rymnc merged commit 9c97c2b into release/v0.59.3 Apr 21, 2025
40 checks passed
@rymnc rymnc deleted the fix/patch-vm-memory-with-test branch April 21, 2025 22:45
rymnc added a commit to FuelLabs/fuel-core that referenced this pull request Apr 22, 2025
…e executions (#2963)

## Linked Issues/PRs
<!-- List of related issues/PRs -->
- vm pr: FuelLabs/fuel-vm#941

## Description
<!-- List of detailed changes -->
adds a test with 2 predicates in one transaction, one that writes to the
heap and one that reallocates and reads from it. it should be zeroed
out.

## Checklist
- [ ] Breaking changes are clearly marked as such in the PR description
and changelog
- [ ] New behavior is reflected in tests
- [ ] [The specification](https://github.com/FuelLabs/fuel-specs/)
matches the implemented behavior (link update PR if changes are needed)

### Before requesting review
- [ ] I have reviewed the code myself
- [ ] I have created follow-up issues caused by this PR and linked them
here

### After merging, notify other teams

[Add or remove entries as needed]

- [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/)
- [ ] [Sway compiler](https://github.com/FuelLabs/sway/)
- [ ] [Platform
documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+)
(for out-of-organization contributors, the person merging the PR will do
this)
- [ ] Someone else?

Co-authored-by: xgreenx <[email protected]>
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.

4 participants