INDY-1944: Audit ledger support #1091

ashcherbakov · 2019-02-15T16:31:50Z

Added audit ledger
Integrated it into consensus protocol
validate audit txn root hash from Primary
TBD: move all ledger creation methods from node.py into a separate helper
TBD: move all catchup-related callbacks from node.py into a separate helper

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

refactor replica unit tests Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

…nto audit-ledger

ArtObr · 2019-02-18T12:25:37Z

plenum/server/node.py

@@ -3487,6 +3529,9 @@ def onBatchRejected(self, ledger_id):
            self.get_req_handler(ledger_id).onBatchRejected()
        else:
            logger.debug('{} did not know how to handle for ledger {}'.format(self, ledger_id))
+
+        self.audit_handler.post_batch_rejected()


ledger_id required. (probable test fault reason)

Yes, thank you

plenum/common/ledger.py

@@ -140,3 +140,16 @@ def reset_uncommitted(self):
        self.uncommittedTxns = []
        self.uncommittedRootHash = None
        self.uncommittedTree = None
+
+    def get_uncommitted_txns(self):
+        return self.uncommittedTxns


plenum/common/transactions.py

@@ -13,4 +13,5 @@ class PlenumTransactions(Transactions):
    # components.
    NODE = "0"
    NYM = "1"
+    AUDIT = "2"


plenum/common/transactions.py

@@ -13,4 +13,5 @@ class PlenumTransactions(Transactions):
    # components.
    NODE = "0"
    NYM = "1"
+    AUDIT = "2"
    GET_TXN = "3"


skhoroshavin · 2019-02-18T12:30:47Z

plenum/server/batch_handlers/batch_request_handler.py

@@ -10,7 +10,7 @@ def __init__(self, database_manager: DatabaseManager, ledger_id):
        self.database_manager = database_manager
        self.ledger_id = ledger_id

-    def commit_batch(self, txn_count, state_root, txn_root, pp_time, prev_result):
+    def commit_batch(self, ledger_id, txn_count, state_root, txn_root, pp_time, prev_result=None):


Also prev_result is a bit ambiguos name: when I first looked at it I though it is result of previous run of this handler, and only after looking at calling code it became clear that it is result of previous call to another handler. This is pretty subjective though.

What will be a better name by your opinion?

prev_handler_result?

plenum/server/database_manager.py

+    @property
+    def ledgers(self):
+        # TODO: change this. Too inefficient to build dict every time
+        return dict((lid, db.ledger) for lid, db in self.databases.items())


plenum/server/batch_handlers/batch_request_handler.py

@@ -10,7 +10,7 @@ def __init__(self, database_manager: DatabaseManager, ledger_id):
        self.database_manager = database_manager
        self.ledger_id = ledger_id


skhoroshavin · 2019-02-18T13:16:51Z

plenum/test/audit_ledger/test_audit_ledger_freshness.py

