-
Notifications
You must be signed in to change notification settings - Fork 370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HashStore is used to recover when the ledger starts, fixed the calcu… #271
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,34 +73,28 @@ def recoverTree(self): | |
.format(type(self.tree))) | ||
|
||
|
||
# ATTENTION! | ||
# This functionality is disabled until better consistency verification | ||
# implemented - always using recovery from transaction log | ||
# from ledger.stores.memory_hash_store import MemoryHashStore | ||
# from ledger.util import ConsistencyVerificationFailed | ||
# if not self.tree.hashStore \ | ||
# or isinstance(self.tree.hashStore, MemoryHashStore) \ | ||
# or self.tree.leafCount == 0: | ||
# logging.info("Recovering tree from transaction log") | ||
# self.recoverTreeFromTxnLog() | ||
# else: | ||
# try: | ||
# logging.info("Recovering tree from hash store of size {}". | ||
# format(self.tree.leafCount)) | ||
# self.recoverTreeFromHashStore() | ||
# except ConsistencyVerificationFailed: | ||
# logging.error("Consistency verification of merkle tree " | ||
# "from hash store failed, " | ||
# "falling back to transaction log") | ||
# self.recoverTreeFromTxnLog() | ||
|
||
logging.debug("Recovering tree from transaction log") | ||
from ledger.stores.memory_hash_store import MemoryHashStore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move these two and all your in-function imports to the top of files There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, done |
||
from ledger.util import ConsistencyVerificationFailed | ||
start = time.perf_counter() | ||
self.recoverTreeFromTxnLog() | ||
if not self.tree.hashStore \ | ||
or isinstance(self.tree.hashStore, MemoryHashStore) \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider doing this in a more generic way, avoid instance checking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
or self.tree.leafCount == 0: | ||
logging.debug("Recovering tree from transaction log") | ||
self.recoverTreeFromTxnLog() | ||
else: | ||
try: | ||
logging.debug("Recovering tree from hash store of size {}". | ||
format(self.tree.leafCount)) | ||
self.recoverTreeFromHashStore() | ||
except ConsistencyVerificationFailed: | ||
logging.error("Consistency verification of merkle tree " | ||
"from hash store failed, " | ||
"falling back to transaction log") | ||
self.recoverTreeFromTxnLog() | ||
|
||
end = time.perf_counter() | ||
t = end - start | ||
logging.debug("Recovered tree from transaction log in {} seconds". | ||
format(t)) | ||
logging.debug("Recovered tree in {} seconds".format(t)) | ||
|
||
def recoverTreeFromTxnLog(self): | ||
# TODO: in this and some other lines specific fields of | ||
|
@@ -118,7 +112,7 @@ def recoverTreeFromHashStore(self): | |
hashes = list(reversed(self.tree.inclusion_proof(treeSize, | ||
treeSize + 1))) | ||
self.tree._update(self.tree.leafCount, hashes) | ||
self.tree.verifyConsistency(self._transactionLog.numKeys) | ||
self.tree.verify_consistency(self._transactionLog.numKeys) | ||
|
||
def add(self, leaf): | ||
self._addToStore(leaf) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,4 @@ | ||
import types | ||
from random import randint | ||
|
||
import pytest | ||
|
||
from plenum.common.constants import DOMAIN_LEDGER_ID, CONSISTENCY_PROOF | ||
from plenum.common.ledger import Ledger | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also add simple test that compares root hashes of trees recovered using transaction log and using hash store? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's there in |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this should be here? Can other implementations of merkle tree have different number of nodes for the same data?
It could be here if it is private, but I see that it is also used in hash_store.py:146,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we store full subtree roots in CompactMerkleTree, i am not sure whether we will need to store them in case of other kinds of trees, but we don't have others as of now. Same argument for HashStore, not sure if we will need HashStore with all tree implementations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to move it to MerkleTree or some utils class?
Even if we are not going to have other implementations we already have class hierarchy for this, so we either should follow it or drop it. MerkleTree is used as a type of tree argument, but then we explicitly use CompactMerkleTree, this is confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will move it when we need it at other places, i don't like idea of moving things to general utility files, maybe later on if we have several kinds of trees and some functions apply to some of those and not others, we will create a
tree_utils
or something like that