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.

Sign up for GitHub

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

Always create system address after block execution #21

Merged
merged 3 commits into from
Oct 16, 2024
Merged

Always create system address after block execution #21

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b88039
state root mismatch fix
debjit-bw Oct 15, 2024
a35802f
fsyntax
debjit-bw Oct 16, 2024
9baffce
comments
debjit-bw Oct 16, 2024
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
26 changes: 25 additions & 1 deletion src/gnosis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use reth::{
use reth_chainspec::ChainSpec;
use reth_errors::BlockValidationError;
use reth_evm::{execute::BlockExecutionError, ConfigureEvm};
use revm_primitives::{Account, AccountInfo, AccountStatus};

pub const SYSTEM_ADDRESS: Address = address!("fffffffffffffffffffffffffffffffffffffffe");

Expand Down Expand Up @@ -184,8 +185,31 @@ where
})
})?;

// figure out if we should create the system account
Copy link
Member

Choose a reason for hiding this comment

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

Explain in more detail why

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to include the system acc in the state, and not remove it
we cannot insert it in the genesis, since that would cause the state root to mismatch

we need to insert it in the first block, and from the next block onwards remove it from the state transitions map (called state in the apply_block_rewards_contract_call function)

ideally this check would be in the first block (if block.number == 1). but keeping the checking as system_account.status == (AccountStatus::Touched | AccountStatus::LoadedAsNotExisting) to keep it generalized to any block. it checks if system is already in state (in which case the status would just be AccountStatus::Touched), and if it is, it's removed from the stats transitions (state)

if it's not in state, we are inserting it in state as a Created account, because if we don't change the status to AccountStatus::Touched | AccountStatus::Created, it'll not be added to the state

the EIP-158/161 impl in revm is what's dictating most of these. gnosis aura has the exception that the system account needs to be present in state no matter if it's empty or not

let mut should_create = false;
if let Some(system_account) = state.get(&SYSTEM_ADDRESS) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not

Suggested change
if let Some(system_account) = state.get(&SYSTEM_ADDRESS) {
let should_create = if let Some(system_account) = state.get(&SYSTEM_ADDRESS) {

Copy link
Member Author

Choose a reason for hiding this comment

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

changed for a more succinct syntax

if system_account.status == (AccountStatus::Touched | AccountStatus::LoadedAsNotExisting) {
should_create = true;
}
} else {
should_create = true;
}

// system account call is only in rewards function because it will be called in every block
// Clean-up post system tx context
state.remove(&SYSTEM_ADDRESS);
if should_create {
// Populate system account on first block
let account = Account {
info: AccountInfo::default(),
storage: Default::default(),
status: AccountStatus::Touched | AccountStatus::Created,
};
state.insert(SYSTEM_ADDRESS, account);
} else {
// Conditionally clear the system address account to prevent being removed
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow this comment, could you explain in more detail?

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to forcibly insert the system acc in state if it's not there (with the AccountStatus::Touched | AccountStatus::Created flag)

If the account is already in state and we don't remove it, it gets flagged as a touched account, and when the EIP 158/161 implementation sees this, it removes it from the state, causing block failures.

state.remove(&SYSTEM_ADDRESS);
}

state.remove(&evm.block().coinbase);
evm.context.evm.db.commit(state);
// re-set the previous env
Expand Down
Loading