bluealloy / revm

Rust implementation of the Ethereum Virtual Machine.
https://bluealloy.github.io/revm/
MIT License
1.59k stars 518 forks source link

Why is NonceChange event not pushed on Call Transactions #1716

Open ercecan opened 3 weeks ago

ercecan commented 3 weeks ago

For Call transactions in deduct_caller_inner this is called:

 // bump the nonce for calls. Nonce for CREATE will be bumped in `handle_create`.
 if matches!(env.tx.transact_to, TxKind::Call(_)) {
     // Nonce is already checked
     caller_account.info.nonce = caller_account.info.nonce.saturating_add(1);
}

For Create transactions in make_call_frame to increment nonce inc_nonce is called and NonceChange is pushed to JournalState

    #[inline]
    pub fn inc_nonce(&mut self, address: Address) -> Option<u64> {
        let account = self.state.get_mut(&address).unwrap();
        // Check if nonce is going to overflow.
        if account.info.nonce == u64::MAX {
            return None;
        }
        Self::touch_account(self.journal.last_mut().unwrap(), &address, account);
        self.journal
            .last_mut()
            .unwrap()
            .push(JournalEntry::NonceChange { address });

        account.info.nonce += 1;

        Some(account.info.nonce)
    }

What I don't understand is why is the same event not pushed for call transactions after the nonce is incremented. If this is normal can you please explain why? Because I think (state diff) rollups need this event for correctly calculating state diffs and determining l1 fees.

If this is not normal I can volunteer and try to fix this.

rakita commented 3 weeks ago

Journaling is needed for reverts to revert changes made for subcall.

For tx call (first call) nonce invrement is never reverted so there was no need to journal that change.

Because I think (state diff) rollups need this event for correctly calculating state diffs and determining l1 fees.

Wdym by this? Would like to hear more as Journaling rn is discarded after tx execution so users dont have access to it. State diff can be obtained by comparing database values and revm output state.

ercecan commented 3 weeks ago

Journaling is needed for reverts to revert changes made for subcall.

I see, thanks for the clarification, didn't know that

Wdym by this? Would like to hear more

At citrea after each tx in order to calculate the l1 fee per transaction we calculate the diff size that transaction makes and deduct the cost from the caller in post_execution.output We normally calculate the diff size by iterating over journal entries but until now never noticed that the NonceChange event is not pushed to the journaled state, so I believe we should change our approach to what you said

ercecan commented 2 weeks ago

We went with adding nonce_changed to tx sender and still listening to journal entries for other changes. Thanks for the response feel free to close the issue if you don't have any other questions or suggestions

rakita commented 2 weeks ago

We went with adding nonce_changed to tx sender and still listening to journal entries for other changes. Thanks for the response feel free to close the issue if you don't have any other questions or suggestions

Wouldn't mind adding that nonce change to the journal. There is an idea to persist journal and use it as a way to revert transaction, it is a low priority on my list.

nkysg commented 2 weeks ago

Wouldn't mind adding that nonce change to the journal. There is an idea to persist journal and use it as a way to revert transaction, it is a low priority on my list.

May I try this?

rakita commented 2 weeks ago

Wouldn't mind adding that nonce change to the journal. There is an idea to persist journal and use it as a way to revert transaction, it is a low priority on my list.

May I try this?

Go for it!