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

niklasad1 · 2018-10-29T15:03:58Z

Attempt to close #6155 & close #9031

What does this PR:

Differentiate between out-of-gas/manual throw
Changes the start_gas from 50_000 to 60_000
If too little gas is submitted in a transaction then we use to the required start_gas replied to us to set the next start_gas if it doesn't exceed the block's gas limit (however this could be changed to set the start_gas regardless of block_limit and continue until we get BlockGasLimitReached error instead, will probably require at least one extra network request)

Example execution with 50_000 as start_gas:

2018-10-29 15:45:05  jsonrpc-eventloop-1 TRACE light_fetch  Placing execution request for 50000 gas in on_demand
2018-10-29 15:45:05  jsonrpc-eventloop-1 TRACE light_fetch  Not enough start gas provided required: 53000, got: 50000
2018-10-29 15:45:05  jsonrpc-eventloop-1 TRACE light_fetch  Placing execution request for 53000 gas in on_demand

ordian · 2018-10-29T15:17:22Z

@niklasad1 looks nice, but I'm yet to test it. Could you add a unit test? :)

niklasad1 · 2018-10-29T16:15:03Z

@ordian
Yes, ideally I should have but it would require to mock a Full Node which is a much bigger task. Is manual testing ok for this one?

Might be a little annoying but change the start_gas in proved_read_only_execution to some value and create eth_call request without gas from an account with zero balance!

cheme

The PR looks good (with the switch to virtual this is a nice optimization and should indeed solve the transaction cost issue (I did not test)), I got a few comment/suggestion.

I still believe we need to switch back to non virtual execution or find a way to circumvent the issue I talk to you about last friday (currently I could not think of any other way than switching back).

rpc/src/v1/helpers/light_fetch.rs

cheme · 2018-10-30T07:45:23Z

rpc/src/v1/helpers/light_fetch.rs

-							} else {
-								return Ok(future::Loop::Continue(params))
-							}
+						// `OutOfGas` exception can't occur here because of virtual execution context


Even if it will not be executed, I think we should keep the existing code (with the new NotEnoughBaseGas exeption). That way we can still support non updated full peers (there should not be much in fact), and we could revert to non virtual full node execution more easily in the future (switching back to non virtual will not imply having to update every light client).

I don't understand this. NotEnoughBasGas is not an exception, see https://github.com/paritytech/parity-ethereum/blob/master/ethcore/vm/src/error.rs#L38-#L86

Clarify please :P

👍 indeed, we could use vm::Error::OutOfGas instead.

Right, got it very good catch 👍

cheme · 2018-10-30T08:00:21Z

rpc/src/v1/helpers/light_fetch.rs

+							Ok(future::Loop::Break(Err(ExecutionError::NotEnoughBaseGas { required, got })))
+						}
+					}
+					// Non-recoverable execution error


Here (we need to test to assert the error actually occurs), there is possibly 'ExecutionError::BlockGasLimitReached', maybe we do not need the previous test.

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

Yes I was just wondering if the test above could be remove, but it seems safe to keep it.

cheme · 2018-10-30T08:08:54Z

rpc/src/v1/helpers/light_fetch.rs

+						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))


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

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.

cheme · 2018-11-08T22:25:18Z

rpc/src/v1/helpers/light_fetch.rs

