Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Ordering Service: on request proposal strategy #2215

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

muratovv
Copy link
Contributor

@muratovv muratovv commented Apr 7, 2019

Description of the Change

The initial intention of the PR is to introduce a strategy for preventing the creation of proposals in OS. But this pr contains dependent changes for the proper working of the strategy.

Benefits

Add round optimization for faster collaboration. Also, Closer to release state.

Possible Drawbacks

A lot of different features which introduce in one PR

List of features

  • Creation strategy interface in OS
  • Strategy which prevents the creation of proposals
  • OS\OG transport contains peer key in net interaction
  • Supermajority has a new method
  • Small changes in validation/answer

Alternate Designs [optional]

The transport layer is not backward compatible - introduces peer key of the sending side. There is an alternative with meta information in the request.

Drawback

The PR doesn't work because it is blocked by hyperledger-iroha/iroha#2066.

Signed-off-by: Fedor Muratov <[email protected]>
Signed-off-by: Fedor Muratov <[email protected]>
Signed-off-by: Fedor Muratov <[email protected]>
Signed-off-by: Fedor Muratov <[email protected]>
Signed-off-by: Fedor Muratov <[email protected]>
Signed-off-by: Fedor Muratov <[email protected]>
Signed-off-by: Fedor Muratov <[email protected]>
*/

#ifndef IROHA_KICK_OUT_PROPOSAL_CREATION_STRATEGY_HPP
#define IROHA_KICK_OUT_PROPOSAL_CREATION_STRATEGY_HPP
Copy link
Contributor

Choose a reason for hiding this comment

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

IROHA_SUPER_KICK_OUT_MEGA_PROPOSAL_ULTRA_CREATION_STRATEGY_9000_HPP
:D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you suggest a more appropriate name?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, It was not an issue

Copy link
Contributor

@igor-egorov igor-egorov left a comment

Choose a reason for hiding this comment

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

Looks good. Summon @MBoldyrev

shared_model::validation::Answer answer;
answer.addReason(std::move(reason));
if (answer.hasErrors()) {
log_->error("Peer key struct isn't parsed, {}", answer.reason());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log_->error("Peer key struct isn't parsed, {}", answer.reason());
log_->error("Peer key struct was not parsed, {}", answer.reason());

@lebdron lebdron changed the title Feature/os on request proposal strategy Ordering Service: on request proposal strategy Apr 10, 2019
std::shared_ptr<MockTransactionBatchFactory> batch_factory;
std::shared_ptr<OnDemandOsServerGrpc> server;
consensus::Round round{1, 2};
};

/**
* Separate action required because CollectionType is non-copyable
* Separate action required because CollectionType is non-copy1able
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Separate action required because CollectionType is non-copy1able
* Separate action required because CollectionType is non-copyable

looks like a typo

@lebdron lebdron requested review from MBoldyrev and removed request for lebdron April 12, 2019 17:47
Signed-off-by: Andrei Lebedev <[email protected]>
Signed-off-by: Andrei Lebedev <[email protected]>
@lebdron lebdron marked this pull request as ready for review April 14, 2019 17:17
@@ -19,6 +19,12 @@ namespace iroha {
agreed, all, detail::kSupermajorityCheckerKfPlus1Bft);
}

bool SupermajorityCheckerBft::hasMajority(PeersNumberType voted,
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline: need to rename the method

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants