feat: compute receipts root #111

ricomateo · 2024-07-02T18:18:42Z

Closes #106

**Motivation** Replace evm code in eftests with an implementation that uses ethrex types **Description** * Implement `execute_tx` using revm * Move `evm` crate into `core` crate * Move `Transaction` into its own module * Implement multiple getters for `Transaction` fields * Implement address recovery from signed transactions * Implement conversions between ef tests types and ethrex types * Add one more ef test  Closes #21

the value must be rlp(receipt) instead of tx_type || rlp(receipt) (if tx_type != 0)

the previous way in which the receipts trie values were computed was the right way

crates/core/src/types/block.rs

MegaRedHand

LGTM

**Motivation**  **Description**   Closes #issue_number

**Motivation** Obtain world state from account info stored in db **Description** * Implement storage root computation * Implement word state  Closes #44

**Motivation** Add support for EIP2930 transaction **Description** Implements EIP2930Transaction + required methods & traits   Closes None, but will enable #135 to also close #24

This PR is based on #138 (as they modify the same code), please merge it first **Motivation** Add support for EIP4844 transactions  **Description** Add `EIP4844` transactions + needed trait and methods and integrate it into existing transaction-related code   Closes #26 (evm already performs validations before executing so adding the transaction itself will sufice)

**Motivation** Add error handling to `execute_tx` and remove unwrap  **Description** Add `EvmError` Map revm's `EVMError` to `EvmError` Add error handling to `execute_tx`   Closes None

**Motivation**  **Description**   Closes #125 --------- Co-authored-by: Tomás Grüner <47506558+MegaRedHand@users.noreply.github.com>

Based on lambdaclass#57 **Motivation** Read genesis file from kurtosis. In order to do this, uncomment the following lines in the [ethereum-package fork](https://github.com/lambdaclass/ethereum-package/blob/7c8cb1b89d098bb81308c44107cc9a7d032d6536/src/el/ethrex/ethrex_launcher.star#L200): ``` "--network={0}".format( network if network in constants.PUBLIC_NETWORKS else constants.GENESIS_CONFIG_MOUNT_PATH_ON_CONTAINER + "/genesis.json" ), ``` **Description** Add `network` arg to CLI. Deserialize genesis file from the path provided by the `network` arg  Closes lambdaclass#62

**Motivation** Enable storing account information in DB **Description** * Rename `Account` into `GenesisAccount` and add a separate `Account` struct containing the storage, code, and info fields, with the info being an `AccountInfo` struct containing the code hash. Also implement conversion from `GenesisAccount` to `Account` * Create `AccountInfos` table mapping addresses to account info * Implement TODO comment related to encoding  Closes lambdaclass#40

**Motivation**  **Description**  - `number_of_words` is now calculated in a more performant way, without iterating calldata. - The initial_call_frame in Create is now initialized with empty bytecode and the corresponding calldata send in the transaction. -> After validations the calldata will be assigned to the bytecode and it will be erased. This is mostly for using calldata for calculating some gas costs.  Closes #1362, #1363 Running tests: - ✓ Summary: 1547/4100 (37.73)

**Motivation**  - Gas consumption order is wrong, we are substracting balance from the user at the end of the transaction and that's not how it should be. - There are some things to fix and organize related to gas usage so this PR can address that. **Description** - Contemplates gas refunds for coinbase fee - Substracts up-front cost from user pre-execution and returns unused gas contemplating gas refunds post-execution. - Changes behavior when reverting create post execution -> If revert reason is the RevertOpcode don't consume remaining gas. - `validate_transaction` is now named `prepare_execution` because it both validates and makes pre-execution changes (that are very related with validations). Is has added functionalities like adding the value to the receiver's balance. - It creates method `post_execution_changes()` for organizing `transact()` method   Tests Passing: 1584 total