-						// TODO: how to distinguish between actual OOG and
-						// exception?
-						if executed.exception.is_some() {
+						// `OutOfGas` exception can only occur on full-nodes that run `Parity-Ethereum v2.1.1` or older


From our chat conversation of today, I think we may still run into out of gas.

Thanks, should be sufficient just to change the comment right?

I think so, (we thankfully kept code for iterative querying).

tomusdrw · 2018-11-13T10:32:22Z

rpc/src/v1/helpers/light_fetch.rs

 							if params.tx.gas > params.hdr.gas_limit() {
+								trace!(target: "light_fetch", "OutOutGas exception received, gas increase: failed");


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.

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

ordian

I'm in favor of merging this PR. This comment can be addressed in a separate one.

cheme · 2018-11-13T16:17:49Z

Same thing here, I just feel that @tomusdrw comment should be addressed (when we multiply gas by 2 we should finish with max block size (if not already eq to max block size)).

Something along the line of :

if params.tx.gas == params.hdr.gas_limit() {
  trace!(target: "light_fetch", "OutOutGas exception received, gas increase: failed");
} else {
  params.tx.gas = params.tx.gas * 2_u32;

  if params.tx.gas > params.hdr.gas_limit() {
    params.tx.gas = params.hdr.gas_limit();
  }
  trace!(target: "light_fetch", "OutOutGas exception received, gas increase: succeed");
  return Ok(future::Loop::Continue(params))
}

* Change START_GAS: 50_000 -> 60_000 * When the `OutOfGas exception` is received then try to double the gas until it succeeds or block gas limit is reached * When `NotEnoughBasGas error` is received then use the required gas provided in the response

Try the `block_gas_limit` before regard the execution as an error

cheme

LGTM, just a small nitpick on the comparison but it should not really be an issue.

rpc/src/v1/helpers/light_fetch.rs

Co-Authored-By: niklasad1 <niklasadolfsson1@gmail.com>

…quired gas from response on failure (#9824) * fix start_gas, handle OOG exceptions & NotEnoughGas * Change START_GAS: 50_000 -> 60_000 * When the `OutOfGas exception` is received then try to double the gas until it succeeds or block gas limit is reached * When `NotEnoughBasGas error` is received then use the required gas provided in the response * fix(light-fetch): ensure block_gas_limit is tried Try the `block_gas_limit` before regard the execution as an error * Update rpc/src/v1/helpers/light_fetch.rs Co-Authored-By: niklasad1 <niklasadolfsson1@gmail.com>

* bump stable 2.1.6 * ethcore: use Machine::verify_transaction on parent block (#9900) * ethcore: use Machine::verify_transaction on parent block also fixes off-by-one activation of transaction permission contract * ethcore: clarify call to verify_transaction * fix: Intermittent failing CI due to addr in use (#9885) Allow OS to set port at runtime * gitlab-ci: make android release build succeed (#9743) * use docker cargo config file for android builds * make android build succeed * Update light-client hardcoded headers for foundation: #6692865, ropsten: #4417537, kovan: #9363457 * Remove rust-toolchain file (#9906) * light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure (#9824) * fix start_gas, handle OOG exceptions & NotEnoughGas * Change START_GAS: 50_000 -> 60_000 * When the `OutOfGas exception` is received then try to double the gas until it succeeds or block gas limit is reached * When `NotEnoughBasGas error` is received then use the required gas provided in the response * fix(light-fetch): ensure block_gas_limit is tried Try the `block_gas_limit` before regard the execution as an error * Update rpc/src/v1/helpers/light_fetch.rs Co-Authored-By: niklasad1 <niklasadolfsson1@gmail.com> * fix #9824 merge artifacts * simplify cargo audit * ci: nuke the gitlab caches (#9855)

* Bump beta to version 2.2.1 * fix: Intermittent failing CI due to addr in use (#9885) Allow OS to set port at runtime * Use Weak reference in PubSubClient (#9886) * Fix json tracer overflow (#9873) * Fix json tracer overflow * Replace trace_executed with a direct trace push * Remove unused variable * Add test for 5a51 * Remove duplicate json! * Fix docker script (#9854) * Dockerfile: change source path of the newly added check_sync.sh (#9869) * Allow to seal work on latest block (#9876) * Allow to seal work on latest block. * Test from @todr to check sealing conditions. * gitlab-ci: make android release build succeed (#9743) * use docker cargo config file for android builds * make android build succeed * ethcore: use Machine::verify_transaction on parent block (#9900) * ethcore: use Machine::verify_transaction on parent block also fixes off-by-one activation of transaction permission contract * ethcore: clarify call to verify_transaction * foundation: #6692865, ropsten: #4417537, kovan: #9363457 * Remove rust-toolchain file (#9906) * EIP-712 implementation (#9631) * EIP-712 impl * added more tests * removed size parsing unwrap * corrected TYPE_REGEX to disallow zero sized fixed length arrays, replaced LinkedHashSet with IndexSet, added API spec to docs, fixed Type::Byte encoding branch * use Option<u64> instead of u64 for Type::Array::Length * replace `.iter()` with `.values()` Co-Authored-By: seunlanlege <seunlanlege@gmail.com> * tabify eip712.rs * use proper comments for docs * Cargo.lock: revert unrelated changes * tabify encode.rs * EIP 191 (#9701) * added sign_191 rpc method * fixed hash_structured_data return type * added ConfirmationPayload::SignMessage for non-prefixed signatures, added tests for sign191 * renamed WithValidator -> PresignedTransaction * rename applicationData to data in test * adds docs for EIP191Version, renamed SignRequest to EIP191SignRequest * light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure (#9824) * fix start_gas, handle OOG exceptions & NotEnoughGas * Change START_GAS: 50_000 -> 60_000 * When the `OutOfGas exception` is received then try to double the gas until it succeeds or block gas limit is reached * When `NotEnoughBasGas error` is received then use the required gas provided in the response * fix(light-fetch): ensure block_gas_limit is tried Try the `block_gas_limit` before regard the execution as an error * Update rpc/src/v1/helpers/light_fetch.rs Co-Authored-By: niklasad1 <niklasadolfsson1@gmail.com> * simplify cargo audit * Use block header for building finality (#9914) * ci: nuke the gitlab caches (#9855)

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 29, 2018

ordian added B1-patch-beta 🕷🕷 B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. labels Oct 29, 2018

ordian added this to the 2.3 milestone Oct 29, 2018

niklasad1 force-pushed the light/readonly-cant-run-out-of-gas branch 2 times, most recently from 452a20d to 418d20f Compare October 29, 2018 15:41

niklasad1 requested a review from cheme October 29, 2018 15:55

niklasad1 closed this Oct 29, 2018

niklasad1 reopened this Oct 29, 2018

cheme reviewed Oct 30, 2018

View reviewed changes

niklasad1 changed the title ~~light: Execute read_only can't run out-of-gas and use required gas from response on failure~~ light-fetch: Differiate between out-of-gas/manual throw and use required gas from response on failure Oct 30, 2018

niklasad1 changed the title ~~light-fetch: Differiate between out-of-gas/manual throw and use required gas from response on failure~~ light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure Oct 30, 2018

niklasad1 force-pushed the light/readonly-cant-run-out-of-gas branch from 418d20f to 592ca94 Compare October 30, 2018 12:59

This was referenced Nov 1, 2018

version: mark 2.2.0 beta #9820

Merged

version: mark 2.1.5 stable #9821

Merged

cheme reviewed Nov 8, 2018

View reviewed changes

niklasad1 force-pushed the light/readonly-cant-run-out-of-gas branch 2 times, most recently from 6f8f743 to 4786558 Compare November 9, 2018 10:36

This was referenced Nov 13, 2018

Backport Beta 2.2.1 #9905

Merged

Backport Stable 2.1.6 #9904

Merged

tomusdrw reviewed Nov 13, 2018

View reviewed changes

ordian approved these changes Nov 13, 2018

View reviewed changes

niklasad1 added 2 commits November 13, 2018 17:45

fix(light-fetch): ensure block_gas_limit is tried

73c00b9

Try the `block_gas_limit` before regard the execution as an error

niklasad1 force-pushed the light/readonly-cant-run-out-of-gas branch from 4786558 to 73c00b9 Compare November 13, 2018 17:31

cheme approved these changes Nov 13, 2018

View reviewed changes

rpc/src/v1/helpers/light_fetch.rs Outdated Show resolved Hide resolved

Update rpc/src/v1/helpers/light_fetch.rs

597fa52

Co-Authored-By: niklasad1 <niklasadolfsson1@gmail.com>

ordian merged commit 23a2943 into master Nov 14, 2018

ordian deleted the light/readonly-cant-run-out-of-gas branch November 14, 2018 10:04

ordian added a commit that referenced this pull request Nov 14, 2018

fix #9824 merge artifacts

e36c89a

ordian added a commit that referenced this pull request Nov 14, 2018

fix #9824 merge artifacts

dc7623a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

niklasad1 commented Oct 29, 2018 •

edited

Loading

ordian commented Oct 29, 2018

niklasad1 commented Oct 29, 2018

cheme left a comment

cheme Oct 30, 2018

niklasad1 Oct 30, 2018

cheme Oct 30, 2018

niklasad1 Oct 30, 2018

cheme Oct 30, 2018

niklasad1 Oct 30, 2018 •

edited

Loading

cheme Oct 30, 2018

cheme Oct 30, 2018

rphmeier Oct 31, 2018

cheme Nov 8, 2018

niklasad1 Nov 9, 2018 •

edited

Loading

cheme Nov 9, 2018

tomusdrw Nov 13, 2018

ordian Nov 13, 2018

ordian left a comment

cheme commented Nov 13, 2018

cheme left a comment

		if params.tx.gas > params.hdr.gas_limit() {
		trace!(target: "light_fetch", "OutOutGas exception received, gas increase: failed");

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

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

Conversation

niklasad1 commented Oct 29, 2018 • edited Loading

ordian commented Oct 29, 2018

niklasad1 commented Oct 29, 2018

cheme left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Oct 30, 2018 • 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

niklasad1 Nov 9, 2018 • 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

ordian left a comment

Choose a reason for hiding this comment

cheme commented Nov 13, 2018

cheme left a comment

Choose a reason for hiding this comment

niklasad1 commented Oct 29, 2018 •

edited

Loading

niklasad1 Oct 30, 2018 •

edited

Loading

niklasad1 Nov 9, 2018 •

edited

Loading