Helper CLI for checking consensus #800
Conversation
AlexandraRoatis
force-pushed
the
re-import
branch
from
9b5d16c to
1df31e8
Compare
AlexandraRoatis
force-pushed
the
re-import
branch
from
1df31e8 to
0685f7a
Compare
|
|
||
| boolean fail = false; | ||
|
|
||
| long start = System.currentTimeMillis(); |
There was a problem hiding this comment.
nanoTime is more recommended for calculating time differences, I believe
https://docs.oracle.com/javase/7/docs/api/java/lang/System.html#nanoTime()
There was a problem hiding this comment.
Since the estimates are very inaccurate due to the nature of the operations being timed, I don't think switching to nanoTime would be very useful here. Especially since at best they give a minimum wait time to completion estimate, which is typically overly optimistic.
There was a problem hiding this comment.
I'll make a note of this suggestion in the initial issue for later enhancement.
AlexandraRoatis
force-pushed
the
re-import
branch
2 times, most recently
from
6958dee to
c3f5431
Compare
| @@ -538,9 +538,10 @@ public synchronized void compactState() { | |||
|
|
|||
| // TEMPORARY: here to support the ConsensusTest | |||
| public Pair<ImportResult, AionBlockSummary> tryToConnectAndFetchSummary( | |||
| AionBlock block, long currTimeSeconds) { | |||
| AionBlock block, long currTimeSeconds, boolean doExistCheck) { | |||
There was a problem hiding this comment.
This PR must have branched off when this change was still in there. It's been removed and we should make sure this doesn't go in
There was a problem hiding this comment.
I think you're referring to the other boolean constant that disabled the time check. The public tryToConnectAndFetchSummary currently appears on all branches. This boolean flag is needed here, otherwise the block will not get imported because it's already in the database.
| for (IByteArrayKeyValueDatabase db : databaseGroup) { | ||
| if (!names.contains(db.getName().get())) { | ||
| LOG.warn("Dropping database " + db.toString()); | ||
| db.drop(); |
There was a problem hiding this comment.
up to you, but I generally like to perform the action first and then broadcast a message, just in case the action failed then you're not left wondering whether the log is incorrect or not
There was a problem hiding this comment.
Added a log after the drop as well. I find the before log useful because if the processing gets stuck, we can know where.
| if (currentBlock % stepSize == 0) { | ||
| long time = System.currentTimeMillis() - start; | ||
|
|
||
| double timePerBlock = time / (currentBlock - startHeight + 1); |
There was a problem hiding this comment.
one of the operands should be cast to a double if you actually want to get the precision here
| try { | ||
| result = | ||
| chain.tryToConnectAndFetchSummary( | ||
| block, System.currentTimeMillis() / THOUSAND_MS, false); |
There was a problem hiding this comment.
TimeUnit is a nice alternative to changing between time units; pretty minor & really up to you
There was a problem hiding this comment.
I'll make a note of this suggestion in the initial issue for later enhancement.
AlexandraRoatis
force-pushed
the
re-import
branch
from
4dcd04f to
749adf3
Compare
AlexandraRoatis
force-pushed
the
re-import
branch
from
749adf3 to
bb71f3e
Compare
Description
Adding a new CLI option
--redo-import/--redo-import <start_height>that imports all the blocks from the genesis block or the given starting block till the top of the chain performing all the consensus checks.When starting from the genesis block all the other databases are dropped before the reimport starts.
Fixes Issue #560.
Type of change
Insert x into the following checkboxes to confirm (eg. [x]):
Testing
Please describe the tests you used to validate this pull request. Provide any relevant details for test configurations as well as any instructions to reproduce these results.
Verification
Insert x into the following checkboxes to confirm (eg. [x]):