fix: cap nonce value at 2^64-1 as per EIP-2681 #163
Merged
sorpaas merged 3 commits intorust-ethereum:masterfrom May 21, 2023
Merged
fix: cap nonce value at 2^64-1 as per EIP-2681 #163sorpaas merged 3 commits intorust-ethereum:masterfrom
sorpaas merged 3 commits intorust-ethereum:masterfrom
Conversation
This was referenced May 15, 2023
sorpaas
approved these changes
May 21, 2023
zjb0807
added a commit
to AcalaNetwork/Acala
that referenced
this pull request
Jan 22, 2024
zjb0807
added a commit
to AcalaNetwork/Acala
that referenced
this pull request
Jan 29, 2024
* update PrecompileHandle ref: rust-ethereum/evm#122 * update fee calculation ref: rust-ethereum/evm#132 * add code_size/code_hash fn in StackState trait ref: rust-ethereum/evm#140 * update evm call stack ref: rust-ethereum/evm#136 * update evm call stack ref: rust-ethereum/evm#155 * add shanghai eips 3651, 3855, 3860 ref: rust-ethereum/evm#152 * update is_precompile ref: rust-ethereum/evm#157 * fix eip-3860 ref: rust-ethereum/evm#160 * update runtime config ref: rust-ethereum/evm#161 * add eip-4399 ref: rust-ethereum/evm#162 * fix eip-2618 ref: rust-ethereum/evm#163 * fix nonce back to U256 ref: rust-ethereum/evm#166 * remove exit_substate in create functions ref: rust-ethereum/evm#168 * record external cost ref: rust-ethereum/evm#170 * add record_external_operation ref: rust-ethereum/evm#171 * add storage_growth ref: rust-ethereum/evm#173 * update evm * switch to shanghai hardfork * update ecrecover ref: polkadot-evm/frontier#964 (#2696)
mattsse
pushed a commit
to mattsse/evm
that referenced
this pull request
Jan 17, 2026
…cutor (rust-ethereum#163) * feat(block): add TransactionOutput type and new BlockExecutor methods Introduces TransactionOutput type alias and two new methods to BlockExecutor trait: - execute_transaction_without_commit: Executes transaction without committing state - commit_transaction: Commits previously executed transaction output This decomposition provides better separation of concerns and flexibility for transaction simulation and custom commit logic. * feat(eth): implement decomposed transaction execution in EthBlockExecutor Implements the new execute_transaction_without_commit and commit_transaction methods in EthBlockExecutor, refactoring execute_transaction_with_commit_condition to use these new methods internally. * feat(op): implement decomposed transaction execution in OpBlockExecutor Implements the new execute_transaction_without_commit and commit_transaction methods in OpBlockExecutor, maintaining compatibility with OP Stack specific features like deposit nonce handling. * fix: address clippy and fmt issues for CI - Remove unnecessary borrow in transact call - Apply nightly rustfmt formatting * refactor: address Matt's review comments on BlockExecutor decomposition Based on review feedback: - Remove TransactionOutput type alias, use ResultAndState directly - Remove Copy bounds from method parameters - Use references (&impl ExecutableTx<Self>) for new methods - Keep original execute_transaction_with_commit_condition signature unchanged - Maintain backward compatibility for all existing methods This provides a cleaner API without introducing new type aliases or unnecessary trait bounds while still achieving the goal of decomposing transaction execution and commitment. * fix: resolve broken documentation link for ResultAndState Use fully qualified path to ResultAndState type in doc comment to fix rustdoc broken-intra-doc-links warning in CI * style: apply rustfmt formatting Break long documentation line to comply with rustfmt rules * refactor(eth): extract gas limit validation helper method Extract duplicated gas limit validation logic into a helper method in EthBlockExecutor to reduce code duplication and improve maintainability. The validation logic was duplicated between execute_transaction_with_commit_condition and execute_transaction_without_commit methods. * refactor: use decomposed methods in default trait implementation Per Arsenii's review feedback, implement execute_transaction_with_commit_condition as a default trait method that delegates to the new decomposed methods. This eliminates all code duplication as concrete implementations no longer need to override this method. Changes: - Add default implementation of execute_transaction_with_commit_condition that uses execute_transaction_without_commit and commit_transaction - Remove overridden implementations from EthBlockExecutor and OpBlockExecutor - Add TODO comment about potential depositor nonce issue in OpBlockExecutor that may need further investigation This approach ensures the execute/commit logic is only implemented once per executor, significantly reducing code duplication. * fix: resolve CI issues with formatting and unused imports - Apply rustfmt formatting fixes - Remove unused imports caught by clippy - Ensure all CI checks pass * refactor: improve deposit nonce handling in OpBlockExecutor - Update comments to clarify the caching of the depositor account prior to state transition for deposit nonces. - Note that this change is relevant post-regolith hardfork, as deposit nonces were not present in earlier versions. - Remove outdated TODO comment regarding depositor nonce verification, as the caching approach addresses the issue. This enhances the clarity and correctness of the deposit transaction handling logic. * refactor: restore OpBlockExecutor override for deposit nonce handling OpBlockExecutor requires capturing the depositor nonce BEFORE transaction execution but using it AFTER in the receipt. The decomposed API doesn't support passing this pre-execution state between methods without introducing stateful coupling. Solution: - Keep the default trait implementation for general use (EthBlockExecutor) - OpBlockExecutor overrides execute_transaction_with_commit_condition - This avoids stateful coupling while correctly handling deposit nonces - The decomposed methods are implemented but marked as unreachable This design allows the decomposition benefits where applicable while handling OP-specific requirements through the override pattern. * refactor: enhance commit_transaction logic in OpBlockExecutor This update improves the commit_transaction method by implementing deposit nonce handling more effectively. Key changes include: - Caching the depositor account before the state transition for deposit nonces, relevant post-regolith hardfork. - Building receipts with enhanced logic to accommodate deposit transactions and their specific requirements. - Ensuring gas usage is accurately tracked and appended to receipts. These modifications clarify the deposit transaction handling and align with the OP-specific requirements while maintaining the benefits of method decomposition. * perf: move hash calculation into error closure to avoid unnecessary copies Per Matt's review feedback, the trie_hash() calculation is expensive and was being performed unconditionally even when no error occurred. Since blocks typically have ~400 transactions, this optimization saves ~400 unnecessary hash computations per block in the success case. Changes: - Move tx.tx().trie_hash() inside the map_err closure - Hash is now only calculated when an error actually occurs - Applied to both EthBlockExecutor and OpBlockExecutor implementations This is a significant performance improvement as trie_hash() involves Keccak256 hashing of the RLP-encoded transaction. * Update block.rs * chore: remove performance optimization comment * refactor: address review feedback from klkvr - Remove Copy bound by using &tx references throughout - Import ResultAndState directly instead of using qualified path - Simplify OpBlockExecutor by removing override of execute_transaction_with_commit_condition - Fetch deposit nonce from DB during commit phase instead of before execution - Remove unused ExecutionResult import The default trait implementation now correctly delegates to our execute_transaction_without_commit and commit_transaction methods, which properly handle deposit nonces by fetching them from the DB during the commit phase as suggested by klkvr. * Update crates/op-evm/src/block/mod.rs Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com> * refactor: simplify commit_transaction signature per review feedback Remove unnecessary reference from commit_transaction's tx parameter as suggested by @mattsse. Since the transaction is only used for reading data at this point and won't be used afterward, we can take it by value instead of by reference. Changes: - Change commit_transaction signature from `tx: &impl ExecutableTx<Self>` to `tx: impl ExecutableTx<Self>` - Update EthBlockExecutor and OpBlockExecutor implementations to match - Add & references within method bodies where needed * fix: remove needless borrows flagged by clippy The compiler automatically borrows these values when calling methods, so explicit borrowing with & is unnecessary. * rm ref --------- Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since geth and other implementations were implicitly capping the nonce value at 2^64-1, EIP-2681 formalized it. Sputnik wasn't following this spec as a nonce value was of type U256, which caused a recently added ethereum test case to fail.
NOTE: #162 should be merged first as this builds on that PR.