Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure #9824

Merged
merged 3 commits into from
Nov 14, 2018
Merged

light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure #9824

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0694b2
fix start_gas, handle OOG exceptions & NotEnoughGas
niklasad1 Oct 29, 2018
73c00b9
fix(light-fetch): ensure block_gas_limit is tried
niklasad1 Nov 13, 2018
597fa52
Update rpc/src/v1/helpers/light_fetch.rs
cheme Nov 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions rpc/src/v1/helpers/light_fetch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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() {
niklasad1 marked this conversation as resolved.
Show resolved Hide resolved
trace!(target: "light_fetch", "OutOutGas exception received, gas increase: failed");
Copy link
Collaborator

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, where block_gas_limit / 2 + 1 is not sufficient, but block_gas_limit / 2 + 2 is enough.

Copy link
Collaborator

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 ?).

} 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))
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
It is not really related to this PR, but likely to happen with some contracts, this PR may makes it more obvious (previously by doubling gas cost there was lesser chance for it to happen).

Copy link
Contributor

Choose a reason for hiding this comment

The 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 estimateGas.

} 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

@niklasad1 niklasad1 Oct 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, If we get BlockGasLimitReached then it is definitely a faulty request and AFAIK any of the others errors can't be recovered!

The possible errors can be found:
https://github.com/paritytech/parity-ethereum/blob/master/ethcore/src/executed.rs#L74-#L118

Copy link
Contributor

Choose a reason for hiding this comment

The 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)),
}
})
Expand Down