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

EIP-6780: SELFDESTRUCT only in same transaction #719

Merged
merged 5 commits into from
Sep 21, 2023

Conversation

thedevbirb
Copy link
Contributor

@thedevbirb thedevbirb commented Sep 14, 2023

Closes #524. Reading the EIP, when the contract is not created in the same tx, SELFDESTRUCT simply becomes a transfer, while keeping the gas cost of the opcode. As such, I tried to modify only the JournaledState to reflect that.

@rakita
Copy link
Member

rakita commented Sep 18, 2023

Didn't forget about this, checking things related to Verkle tree.

@rakita rakita added the hf-cancun Cancun hardfork label Sep 18, 2023
Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

good direction, left comments

crates/revm/src/journaled_state.rs Outdated Show resolved Hide resolved
crates/revm/src/journaled_state.rs Show resolved Hide resolved
crates/revm/src/journaled_state.rs Outdated Show resolved Hide resolved
// touch target account

let acc = if address != target {
let [acc, target_account] = self.state.get_many_mut([&address, &target]).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh forget this, Hashbrown has it

});
}
} else {
Self::touch_account(self.journal.last_mut().unwrap(), &address, acc);
Copy link
Member

@rakita rakita Sep 19, 2023

Choose a reason for hiding this comment

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

This line is not needed, address is considered touched as this is the account that we are currently executing.

crates/revm/src/journaled_state.rs Outdated Show resolved Hide resolved
crates/revm/src/journaled_state.rs Outdated Show resolved Hide resolved
Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

Meant to request changes

@thedevbirb
Copy link
Contributor Author

I should have resolved all your comments, thanks for the support. I'm happy to see that tests are passing, even if in local they were failing for some reason..
Note that now if cancun is enabled and address === target, the BalanceTransfer entry in the JournledState is not pushed in since it is useless. As the EIP says, in such case there is no net change of balances.
Let me know what you think!

Copy link
Member

@rakita rakita 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 nit comments to add doc

@rakita
Copy link
Member

rakita commented Sep 21, 2023

I should have resolved all your comments, thanks for the support. I'm happy to see that tests are passing, even if in local they were failing for some reason.. Note that now if cancun is enabled and address === target, the BalanceTransfer entry in the JournledState is not pushed in since it is useless. As the EIP says, in such case there is no net change of balances. Let me know what you think!

Tests were failing for some other reason, found the problem and merged in on main, so we are good

Copy link
Collaborator

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

other than the current comments, this looks good to me, will test this with hive once merged!

@rakita rakita merged commit 4b5fa61 into bluealloy:main Sep 21, 2023
8 checks passed
@thedevbirb thedevbirb deleted the eip-6780 branch September 22, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hf-cancun Cancun hardfork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EIP-6780: SELFDESTRUCT only in same transaction
3 participants