ETCM-732: Handle missing state node in regular sync after fast sync is done #961
Conversation
milenkara
requested review from
leo-bogastry,
AnastasiiaL,
bsuieric,
dzajkowski,
grale88,
jvdp and
1015bit
| blockImporter ! BlockFetcher.PickedBlocks(NonEmptyList.fromListUnsafe(List(invalidBlock))) | ||
|
|
||
| eventually { | ||
| Thread.sleep(1000) |
There was a problem hiding this comment.
Why the Thread.sleep if you are using eventually?
There was a problem hiding this comment.
I saw the Thread.sleep in some of the existing tests, so I reused it here. I will try to remove them and define a custom akka Patience for the eventually
There was a problem hiding this comment.
I have tried to use eventually without any Thread.sleeps and defining implicit/explicit Patience but for some reason it does not re-run the test if it fails. This is probably the reason that Thread.sleeps are introduced.
milenkara
force-pushed
the
bug/ETCM-732-MissingRootNode
branch
from
a8dd010 to
47b5836
Compare
| @@ -150,7 +152,7 @@ class BlockImporterItSpec | |||
| blockImporter ! BlockFetcher.PickedBlocks(NonEmptyList.fromListUnsafe(newBranch)) | |||
|
|
|||
| //because the blocks are not valid, we shouldn't reorganise, but at least stay with a current chain, and the best block of the current chain is oldBlock4 | |||
| eventually { blockchain.getBestBlock().get shouldEqual oldBlock4 } | |||
| eventually {Thread.sleep(1000); blockchain.getBestBlock().get shouldEqual oldBlock4 } | |||
There was a problem hiding this comment.
wouldn't it be cleaner to adjust the implicit def patienceConfig: PatienceConfig = defaultPatienceConfig?
There was a problem hiding this comment.
@dzajkowski I have tried both mixing our LongPatience trait into the test, and defining patience config directly before the eventually block. From the debug, I see that the my values are used, but for some reason method is not re-run.
There was a problem hiding this comment.
interesting, let me give it a look
There was a problem hiding this comment.
eventually does not mix well with AsyncFlatSpecLike. either a Future and AsyncFlatSpecLike or AnyFlatSpecLike with eventually
There was a problem hiding this comment.
@dzajkowski Done.
| case MPTError(reason) if reason.isInstanceOf[MissingNodeException] => | ||
| BlockImportFailedDueToMissingNode(reason.asInstanceOf[MissingNodeException]) | ||
| case err => BlockImportFailed(s"Error while trying to reorganise chain: $err") | ||
| }, |
There was a problem hiding this comment.
this could also be written as:
{
case MPTError(reason: MissingNodeException) => BlockImportFailedDueToMissingNode(reason)
case err => BlockImportFailed(s"Error while trying to reorganise chain: $err")
},
milenkara
force-pushed
the
bug/ETCM-732-MissingRootNode
branch
2 times, most recently
from
2db9ba8 to
7f30074
Compare
milenkara
force-pushed
the
bug/ETCM-732-MissingRootNode
branch
from
7f30074 to
98b2915
Compare
Description
Instead of handling failures of a
Task,BlockImporternow handles appropriate error that indicates that block could not be imported due to a missing state node.Proposed Solution
Since the error is now wrapped into instance of an
Either, using pattern matching we can act on a newly introduced error typeBlockImportFailedDueToMissingNodeTesting
Issue cannot be reproduced on every run of fast sync. Once it is reproduced, observe that missing state node error is logged (during regular sync) and that
BlockFetcherwill try to fetch missing state node, after which regular sync continues as usual without any errors.