…ge) (#1419) **Motivation** When attempting to sync with a geth node I got different storage values than the ones expected. Upon further investigation it looks like our encoding of storage values is incorrect. We send the RLP-encoded U256 values while they send the RLP-encoding of the bytes obtained from RLP-encoding the storage value. **Description** * Fixes encoding and decoding of storage values in `StorageRanges` message to match the encoding used by geth   This fix is needed to fully support snap-syncing against other nodes

…1435) **Motivation**  - In some cases we run into stack overflow when executing ef tests. This can be solved by executing in a more optimized mode. - Add things that I think convenient to EFTests and CI. - This will be helpful for CI in general and for [this other PR](#1424) that I'm working on. **Description** - Disables spinner by default -> This is because it improves performance a lot - LEVM EF Tests Execution with spinner: 6 minutes - LEVM EF Tests Execution without spinner (with prints): 30 seconds - Changes `Makefile` to use `release` mode for running tests - Creates a `Makefile` for the folder in which the report is printed. This was made in order to access the report file from the same directory in which we ran `make run-evm-ef-tests` - Adds verbose flag for printing tests names. This can be extended for printing more things but before they were being printed by default and we found that in a basic debugging scenario we don't usually look at those prints. - Enables adding flags to make run-evm-ef-tests that facilitate the command for testing Example of flags: `make run-evm-ef-tests flags="--tests stEIP1559"`   **Why use --release** I tried to avoid using the --release profile and wanted to update our test profile for running with optimizations by default but I had issues doing it because LEVM is a crate of the repository, and: [Cargo only looks at the profile settings in the Cargo.toml manifest at the root of the workspace. Profile settings defined in dependencies will be ignored. ](https://doc.rust-lang.org/cargo/reference/profiles.html) I didn't find a simple way to override the test profile without affecting the rest of the crates from the repository, so I decided it wasn't worth the hassle because the benefits from it weren't as high. Closes #issue_number

**Motivation** Fix the loc test done in `daily_reports.yaml`. ATTOW it does not include line changes.  **Description** The loc directive should send a message to the slack channel showing lines added and removed. This implied changes to the CI.   No issue for this PR.

**Motivation**  - Generic Call works well for basic call cases but it handles poorly some scenarios, and we still have to figure out how to solve the stack overflow problem when depth is too high. - Fix other things I see along the way **Description**  - It replaces `OpcodeSuccess::Stop` for `OpcodeSuccess::Continue` in some cases because we don't want to stop execution of current callframe, we just want to push to it's stack and continue. - It fixes `op_gas`, that previously was calculated based on used gas in environment, and it should be based upon used gas in current callframe. - It fixes `op_gaslimit`, that previously returned the transaction gaslimit, when instead it should return the block's gas limit. **Status** - Tests passing with current changes: 1930/4100

**Motivation**  - We need to run with optimizations because otherwise some tests related to call depth will fail and halt execution of EFTests. **Description**   Closes #issue_number

**Motivation**  - Currently we have some errors in gas calculations related to memory when "size" is set to 0. Memory shouldn't ever expand on that scenario. - Tidy code for calculating memory size. **Description**  - Create function `calculate_memory_size` in memory module - Use function in every opcode that needs it  **Results** - 7 more tests pass when comparing directly to last main changes. Closes #1442

**Motivation** The spammer hanged in one of 2 forms, either it wait for tx that weren't added in the second phase of each spam and timeout after 5 minutes resuming the execution or it hangs ad infinitum if the issue happens in the first phase. This indicate that we were losing transactions. **Description** This is a mid-term solution, the explanation about what happened is done on the issue, [in this particular comment](#1120 (comment)), but TLDR: we were receiving `engine_getPayload` request with old `payload_ids` and we filled *AGAIN* the block but with new transactions. the block was discarded as orphan by consensus and we lost those txs from mempool on the immediate subsequent request. This was caused because we were missing the update of the payload when it was modified in the second step of our build process. The current solution stores the whole payload, i.e. the block, block value and the blobs bunde. Given our current implementation (a naive 2-step build process) we either filled the transaction and ended the build process or not, a simple closed flag was added to the payload store to signal this and avoid refilling transactions, this is clearer than just check if there are any but could be changed if preferred. | Spammer logs | Dora explorer | :-------------------------:|:-------------------------: ![image](https://github.com/user-attachments/assets/f768879b-4bba-41f6-991b-e81abe0531f4) | ![image](https://github.com/user-attachments/assets/bd642c92-4a99-42fa-b99d-bc9036b14fff) **Next Steps** Enhance the build process to make it async and interactive instead of the naive 2-step all-or-nothing implementation we had right now.  Resolves #1120

**Motivation** Support snap-sync  **Description** Implements the snap-sync cycle with the following caveats: - No state healing is implemented (We asume all the state is available) - Storage ranges are fetched in full (We don't partition storage range requests, if an account's storage wasn't fully fetched as part of a batch it is requested again) - Minimum parallelization (We are currenltly testing against a single node so not all parallelizable processes are being parallelized. - Receipts are not fetched (waiting on #386) - State is not blocked during sync (this could lead to the wrong data being sent by rpc and p2p during an active sync) Other changes: - Move `crates/networking/p2p/README.rs` to `crates/networking/docs/Network.md` - Add doc for Sync in `crates/networking/docs/Sync` - Reduce size of `RLPxError`   Closes #issue_number

**Motivation** This PR batches the three current heaviest db write operations on the node (`add_block`, `store_receipts`, and `commit_node`) so that a single db transaction is used for them. This reduces the time spent flushing to disc with `msync` (on `libmdbx`) or `fsync` (on `redb`) from around ~40-50% to ~0.35%. Non-batched version: ![flamegraph_not_batched](https://github.com/user-attachments/assets/7d698997-d8d4-4cac-9e6d-1fb1e94824bd) Batched version (the `msync` on this flamegraph is one of the tiny red bars to the left): ![flamegraph_batched](https://github.com/user-attachments/assets/48ad899d-51ce-4b48-a9f3-7254567c54fe) **Description**

**Motivation** The motivation is to make tests of the vmIOandFlowOperations sub folder pass. **Description** Previously, when accessing a storage slot from memory the slot was not added to the storage of the account. Now this is done, because when update_account_storage is called, it searches the accounts storage, and if the storage does not have the slot the original value is set with 0.  Closes #issue_number

…e end of transaction) and modifying substate (#1448) **Motivation**  **Description** - We weren't checking if the account that executed SELFDESTRUCT had positive balance, that is useful for gas calculations - Created set of `created_accounts` and use it in selfdestruct to know if an address was created in that transaction. -> Add to that set the address in `Create` transactions and opcodes. - "Delete" (remove from cache) the accounts registered to be destroyed. - In Cancun it is enough to remove them from cache because the accounts that are registered to be destroyed are always accounts created in the same transaction, so they never existed in the database in the first place. But we will have to change this when thinking of implementing this for previous forks. - I moved touched accounts, touched storage slots and the selfdestruct set to the substate, so that it reverts on execution Tests passing: From 1941 in main to 1989 in this branch. Every little change in this PR made some tests pass so there is no change that breaks things.  Closes #1441

If the offset is larger than calldata, then there is no data to show. Return 0 to the stack **Motivation** The calldata operand tried to cast the value it got as an argument to usize everytime. However, that number could be larger than usize and an error is raised.  **Description** This tries to check the offset value before casting it to usize. If the offset value (in U256) is _larger_ than the actual length of contents of the calldata (also in U256), then we simply return 0 (since the offset would've removed all the values from calldata).

**Motivation**  **Description**  Selfbalance wants the balance of the callframe executing, and that would be the `current_call_frame.to`, not the `code_address`  Closes #issue_number --------- Co-authored-by: Maximo Palopoli <96491141+maximopalopoli@users.noreply.github.com>

…e bytecode length. (#1465) **Motivation** Currently, the offset is cast to usize immediately . This causes an a `VeryLargeNumber` number error, and thus the memory is never updated.  **Description** Now, we check if the offset is larger than the bytecode itself. If that is the case, then we simply return an array of `size` many zeros.   This won't compile because it depends on the changes made here: #1458

#1471) **Motivation** Postpone `extcodcopy`'s cast to usize until it's checked that it can fit.  **Description** extcodecopy receives as input an offset. That offset can be of size u256. This need to be casted to usize to index the code it want to access. However, it is done too early; which causes a problem when the offset is larger than the code itself.   Closes: #1469

**Motivation**  **Description**  - When changing the bytecode of current callframe we also have to calculate valid jump destinations of that bytecode. There is only one case in which we are doing this right now. In a Create type Transaction at the end of the validations the calldata is assigned to the bytecode.  Closes #issue_number

**Motivation**  **Description**  - We only want to keep track of the consumed gas in the current callframe. We don't need to keep track of it in the environment. - This change will allow us to implement accurate behavior related to gas consumption in XCALL and CREATE opcodes. Note that the gas consumed by the first callframe will indeed be the gas consumed by total execution, because the gas consume by child CallFrames will be added to the "father". (It is not as straightforward but that's the idea). This change doesn't fix nor break any test, but will be useful for following changes  Closes #issue_number

…en when there a `restore-keys` is used. (#1438) Even when there is no cache-hit, the file is fetched. If it came from the `restore-keys` directive, it doesn't count as a cache-hit even if the file is fetched. **Motivation** Show LOC diffs in the different slack channels.  **Description** This description was taken from this comment: #1433 (comment)  We are saving the log_report.json file on a cache entry that has as key loc-report-${{ github.ref_name }}. github.ref_name is the name of a branch not a commit hash. That name is not "main" but rather the name of the PR it came from. In this case ci-diffs. What's the problem with that? If the branch names don't match EXACTLY, the cache fails. HOWEVER, we have a restore-keys as a backup (here: https://github.com/lambdaclass/ethrex/blob/main/.github/workflows/daily_reports.yaml#L124-L125). restor-keys is a backup that acts like a sort of regex. It tells github-cache "Hey, seen as you failed fetching that EXACT key, give me, as a backup, a value that matches the following prefix". That prefix should match everytime, since every key we generate uses that exact same prefix. So what's the problem? The problem is that we are only changing the file name IF there is a cache hit aka this if: https://github.com/lambdaclass/ethrex/blob/main/.github/workflows/daily_reports.yaml#L132. So, once again, what's the problem? We are getting a cache hit everytime, we either get it with the exact key or using the restore-keys as a fallback. And there is the problem, if the value is fetched using restore-keys, it doesn't count as a cache hit. This behavior is described here: https://github.com/actions/cache/blob/main/README.md?plain=1#L129. That would also explain why the script worked while I was working on this PR: I was always on the same branch, so it was always a cache hit, it was never using restore-keys. However, when the cron job is executed, it probably fails every time and ends up using the backup. That is also strange, one would think that it would end up using "main" branch as a key name. My hunch is that it uses commits coming from PR's, so it's maybe using those PR branch names for keys. I'm gonna create a new PR for it to be merged on Tuesday, so we can see the difference in output and check that this indeed is the issue.  No associated issue number

**Motivation**  **Description**  - Fix returndatacopy: If offset + size is larger than returndatasize then we have to revert execution, and we weren't doing that. [EVM Codes](https://www.evm.codes/?fork=cancun#3e) - Changes name of `OutOfOffset` error for `OutOfBounds` because I consider it more appropriate.  Closes #issue_number

**Motivation**  - Create opcode has some issues with it's implementation and it doesn't work properly in most cases. The idea behind this PR is to review the opcode and fix every wrong thing. Always comparing with EFTests after each fix. **Description**   Closes #issue_number

…r fixes (#1492) **Motivation**  **Description** - Removes in `generic_call` double definition of `should_transfer_value` - Organizes call opcodes - Now we don't return early when bytecode is empty. This was inaccurate behavior   Closes #issue_number

**Motivation**  - remove unnecessary stuff and make things clearer **Description**  - Adds comments to `CallFrame` - Changes `returndata` name for `output`, because it had that role and it was confusing when comparing with Exec specs. - Remove `sub_return_data_offset` and `sub_return_data_size` because from `CallFrame` they were unnecessary  Closes #issue_number

…er` & `get_earliest_block_number` (#1525) **Motivation** Both values are written since genesis and never deleted so them missing should be treated as a Core Storage error (either the genesis was not loaded or the db is corrupted) instead of as a regular missing value.  **Description** Remove the `Option` from the return types of the `Store` methods `get_latest_block_number` & `get_earliest_block_number`   Closes #791

**Motivation** I ran into errors due to the frequency of docker pulls performed when adding more elements to the `run-hive` matrix strategy. I think we can solve it by only setting up hive once and then passing on the executable to the different hive test jobs.  **Description** * Create a `setup-hive` job that creates the `hive` executable * Retrieve the `hive` executable instead of creating it for each parallel run in `run-hive`   Closes #issue_number

…ayloadAttributes` (#1543) **Motivation** When we receive a payload with null payload attributes we return an error: `Could not parse params invalid type: null, expected struct PayloadAttributesV3`, when this is perfectly acceptable behaviour **Description** * Parse payload attributes as `Option<PayloadAttributes>` instead of just `PayloadAttributes`   Closes #issue_number

**Motivation** The execution client uses `u64` for measuring gas, while LEVM uses `U256`. This PR makes LEVM also use u64 for gas **Description**   Closes #1499

**Motivation**  **Description** In [execution specs](https://ethereum.github.io/execution-specs/src/ethereum/cancun/vm/instructions/log.py.html) the address logged is the current executing account.   Closes #issue_number

**Motivation**  **Description** TLDR: We don't want to interact directly with the db without consulting before with the cache. This won't fix nor break tests because in every EFTest we send the cache empty, but it is a necessary fix for executing multiple transactions within a block. As a side effect we have to move contract creation and insertion to cache to `transact()`, so that we can revert if address is already occupied.  Idea: - Move contract creation and insertion to cache to `transact()`, not to `new()`. - This is because we really want to check if the contract already exists both in the cache and in the db, and if when you want to create it you see that it already exists in one of those 2 there you revert. Currently we only check it in the db but the tests work because they test only one transaction each, so the cache always starts empty. - The idea is to stop using `db.get_account_info()` in the `VM::new()`, we want to query the cache first and then the DB. For this we have the `get_account()` method in VM, but in `VM::new()` it doesn't work because there is no vm created yet. The idea is to grab that behavior and take it to a `get_account(address, mut cache, db)` function that has the same behavior as now. The VM get_account method can be a handrail to call this, which does `get_account(address, self.cache, self.db)`. Additional Changes: - Remove unnecessary stuff in `VM::new()` - Remove receiver account every time a transaction reverts, independently of it's type.  Closes #issue_number

MegaRedHand and others added 10 commits July 1, 2024 13:03

feat: compute transactions root for a block

a685ff2

fix: was encoding LegacyTransaction incorrectly

611bc86

fix: RLP encode index

49f0374

Merge branch 'main' into compute-transactions-root

7080893

Merge branch 'main' into compute-transactions-root

5f9e6eb

refactor: add Transaction::encode_with_type

a81897f

refactor: add TxType enum for encoding tx type

b966cf4

refactor: add TxKind struct

2c59b6d

Merge branch 'main' into compute-transactions-root

830ecac

add receipts root computation

67f1db0

ricomateo added 4 commits July 2, 2024 17:07

fix: the receipt trie value was being computed wrong

3f2dac6

the value must be rlp(receipt) instead of tx_type || rlp(receipt) (if tx_type != 0)

fix: compute receipts trie value correctly

8e23699

the previous way in which the receipts trie values were computed was the right way

remove unnecessary constructor

3b32d29

merge main

ff376e1

ricomateo marked this pull request as ready for review July 2, 2024 20:59

ricomateo requested a review from a team as a code owner July 2, 2024 20:59

MegaRedHand reviewed Jul 2, 2024

View reviewed changes

crates/core/src/types/block.rs Outdated Show resolved Hide resolved

fix: move compute_receipts_root out of BlockBody impl

f02abf1

MegaRedHand approved these changes Jul 3, 2024

View reviewed changes

MegaRedHand merged commit b3a16bf into main Jul 3, 2024
3 checks passed

MegaRedHand deleted the compute-receipts-root branch July 3, 2024 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: compute receipts root #111

feat: compute receipts root #111

ricomateo commented Jul 2, 2024

MegaRedHand left a comment

feat: compute receipts root #111

feat: compute receipts root #111

Conversation

ricomateo commented Jul 2, 2024

MegaRedHand left a comment

Choose a reason for hiding this comment