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

set_refund_for_current_tx #2361

Merged
merged 2 commits into from
Jul 2, 2024
Merged

set_refund_for_current_tx #2361

merged 2 commits into from
Jul 2, 2024

Conversation

montekki
Copy link
Contributor

@montekki montekki commented Jul 1, 2024

What ❔

Fixes refund-related bugs in the glue code for the new VM:

  • Computes pubdata_published as a difference with the previous transaction
  • Sets refund for the bootloader state

Copy link
Contributor

@slowli slowli left a comment

Choose a reason for hiding this comment

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

Locally, the changes seem to allow the load test set up to complete, and the load test itself now runs fine 🎉 There are still issues with batch executor unit tests though, like extra storage logs and L2-to-L1 logs being produced by the new VM.

core/lib/multivm/src/versions/shadow.rs Show resolved Hide resolved
core/lib/multivm/src/versions/shadow.rs Outdated Show resolved Hide resolved
@@ -145,14 +146,15 @@ impl<S: ReadStorage> Vm<S> {
.as_u64();

let pubdata_published = self.inner.world_diff.pubdata.0 as u32;
//pubdata_published -= pubdata_before;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove if this code isn't needed. BTW, how would pubdata computations work if the entire batch is executed (i.e., execution_mode == VmExecutionMode::Batch)? I'd think that pubdata_before needs to be updated somewhere in run() (in TxHasEnded hook handler?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to be updated here on the spot, as for now i do not have a better theory about where it should be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's correct (it would probably lead to the previous issue of pubdata_published for a transaction including pubdata produced by the bootloader / system contracts after executing the previous transaction), but let's fix this sequentially.

@montekki montekki marked this pull request as ready for review July 2, 2024 09:58
@slowli slowli merged commit 8468cd3 into jms-vm2+aov Jul 2, 2024
21 of 49 checks passed
@slowli slowli deleted the jms-vm2+aov+fvs branch July 2, 2024 12:22
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