Add Error message when sync is still in progress. by seunlanlege · Pull Request #9475 · openethereum/parity-ethereum

seunlanlege · 2018-09-04T21:53:35Z

parity-cla-bot · 2018-09-04T21:53:37Z

It looks like @seunlanlege hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

seunlanlege · 2018-09-04T21:55:30Z

[clabot:check]

parity-cla-bot · 2018-09-04T21:55:33Z

It looks like @seunlanlege signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

dvdplm · 2018-09-05T05:45:29Z

+		if let Ok(SyncStatus::Info(_)) = self.syncing() {
+			// alas, we're still syncing!
+			warn!("Attempted to fetch transaction via hash while sync is in progress");
+			return Box::new(future::err(errors::ancient_block_missing()))


Isn't it possible to be in a syncing state even while we're quite close to the head of the chain? As a user I think I'd be surprised to see an error about "ancient" blocks if I were essentially synched. Maybe the error message should be something more generic like Block is unavailable?

dvdplm · 2018-09-05T05:49:11Z

+			if  res.is_none() {
+				if let Ok(SyncStatus::Info(_)) = self.syncing() {
+					// alas, we're still syncing!
+					warn!("Call made to transaction_by_block_hash_and_index while sync is in progress");


Not sure that using the full method name here is a good idea. How about "RPC call is unavailable while synching".

dvdplm · 2018-09-05T05:51:00Z

+		if let Ok(SyncStatus::Info(_)) = self.syncing() {
+			// alas, we're still syncing!
+			warn!("Attempted to fetch transaction via hash while sync is in progress");
+			return Box::new(future::err(errors::ancient_block_missing()))


Same comments from above apply here.

Maybe there's a way to add these warnings to all RPC calls higher up in the call chain so we don't have to pollute each method with log/warn calls?

andresilva

I don't think this is correct and I don't think it's easy to implement this properly. The main issue is that if I give you an hash how do you distinguish from you not having the data (because you're still syncing) or the data not existing in the first place? Also the current checks for self.syncing() only check for normal sync as far as I remember, and even though I'm syncing I may already have the data available, e.g. I'm synced to block #2000 (but still syncing up to say #4000) and you ask for data from block #1000.

andresilva · 2018-09-05T10:56:52Z

(reposting from private chat)

I guess for some RPCs we could implement a best-effort approach. For example to fetch transaction by hash:

We try to fetch it from the current chain, if we find something we return it (same behavior as now)
If we don't find it and we know that we're still syncing ancient blocks then we return with an error saying that the transaction wasn't found but we still don't have all the data yet, i.e. this could be a false negative (so the user can retry later when sync is finished)
If we don't find the data but we are fully synced we just return None in this case

Can we use this strategy for all RPC methods? Do we want to implement something like this?

tomusdrw

As in #9188 (comment) it's not really possible to implement this with significant changes to snapshot format. What's more I don't really think it's a good idea to store the tx hashes in snapshots anyway (perhaps we could store at most a bloom filter).

That said, I like @andresilva's suggestion to return an error instead of None in case we know we have not fetched ancient blocks yet (note it's very different from syncing, so self.syncing() is not the right method to use here).

If Andre's suggestion gets implemented we should also consider disabling that behaviour in case someone is consciously running with --no-ancient-blocks.

tomusdrw · 2018-09-05T11:41:42Z

#7411 might be a bit more straightforward - we have the block number there so we can just return an error in case block_number < latest_block_number and the data is missing (would be worth to handle logs as well, as in the comment in that issue).

tomusdrw

Still incorrect, but better. We're almost there.

tomusdrw · 2018-09-05T12:57:11Z

+RpcResult<Option<T>> {
+	move |res| {
+		if res.is_none() {
+			if let Ok(SyncStatus::Info(_)) = sync_status {


The syncing state doesn't really matter, unless there is a variant that tells you that: "The chain is incomplete - We are still syncing/importing ancient blocks" (Which is completely different than regular sync, some more details about warp sync: https://wiki.parity.io/Warp-Sync.html)

tomusdrw · 2018-09-05T13:01:50Z

+		if let BlockNumber::Num(number) = num {
+			match self.client.chain_info().best_block_number {
+				BlockNumber::Num(best_block_number) => {
+					if num < best_block_number {


This is quite wrong, actually these are the only queries that make sense at all, since:

In database we store blocks from [0...best_block_number]

Querying anything from that range should succeed

Querying anything greater than best_block_number will always fail

With ancient blocks the case is that the database is not complete, so we have a situation where:

We have [0..best_ancient_block] ... [first_block .. best_block]

The latter range has been restored together with the snapshot

The former range has been downloaded during ancient block import

Any query that tries to get num < best_block_number and fails (i.e. returns None) most likely falls into (best_ancient_block .. first_block) range, so we should detect that and return the error only in such case.

tomusdrw · 2018-09-05T13:02:50Z

 	fn block_by_number(&self, num: BlockNumber, include_txs: bool) -> BoxFuture<Option<RichBlock>> {
-		Box::new(future::done(self.rich_block(num.into(), include_txs)))
+		let result = self.check_block_existence(num)
+			.and_then(|_| self.rich_block(num.into(), include_txs));


To make it correct you should:

First query the block

If it's None and num < best_block_number return ancient block unavailable error.

Tbaut · 2018-09-05T20:57:42Z

Despite trying hard, I still can't manage to know all issues by number :) Having a human understandable description and title for a PR is therefore requested!

andresilva

Overall LGTM. Just minor comments.

andresilva · 2018-09-17T18:14:44Z

+				return Err(Error {
+					code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST),
+					// this error message feels pretty straightforward to me. ¯\_(ツ)_/¯
+					message: "We cannot tell for sure if the thing you requested is not available or we just don't have it".into(),


Probably better to be explicit about it? "Ancient block sync is still in progress."

andresilva · 2018-09-17T18:37:29Z

+			let BlockChainInfo { ancient_block_hash, first_block_hash, .. } = client.chain_info();
+			// block information was requested, but unfortunately we couldn't find it and there
+			// are gaps in the database ethcore/src/blockchain/blockchain.rs:202
+			if ancient_block_hash.is_some() && first_block_hash.is_some() {


I think first_block_hash will always be some.

andresilva · 2018-09-17T18:39:51Z

+pub fn unavailable_block() -> Error {
+	Error {
+		code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST),
+		message: "Block is unavailable".into(),


Probably better to have a more explicit message about ancient block sync?

andresilva · 2018-09-17T18:41:40Z

Please rebase/merge master to fix conflicts.

tomusdrw

lgtm, minor grumble regarding error message - we can distinguish between:

Knowing for sure that the blocks is not there because of ancient sync
Assuming that it might be not found, because we don't have all the blocks yet.

Also I believe if we merge that we should also have an option to disable this behaviour since it's a breaking change (instead of returning Ok(None) we return an Error)

tomusdrw · 2018-09-20T08:48:15Z

+{
+	move |response| {
+		if response.is_none() {
+			let BlockChainInfo { ancient_block_hash, first_block_hash, .. } = client.chain_info();


first_block_hash is unused

tomusdrw · 2018-09-20T08:50:32Z

+			// block information was requested, but unfortunately we couldn't find it and there
+			// are gaps in the database ethcore/src/blockchain/blockchain.rs:202
+			if ancient_block_hash.is_some()  {
+				return Err(unavailable_block())


This is a bit different than check_block_number_existence. Here we are unsure if the block is not valid or was not found, in the former we actually have certainty that the block is missing because we are still syncing ancient blocks.

TBH I think we should express that in the error message as well.

sorpaas

Current changes LGTM. A few grumbles. And I think there're also other APIs in eth_ namespace that can apply the same strategy:

block_transaction_count_by_hash
block_transaction_count_by_number
block_uncles_count_by_hash
block_uncles_count_by_number
uncle_by_block_hash_and_index
uncle_by_block_number_and_index

sorpaas · 2018-11-25T15:53:02Z

 	keep_alive: Option<bool>,
 	experimental_rpcs: Option<bool>,
 	poll_lifetime: Option<u32>,
+	jsonrpc_allow_missing_blocks: Option<bool>,


nit: maybe name this just allow_missing_blocks? We need the jsonrpc prefix for args but they're already under the category for configs.

sorpaas · 2018-11-25T15:56:37Z

+					deps.miner.clone(),
+					$nonces,
+					deps.gas_price_percentile,
+					);


nit: indentation is wrong.

sorpaas · 2018-11-25T15:57:02Z

 					}
-				}
-			}
+				}};


nit: this indentation looks weird

sorpaas · 2018-11-25T16:09:41Z

Also there's a test failure:

error[E0063]: missing field `jsonrpc_allow_missing_blocks` in initializer of `v1::impls::eth::EthClientOptions`
   --> rpc/src/v1/tests/eth.rs:143:4
    |
143 |             EthClientOptions {
    |             ^^^^^^^^^^^^^^^^ missing `jsonrpc_allow_missing_blocks`

block_transaction_count_by_hash block_transaction_count_by_number block_uncles_count_by_hash block_uncles_count_by_number uncle_by_block_hash_and_index uncle_by_block_number_and_index fix PR grumbles

revert config name Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

revert cli arg Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

revert config name Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

sorpaas · 2018-11-26T17:00:21Z

-			let best_block = self.client.chain_info().best_block_number;
-			if let Some(receipt) = self.miner.pending_receipt(best_block, &hash) {
-				return Box::new(future::ok(Some(receipt.into())));
+		match (self.miner.pending_receipt(best_block, &hash), self.options.allow_pending_receipt_query) {


This will unnecessarily call self.miner.pending_receipt even when self.options.allow_pending_receipt_query is false. I would suggest to change it back to the old two if form.

…to issue-9188

sorpaas · 2018-11-26T17:39:38Z

 	}

 	fn transaction_receipt(&self, hash: RpcH256) -> BoxFuture<Option<Receipt>> {
+		let best_block = self.client.chain_info().best_block_number;


This is only used inside if self.options.allow_pending_receipt_query {, so maybe it's better to move it back.

* closes #9188 * check_for_unavailable_block in eth_getTransactionByHash eth_getTransactionByBlockHashAndIndex eth_getTransactionByBlockNumberAndIndex eth_getTransactionReceipt * check for unavailable block in eth_getBlockByNumber * corrected checks for unavailable_block * check for block gaps in db * corrected error messages * corrected error information * added allow-empty-block-result cli flag * address grumbles * --jsonrpc-allow-missing-blocks * fix tests * added checks to block_transaction_count_by_hash block_transaction_count_by_number block_uncles_count_by_hash block_uncles_count_by_number uncle_by_block_hash_and_index uncle_by_block_number_and_index fix PR grumbles * Update parity/cli/mod.rs revert config name Co-Authored-By: seunlanlege <seunlanlege@gmail.com> * Update parity/cli/mod.rs revert cli arg Co-Authored-By: seunlanlege <seunlanlege@gmail.com> * Apply suggestions from code review revert config name Co-Authored-By: seunlanlege <seunlanlege@gmail.com> * fix PR grumbles * fix more PR grumbles

seunlanlege requested a review from andresilva September 4, 2018 21:55

dvdplm previously requested changes Sep 5, 2018

View reviewed changes

andresilva reviewed Sep 5, 2018

View reviewed changes

5chdn added this to the 2.1 milestone Sep 5, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. labels Sep 5, 2018

lexfrl added the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Sep 5, 2018

tomusdrw reviewed Sep 5, 2018

View reviewed changes

tomusdrw suggested changes Sep 5, 2018

View reviewed changes

5chdn removed the A0-pleasereview 🤓 Pull request needs code review. label Sep 6, 2018

seunlanlege changed the title ~~closes #9188~~ Adds Error message for when sync is still in progress. Sep 6, 2018

niklasad1 changed the title ~~Adds Error message for when sync is still in progress.~~ Add Error message when sync is still in progress. Sep 6, 2018

5chdn modified the milestones: 2.1, 2.2 Sep 11, 2018

andresilva reviewed Sep 17, 2018

View reviewed changes

5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 18, 2018

seunlanlege force-pushed the issue-9188 branch from d8c0cd2 to 904daaf Compare September 18, 2018 14:08

tomusdrw approved these changes Sep 20, 2018

View reviewed changes

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 21, 2018

andresilva mentioned this pull request Sep 25, 2018

Introduce a pruning mode which keeps all logs #9541

Closed

5chdn removed the A8-looksgood 🦄 Pull request is reviewed well. label Nov 17, 2018

fix tests

eddab46

seunlanlege added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Nov 20, 2018

5chdn added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Nov 21, 2018

Merge branch 'master' into issue-9188

9702290

sorpaas reviewed Nov 25, 2018

View reviewed changes

sorpaas added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Nov 25, 2018

added checks to

430cebd

block_transaction_count_by_hash block_transaction_count_by_number block_uncles_count_by_hash block_uncles_count_by_number uncle_by_block_hash_and_index uncle_by_block_number_and_index fix PR grumbles

sorpaas suggested changes Nov 26, 2018

View reviewed changes

Comment thread parity/cli/mod.rs Outdated

Comment thread parity/cli/mod.rs Outdated

sorpaas and others added 2 commits November 26, 2018 13:20

Update parity/cli/mod.rs

251be48

revert config name Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

Update parity/cli/mod.rs

154751c

revert cli arg Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

sorpaas reviewed Nov 26, 2018

View reviewed changes

Comment thread parity/cli/mod.rs Outdated

sorpaas reviewed Nov 26, 2018

View reviewed changes

Comment thread parity/configuration.rs Outdated

Apply suggestions from code review

5b3b60e

revert config name Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

sorpaas reviewed Nov 26, 2018

View reviewed changes

seunlanlege added 2 commits November 26, 2018 18:29

fix PR grumbles

85f7612

Merge branch 'issue-9188' of github.com:paritytech/parity-ethereum in…

6ba18ad

…to issue-9188

sorpaas reviewed Nov 26, 2018

View reviewed changes

sorpaas approved these changes Nov 26, 2018

View reviewed changes

sorpaas added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 26, 2018

fix more PR grumbles

162aa49

sorpaas merged commit f2281dc into master Nov 26, 2018

sorpaas deleted the issue-9188 branch November 26, 2018 18:58

seunlanlege mentioned this pull request Oct 1, 2019

Warn when reading date from non-existing ancient blocks. #7411

Closed

Conversation

seunlanlege commented Sep 4, 2018 • edited by 5chdn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Sep 4, 2018

Uh oh!

seunlanlege commented Sep 4, 2018

Uh oh!

parity-cla-bot commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

andresilva commented Sep 5, 2018

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw commented Sep 5, 2018

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tbaut commented Sep 5, 2018

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva commented Sep 17, 2018

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorpaas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorpaas commented Nov 25, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

seunlanlege commented Sep 4, 2018 •

edited by 5chdn

Loading

tomusdrw Sep 5, 2018 •

edited

Loading