+    check_audit_ledger_updated(audit_size_initial, txnPoolNodeSet,
+                               audit_txns_added=3)
+    for node in txnPoolNodeSet:
+        check_audit_txn(txn=node.auditLedger.getBySeqNo(node.auditLedger.size - 2),


audit_size_initial + 1 would be a bit more intuitive here

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

…lenum into audit-ledger

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Fix problem with CatchupRep splitting and catchup tests

…to audit-ledger

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

…oesn't have any audit txns Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

skhoroshavin

I didn't finish review - probably there will be more comments. I really like what is being done here, there is only one issue that probably needs immediate attention, all others comments are quite minor or even just some rants.

plenum/server/batch_handlers/audit_batch_handler.py

skhoroshavin · 2019-02-22T10:11:34Z

plenum/server/batch_handlers/audit_batch_handler.py

+        self.ledger.append_txns_metadata([txn], three_pc_batch.pp_time)
+
+        # 3. Add to the Ledger
+        self.ledger.appendTxns([txn])


It's not a change request for this code, but - do we really need to have separate methods to append txns and txns metadata, why not one atomic method?

Agree. Will be changed in the next PR.

skhoroshavin · 2019-02-22T10:15:42Z

plenum/server/batch_handlers/audit_batch_handler.py

+            if lid == AUDIT_LEDGER_ID:
+                continue
+            # 2. ledger size
+            txn[AUDIT_TXN_LEDGERS_SIZE][str(lid)] = ledger.uncommitted_size


Do we really need to convert ledger id to string here, why not just store integer?

skhoroshavin · 2019-02-22T10:17:57Z

plenum/server/batch_handlers/audit_batch_handler.py

+            self.__fill_ledger_root_hash(txn, three_pc_batch, lid, last_audit_txn)
+
+        # 4. state root hash
+        txn[AUDIT_TXN_STATE_ROOT][str(three_pc_batch.ledger_id)] = Ledger.hashToStr(three_pc_batch.state_root)


Is there any reason why it's different from how ledger root hashes are handled?

Yes.

state is never updated without ledger, so it's sufficient to have refs to previous updates in ledger part only.

this is done like to simplify future support of combined state trie

skhoroshavin · 2019-02-22T10:21:40Z

plenum/server/batch_handlers/audit_batch_handler.py

+
+        # 1. ledger is changed in this batch => root_hash
+        if lid == target_ledger_id:
+            txn[AUDIT_TXN_LEDGER_ROOT][str(lid)] = Ledger.hashToStr(three_pc_batch.txn_root)


We have ledger id as integer everywhere, why convert it to string when storing?

I think it's done as string to be able to support "all" key when we combine all the states into one.
But even in this case we can use -1.
So, agree. Will be changed in a PR.

skhoroshavin · 2019-02-22T10:34:00Z

plenum/server/batch_handlers/batch_request_handler.py

        pass

    @abstractmethod
-    def post_batch_rejected(self):
+    def post_batch_rejected(self, ledger_id, prev_handler_result=None):


I might be wrong here, but do we really need ledger_id here (and in commit_batch as well)? We are just rejecting (or committing) last applied batch, but we should have already received this batch from post_batch_applied if I understand logic correctly. Furthermore, these parameters are not used in AuditBatchHandler despite it actually handles multiple ledgers.

Also names look a bit misleading - post_batch_applied and post_batch_rejected look like either of them could be called, while in fact flow is more like post_batch_applied followed by either commit_batch or post_batch_rejected. So name like reject_batch looks a bit better. Again - this applies only if I really understand logic correctly, and even if so - this might be out of scope of this PR, since this comment relates to new request handlers, not audit ledger itself.

Agree. Will be done in next PR.

skhoroshavin · 2019-02-22T10:44:08Z

plenum/server/database_manager.py

@@ -11,7 +11,7 @@ def __init__(self):
        self.databases = {}  # type: Dict[int, Database]
        self.stores = {}

-    def register_new_database(self, lid, ledger: Ledger, state: State):
+    def register_new_database(self, lid, ledger: Ledger, state: State = None):


Suggested change

def register_new_database(self, lid, ledger: Ledger, state: State = None):

def register_new_database(self, lid, ledger: Ledger, state: Optional[State] = None):

Agree. Will be done in next PR.

skhoroshavin · 2019-02-22T11:00:10Z

plenum/server/replica.py

+        # 3. VALIDATE APPLIED
+        invalid_from_pp = invalid_index_serializer.deserialize(pre_prepare.discarded)
+        why_not_applied = self._validate_applied_pre_prepare(pre_prepare,
+                                                             reqs, invalid_indices, invalid_from_pp)


Hooray, _apply_pre_prepare monstrosity is finally split into smaller chunks! Thanks a lot! 🎉

skhoroshavin · 2019-02-22T11:05:26Z

plenum/server/replica.py

        if digest != pre_prepare.digest:
-            if self.isMaster:
-                revert()
            return PP_APPLY_WRONG_DIGEST


Just a small note - this check can be done before PREPREPARE is event applied. I remember we have some kind of static validation for messages (mostly schema checking) - can we move this check to that part somehow?

Agree. Not in the scope of this work, but will be done in a separate PR.

skhoroshavin · 2019-02-22T11:56:42Z

plenum/server/request_managers/write_request_manager.py

-    def post_apply_batch(self, ledger_id, state_root):
-        handlers = self.batch_handlers.get(ledger_id, None)
+    # TODO: pass PrePrepare here
+    def post_apply_batch(self, three_pc_batch):


Why do you think that PrePrepare would be better here? three_pc_batch looks good enough here.

Yes, old TODO comment added before three_pc_batch was added. The comment will be removed in the next PR.

skhoroshavin · 2019-02-22T11:58:35Z

plenum/server/batch_handlers/audit_batch_handler.py

+        _, txn_count = self.tracker.reject_batch()
+        self.ledger.discardTxns(txn_count)
+
+    def commit_batch(self, ledger_id, txn_count, state_root, txn_root, pp_time, prev_result=None):


Suggested change

def commit_batch(self, ledger_id, txn_count, state_root, txn_root, pp_time, prev_result=None):

def commit_batch(self, ledger_id, txn_count, state_root, txn_root, pp_time, prev_handler_result=None):

ashcherbakov added 16 commits February 8, 2019 06:11

add audit ledger

43bf968

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

Audit Batch Request Handler is implemented

dce1a7d

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

more tests and improvements

5cd095b

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

more tests and integration of audit ledger into consensus

e460e54

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

revert non-finished changes

30618f1

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

flake8 fixes

d589b43

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

8f1de64

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix for tests

7300584

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

integrate audit txn root check into PrePrepare processing;

5b81905

refactor replica unit tests Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

d26e7ce

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix registering of plugin DBs

82fa8ec

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

change onBatchCreated interface to take into account all the parameters

ae78c40

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

2920511

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

flake8 fixes

5ffca5c

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

do not apply to audit ledger if pre-prepare doesn't have it

4f2cbac

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

b6ac6e1

…nto audit-ledger

ArtObr reviewed Feb 18, 2019

View reviewed changes

skhoroshavin reviewed Feb 18, 2019

View reviewed changes

Toktar and others added 12 commits February 19, 2019 10:39

INDY-1944: add a fix in DBs creating in Node

c0a4730

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1944: bugfixing for CatchupRep splitting

a07ec22

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

fix tests

49ccba4

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

61d713a

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

Merge branch 'audit-ledger' of https://github.com/ashcherbakov/indy-p…

470e150

…lenum into audit-ledger

INDY-1944: fix test_large_catchup.py

4e8d354

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

fix tests

61713c7

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

INDY-1944: update catchup tests

aaaf4ec

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Merge pull request #5 from Toktar/audit-ledger

16ee66d

Fix problem with CatchupRep splitting and catchup tests

Merge branch 'audit-ledger' of github.com:ashcherbakov/indy-plenum in…

abad44e

…to audit-ledger

fix tests

b522d7e

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

13f187d

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

ashcherbakov added 7 commits February 20, 2019 23:54

fix tests

b81fa25

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

da66fbf

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

673faac

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

use ledger tracker in audit handler to fix the case when PrePrepare d…

d7c44d6

…oesn't have any audit txns Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

7a8aa99

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

7981c8f

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

fix tests

0e09675

Signed-off-by: ashcherbakov <alexander.sherbakov@dsr-corporation.com>

ashcherbakov changed the title ~~[WIP] INDY-1944: Audit ledger support~~ INDY-1944: Audit ledger support Feb 21, 2019

skhoroshavin reviewed Feb 22, 2019

View reviewed changes

skhoroshavin approved these changes Feb 22, 2019

View reviewed changes

andkononykhin approved these changes Feb 22, 2019

View reviewed changes

ashcherbakov merged commit 5a53794 into hyperledger:master Feb 22, 2019

		@@ -10,7 +10,7 @@ def __init__(self, database_manager: DatabaseManager, ledger_id):
		self.database_manager = database_manager
		self.ledger_id = ledger_id

	def register_new_database(self, lid, ledger: Ledger, state: State = None):
	def register_new_database(self, lid, ledger: Ledger, state: Optional[State] = None):

	def commit_batch(self, ledger_id, txn_count, state_root, txn_root, pp_time, prev_result=None):
	def commit_batch(self, ledger_id, txn_count, state_root, txn_root, pp_time, prev_handler_result=None):

INDY-1944: Audit ledger support #1091

INDY-1944: Audit ledger support #1091

Conversation

ashcherbakov commented Feb 15, 2019 • edited

ArtObr Feb 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

skhoroshavin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashcherbakov commented Feb 15, 2019 •

edited

ArtObr Feb 18, 2019 •

edited