Another minor estimation fix #4133

gavofyork · 2017-01-11T14:55:02Z

No description provided.

tomusdrw · 2017-01-11T14:58:00Z

ethcore/src/client/client.rs

@@ -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();


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

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.

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

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

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.

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.

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

arkpar · 2017-01-11T16:21:46Z

ethcore/src/client/client.rs

@@ -893,14 +893,13 @@ impl BlockChainClient for Client {
 			ExecutionError::TransactionMalformed(message)
 		})?;
 		let balance = original_state.balance(&sender);
-		let needed_balance = t.value + t.gas * t.gas_price;
+		let needed_balance = t.value + U256::from(UPPER_CEILING) * t.gas_price;


Maybe use saturating ops to avoid overflow panic?

there's already a lot of these in the codebase (e.g. Client::call) - better to sweep them all at once if it's actually required.

tomusdrw · 2017-01-11T16:23:21Z

ethcore/src/client/client.rs

@@ -892,7 +893,7 @@ impl BlockChainClient for Client {
 			ExecutionError::TransactionMalformed(message)
 		})?;
 		let balance = original_state.balance(&sender);
-		let needed_balance = t.value + t.gas * t.gas_price;
+		let needed_balance = t.value + U256::from(UPPER_CEILING) * t.gas_price;


Should probably use saturating_add (if implemented for U256) to avoid overflows.

there's no U256::saturating_add...

error: no method named `saturating_add` found for type `util::U256` in the current scope --> ethcore/src/client/client.rs:903:34 | 903 | let needed_balance = tx.value.saturating_add(tx.gas.saturating_mul(tx.gas_price)); | ^^^^^^^^^^^^^^

coveralls · 2017-01-11T16:45:13Z

Changes Unknown when pulling c585cc1 on fix-est into ** on master**.

* Return 0 instead of error with out of gas on estimate_gas * Fix stuff up. * Another estimate gas fix. * Alter balance to maximum possible rather than GP=0. * Only increase to amount strictly necessary.

* Fix broken transfer total balance (#4127) * Add proper label to method decoding inputs (#4136) * Another minor estimation fix (#4133) * Return 0 instead of error with out of gas on estimate_gas * Fix stuff up. * Another estimate gas fix. * Alter balance to maximum possible rather than GP=0. * Only increase to amount strictly necessary. * Get rid of unsafe code in ethkey, propagate incorrect Secret errors. (#4119) * Implementing secret * Fixing tests * Refactor VoteCollector (#4101) * dir * simple validator list * stub validator contract * make the engine hold Weak<Client> instead of IoChannel * validator set factory * register weak client with ValidatorContract * check chain security * add address array to generator * register provider contract * update validator set on notify * add validator contract spec * simple list test * split update and contract test * contract change * use client in tendermint * fix deadlock * step duration in params * adapt tendermint tests * add storage fields to test spec * constructor spec * execute under wrong address * create under correct address * revert * validator contract constructor * move genesis block lookup * add removal ability to contract * validator contract adding validators * fix basic authority * validator changing test * more docs * update sync tests * remove env_logger * another env_logger * cameltoe * hold EngineClient instead of Client * return error on misbehaviour * nicer return * sprinkle docs * Reenable mainnet update server. (#4137) * basic tests for subscribeToEvents (#4115) * subscribeToEvent fixtures ✅ * subscribeToEvent tests ✅ * temporarily skip failing test (#4138) * Improvements and optimisations to estimate_gas (#4142) * Return 0 instead of error with out of gas on estimate_gas * Fix stuff up. * Another estimate gas fix. * Alter balance to maximum possible rather than GP=0. * Only increase to amount strictly necessary. * Improvements and optimisations to estimate_gas. - Introduce proper error type - Avoid building costly traces * Fix tests. * Actually fix testsActually fix tests * Use estimateGas error (as per updated implementation) (#4131) * Use estimateGas error (as per updated implementation) * EXCEPTION_ERROR as per #4142 * Better error log reporting & handling (#4128) * Don't pop-up notifications after network switch (#4076) * Better notifications * Don't pollute with notifs if switched networks * Better connection close/open events / No more notifs on change network * PR Grumbles * Add close and open events to HTTP // Add tests * Fix tests * WIP Signer Fix * Fix Signer // Better reconnection handling * PR Grumbles * PR Grumbles * Fixes wrong fetching of balances + Notifications * Secure API WIP * Updated Secure API Connection + Status * Linting * Linting * Updated Secure API Logic * Proper handling of token updates // Fixing poping notifications * PR Grumbles * PR Grumbles * Fixing tests * Trim spaces from InputAddress (#4126) * Trim spaces for addresses * onSubmit has only value, not event * onSubmit (again) * Length check on trimmed value * Remove bindActionCreators({}, dispatch) (empty) (#4135)

gavofyork added 4 commits January 10, 2017 19:42

Return 0 instead of error with out of gas on estimate_gas

7b93fc9

Fix stuff up.

f11f8c2

Another estimate gas fix.

82e0775

Merge remote-tracking branch 'origin/master' into fix-est

a04ff60

gavofyork added A0-pleasereview 🤓 Pull request needs code review. B0-patch M4-core ⛓ Core client code / Rust. labels Jan 11, 2017

tomusdrw reviewed Jan 11, 2017

View reviewed changes

gavofyork changed the title ~~Another miner estimation fix~~ Another minor estimation fix Jan 11, 2017

Alter balance to maximum possible rather than GP=0.

116386e

arkpar reviewed Jan 11, 2017

View reviewed changes

Only increase to amount strictly necessary.

c585cc1

tomusdrw reviewed Jan 11, 2017

View reviewed changes

arkpar added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 11, 2017

arkpar merged commit 56c546f into master Jan 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Another minor estimation fix #4133

Another minor estimation fix #4133

gavofyork commented Jan 11, 2017

tomusdrw Jan 11, 2017

gavofyork Jan 11, 2017 •

edited

Loading

arkpar Jan 11, 2017

tomusdrw Jan 11, 2017

gavofyork Jan 11, 2017 •

edited

Loading

gavofyork Jan 11, 2017 •

edited

Loading

tomusdrw Jan 11, 2017

arkpar Jan 11, 2017

gavofyork Jan 11, 2017

tomusdrw Jan 11, 2017

gavofyork Jan 11, 2017 •

edited

Loading

gavofyork Jan 11, 2017

coveralls commented Jan 11, 2017

Another minor estimation fix #4133

Another minor estimation fix #4133

Conversation

gavofyork commented Jan 11, 2017

Choose a reason for hiding this comment

gavofyork Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

gavofyork Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 11, 2017

gavofyork Jan 11, 2017 •

edited

Loading

gavofyork Jan 11, 2017 •

edited

Loading

gavofyork Jan 11, 2017 •

edited

Loading

gavofyork Jan 11, 2017 •

edited

Loading