fix (light/provider) : Make `read_only executions` read-only #9591

niklasad1 · 2018-09-19T09:28:44Z

Attempt to close #9551 and fix #9535

/cc @Tbaut can you try this with fether?

This changes so all ExecutionRequests from light-clients are executed
as read-only which the virtual flag == true ensures.

This boost up the current transaction to always succeed such as account balance and similar!

Note, this only affects eth_estimateGas and eth_call AFAIK.

Manual tests on (kovan)

Started a provider (full node)
Request from light client with unknown account 0x0066Dc48bb833d2B59f730F33952B3c29fE926ff

curl --data '{"method":"eth_estimateGas","params":[{"from":"0x0066Dc48bb833d2B59f730F33952B3c29fE926ff", "gas":"0xffff" }, "latest"],"id":1,"jsonrpc":"2.0"}' -H "Content-Type: application/json" -X POST localhost:8555
{"jsonrpc":"2.0","result":"0xcf08","id":1}

niklasad1 · 2018-09-19T09:34:12Z

ethcore/src/client/client.rs

@@ -2357,7 +2358,7 @@ impl ProvingBlockChainClient for Client {
 			self.engine.machine(),
 			&env_info,
 			self.factories.clone(),
-			false,
+			true,


This will now use transact_virtual to ensure that the account has enough balance for the execution

What do you think about removing virt parameter from state::prove_transaction (always set to true) and add docs to prove_transaction trait method?

niklasad1 · 2018-09-19T09:35:05Z

ethcore/light/src/on_demand/request.rs

@@ -61,7 +61,7 @@ pub enum Request {
 	/// A request for a contract's code.
 	Code(Code),
 	/// A request for proof of execution.
-	Execution(TransactionProof),
+	Execution(ReadOnlyTransactionProof),


More explicit name

I have a mixed feelings about this rename, because ReadOnlyTransactionProof implies there is NonReadOnlyTransactionProof.

Ok, I will revert this!

Tbaut · 2018-09-19T13:28:46Z

Thanks for tackling that @niklasad1
I gave it a shot and unfortunately this did not succeed. Here is a trace with the RPCs: https://gist.github.com/Tbaut/f13f6c26e80d20227cb46d5c3765daab

ordian · 2018-09-19T13:31:50Z

@Tbaut note, that it fixes full node's side of light client, so to properly test this, you need to connect light client to a patched full node (reserved and reserved-only connection is broken on master)

Tbaut · 2018-09-19T13:41:38Z

Thanks @ordian, I definitely did not test it (and I couldn't have had indeed :( ).

niklasad1 · 2018-09-19T14:26:58Z

@Tbaut @ordian

I brute-forced this and spinned up a regular full-node (not reserved) along with a light client and it works (eventually found) my full node!

From your logs it looks like eth_call is the problem which this PR fixes.
Also, eth_getBalance will return as long as it gets at least one reply from its peers regardless whether the account exists or not!

Tbaut · 2018-09-19T15:40:32Z

FWIW, I tested Fether about 1-2 months ago by connecting it directly to a light client that was connected to a full node using reserved peers, and I had fewer problems than when the light client was connecting to nodes in the wild.

ordian

Can confirm that it fixes estimateGas problem. Not sure about #9551 though.

ordian · 2018-09-27T20:59:38Z

ethcore/light/src/on_demand/request.rs

@@ -61,7 +61,7 @@ pub enum Request {
 	/// A request for a contract's code.
 	Code(Code),
 	/// A request for proof of execution.
-	Execution(TransactionProof),
+	Execution(ReadOnlyTransactionProof),


I have a mixed feelings about this rename, because ReadOnlyTransactionProof implies there is NonReadOnlyTransactionProof.

ordian · 2018-09-27T20:59:42Z

ethcore/src/client/client.rs

@@ -2374,6 +2374,7 @@ impl ProvingBlockChainClient for Client {
 	}

 	fn prove_transaction(&self, transaction: SignedTransaction, id: BlockId) -> Option<(Bytes, Vec<DBValue>)> {
+		trace!(target: "pip_provider", "prove_transaction: {:?}", transaction);


Looks like prove_transaction is also used in full node to ensure genesis epoch proof in the DB, so not sure about this trace target.

ordian · 2018-09-27T21:00:26Z

ethcore/src/client/client.rs

@@ -2357,7 +2358,7 @@ impl ProvingBlockChainClient for Client {
 			self.engine.machine(),
 			&env_info,
 			self.factories.clone(),
-			false,
+			true,


What do you think about removing virt parameter from state::prove_transaction (always set to true) and add docs to prove_transaction trait method?

This changes so all `ExecutionRequests` from light-clients are executed as read-only which the `virtual``flag == true ensures. This boost up the current transaction to always succeed Note, this only affects `eth_estimateGas` and `eth_call` AFAIK.

Remove the boolean flag to determine that a `state::prove_transaction` whether it should be executed in a virtual context or not. Because of that also rename the function to `state::prove_transction_virtual` to make more clear

cheme · 2018-10-08T14:00:34Z

I am starting to wonder if it is really fine to run transaction as virtual in this case. For instance if I want to send a transaction and do not put enough gas maybe I would rather have a failure than a success (success leading me to send my transaction and lose my gas).

What I am saying, is that we probably want light client 'eth_estimateGas' call to run as virtual, but 'eth_call' to run as normal. With this PR it always run as virtual.
I am also not sure with the renaming (for the same reason as stated before).

I need confirmation that I am incorrect before approving, maybe @rphmeier .

Even if I am correct, the problem remains: in 'rpc/src/v1/helpers/light_fetch.rs' both call and estimategas run on the same rpc call (proved_execution) and we probably want to run virtual only for estimateGas which does not seems possible with current LES definition. So the change from this PR will make estimateGas works for light client but might break call for light client. I do not really see a good solution that does not required changing LES a bit (if changing LES there is also certainly something to do about #6155 ).

ordian · 2018-10-08T15:26:56Z

@cheme IIUC eth_call is a read-only operation by design and for full node it is implemented as virtual call.

cheme · 2018-10-08T16:48:44Z

Indeed, so it is the same behavior, must be correct.

cheme

LGTM. Maybe the renaming of the function is a bit overdoing it (I do not think that there is other version of the function, the comment change would have been enough for me).

…mon-deps * origin/master: fix (light/provider) : Make `read_only executions` read-only (#9591) ethcore: fix detection of major import (#9552) return 0 on error (#9705) ethcore: delay ropsten hardfork (#9704) make instantSeal engine backwards compatible, closes #9696 (#9700) Implement CREATE2 gas changes and fix some potential overflowing (#9694) Don't hash the init_code of CREATE. (#9688) ethcore: minor optimization of modexp by using LR exponentiation (#9697) removed redundant clone before each block import (#9683)

* `ExecutionsRequest` from light-clients as read-only This changes so all `ExecutionRequests` from light-clients are executed as read-only which the `virtual``flag == true ensures. This boost up the current transaction to always succeed Note, this only affects `eth_estimateGas` and `eth_call` AFAIK. * grumbles(revert renaming) : TransactionProof * grumbles(trace) : remove incorrect trace * grumbles(state/prove_tx) : explicit `virt` Remove the boolean flag to determine that a `state::prove_transaction` whether it should be executed in a virtual context or not. Because of that also rename the function to `state::prove_transction_virtual` to make more clear

* parity-version: bump beta to 2.1.2 * docs(rpc): push the branch along with tags (#9578) * docs(rpc): push the branch along with tags * ci: remove old rpc docs script * Remove snapcraft clean (#9585) * Revert " add snapcraft package image (master) (#9584)" This reverts commit ceaedbb. * Update package-snap.sh * Update .gitlab-ci.yml * ci: fix regex 🙄 (#9597) * docs(rpc): annotate tag with the provided message (#9601) * Update ropsten.json (#9602) * HF in POA Sokol (2018-09-19) (#9607) poanetwork/poa-chain-spec#86 * fix(network): don't disconnect reserved peers (#9608) The priority of && and || was borked. * fix failing node-table tests on mac os, closes #9632 (#9633) * ethcore-io retries failed work steal (#9651) * ethcore-io uses newer version of crossbeam && retries failed work steal * ethcore-io non-mio service uses newer crossbeam * remove master from releasable branches (#9655) * remove master from releasable branches need backporting in beta fix https://gitlab.parity.io/parity/parity-ethereum/-/jobs/101065 etc * add except for snap packages for master * Test fix for windows cache name... (#9658) * Test fix for windows cache name... * Fix variable name. * fix(light_fetch): avoid race with BlockNumber::Latest (#9665) * Calculate sha3 instead of sha256 for push-release. (#9673) * Calculate sha3 instead of sha256 for push-release. * Add pushes to the script. * Hardfork the testnets (#9562) * ethcore: propose hardfork block number 4230000 for ropsten * ethcore: propose hardfork block number 9000000 for kovan * ethcore: enable kip-4 and kip-6 on kovan * etcore: bump kovan hardfork to block 9.2M * ethcore: fix ropsten constantinople block number to 4.2M * ethcore: disable difficulty_test_ropsten until ethereum/tests are updated upstream * ci: fix push script (#9679) * ci: fix push script * Fix copying & running on windows. * CI: Remove unnecessary pipes (#9681) * ci: reduce gitlab pipelines significantly * ci: build pipeline for PR * ci: remove dead weight * ci: remove github release script * ci: remove forever broken aura tests * ci: add random stuff to the end of the pipes * ci: add wind and mac to the end of the pipe * ci: remove snap artifacts * ci: (re)move dockerfiles * ci: clarify job names * ci: add cargo audit job * ci: make audit script executable * ci: ignore snap and docker files for rust check * ci: simplify audit script * ci: rename misc to optional * ci: add publish script to releaseable branches * ci: more verbose cp command for windows build * ci: fix weird binary checksum logic in push script * ci: fix regex in push script for windows * ci: simplify gitlab caching * docs: align README with ci changes * ci: specify default cargo target dir * ci: print verbose environment * ci: proper naming of scripts * ci: restore docker files * ci: use docker hub file * ci: use cargo home instead of cargo target dir * ci: touch random rust file to trigger real builds * ci: set cargo target dir for audit script * ci: remove temp file * ci: don't export the cargo target dir in the audit script * ci: fix windows unbound variable * docs: fix gitlab badge path * rename deprecated gitlab ci variables https://docs.gitlab.com/ee/ci/variables/#9-0-renaming * ci: fix git compare for nightly builds * test: skip c++ example for all platforms but linux * ci: add random rust file to trigger tests * ci: remove random rust file * disable cpp lib test for mac, win and beta (#9686) * cleanup ci merge * ci: fix tests * fix bad-block reporting no reason (#9638) * ethcore: fix detection of major import (#9552) * sync: set state to idle after sync is completed * sync: refactor sync reset * Don't hash the init_code of CREATE. (#9688) * Docker: run as parity user (#9689) * Implement CREATE2 gas changes and fix some potential overflowing (#9694) * Implement CREATE2 gas changes and fix some potential overflowing * Ignore create2 state tests * Split CREATE and CREATE2 in gasometer * Generalize rounding (x + 31) / 32 to to_word_size * make instantSeal engine backwards compatible, closes #9696 (#9700) * ethcore: delay ropsten hardfork (#9704) * fix (light/provider) : Make `read_only executions` read-only (#9591) * `ExecutionsRequest` from light-clients as read-only This changes so all `ExecutionRequests` from light-clients are executed as read-only which the `virtual``flag == true ensures. This boost up the current transaction to always succeed Note, this only affects `eth_estimateGas` and `eth_call` AFAIK. * grumbles(revert renaming) : TransactionProof * grumbles(trace) : remove incorrect trace * grumbles(state/prove_tx) : explicit `virt` Remove the boolean flag to determine that a `state::prove_transaction` whether it should be executed in a virtual context or not. Because of that also rename the function to `state::prove_transction_virtual` to make more clear * CI: Skip docs job for nightly (#9693) * ci: force-tag wiki changes * ci: force-tag wiki changes * ci: skip docs job for master and nightly * ci: revert docs job checking for nightly tag * ci: exclude docs job from nightly builds in gitlab script

* `ExecutionsRequest` from light-clients as read-only This changes so all `ExecutionRequests` from light-clients are executed as read-only which the `virtual``flag == true ensures. This boost up the current transaction to always succeed Note, this only affects `eth_estimateGas` and `eth_call` AFAIK. * grumbles(revert renaming) : TransactionProof * grumbles(trace) : remove incorrect trace * grumbles(state/prove_tx) : explicit `virt` Remove the boolean flag to determine that a `state::prove_transaction` whether it should be executed in a virtual context or not. Because of that also rename the function to `state::prove_transction_virtual` to make more clear

* parity-version: bump stable to 2.0.7 * HF in POA Sokol (2018-09-19) (#9607) poanetwork/poa-chain-spec#86 * fix failing node-table tests on mac os, closes #9632 (#9633) * fix(light_fetch): avoid race with BlockNumber::Latest (#9665) * CI: Remove unnecessary pipes (#9681) * ci: reduce gitlab pipelines significantly * ci: build pipeline for PR * ci: remove dead weight * ci: remove github release script * ci: remove forever broken aura tests * ci: add random stuff to the end of the pipes * ci: add wind and mac to the end of the pipe * ci: remove snap artifacts * ci: (re)move dockerfiles * ci: clarify job names * ci: add cargo audit job * ci: make audit script executable * ci: ignore snap and docker files for rust check * ci: simplify audit script * ci: rename misc to optional * ci: add publish script to releaseable branches * ci: more verbose cp command for windows build * ci: fix weird binary checksum logic in push script * ci: fix regex in push script for windows * ci: simplify gitlab caching * docs: align README with ci changes * ci: specify default cargo target dir * ci: print verbose environment * ci: proper naming of scripts * ci: restore docker files * ci: use docker hub file * ci: use cargo home instead of cargo target dir * ci: touch random rust file to trigger real builds * ci: set cargo target dir for audit script * ci: remove temp file * ci: don't export the cargo target dir in the audit script * ci: fix windows unbound variable * docs: fix gitlab badge path * rename deprecated gitlab ci variables https://docs.gitlab.com/ee/ci/variables/#9-0-renaming * ci: fix git compare for nightly builds * test: skip c++ example for all platforms but linux * ci: add random rust file to trigger tests * ci: remove random rust file * disable cpp lib test for mac, win and beta (#9686) * cleanup ci merge * parity: bump clib * ci: fix tests * ci: disable c++ example * Docker: run as parity user (#9689) * CI: Skip docs job for nightly (#9693) * ci: force-tag wiki changes * ci: force-tag wiki changes * ci: skip docs job for master and nightly * ci: revert docs job checking for nightly tag * ci: exclude docs job from nightly builds in gitlab script * fix (light/provider) : Make `read_only executions` read-only (#9591) * `ExecutionsRequest` from light-clients as read-only This changes so all `ExecutionRequests` from light-clients are executed as read-only which the `virtual``flag == true ensures. This boost up the current transaction to always succeed Note, this only affects `eth_estimateGas` and `eth_call` AFAIK. * grumbles(revert renaming) : TransactionProof * grumbles(trace) : remove incorrect trace * grumbles(state/prove_tx) : explicit `virt` Remove the boolean flag to determine that a `state::prove_transaction` whether it should be executed in a virtual context or not. Because of that also rename the function to `state::prove_transction_virtual` to make more clear * ethcore: fix detection of major import (#9552) * sync: set state to idle after sync is completed * sync: refactor sync reset * parity: revert clib bump and fix tests * Fix path to parity.h (#9274) * Fix path to parity.h * Fix other paths as well * ethcore-io retries failed work steal (#9651) * ethcore-io uses newer version of crossbeam && retries failed work steal * ethcore-io non-mio service uses newer crossbeam

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

niklasad1 commented Sep 19, 2018

View reviewed changes

niklasad1 force-pushed the light/introduce-readonly-exec branch from 6eb3253 to 7d0608e Compare September 19, 2018 09:36

5chdn added this to the 2.2 milestone Sep 19, 2018

niklasad1 mentioned this pull request Sep 19, 2018

[Light client] Disconnect all its peers subsequently to an eth_call #9551

Closed

niklasad1 changed the title ~~[light/provider] Fix that read_only executions always succeed~~ fix (light/provider) : Make read_only executions only read-only Sep 26, 2018

niklasad1 force-pushed the light/introduce-readonly-exec branch from a0abd2c to a98e014 Compare September 26, 2018 18:30

ordian approved these changes Sep 27, 2018

View reviewed changes

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 27, 2018

ordian added the B0-patchthis 🕷 label Sep 27, 2018

niklasad1 force-pushed the light/introduce-readonly-exec branch from a98e014 to 1b0f601 Compare September 28, 2018 08:24

Tbaut mentioned this pull request Sep 28, 2018

eth_estimateGas returns "The execution failed due to an exception." #9669

Closed

This was referenced Sep 30, 2018

Backports for beta 2.1.2 #9649

Merged

Backports for stable 2.0.7 #9648

Merged

niklasad1 force-pushed the light/introduce-readonly-exec branch from 1b0f601 to 9acd419 Compare October 1, 2018 09:01

niklasad1 added 4 commits October 2, 2018 11:29

grumbles(revert renaming) : TransactionProof

b59d35f

grumbles(trace) : remove incorrect trace

b1cabf4

grumbles(state/prove_tx) : explicit virt

9a36cf3

Remove the boolean flag to determine that a `state::prove_transaction` whether it should be executed in a virtual context or not. Because of that also rename the function to `state::prove_transction_virtual` to make more clear

niklasad1 force-pushed the light/introduce-readonly-exec branch from 9acd419 to 9a36cf3 Compare October 2, 2018 09:30

niklasad1 changed the title ~~fix (light/provider) : Make read_only executions only read-only~~ fix (light/provider) : Make read_only executions read-only Oct 2, 2018

ordian requested a review from cheme October 8, 2018 12:33

ordian added the B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. label Oct 8, 2018

cheme approved these changes Oct 8, 2018

View reviewed changes

niklasad1 merged commit 5b54442 into master Oct 8, 2018

niklasad1 deleted the light/introduce-readonly-exec branch October 8, 2018 19:30

ordian mentioned this pull request Oct 16, 2018

fix(light/response) : handle bad responses #9756

Closed

2 tasks

cheme mentioned this pull request Oct 25, 2018

[Light client] NotEnoughBaseGas for RPC without specifying any Gas. #9031

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix (light/provider) : Make `read_only executions` read-only #9591

fix (light/provider) : Make `read_only executions` read-only #9591

niklasad1 commented Sep 19, 2018 •

edited by ordian

Loading

niklasad1 Sep 19, 2018

ordian Sep 27, 2018

niklasad1 Sep 19, 2018

ordian Sep 27, 2018

niklasad1 Sep 28, 2018

Tbaut commented Sep 19, 2018

ordian commented Sep 19, 2018

Tbaut commented Sep 19, 2018

niklasad1 commented Sep 19, 2018

Tbaut commented Sep 19, 2018

ordian left a comment

ordian Sep 27, 2018

ordian Sep 27, 2018 •

edited

Loading

ordian Sep 27, 2018

cheme commented Oct 8, 2018

ordian commented Oct 8, 2018

cheme commented Oct 8, 2018

cheme left a comment

fix (light/provider) : Make read_only executions read-only #9591

fix (light/provider) : Make read_only executions read-only #9591

Conversation

niklasad1 commented Sep 19, 2018 • edited by ordian Loading

Manual tests on (kovan)

niklasad1 Sep 19, 2018

Choose a reason for hiding this comment

ordian Sep 27, 2018

Choose a reason for hiding this comment

niklasad1 Sep 19, 2018

Choose a reason for hiding this comment

ordian Sep 27, 2018

Choose a reason for hiding this comment

niklasad1 Sep 28, 2018

Choose a reason for hiding this comment

Tbaut commented Sep 19, 2018

ordian commented Sep 19, 2018

Tbaut commented Sep 19, 2018

niklasad1 commented Sep 19, 2018

Tbaut commented Sep 19, 2018

ordian left a comment

Choose a reason for hiding this comment

ordian Sep 27, 2018

Choose a reason for hiding this comment

ordian Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

ordian Sep 27, 2018

Choose a reason for hiding this comment

cheme commented Oct 8, 2018

ordian commented Oct 8, 2018

cheme commented Oct 8, 2018

cheme left a comment

Choose a reason for hiding this comment

fix (light/provider) : Make `read_only executions` read-only #9591

fix (light/provider) : Make `read_only executions` read-only #9591

niklasad1 commented Sep 19, 2018 •

edited by ordian

Loading

ordian Sep 27, 2018 •

edited

Loading