INDY-2032: Fix phantom transactions in audit ledger #1151
Conversation
ashcherbakov
commented
Apr 5, 2019
ashcherbakov
commented
- Fix sending of 3PC messages by a primary on an old view during or in between view changes. This was leading to phantom txns in audit ledger
- More integration tests
- Get rid of stashedOrderedRequest
- Improved replica 3PC validation to allow only Commits during the view change.
Signed-off-by: ashcherbakov <[email protected]>
Signed-off-by: ashcherbakov <[email protected]>
Signed-off-by: ashcherbakov <[email protected]>
Signed-off-by: ashcherbakov <[email protected]>
ashcherbakov
changed the title
plenum/server/node.py
Outdated
| @property | ||
| def pre_view_change_in_progress(self): | ||
| return False if self.view_changer is None \ | ||
| else self.view_changer.pre_view_change_in_progress |
There was a problem hiding this comment.
I'd prefer something more readable and more clear intentions like
if self.view_changer is None:
return False
return self.view_changer.pre_view_change_in_progress
plenum/server/node.py
Outdated
| self.processOrdered(msg) | ||
| else: | ||
| logger.info("{} stashing {} since mode is {}".format(self, msg, self.mode)) | ||
| self.stashedOrderedReqs.append(msg) | ||
| logger.info("{} can not process Ordered message {} since mode is {}".format(self, msg, self.mode)) |
There was a problem hiding this comment.
I think this should be at least warning, since here we're discarding ordering result, and if I understand correctly we should never reach this point in production code.
| def _apply_reqs(self, requests, three_pc_batch: ThreePcBatch): | ||
| for req in requests: | ||
| self._node.applyReq(req, three_pc_batch.pp_time) | ||
| self._node.onBatchCreated(three_pc_batch) |
There was a problem hiding this comment.
Why not use one from node? It looks like this should be really an atomic operation available through node interface.
There was a problem hiding this comment.
The one from node was deleted, since it's not used anywhere else.
There was a problem hiding this comment.
Fixed. The method is in Node again.
| @pre_view_change_in_progress.setter | ||
| def pre_view_change_in_progress(self, value: bool): | ||
| self._pre_view_change_in_progress = value | ||
|
|
There was a problem hiding this comment.
Do we really need to make this a property if we are just using trivial getter and setter without any additional logic?
There was a problem hiding this comment.
As a quick fix, I did it similar to view_change_in_progress, but yes, agree.
| @@ -52,7 +55,7 @@ def validate_3pc_msg(self, msg): | |||
| if view_no == self.replica.viewNo and node.view_change_in_progress: | |||
| return STASH_VIEW, FUTURE_VIEW | |||
|
|
|||
| # If Catchup in View Change finished then process a message | |||
| # If Catchup in View Change finished then process Commit messages | |||
| if node.is_synced and node.view_change_in_progress: | |||
| return PROCESS, None | |||
There was a problem hiding this comment.
Explicit check that this is a really commit message would be good here.
There was a problem hiding this comment.
The explicit check is already done above.
Signed-off-by: ashcherbakov <[email protected]>
Signed-off-by: ashcherbakov <[email protected]>
Signed-off-by: ashcherbakov <[email protected]>
|
(ci) test this please |
Signed-off-by: ashcherbakov <[email protected]>
|
(ci) test this please |
Signed-off-by: ashcherbakov <[email protected]>
| @@ -2670,7 +2670,23 @@ def processOrdered(self, ordered: Ordered): | |||
| ordered.stateRootHash, | |||
| ordered.txnRootHash)) | |||
|
|
|||
| self.executeBatch(ThreePcBatch.from_ordered(ordered), | |||
| three_pc_batch = ThreePcBatch.from_ordered(ordered) | |||
| if self.db_manager.ledgers[AUDIT_LEDGER_ID].uncommittedRootHash is None: | |||
There was a problem hiding this comment.
Could it happen so that uncommittedRootHash is not None but due to other batch in flight, so that we still need to manually apply transactions?
There was a problem hiding this comment.
I think no, because here we always have apply-commit cycle, so that even if this is an Ordered msg came in between catchup rounds during view change, each Ordered will be re-applied and committed immediately, so that it always end up with empty uncommitted state if the audit ledger.
