-
Notifications
You must be signed in to change notification settings - Fork 1.7k
light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure #9824
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,12 +19,12 @@ | |
| use std::cmp; | ||
| use std::sync::Arc; | ||
|
|
||
| use light::on_demand::error::Error as OnDemandError; | ||
| use ethcore::basic_account::BasicAccount; | ||
| use ethcore::encoded; | ||
| use ethcore::filter::Filter as EthcoreFilter; | ||
| use ethcore::ids::BlockId; | ||
| use ethcore::receipt::Receipt; | ||
| use ethcore::executed::ExecutionError; | ||
|
|
||
| use jsonrpc_core::{Result, Error}; | ||
| use jsonrpc_core::futures::{future, Future}; | ||
|
|
@@ -38,6 +38,7 @@ use light::on_demand::{ | |
| request, OnDemand, HeaderRef, Request as OnDemandRequest, | ||
| Response as OnDemandResponse, ExecutionResult, | ||
| }; | ||
| use light::on_demand::error::Error as OnDemandError; | ||
| use light::request::Field; | ||
|
|
||
| use sync::LightSync; | ||
|
|
@@ -202,8 +203,8 @@ impl LightFetch { | |
| /// Helper for getting proved execution. | ||
| pub fn proved_read_only_execution(&self, req: CallRequest, num: Trailing<BlockNumber>) -> impl Future<Item = ExecutionResult, Error = Error> + Send { | ||
| const DEFAULT_GAS_PRICE: u64 = 21_000; | ||
| // starting gas when gas not provided. | ||
| const START_GAS: u64 = 50_000; | ||
| // (21000 G_transaction + 32000 G_create + some marginal to allow a few operations) | ||
| const START_GAS: u64 = 60_000; | ||
|
|
||
| let (sync, on_demand, client) = (self.sync.clone(), self.on_demand.clone(), self.client.clone()); | ||
| let req: CallRequestHelper = req.into(); | ||
|
|
@@ -615,28 +616,41 @@ struct ExecuteParams { | |
| sync: Arc<LightSync>, | ||
| } | ||
|
|
||
| // has a peer execute the transaction with given params. If `gas_known` is false, | ||
| // this will double the gas on each `OutOfGas` error. | ||
| // Has a peer execute the transaction with given params. If `gas_known` is false, this will set the `gas value` to the | ||
| // `required gas value` unless it exceeds the block gas limit | ||
| fn execute_read_only_tx(gas_known: bool, params: ExecuteParams) -> impl Future<Item = ExecutionResult, Error = Error> + Send { | ||
| if !gas_known { | ||
| Box::new(future::loop_fn(params, |mut params| { | ||
| execute_read_only_tx(true, params.clone()).and_then(move |res| { | ||
| match res { | ||
| Ok(executed) => { | ||
| // TODO: how to distinguish between actual OOG and | ||
| // exception? | ||
| if executed.exception.is_some() { | ||
| let old_gas = params.tx.gas; | ||
| params.tx.gas = params.tx.gas * 2u32; | ||
| if params.tx.gas > params.hdr.gas_limit() { | ||
| params.tx.gas = old_gas; | ||
| // `OutOfGas` exception, try double the gas | ||
| if let Some(vm::Error::OutOfGas) = executed.exception { | ||
| // block gas limit already tried, regard as an error and don't retry | ||
| if params.tx.gas >= params.hdr.gas_limit() { | ||
| trace!(target: "light_fetch", "OutOutGas exception received, gas increase: failed"); | ||
| } else { | ||
| params.tx.gas = cmp::min(params.tx.gas * 2_u32, params.hdr.gas_limit()); | ||
| trace!(target: "light_fetch", "OutOutGas exception received, gas increased to {}", | ||
| params.tx.gas); | ||
| return Ok(future::Loop::Continue(params)) | ||
| } | ||
| } | ||
|
|
||
| Ok(future::Loop::Break(Ok(executed))) | ||
| } | ||
| Err(ExecutionError::NotEnoughBaseGas { required, got }) => { | ||
| trace!(target: "light_fetch", "Not enough start gas provided required: {}, got: {}", | ||
| required, got); | ||
| if required <= params.hdr.gas_limit() { | ||
| params.tx.gas = required; | ||
| return Ok(future::Loop::Continue(params)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am just thinking about it, but we probably want to apply a margin (a n% fix additional cost) : since the transaction may not run in the same block (and even in the same block it can vary) the cost may change when running the actual transaction.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is something that we have historically decided not to handle on the node side in order to give the most faithful estimate we can. Wrappers around the RPCs typically add a few percent to the result of |
||
| } else { | ||
| warn!(target: "light_fetch", | ||
| "Required gas is bigger than block header's gas dropping the request"); | ||
| Ok(future::Loop::Break(Err(ExecutionError::NotEnoughBaseGas { required, got }))) | ||
| } | ||
| } | ||
| // Non-recoverable execution error | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here (we need to test to assert the error actually occurs), there is possibly 'ExecutionError::BlockGasLimitReached', maybe we do not need the previous test.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, If we get The possible errors can be found:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I was just wondering if the test above could be remove, but it seems safe to keep it. |
||
| failed => Ok(future::Loop::Break(failed)), | ||
| } | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why dont we try with the
params.hdr.gas_limit()at the very end? I can easily imagine a situation, whereblock_gas_limit / 2 + 1is not sufficient, butblock_gas_limit / 2 + 2is enough.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is todo to use binary search, maybe we should implement that (copy https://github.com/paritytech/parity-ethereum/blob/5f3ae4dee34ede2738473b35db22f8d65351b69b/ethcore/src/client/client.rs#L1539 ?).