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

Another minor estimation fix #4133

Merged
merged 6 commits into from
Jan 11, 2017
Merged

Another minor estimation fix #4133

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7b93fc9
Return 0 instead of error with out of gas on estimate_gas
gavofyork Jan 10, 2017
f11f8c2
Fix stuff up.
gavofyork Jan 11, 2017
82e0775
Another estimate gas fix.
gavofyork Jan 11, 2017
a04ff60
Merge remote-tracking branch 'origin/master' into fix-est
gavofyork Jan 11, 2017
116386e
Alter balance to maximum possible rather than GP=0.
gavofyork Jan 11, 2017
c585cc1
Only increase to amount strictly necessary.
gavofyork Jan 11, 2017
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
7 changes: 4 additions & 3 deletions ethcore/src/client/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,7 @@ impl BlockChainClient for Client {
}

fn estimate_gas(&self, t: &SignedTransaction, block: BlockId) -> Result<U256, CallError> {
const UPPER_CEILING: u64 = 1_000_000_000_000u64;
let header = self.block_header(block).ok_or(CallError::StatePruned)?;
let last_hashes = self.build_last_hashes(header.parent_hash());
let env_info = EnvInfo {
Expand All @@ -883,7 +884,7 @@ impl BlockChainClient for Client {
difficulty: header.difficulty(),
last_hashes: last_hashes,
gas_used: U256::zero(),
gas_limit: U256::max_value(),
gas_limit: UPPER_CEILING.into(),
};
// that's just a copy of the state.
let mut original_state = self.state_at(block).ok_or(CallError::StatePruned)?;
Expand All @@ -899,6 +900,7 @@ impl BlockChainClient for Client {
}
let options = TransactOptions { tracing: true, vm_tracing: false, check_nonce: false };
let mut tx = t.clone();
tx.gas_price = 0.into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

That kind of breaks the whole purpose of accepting (optional) gas_price in the RPC, shouldn't the caller decide this?

Copy link
Contributor Author

@gavofyork gavofyork Jan 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

not really relevant for estimate_gas; i guess a slightly nicer solution might be to introduce additional flags to skip the gas payment part of tx execution, but otherwise i don't really see a cleaner way if the sender account doesn't necessarily have enough of a balance to pay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Balance increase at line 898 is no longer necessary if the gas price is zero.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It affects the value that can be transfered (lower by gas*gas_price) and also may affect executed code (GASPRICE opcode)

Copy link
Contributor Author

@gavofyork gavofyork Jan 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

yeah - there is literally nothing we can do to avoid that. unless we alter the state or the transaction, then it becomes impossible to estimate the gas "correctly" and the transaction can always evade an accurate result through the use of GASPRICE, GAS, BALANCE operations. indeed, it is possible to craft a transaction for which estimate_gas could never return a correct result by relying on such things as BLOCKHASH.

the model we assume is simplistic and has issues with certain edge circumstances, however estimate gas is literally unsolvable in the general case (halting problem). for 99% of cases, this approach works. for the rest, the user shouldn't be using it.

Copy link
Contributor Author

@gavofyork gavofyork Jan 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ok - i'll introduce an alternative way - increasing the sender account balance to the maximum amount possibly needed. slightly better than altering the tx, since we change that already in the case that there's not enough value in it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering why lowering the gas price is needed at all?


let mut cond = |gas| {
let mut state = original_state.clone();
Expand All @@ -909,11 +911,10 @@ impl BlockChainClient for Client {
.unwrap_or(false)
};

let mut upper = env_info.gas_limit;
let mut upper = header.gas_limit();
if !cond(upper) {
// impossible at block gas limit - try `UPPER_CEILING` instead.
// TODO: consider raising limit by powers of two.
const UPPER_CEILING: u64 = 1_000_000_000_000u64;
upper = UPPER_CEILING.into();
if !cond(upper) {
trace!(target: "estimate_gas", "estimate_gas failed with {}", upper);
Expand Down
1 change: 0 additions & 1 deletion rpc/src/v1/impls/eth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,6 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM> Eth for EthClient<C, SN, S, M, EM> where
num => take_weak!(self.client).call(&signed, num.into(), Default::default()),
};


result
.map(|b| b.output.into())
.map_err(errors::from_call_error)
Expand Down