Change how RPCs eth_call and eth_estimateGas handle "Pending" #11127

dvdplm · 2019-10-04T14:14:22Z

Before this PR we would return a rather confusing error when calling eth_call and eth_estimateGas with "Pending", e.g.:

{"jsonrpc":"2.0","error":{"code":-32000,"message":"This request is not supported because your node is running with state pruning. Run with --pruning=archive."},"id":"e237678f6648ed12ff05a74933d06d17"}

In reality what is going on is that users often use "Pending" when they really mean "Latest" (e.g. MyCrypto…) and when the block in question is not actually pending. This changes our behaviour for these two RPC calls to fall back to "Latest" when the query with "Pending" fails.
Note that we already behave this way for many other RPCs:

- eth_call (after this PR)
- eth_estimateGas (after this PR)
- eth_getBalance
- eth_getCode
- eth_getStorageAt

Closes #10096 #9707

Before this PR we would return a rather confusing error when calling `eth_call` and `eth_estimateGas` with `"Pending"`, e.g.: ``` {"jsonrpc":"2.0","error":{"code":-32000,"message":"This request is not supported because your node is running with state pruning. Run with --pruning=archive."},"id":"e237678f6648ed12ff05a74933d06d17"} ``` In reality what is going on is that users often use `"Pending"` when they really mean `"Latest"` (e.g. MyCrypto…) and when the block in question is not actually pending. This changes our behaviour for these two RPC calls to fall back to `"Latest"` when the query with `"Pending"` fails. Note that we already behave this way for many other RPCs: - eth_call (after this PR) - eth_estimateGas (after this PR) - eth_getBalance - eth_getCode - eth_getStorageAt Closes #10096

