Client: Warning on not possible further block execution #1065

holgerd77 · 2021-01-20T17:47:55Z

My own client (blockchain) DB is currently in a state where block execution is not further done due to a somewhat inconsistent db state. In the client this gets noticeable by the execution messages not further showing up, there are only the block import logs displayed.

I could identify the root cause of the no-execution behavior: in the Blockchain iterator function there is no block found in the DB following the current iterator head. Therefore no blocks are executed. I have no idea though how and when the blockchain DB got into this state. I do know though that I've been there (in this state) a couple of times before with block execution stopping.

This PR is approaching this on a low level by not changing any logic or behavior but as a first step providing feedback when block execution is not further processing. For this it:

Expands the Blockchain.iterator() interface to allow for a number value to be returned (so now allowing Promise<void | number>, keeping void is for backwards compatibility) and changing our own interface implementation in the blockchain class to return the number of blocks which have been actually iterated. This will likely generally be useful for a number of use cases.
The VM.runBlockchain() method now similarly also returns the number of iterated blocks
This allows in the client to give an instant warning feedback message together with the current latest head hash and number and stop further block execution

Output looks like this:

This should allow to better tracing/debugging this in the future and there is now a clear consumer feedback if this behavior occurs.

We might want to decide if we want to further act upon this case, a possibility would be to set the latest blockchain head to the VM iterator head, so that sync will restart from there. I hesitated to directly integrate since it is not clear what corrupted the DB in the first place. We can decide this independently from this PR though.

codecov · 2021-01-20T17:49:50Z

Codecov Report

Merging #1065 (f2bbb08) into master (ea2a09e) will increase coverage by 0.34%.
The diff coverage is 80.00%.

