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 selfdestruct for EIP-6780 with non-empty balances #493

Merged
merged 6 commits into from
Aug 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions trace_decoder/src/decoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,23 @@ fn apply_deltas_to_trie_state(
trie_state.state.insert_by_key(val_k, account)?;
}

// Remove any accounts that self-destructed.
for hashed_acc_addr in deltas.self_destructed_accounts.iter() {
let val_k = TrieKey::from_hash(*hashed_acc_addr);

trie_state.storage.remove(hashed_acc_addr);

if let Some(remaining_account_key) =
delete_node_and_report_remaining_key_if_branch_collapsed(
trie_state.state.as_mut_hashed_partial_trie_unchecked(),
&val_k,
)?
{
out.additional_state_trie_paths_to_not_hash
.push(remaining_account_key);
}
}

Ok(out)
}

Expand Down
4 changes: 4 additions & 0 deletions trace_decoder/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,10 @@ pub struct TxnTrace {
/// Contract code that this account has accessed or created
#[serde(skip_serializing_if = "Option::is_none")]
pub code_usage: Option<ContractCodeUsage>,

/// True if the account got self-destructed at the end of this txn.
#[serde(skip_serializing_if = "Option::is_none")]
pub self_destructed: Option<bool>,
}

/// Contract code access type. Used by txn traces.
Expand Down
5 changes: 5 additions & 0 deletions trace_decoder/src/processed_block_trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ impl TxnInfo {
}
}
}

if trace.self_destructed.unwrap_or_default() {
nodes_used_by_txn.self_destructed_accounts.push(hashed_addr);
}
}

for &hashed_addr in extra_state_accesses {
Expand Down Expand Up @@ -223,6 +227,7 @@ pub(crate) struct NodesUsedByTxn {
#[allow(clippy::type_complexity)]
pub storage_writes: Vec<(H256, Vec<(TrieKey, Vec<u8>)>)>,
pub state_accounts_with_no_accesses_but_storage_tries: HashMap<H256, H256>,
pub self_destructed_accounts: Vec<H256>,
}

#[derive(Debug)]
Expand Down
8 changes: 7 additions & 1 deletion trace_decoder/src/typed_mpt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ impl<T> TypedMpt<T> {
fn as_hashed_partial_trie(&self) -> &HashedPartialTrie {
&self.inner
}
fn as_mut_hashed_partial_trie_unchecked(&mut self) -> &mut HashedPartialTrie {
&mut self.inner
}
fn root(&self) -> H256 {
self.inner.hash()
}
Expand Down Expand Up @@ -272,6 +275,9 @@ impl StateTrie {
pub fn as_hashed_partial_trie(&self) -> &mpt_trie::partial_trie::HashedPartialTrie {
self.typed.as_hashed_partial_trie()
}
pub fn as_mut_hashed_partial_trie_unchecked(&mut self) -> &mut HashedPartialTrie {
Nashtare marked this conversation as resolved.
Show resolved Hide resolved
self.typed.as_mut_hashed_partial_trie_unchecked()
}
pub fn remove(&mut self, key: TrieKey) -> Result<Option<AccountRlp>, Error> {
self.typed.remove(key)
}
Expand Down Expand Up @@ -328,7 +334,7 @@ impl StorageTrie {
pub fn root(&self) -> H256 {
self.untyped.hash()
}
pub fn as_hashed_partial_trie(&self) -> &mpt_trie::partial_trie::HashedPartialTrie {
pub fn as_hashed_partial_trie(&self) -> &HashedPartialTrie {
&self.untyped
}

Expand Down
3,346 changes: 3,346 additions & 0 deletions trace_decoder/tests/data/witnesses/zero_jerigon/b20472570_main.json

Large diffs are not rendered by default.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions zero_bin/rpc/src/native/txn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,15 @@ async fn process_tx_traces(
);
let code = process_code(post_state, read_state, &mut code_db).await;
let nonce = process_nonce(post_state, &code);
let self_destructed = process_self_destruct(post_state, pre_state);

let result = TxnTrace {
balance,
nonce,
storage_read,
storage_written,
code_usage: code,
self_destructed,
};

traces.insert(address, result);
Expand All @@ -208,6 +210,30 @@ fn process_nonce(
})
}

/// Processes the self destruct for the given account state.
/// This wraps the actual boolean indicator into an `Option` so that we can skip
/// serialization of `None` values, which represent most cases.
fn process_self_destruct(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: returning an Option is a bit cryptic - could you document what it means?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also you never return Some(false), which feels odd

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to be fair I just brought back what had been removed from a previous PR, but I can change it

Copy link
Collaborator Author

@Nashtare Nashtare Aug 15, 2024

Choose a reason for hiding this comment

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

I believe it's done that way to easily skip serde from serializing None variants, without having to provide a custom fn to deal with false values, given that these are 99% of the cases encountered. But I could be wrong.

post_state: Option<&AccountState>,
pre_state: Option<&AccountState>,
) -> Option<bool> {
if post_state.is_none() {
// EIP-6780:
// A contract is considered created at the beginning of a create
// transaction or when a CREATE series operation begins execution (CREATE,
// CREATE2, and other operations that deploy contracts in the future). If a
// balance exists at the contract’s new address it is still considered to be a
// contract creation.
if let Some(acc) = pre_state {
if acc.code.is_none() && acc.storage.keys().collect::<Vec<_>>().is_empty() {
return Some(true);
}
}
}

None
}

/// Processes the storage for the given account state.
///
/// Returns the storage read and written for the given account in the
Expand Down
Loading