…pending * master: Refactor parity_listStorageKeys with count parameter optional (#11124)

rpc/src/v1/impls/eth.rs

…oid potential data races and do less work

ethcore/src/client/client.rs

niklasad1 · 2019-10-11T09:53:18Z

rpc/src/v1/impls/eth.rs

+		let best_block_number = self.client.chain_info().best_block_number;
+
+		let (maybe_state, maybe_header) = (
+				// todo:[dvdplm]: this takes the queue lock twice and is inelegant. It would be


Ok, I think that makes sense.

Or maybe just provide the iterator API instead of map_existing_pending_block

rpc/src/v1/impls/eth.rs

niklasad1 · 2019-10-11T10:03:53Z

ethcore/client-traits/src/lib.rs

@@ -413,7 +413,7 @@ pub trait StateClient {
 	type State: StateInfo;

 	/// Get a copy of the best block's state.
-	fn latest_state(&self) -> Self::State;
+	fn latest_state_and_header(&self) -> (Self::State, Header);


I'm not a big fan of methods with several responsibilities. Why not a separate method fn best_header/latest_header()?

Is this some kind of optimization that I don't understand?

Also, seems that we call this method a couple of this without caring about the header. I think the header type is quite big and might carry some cost to return by value when it's not needed.

Looking up latest state first looks up latest header, so when we need both we:

look up the header

use the header to fetch the state

toss away the header

look up the header again

So while I agree with you that it's generally best to have methods do a single thing, I think returning both pieces of data is warranted here.

I think it was made to ensure atomicity of the operation - i.e. that the we don't import the block in the meantime and the state does not match header.

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

tomusdrw · 2019-10-11T10:29:22Z

ethcore/client-traits/src/lib.rs

@@ -413,7 +413,7 @@ pub trait StateClient {
 	type State: StateInfo;

 	/// Get a copy of the best block's state.


Docs could do with an update.

tomusdrw · 2019-10-11T10:30:51Z

ethcore/client-traits/src/lib.rs

@@ -413,7 +413,7 @@ pub trait StateClient {
 	type State: StateInfo;

 	/// Get a copy of the best block's state.
-	fn latest_state(&self) -> Self::State;
+	fn latest_state_and_header(&self) -> (Self::State, Header);


I think it was made to ensure atomicity of the operation - i.e. that the we don't import the block in the meantime and the state does not match header.

rpc/src/v1/impls/eth.rs

…ithub.com:paritytech/parity-ethereum into dp/fix/improve-handling-of-rpc-calls-with-pending * 'dp/fix/improve-handling-of-rpc-calls-with-pending' of github.com:paritytech/parity-ethereum: Update rpc/src/v1/impls/eth.rs

* Change how RPCs eth_call and eth_estimateGas handle "Pending" Before this PR we would return a rather confusing error when calling `eth_call` and `eth_estimateGas` with `"Pending"`, e.g.: ``` {"jsonrpc":"2.0","error":{"code":-32000,"message":"This request is not supported because your node is running with state pruning. Run with --pruning=archive."},"id":"e237678f6648ed12ff05a74933d06d17"} ``` In reality what is going on is that users often use `"Pending"` when they really mean `"Latest"` (e.g. MyCrypto…) and when the block in question is not actually pending. This changes our behaviour for these two RPC calls to fall back to `"Latest"` when the query with `"Pending"` fails. Note that we already behave this way for many other RPCs: - eth_call (after this PR) - eth_estimateGas (after this PR) - eth_getBalance - eth_getCode - eth_getStorageAt Closes #10096 * Fetch jsonrpc from git * No real need to wait for new jsonrpc * Add tests for calling eth_call/eth_estimateGas with "Pending" * Fix a todo, add another * Change client.latest_state to return the best header as well so we avoid potential data races and do less work * Impl review suggestions * Update rpc/src/v1/impls/eth.rs Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com> * Review grumbles * update docs

* ropsten #6631425 foundation #8798209 (#11201) * [stable] builtin, istanbul and mordor testnet backports (#11234) * ethcore-builtin (#10850) * [builtin]: support `multiple prices and activations` in chain spec (#11039) * [chain specs]: activate `Istanbul` on mainnet (#11228) * ethcore/res: add mordor testnet configuration (#11200) * Update list of bootnodes for xDai chain (#11236) * ethcore: remove `test-helper feat` from build (#11047) * Secret store: fix Instant::now() related race in net_keep_alive (#11155) (#11159) * [stable]: backport #10691 and #10683 (#11143) * Fix compiler warning (that will become an error) (#10683) * Refactor Clique stepping (#10691) * Add Constantinople eips to the dev (instant_seal) config (#10809) * Add cargo-remote dir to .gitignore (?) * Insert explicit warning into the panic hook (#11225) * Fix docker centos build (#11226) * Update MIX bootnodes. (#11203) * Use provided usd-per-eth value if an endpoint is specified (#11209) * Add new line after writing block to hex file. (#10984) * Type annotation for next_key() matching of json filter options (#11192) (but no `FilterOption` in 2.5 so…) * Upgrade jsonrpc to latest (#11206) * [CI] check evmbin build (#11096) * Correct EIP-712 encoding (#11092) * [client]: Fix for incorrectly dropped consensus messages (#11086) * Fix block detail updating (#11015) * Switching sccache from local to Redis (#10971) * Made ecrecover implementation trait public (#11188) * [dependencies]: jsonrpc `14.0.1` (#11183) * [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179) * [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) * util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) * ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) * Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) * Cleanup stratum a bit (#11161) * Upgrade to jsonrpc v14 (#11151) * SecretStore: expose restore_key_public in HTTP API (#10241)

* [CI] check evmbin build (#11096) * Correct EIP-712 encoding (#11092) * [client]: Fix for incorrectly dropped consensus messages (#11082) (#11086) * Update hardcoded headers (foundation, classic, kovan, xdai, ewc, ...) (#11053) * Add cargo-remote dir to .gitignore (?) * Update light client headers: ropsten 6631425 foundation 8798209 (#11201) * Update list of bootnodes for xDai chain (#11236) * ethcore/res: add mordor testnet configuration (#11200) * [chain specs]: activate Istanbul on mainnet (#11228) * [builtin]: support multiple prices and activations in chain spec (#11039) * [receipt]: add sender & receiver to RichReceipts (#11179) * [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) * Made ecrecover implementation trait public (#11188) * Fix docker centos build (#11226) * Update MIX bootnodes. (#11203) * Insert explicit warning into the panic hook (#11225) * Use provided usd-per-eth value if an endpoint is specified (#11209) * Cleanup stratum a bit (#11161) * Add Constantinople EIPs to the dev (instant_seal) config (#10809) (already backported) * util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) * ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) * Type annotation for next_key() matching of json filter options (#11192) * Upgrade jsonrpc to latest (#11206) * [dependencies]: jsonrpc 14.0.1 (#11183) * Upgrade to jsonrpc v14 (#11151) * Switching sccache from local to Redis (#10971) * Snapshot restoration overhaul (#11219) * Add new line after writing block to hex file. (#10984) * Pause pruning while snapshotting (#11178) * Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) * Fix block detail updating (#11015) * Make InstantSeal Instant again #11186 * Filter out some bad ropsten warp snapshots (#11247)

dvdplm self-assigned this Oct 4, 2019

dvdplm added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Oct 4, 2019

Fetch jsonrpc from git

6094ce7

dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 4, 2019

dvdplm marked this pull request as ready for review October 4, 2019 16:06

dvdplm added 3 commits October 4, 2019 18:42

Merge branch 'master' into dp/fix/improve-handling-of-rpc-calls-with-…

2af0f05

…pending * master: Refactor parity_listStorageKeys with count parameter optional (#11124)

No real need to wait for new jsonrpc

35cafcc

Add tests for calling eth_call/eth_estimateGas with "Pending"

0e88191

niklasad1 requested a review from tomusdrw October 7, 2019 13:25

tomusdrw reviewed Oct 7, 2019

View reviewed changes

rpc/src/v1/impls/eth.rs Outdated Show resolved Hide resolved

rpc/src/v1/impls/eth.rs Outdated Show resolved Hide resolved

rpc/src/v1/impls/eth.rs Show resolved Hide resolved

dvdplm added 2 commits October 7, 2019 22:34

Fix a todo, add another

dc1708d

Change client.latest_state to return the best header as well so we av…

cee6661

…oid potential data races and do less work

dvdplm added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Oct 9, 2019

niklasad1 reviewed Oct 10, 2019

View reviewed changes

ethcore/src/client/client.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 10, 2019

View reviewed changes

ethcore/src/client/client.rs Outdated Show resolved Hide resolved

Impl review suggestions

c7a95f0

dvdplm requested a review from tomusdrw October 10, 2019 20:02

niklasad1 reviewed Oct 11, 2019

View reviewed changes

rpc/src/v1/impls/eth.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 11, 2019

View reviewed changes

rpc/src/v1/impls/eth.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 11, 2019

View reviewed changes

niklasad1 added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Oct 11, 2019

Update rpc/src/v1/impls/eth.rs

1624f65

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

tomusdrw approved these changes Oct 11, 2019

View reviewed changes

dvdplm added 2 commits October 11, 2019 13:26

Review grumbles

f51b4dd

niklasad1 approved these changes Oct 11, 2019

View reviewed changes

update docs

c93387f

dvdplm merged commit aefa8d5 into master Oct 11, 2019

niklasad1 deleted the dp/fix/improve-handling-of-rpc-calls-with-pending branch October 11, 2019 14:15

ordian added this to the 2.7 milestone Oct 16, 2019

ordian mentioned this pull request Oct 16, 2019

eth_call 'pending' state error #9707

Closed

This was referenced Nov 5, 2019

v2.5.10 stable #11239

Merged

v2.6.5 beta #11240

Merged

seunlanlege mentioned this pull request Nov 9, 2019

pending for defaultBlock causes issues during estimateGas #9288

Closed

ezdac mentioned this pull request Nov 23, 2020

Revert special behaviour for Parity's call( ) with pending block-id raiden-network/raiden#6695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change how RPCs eth_call and eth_estimateGas handle "Pending" #11127

Change how RPCs eth_call and eth_estimateGas handle "Pending" #11127

dvdplm commented Oct 4, 2019 •

edited

Loading

niklasad1 Oct 11, 2019

niklasad1 Oct 11, 2019 •

edited

Loading

dvdplm Oct 11, 2019

tomusdrw Oct 11, 2019

tomusdrw Oct 11, 2019

tomusdrw Oct 11, 2019

		@@ -413,7 +413,7 @@ pub trait StateClient {
		type State: StateInfo;

		/// Get a copy of the best block's state.

Change how RPCs eth_call and eth_estimateGas handle "Pending" #11127

Change how RPCs eth_call and eth_estimateGas handle "Pending" #11127

Conversation

dvdplm commented Oct 4, 2019 • edited Loading

niklasad1 Oct 11, 2019

Choose a reason for hiding this comment

niklasad1 Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

dvdplm Oct 11, 2019

Choose a reason for hiding this comment

tomusdrw Oct 11, 2019

Choose a reason for hiding this comment

tomusdrw Oct 11, 2019

Choose a reason for hiding this comment

tomusdrw Oct 11, 2019

Choose a reason for hiding this comment

dvdplm commented Oct 4, 2019 •

edited

Loading

niklasad1 Oct 11, 2019 •

edited

Loading