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

fix(levm): create to an existing address #1523

Merged
merged 21 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions crates/vm/levm/src/opcode_handlers/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,14 +528,22 @@ impl VM {
};

// 3. Account has nonce or code.
if self.get_account(new_address).has_code_or_nonce() {
let new_account = self.get_account(new_address);
if new_account.has_code_or_nonce() {
self.increment_account_nonce(deployer_address)?;
current_call_frame.stack.push(CREATE_DEPLOYMENT_FAIL)?;
return Ok(OpcodeSuccess::Continue);
}

// THIRD: Changes to the state
// 1. Creating contract.
let new_account = Account::new(value_in_wei_to_send, Bytes::new(), 1, Default::default());

// If the account already exists, we need to add the value to the current balance.
damiramirez marked this conversation as resolved.
Show resolved Hide resolved
let balance = value_in_wei_to_send
damiramirez marked this conversation as resolved.
Show resolved Hide resolved
.checked_add(new_account.info.balance)
.ok_or(VMError::BalanceOverflow)?;

let new_account = Account::new(balance, Bytes::new(), 1, Default::default());
cache::insert_account(&mut self.cache, new_address, new_account);

// 2. Increment sender's nonce.
Expand Down
53 changes: 42 additions & 11 deletions crates/vm/levm/src/vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,17 @@ impl VM {

default_touched_accounts.insert(new_contract_address);

let created_contract = Account::new(value, Bytes::new(), 1, HashMap::new());

cache::insert_account(&mut cache, new_contract_address, created_contract);
// Since we are in a CREATE transaction, we need to check if the address is already occupied.
// If it is, we should not continue with the transaction. We will handle the revert in the next step.
let new_account = db.get_account_info(new_contract_address);
let balance = value
.checked_add(new_account.balance)
.ok_or(VMError::BalanceOverflow)?;

if !new_account.has_code() && !new_account.has_nonce() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if !new_account.has_code() && !new_account.has_nonce() {
if !new_account.has_code_or_nonce() {

Thank you De Morgan (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not necessary to change it. Maybe it is more readable the original way haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes hahaha I think the first implementation is more readable.

let created_contract = Account::new(balance, Bytes::new(), 1, HashMap::new());
cache::insert_account(&mut cache, new_contract_address, created_contract);
}

let initial_call_frame = CallFrame::new(
env.origin,
Expand Down Expand Up @@ -616,14 +624,6 @@ impl VM {
/// - It calculates and adds intrinsic gas to the 'gas used' of callframe and environment.
/// See 'docs' for more information about validations.
fn prepare_execution(&mut self, initial_call_frame: &mut CallFrame) -> Result<(), VMError> {
//TODO: This should revert the transaction, not throw an error. And I don't know if it should be done here...
// if self.is_create() {
// // If address is already in db, there's an error
// let new_address_acc = self.db.get_account_info(call_frame.to);
// if !new_address_acc.is_empty() {
// return Err(VMError::AddressAlreadyOccupied);
// }
// }
let sender_address = self.env.origin;
let sender_account = self.get_account(sender_address);

Expand Down Expand Up @@ -880,6 +880,17 @@ impl VM {

self.prepare_execution(&mut initial_call_frame)?;

// The transaction should be reverted if:
// - The transaction is a CREATE transaction and
// - The address is already in the database and
// - The address is not empty
if self.is_create() {
let new_address_acc = self.db.get_account_info(initial_call_frame.to);
if new_address_acc.has_code() || new_address_acc.has_nonce() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Should this be an || or a &&?
If an || is needed, then I think a function already exists for it

Suggested change
if new_address_acc.has_code() || new_address_acc.has_nonce() {
if new_address_acc.has_code_or_nonce() {

Having said that, I think the function itself is a bit overkill.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is an ||. We created the function because of the specs. It may be a little bit overkill but we can use it I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type of new_address_acc is AccountInfo. This type doesn't have that method :( Maybe we can implement it in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is an ||. We created the function because of the specs. It may be a little bit overkill but we can use it I guess.

Noted, thank you for the link Jere!

The type of new_address_acc is AccountInfo. This type doesn't have that method :( Maybe we can implement it in the future.

Oh, my mistake Dami.

return self.handle_create_non_empty_account(&initial_call_frame);
}
}

let mut report = self.execute(&mut initial_call_frame)?;
if self.is_create() && !report.is_success() {
remove_account(&mut self.cache, &initial_call_frame.to);
Expand Down Expand Up @@ -1142,6 +1153,26 @@ impl VM {
}
}
}

fn handle_create_non_empty_account(
&mut self,
initial_call_frame: &CallFrame,
) -> Result<TransactionReport, VMError> {
let mut report = TransactionReport {
result: TxResult::Revert(VMError::AddressAlreadyOccupied),
gas_used: self.env.gas_limit.low_u64(),
gas_refunded: 0,
logs: vec![],
new_state: self.cache.clone(),
output: Bytes::new(),
created_address: None,
};

self.post_execution_changes(initial_call_frame, &mut report)?;
report.new_state.clone_from(&self.cache);

Ok(report)
}
}

fn get_n_value(op: Opcode, base_opcode: Opcode) -> Result<usize, VMError> {
Expand Down
Loading