Flag	Coverage Δ
block	`77.93% <91.30%> (+<0.01%)`	⬆️
blockchain	`78.52% <91.30%> (+0.59%)`	⬆️
client	`88.03% <20.00%> (+0.05%)`	⬆️
common	`92.37% <ø> (+0.43%)`	⬆️
devp2p	`82.48% <ø> (+0.16%)`	⬆️
ethash	`82.08% <ø> (ø)`
tx	`90.00% <ø> (?)`
vm	`83.09% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

holgerd77 · 2021-01-20T17:50:43Z

packages/blockchain/src/index.ts

+    return await this.initAndLock<number>(
+      async (): Promise<number> => {
+        const headHash = this._heads[name] || this._genesis
+        let lastBlock: Block | undefined


I've also renamed some variable names for better clarity, not sure why this is show with such a large gap here, might be a bit confusing, but code flow remains the same here.

holgerd77 · 2021-01-20T17:53:13Z

packages/blockchain/src/index.ts


-          if (!block) {
-            break
-          }


This could be removed since _getBlock() is not allowing for an undefined or null return but is just throwing a NotFoundError when no block is found (which is caught later on).

Apart from this code flow is the same here as well (just switched the if clause order in the catch block for better readability, that's all).

holgerd77 · 2021-01-20T17:56:00Z

packages/vm/lib/runBlockchain.ts

@@ -5,13 +5,13 @@ import VM from './index'
 /**
 * @ignore
 */
-export default async function runBlockchain(this: VM, blockchain?: Blockchain, maxBlocks?: number) {


I would argue that return value can be changed this way here (remaining non-breaking), since no one will have done something with the void return value before and just used await vm.runBlockchain(). So the updated return value is more of an optional extra thing now which now can be used if makes sense.

great, sounds good, could the same reasoning be used for the return type of iterator? I think I would find void | number confusing.

Hmm, not for the interface I guess, since this would mean that if someone implements that interface it would break their implementation. 😕 I've added this (remove void return type part) to #1024 though, and the iterator implementation itself has no void return type.

When thinking about it I'm afraid that it's rather the other way around and we need to also return Promise<void | number> in the VM for the non-breaking release. Otherwise if someone keeps their own Blockchain implementation with the void return and uses this for the VM.runBlockchain() method, this will break on TypeScript since the method is expecting a chained return value of number for the output.

…re capable iterator result evaluation, expand interface (backwards-compatible)

…nBlockchain(), client logger warning on no further block execution

… with Blockchain implemenations with an iterator void return value

jochem-brouwer · 2021-01-21T17:06:32Z

It would be nice to figure out how the DB got corrupted. It could possibly be that we do not explicitly flush the blockchain DB once we kill the client, so part of it stays in memory? We'd have to explicitly flush the DB once we get a kill signal.

packages/client/lib/sync/fullsync.ts

packages/vm/lib/runBlockchain.ts

packages/client/lib/sync/fullsync.ts

cgewecke · 2021-01-21T17:10:53Z

packages/blockchain/src/index.ts

-          if (error.type !== 'NotFoundError') {
-            throw error
+        const headBlockNumber = await this.dbManager.hashToNumber(headHash)
+        const nextBlockNumber = headBlockNumber.addn(1)


Suggested change

const nextBlockNumber = headBlockNumber.addn(1)

const nextBlockNumber = headBlockNumber.iaddn(1)

...not sure this change would be meaningful but iaddn is used in the loop below to increment nextBlockNumber

[EDIT - this difference pre-dates the PR fwiw)

cgewecke

LGTM!!

jochem-brouwer · 2021-01-21T18:54:52Z

Doesn't this bug also indicate we have a problem in our block fetcher? If the 'bad block' gets deleted from the chain, then we should re-request the block from our peers (by block number, not by block hash). So, if a block gets deleted, send a request to our peers to retrieve the deleted block number(s).

jochem-brouwer · 2021-01-21T21:40:04Z

What will happen if we are at the tip of the chain? Won't we then get this warning, which should not get logged, as there simply is no valid block available at this point in time?

holgerd77 · 2021-01-22T10:58:15Z

Need to merge for some subsequent work, will answer the remaining questions.

holgerd77 · 2021-01-22T11:08:34Z

@jochem-brouwer I think "tip of the chain" behavior generally needs some dedicated work on its own - once we reach a tip 😋 - would very much expect this not to work on first run.

holgerd77 added PR state: needs review package: blockchain package: vm package: client labels Jan 20, 2021

holgerd77 requested review from ryanio, cgewecke and jochem-brouwer January 20, 2021 17:47

holgerd77 commented Jan 20, 2021

View reviewed changes

blockchain -> iterator: added blocksRanCounter as return value for mo…

3cce181

…re capable iterator result evaluation, expand interface (backwards-compatible)

holgerd77 force-pushed the improve-blockchain-iterator branch from 8b0693b to e88af94 Compare January 20, 2021 18:04

vm, client: added number of blocks iterated as return value for VM.ru…

52a2a47

…nBlockchain(), client logger warning on no further block execution

holgerd77 force-pushed the improve-blockchain-iterator branch 3 times, most recently from 52c8071 to 617d62f Compare January 21, 2021 11:47

vm, client: made VM.runBlockchain() return value backwards-compatible…

f2bbb08

… with Blockchain implemenations with an iterator void return value

holgerd77 force-pushed the improve-blockchain-iterator branch from 617d62f to f2bbb08 Compare January 21, 2021 12:01

jochem-brouwer reviewed Jan 21, 2021

View reviewed changes

packages/client/lib/sync/fullsync.ts Show resolved Hide resolved

jochem-brouwer reviewed Jan 21, 2021

View reviewed changes

packages/vm/lib/runBlockchain.ts Show resolved Hide resolved

jochem-brouwer reviewed Jan 21, 2021

View reviewed changes

packages/client/lib/sync/fullsync.ts Show resolved Hide resolved

cgewecke reviewed Jan 21, 2021

View reviewed changes

cgewecke approved these changes Jan 21, 2021

View reviewed changes

holgerd77 mentioned this pull request Jan 21, 2021

Client: "Error: invalid transaction trie" leading to corrupted state DB #1066

Closed

holgerd77 merged commit 9a3b800 into master Jan 22, 2021

holgerd77 deleted the improve-blockchain-iterator branch January 22, 2021 10:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client: Warning on not possible further block execution #1065

Client: Warning on not possible further block execution #1065

holgerd77 commented Jan 20, 2021

codecov bot commented Jan 20, 2021 •

edited

Loading

holgerd77 Jan 20, 2021

holgerd77 Jan 20, 2021 •

edited

Loading

holgerd77 Jan 20, 2021

ryanio Jan 20, 2021

holgerd77 Jan 20, 2021

holgerd77 Jan 20, 2021

jochem-brouwer commented Jan 21, 2021

cgewecke Jan 21, 2021 •

edited

Loading

cgewecke left a comment

jochem-brouwer commented Jan 21, 2021

jochem-brouwer commented Jan 21, 2021

holgerd77 commented Jan 22, 2021

holgerd77 commented Jan 22, 2021

	const nextBlockNumber = headBlockNumber.addn(1)
	const nextBlockNumber = headBlockNumber.iaddn(1)

Client: Warning on not possible further block execution #1065

Client: Warning on not possible further block execution #1065

Conversation

holgerd77 commented Jan 20, 2021

codecov bot commented Jan 20, 2021 • edited Loading

Codecov Report

holgerd77 Jan 20, 2021

Choose a reason for hiding this comment

holgerd77 Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

holgerd77 Jan 20, 2021

Choose a reason for hiding this comment

ryanio Jan 20, 2021

Choose a reason for hiding this comment

holgerd77 Jan 20, 2021

Choose a reason for hiding this comment

holgerd77 Jan 20, 2021

Choose a reason for hiding this comment

jochem-brouwer commented Jan 21, 2021

cgewecke Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

cgewecke left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Jan 21, 2021

jochem-brouwer commented Jan 21, 2021

holgerd77 commented Jan 22, 2021

holgerd77 commented Jan 22, 2021

codecov bot commented Jan 20, 2021 •

edited

Loading

holgerd77 Jan 20, 2021 •

edited

Loading

cgewecke Jan 21, 2021 •

edited

Loading