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

feat(levm): opcodes revert invalid selfdestruct #946

Merged
merged 61 commits into from
Nov 4, 2024

Conversation

JereSalo
Copy link
Contributor

@JereSalo JereSalo commented Oct 23, 2024

Motivation

Implement opcodes

This PR is going to be resumed when Db changes are merged to main.

Description

  • Opcodes Revert, Invalid and Selfdestruct implementation

Caveat:

Closes #535
Closes #536
Closes #537

@JereSalo JereSalo added the levm Lambda EVM implementation label Oct 23, 2024
@JereSalo JereSalo self-assigned this Oct 23, 2024
@JereSalo JereSalo changed the title Levm/feat/revert invalid selfdestruct feat(levm): opcodes revert invalid selfdestruct Oct 23, 2024
Copy link
Contributor

@ilitteri ilitteri left a comment

Choose a reason for hiding this comment

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

LGTM, left some suggestions

let target_account = match self.db.accounts.get_mut(&target_address) {
Some(acc) => acc,
None => {
// I'm considering that if address is not in the database, it means that it is an empty account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a method account_is_empty or similar to the db implementation that returns a boolean whether with a given target address self.db.accounts.get_mut(&target_address) .is_none() or !.is_none().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!
I did something like:

pub fn account_is_empty(&self, address: &Address) -> bool { 
    !self.accounts.contains_key(address)
}

Comment on lines 349 to 350
self.db.accounts.insert(target_address, Account::default());
self.db.accounts.get_mut(&target_address).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

You can insert the account with the balance directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

crates/vm/levm/src/vm.rs Outdated Show resolved Hide resolved
Comment on lines 327 to 337
let current_account_balance = self
.db
.accounts
.get(&current_call_frame.to)
.unwrap()
.balance;
self.db
.accounts
.get_mut(&current_call_frame.to)
.unwrap()
.balance = U256::zero();
Copy link
Contributor

@ilitteri ilitteri Oct 28, 2024

Choose a reason for hiding this comment

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

Handle these unwraps

Copy link
Contributor Author

@JereSalo JereSalo Oct 29, 2024

Choose a reason for hiding this comment

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

In the new PR that implements Database changes the db works differently and if an account "doesn't exist" then it will return an empty account so there will be no need to handle this kind of errors. Maybe after merging that one to main I can make changes to this PR so that it fits the new database implementation.
Also, I will have to access Cache first, and if data is not cached then I will make a request to the database. That will also let me implement gas cost for cold and warm addresses.

Account::default().with_balance(current_account_balance),
);
} else {
let target_account = self.db.accounts.get_mut(&target_address).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor

@ilitteri ilitteri left a comment

Choose a reason for hiding this comment

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

The PR is looking good. Left some comments.

@JereSalo JereSalo marked this pull request as draft October 29, 2024 21:42
@JereSalo JereSalo marked this pull request as ready for review November 1, 2024 22:08
@ilitteri ilitteri added this pull request to the merge queue Nov 4, 2024
Merged via the queue into main with commit 5ded681 Nov 4, 2024
17 checks passed
@ilitteri ilitteri deleted the levm/feat/revert_invalid_selfdestruct branch November 4, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
levm Lambda EVM implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opcode: SELFDESTRUCT Opcode: INVALID Opcode: REVERT
2 participants