Skip to content
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

INDY-1843: Discard pre-prepare with ordered request #990

Merged

Conversation

Toktar
Copy link
Contributor

@Toktar Toktar commented Nov 22, 2018

If primary will include digest of already ordered request in pre-prepare message, then this pre-prepare will not be considered as faulty one and ordered request will be re-asked from every node.
For fix this bug added check for received pre-prepare message with not finalize request. Pre-prepare message will be discarded if one or more requests are already ordered.

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
@@ -1122,6 +1122,12 @@ def report_suspicious(reason):
"then the latest one - ignoring it".format(key))
elif why_not == PP_CHECK_REQUEST_NOT_FINALIZED:
non_fin_reqs = self.nonFinalisedReqs(pre_prepare.reqIdr)
for req in non_fin_reqs:
if req not in self.requests and self.node.seqNoDB.get(req) != (None, None):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be discuss because it's not obviously for backup replicas.

if req not in self.requests and self.node.seqNoDB.get(req) != (None, None):
self.logger.info("Request digest {} already ordered. Discard {} "
"from {}".format(req, pre_prepare, sender))
report_suspicious(Suspicions.DUPLICATE_PPR_SENT)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be DUPLICATE_PPR_SENT or a new suspicions PPR_WITH_ORDERED_REQUEST?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think PPR_WITH_ORDERED_REQUEST is better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ArtObr
Copy link
Contributor

ArtObr commented Nov 22, 2018

What about repeated requests, which are finalized? (for example from previous ordered 3pc batch, before _gc called)

@Toktar
Copy link
Contributor Author

Toktar commented Nov 22, 2018

@ArtObr We are not catch this problem in the fix. But it needs to solve, of course.



def test_process_pre_prepare_with_not_ordered_request(fake_node):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the test be called test_process_pre_prepare_with_ordered_request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
@ashcherbakov ashcherbakov merged commit a3d52f5 into hyperledger:master Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants