-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Remove medstate from receipts #98
Comments
We would also need to load state touched by all txs in the block up to the tx you want to replay. Since we don't know what it will touch ahead of time it, it is a serial process frequently blocked by getting state from the network. Just want to make sure we are properly enumerating the impacts on the light client strategy. |
Other Metropolis EIPs are specified for |
@vbuterin does this EIP have an associated PR? |
Closing issue. Please direct discussion of EIP 98 to pull request 658 per this comment. |
This EIP is referenced by #658, which has already been merged. As such, it needs to either be finalised and merged, or the dependency in the existing EIP needs to be removed. |
Why: * Issue #757 was created after we noticed a few constraint violations for the `error` column in the `transactions` table. The constraint violations came about as we attempted to update transactions with a successful status but some value for the error field. The constraint is violated in this scenario because we expect successful transactions to not have an error. (EIP 658)[ethereum/EIPs#658], which apparently is also called (EIP 98)[ethereum/EIPs#98 (comment)] , says that the status should be set to 1 (success) if the outermost code execution succeeded (internal transaction with index 0), or it should be set to 0 (failure) if the outermost code execution failed. We're currently deriving the status from the last internal transaction when in fact we should be deriving it from the first internal transaction. * Issue link: #757 This change addresses the need by: * Editing `Explorer.Chain.Import.update_transactions/2` to set the error for a transaction equal to the error, if any, of the internal transaction with index 0. * Editing `Explorer.Chain.Import.update_transactions/2` to set a transaction's status as successful if it's internal transaction at index 0 doesn't have an error.
Why: * Issue #757 was created after we noticed a few constraint violations for the `error` column in the `transactions` table. The constraint violations came about as we attempted to update transactions with a successful status but some value for the error field. The constraint is violated in this scenario because we expect successful transactions to not have an error. [EIP 658](ethereum/EIPs#658), which apparently is also called [EIP 98](ethereum/EIPs#98 (comment)) , says that the status should be set to 1 (success) if the outermost code execution succeeded (internal transaction with index 0), or it should be set to 0 (failure) if the outermost code execution failed. We're currently deriving the status from the last internal transaction when in fact we should be deriving it from the first internal transaction. * Issue link: #757 This change addresses the need by: * Editing `Explorer.Chain.Import.update_transactions/2` to set the error for a transaction equal to the error, if any, of the internal transaction with index 0. * Editing `Explorer.Chain.Import.update_transactions/2` to set a transaction's status as successful if it's internal transaction at index 0 doesn't have an error.
Why: * Issue #757 was created after we noticed a few constraint violations for the `error` column in the `transactions` table. The constraint violations came about as we attempted to update transactions with a successful status but some value for the error field. The constraint is violated in this scenario because we expect successful transactions to not have an error. [EIP 658](ethereum/EIPs#658), which apparently is also called [EIP 98](ethereum/EIPs#98 (comment)) , says that the status should be set to 1 (success) if the outermost code execution succeeded (internal transaction with index 0), or it should be set to 0 (failure) if the outermost code execution failed. We're currently deriving the status from the last internal transaction when in fact we should be deriving it from the first internal transaction. * Issue link: #757 This change addresses the need by: * Editing `Explorer.Chain.Import.update_transactions/2` to set the error for a transaction equal to the error, if any, of the internal transaction with index 0. * Editing `Explorer.Chain.Import.update_transactions/2` to set a transaction's status as successful if it's internal transaction at index 0 doesn't have an error, and as failed if it does. This only applies to pre-Byzantium and Ethereum Classic, for which transaction status is not set from receipts.
EIP98 was swallowed by EIP658. - ethereum/EIPs#98 (comment)
EIP98 was swallowed by EIP658. - ethereum/EIPs#98 (comment)
EIP98 was swallowed by EIP658. - ethereum/EIPs#98 (comment)
# Introduction Presently, if a transaction is reverted, the base rollup does not mark it as reverted: it merely has its revertible side effects dropped. This issue will update the base rollup to mark reverted transactions. # Solution Design ## New `RevertCode` in `TxEffect` Create a new class in `circuits.js` to hold a `RevertCode`. It will be a safe wrapper around an `Fr` that will be used to store the revert code of a transaction. `0` will indicate success, and any other value will indicate failure. Presently, only `1` is used to indicate general failure, but we'll leave the door open for future expansion. `TxEffect` will be updated to include a `RevertCode`. ### Ethereum `status` Ethereum stores their `status` in a `uint64`, but only currently use `0` to indicate failure and `1` for success. [More info](ethereum/EIPs#98) (Thanks @spalladino !) ### Size considerations We'll eventually want to add `gasUsed` and `gasPrice` to `TxEffect`. Using a full field is wasteful, but using a smaller size will make the encoding and hashing more complex (see below). We'll implement with a full field and have a stacked PR to optimize the size. ## Kernel Constraints The `RevertCode` will become part of the content commitment for a transaction. The content commitment is computed by the base rollup in `compute_tx_effects_hash`, which: - accepts a `CombinedAccumulatedData` - operates over `Field`s We will therefore update the `CombinedAccumulatedData` to include the `RevertCode`. A fallout from that is we will move the current `reverted` flag from `PublicKernelCircuitPublicInputs` to be part of: - `PrivateAccumulatedNonRevertibleData` - `PublicAccumulatedNonRevertibleData` This is because we: 1. build up a `CombinedAccumulatedData` during private execution 2. split them into `PrivateAccumulatedNonRevertibleData` and `PrivateAccumulatedRevertibleData` in the private kernel tail 3. convert those `PrivateAccumulated...` into `PublicAccumulated...` during public kernel execution as needed 4. recombine them back into `CombinedAccumulatedData` before feeding back into base rollup ## Encoding The `RevertCode` will come first in the encoded `TxEffect`. That is we will publish: ``` || 32 bytes for RevertCode || ... Existing PublishedTxEffect ... || ``` ## L1 Tx Decoder We'll need to update the availability oracle to compensate for the new flag. ## `TxReceipt` We'll add a new `TxStatus` that is `REVERTED = 'reverted'`. We need to update `getSettledTxReceipt` to inspect the `TxEffect` and set the `status` accordingly. # Test Plan ## Unit Tests - Serde of `TxEffect` in TS and solidity. ## E2E Tests - Add checks for tx statuses of `REVERTED` in the `e2e_fees`, `e2e_deploy_contract`, and `dapp_testing`. # Documentation Plan Will start a page in the "Data publication and availability" section of the yellow paper describing the format of the block submitted. # Especially Relevant Parties - @PhilWindle - @benesjan - @alexghr - @LeilaWang - @LHerskind # Plan Approvals **_👋 Please add a +1 or comment to express your approval or disapproval of the plan above._**
# Introduction Presently, if a transaction is reverted, the base rollup does not mark it as reverted: it merely has its revertible side effects dropped. This issue will update the base rollup to mark reverted transactions. # Solution Design ## New `RevertCode` in `TxEffect` Create a new class in `circuits.js` to hold a `RevertCode`. It will be a safe wrapper around an `Fr` that will be used to store the revert code of a transaction. `0` will indicate success, and any other value will indicate failure. Presently, only `1` is used to indicate general failure, but we'll leave the door open for future expansion. `TxEffect` will be updated to include a `RevertCode`. ### Ethereum `status` Ethereum stores their `status` in a `uint64`, but only currently use `0` to indicate failure and `1` for success. [More info](ethereum/EIPs#98) (Thanks @spalladino !) ### Size considerations We'll eventually want to add `gasUsed` and `gasPrice` to `TxEffect`. Using a full field is wasteful, but using a smaller size will make the encoding and hashing more complex (see below). We'll implement with a full field and have a stacked PR to optimize the size. ## Kernel Constraints The `RevertCode` will become part of the content commitment for a transaction. The content commitment is computed by the base rollup in `compute_tx_effects_hash`, which: - accepts a `CombinedAccumulatedData` - operates over `Field`s We will therefore update the `CombinedAccumulatedData` to include the `RevertCode`. A fallout from that is we will move the current `reverted` flag from `PublicKernelCircuitPublicInputs` to be part of: - `PrivateAccumulatedNonRevertibleData` - `PublicAccumulatedNonRevertibleData` This is because we: 1. build up a `CombinedAccumulatedData` during private execution 2. split them into `PrivateAccumulatedNonRevertibleData` and `PrivateAccumulatedRevertibleData` in the private kernel tail 3. convert those `PrivateAccumulated...` into `PublicAccumulated...` during public kernel execution as needed 4. recombine them back into `CombinedAccumulatedData` before feeding back into base rollup ## Encoding The `RevertCode` will come first in the encoded `TxEffect`. That is we will publish: ``` || 32 bytes for RevertCode || ... Existing PublishedTxEffect ... || ``` ## L1 Tx Decoder We'll need to update the availability oracle to compensate for the new flag. ## `TxReceipt` We'll add a new `TxStatus` that is `REVERTED = 'reverted'`. We need to update `getSettledTxReceipt` to inspect the `TxEffect` and set the `status` accordingly. # Test Plan ## Unit Tests - Serde of `TxEffect` in TS and solidity. ## E2E Tests - Add checks for tx statuses of `REVERTED` in the `e2e_fees`, `e2e_deploy_contract`, and `dapp_testing`. # Documentation Plan Will start a page in the "Data publication and availability" section of the yellow paper describing the format of the block submitted. # Especially Relevant Parties - @PhilWindle - @benesjan - @alexghr - @LeilaWang - @LHerskind # Plan Approvals **_👋 Please add a +1 or comment to express your approval or disapproval of the plan above._**
# Introduction Presently, if a transaction is reverted, the base rollup does not mark it as reverted: it merely has its revertible side effects dropped. This issue will update the base rollup to mark reverted transactions. # Solution Design ## New `RevertCode` in `TxEffect` Create a new class in `circuits.js` to hold a `RevertCode`. It will be a safe wrapper around an `Fr` that will be used to store the revert code of a transaction. `0` will indicate success, and any other value will indicate failure. Presently, only `1` is used to indicate general failure, but we'll leave the door open for future expansion. `TxEffect` will be updated to include a `RevertCode`. ### Ethereum `status` Ethereum stores their `status` in a `uint64`, but only currently use `0` to indicate failure and `1` for success. [More info](ethereum/EIPs#98) (Thanks @spalladino !) ### Size considerations We'll eventually want to add `gasUsed` and `gasPrice` to `TxEffect`. Using a full field is wasteful, but using a smaller size will make the encoding and hashing more complex (see below). We'll implement with a full field and have a stacked PR to optimize the size. ## Kernel Constraints The `RevertCode` will become part of the content commitment for a transaction. The content commitment is computed by the base rollup in `compute_tx_effects_hash`, which: - accepts a `CombinedAccumulatedData` - operates over `Field`s We will therefore update the `CombinedAccumulatedData` to include the `RevertCode`. A fallout from that is we will move the current `reverted` flag from `PublicKernelCircuitPublicInputs` to be part of: - `PrivateAccumulatedNonRevertibleData` - `PublicAccumulatedNonRevertibleData` This is because we: 1. build up a `CombinedAccumulatedData` during private execution 2. split them into `PrivateAccumulatedNonRevertibleData` and `PrivateAccumulatedRevertibleData` in the private kernel tail 3. convert those `PrivateAccumulated...` into `PublicAccumulated...` during public kernel execution as needed 4. recombine them back into `CombinedAccumulatedData` before feeding back into base rollup ## Encoding The `RevertCode` will come first in the encoded `TxEffect`. That is we will publish: ``` || 32 bytes for RevertCode || ... Existing PublishedTxEffect ... || ``` ## L1 Tx Decoder We'll need to update the availability oracle to compensate for the new flag. ## `TxReceipt` We'll add a new `TxStatus` that is `REVERTED = 'reverted'`. We need to update `getSettledTxReceipt` to inspect the `TxEffect` and set the `status` accordingly. # Test Plan ## Unit Tests - Serde of `TxEffect` in TS and solidity. ## E2E Tests - Add checks for tx statuses of `REVERTED` in the `e2e_fees`, `e2e_deploy_contract`, and `dapp_testing`. # Documentation Plan Will start a page in the "Data publication and availability" section of the yellow paper describing the format of the block submitted. # Especially Relevant Parties - @PhilWindle - @benesjan - @alexghr - @LeilaWang - @LHerskind # Plan Approvals **_👋 Please add a +1 or comment to express your approval or disapproval of the plan above._**
Parameters
METROPOLIS_FORK_BLKNUM
: TBASpecification
Option 1: For blocks where
block.number >= METROPOLIS_FORK_BLKNUM
, the receipt should remove the intermediate state root parameter.Option 2: For blocks where
block.number >= METROPOLIS_FORK_BLKNUM
, the intermediate state root parameter in the receipt should be set to zero instead of the actual state root.Option 3 (update 2017.07.28: we are going with this one): For blocks where
block.number >= METROPOLIS_FORK_BLKNUM
, the intermediate state root parameter in the receipt should be set to a\x01
byte if the outermost code execution succeeded, or a zero byte if the outermost code execution failed.Rationale
Not calculating the state root after each transaction allows for the process of computing the state root to be parallelized, and for fewer merkle tree branches to be calculated because repeated updates can be hashed. Additionally, it sets the stage for future scalability upgrades where transaction processing itself is done in parallel.
This change DOES mean that if a miner creates a block where one state transition is processed incorrectly, then it is impossible to make a fraud proof specific to that transaction; instead, the fraud proof must consist of the entire block. However, (1) block times are low and we expect block times to reduce in the future with Casper, and (2) we have already accepted the principle that light clients must be capable of downloading entire blocks if need be, because of how verifying bloom filter entries works.
The text was updated successfully, but these errors were encountered: