blockchain: Save iterator head to last successfuly executed even on errors #2680
Conversation
| if (releaseLockOnCallback === true) { | ||
| this._lock.release() | ||
| } | ||
| try { |
There was a problem hiding this comment.
lots of diff here is because of formatting change introduced because of this try
| throw new Error( | ||
| 'cannot get iterator head: blockchain has no getTotalDifficulty function' | ||
| ) | ||
| this.vmPromise = blockchain |
There was a problem hiding this comment.
again most of diff here is because of formatting changed because of introduction to the catch to the iterator promise below
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
| } | ||
| } | ||
| return blocksRanCounter | ||
| } finally { |
There was a problem hiding this comment.
To make sure I understand what's going on here, this outer try...finally block just makes sure save the current head and then throws the triggering error out to the client so it can exit gracefully?
There was a problem hiding this comment.
yes correct! this.heads['vm'] always have the last successfuly executed block
| true | ||
| ) | ||
| // Ensure to catch and not throw as this would lead to unCaughtException with process exit | ||
| .catch(async (error) => { |
There was a problem hiding this comment.
If we catch the error here, what error is thrown that tells the client to exit?
There was a problem hiding this comment.
no error is to be thrown for client to exit, when client catches Ctrl + C it stops execution which causes this iterator to throw error and the catch here logs and returns actualExecuted or null If the error is thrown here, it seems to be going in territory of unCaughtException
Problem to solve here is to cleanly exit which is the aim of the PR
There was a problem hiding this comment.
Ah, right, this is just handling an error that occurs when the client is already in the process of exiting. Makes sense.
yes : first run client with just sync: after it has synced couple thousand blocks, just not test with execution and keep doing Ctrl C and restart to
|
|
There are a couple of reorg related |
fixed an edge case 👍 |
On further debugging and testing another case of
vmHeadcorruption was observed:client was executing 749796-749896 range but got an interrup signal and 749890 failed
next time client started vmHead was already updated to 749896 and execution started from block 749897 which obviously would fail
The synopsis of the issue:
In vmExecution iterator, block error was being ignored leading to incorrect vmHead forward movement. So the right thing to do was to throw error.
However in blockchain, iterator head was not being properly saved to last successfully executed if
onBlockcallbacks errored.This PR addressed both the issues and also moves out iterator error handling out side of the vm iterator callback for further simplicity.