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

Always create system address after block execution #21

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

debjit-bw
Copy link
Member

@debjit-bw debjit-bw commented Oct 15, 2024

  • adds conditional logic for system address to maintain consistency with nethermind in both genesis hash and state root
    • adds system address as new account if it's loaded without existing (converted to touched + created)
    • removes system address from state transitions from block 2 onwards to prevent it being wiped in accordance to EIP-158/161

@debjit-bw debjit-bw marked this pull request as ready for review October 15, 2024 11:08
@debjit-bw debjit-bw requested a review from dapplion October 15, 2024 11:08
@debjit-bw debjit-bw mentioned this pull request Oct 15, 2024
10 tasks
src/gnosis.rs Outdated
@@ -184,8 +185,31 @@ where
})
})?;

// figure out if we should create the system account
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

src/gnosis.rs Outdated
};
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.

src/gnosis.rs Outdated
@@ -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

@dapplion dapplion mentioned this pull request Oct 16, 2024
Base automatically changed from ci-fix to master October 16, 2024 17:00
@dapplion
Copy link
Member

Thank you for the clarifications! But please commit them in the code as comments. Imagine these comments to be high quality documentation for technical readers that have a basic understatement of the system but not detail knowledge like you and I.

@dapplion dapplion changed the title state root mismatch fix Always create system address after block execution Oct 16, 2024
@dapplion dapplion merged commit 5edb203 into master Oct 16, 2024
6 checks passed
@dapplion dapplion deleted the stateRoot-mismatch-fix branch October 16, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants