-
Notifications
You must be signed in to change notification settings - Fork 235
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
fix: upgrade ckb-vm to fix snapshot behavior #3177
Conversation
c5f94ab
to
cde0fcd
Compare
Suggest to add unit test to cover this PR.
…On Mon, Nov 15, 2021, 18:04 Mohanson ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In script/src/syscalls/load_cell_data.rs
<#3177 (comment)>:
> @@ -104,13 +107,15 @@ impl<'a, DL: CellDataProvider + 'a> LoadCellData<'a, DL> {
.load_cell_data(cell)
.ok_or(VMError::Unexpected)?
.slice((content_offset as usize)..(content_end as usize));
- machine.memory_mut().init_pages(
- addr,
- memory_size,
- FLAG_DIRTY | FLAG_EXECUTABLE | FLAG_FREEZED,
- Some(data),
- 0,
- )?;
+ // To be consistent with the mainnet, add this flag after hardfork
I made a mistake before. Any write operation to the ckb-vm memory will be
marked as dirty, and the correct logic should be:
machine.memory_mut().init_pages(
addr,
memory_size,
FLAG_EXECUTABLE | FLAG_FREEZED,
Some(data),
0,
)?;
if !self.dirty_flag_enable {
let mut current_addr = addr;
while current_addr < addr + memory_size {
let page = current_addr / RISCV_PAGESIZE as u64;
machine.memory_mut().clear_flag(page, FLAG_DIRTY)?;
current_addr += RISCV_PAGESIZE as u64;
}
}
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#3177 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACGHTTDGHO4O5A25UWTWLUMDLMVANCNFSM5IBHDRYA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
cde0fcd
to
718306c
Compare
I pushed #3178 to add more tests, I think that PR should be merged before this PR. Notice#3178 doesn't fix the snapshot behavior. How to enable the test for resuming from snapshot?
|
2883611
to
189578d
Compare
189578d
to
f412f28
Compare
bors r= yangby-cryptape,mohanson,zhangsoledad |
What problem does this PR solve?
chunk run with the snapshot unable to support the binary replacement behavior of exec, which will cause abnormal behavior after resume.
The upgraded VM supports backing up the current code(executable pages) to the snapshot to fix the above problems. nervosnetwork/ckb-vm#218
and #3176 should keep its behavior until hardfork activated
Check List
Tests
Release note