On-demand ordering gate and related fixes #1675

lebdron · 2018-08-24T19:36:49Z

Description of the Change

On-demand ordering gate, which requests proposals from the on-demand ordering service, and uses voting to pass committed proposal to the pipeline

Benefits

Ordering gate which does not require block reject if peers do not agree on a proposal

Possible Drawbacks

TransactionBatch is modified because it was impossible to build ordering gate with validator linking issues

muratovv · 2018-08-27T10:05:49Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+   * @param round current round
+   * @return next reject round
+   */
+  constexpr transport::RoundType nextRejectRound(


What is the reason for constexpr here?

muratovv · 2018-08-27T10:07:52Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+        visit_in_place(event,
+                       [this](BlockEvent block) {
+                         // block committed, increment block round
+                         current_round_ = {block.height, 1};


This, also, possible to move in the separate function as nextRejectRound.

It is used only once now, so it should not be required.

muratovv · 2018-08-27T10:26:48Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+
+    return visit_in_place(
+        outcome,
+        [&](network::ProposalCommit commit) {


Is it true that outcome value will move to commit variable? Or this is just redundant copy?

muratovv · 2018-08-27T12:35:55Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+
+rxcpp::observable<std::shared_ptr<shared_model::interface::Proposal>>
+OnDemandOrderingGate::on_proposal() {
+  using RetType = decltype(on_proposal());


Maybe set more meaningful name for the type?

muratovv · 2018-08-27T12:39:51Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+              [&] { return reject_case(commit.round); });
+        },
+        [&](network::ProposalReject reject) {
+          return reject_case(reject.round);


Why have we reject case here? I expected that this logic will be in block agreement, isn't it?

muratovv · 2018-08-27T12:46:14Z

irohad/ordering/impl/on_demand_ordering_gate.hpp

+      std::function<shared_model::interface::TransactionBatch(
+          std::shared_ptr<const shared_model::interface::Transaction>)>
+          batch_factory_;
+      rxcpp::composite_subscription subscription_;


subscription_ name is not obvious for a reader. Maybe rename it with a business logic oriented name?

muratovv · 2018-08-27T12:54:10Z

shared_model/interfaces/iroha_internal/transaction_batch_factory.cpp

+        const validation::FieldValidator &field_validator);
+
+  }  // namespace interface
+}  // namespace shared_model


Add a new line, plz.

muratovv · 2018-08-27T13:03:29Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+void OnDemandOrderingGate::propagateTransaction(
+    std::shared_ptr<const shared_model::interface::Transaction> transaction)
+    const {
+  propagateBatch(batch_factory_(std::move(transaction)));


I suggest not doing support this method because we have been moving to batch API.
So, maybe throw an exception instead?

Changes done

kamilsa · 2018-08-28T07:27:13Z

irohad/ordering/impl/on_demand_ordering_service_impl.cpp

+void OnDemandOrderingServiceImpl::onCollaborationOutcome(
+    transport::RoundType round) {
+  log_->info(
+      "onCollaborationOutcome => round[{}, {}]", round.first, round.second);


Can we change round type, so that we can write round.block_round, round.reject_round instead of round.first and round.second. It is really hard to read sometimes

kamilsa · 2018-08-29T06:53:12Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+
+void OnDemandOrderingGate::propagateBatch(
+    const shared_model::interface::TransactionBatch &batch) const {
+  // shared lock


Seems like an obvious comment

kamilsa · 2018-08-29T08:22:20Z

shared_model/interfaces/iroha_internal/transaction_batch_factory.hpp

+namespace shared_model {
+  namespace interface {
+
+    class TransactionBatchFactory {


Please add short description of the class

kamilsa · 2018-08-29T08:37:20Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+        std::lock_guard<std::shared_timed_mutex> lock(mutex_);
+
+        visit_in_place(event,
+                       [this](BlockEvent block) {


why not const ref?

kamilsa · 2018-08-29T08:37:27Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+                         // block committed, increment block round
+                         current_round_ = {block.height, 1};
+                       },
+                       [this](EmptyEvent empty) {


why not const ref?

kamilsa · 2018-08-29T08:39:10Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+                       },
+                       [this](EmptyEvent empty) {
+                         // no blocks committed, increment reject round
+                         current_round_ = {current_round_.first,


can we use something else instead of pair. I mean round.first and round.second is hard to understand if you don't know that round is pair of (current round and reject round if I am not mistaken)

muratovv · 2018-08-31T08:25:01Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+void OnDemandOrderingGate::propagateTransaction(
+    std::shared_ptr<const shared_model::interface::Transaction> transaction)
+    const {
+  throw std::logic_error("Not implemented");


Maybe write "deprecated" instead of "logic error"?

I miss with that. I mean "deprecated" instead of "Not implemented". Sorry about that.

muratovv · 2018-08-31T08:27:03Z

irohad/ordering/impl/on_demand_ordering_gate.cpp

+
+void OnDemandOrderingGate::setPcs(
+    const iroha::network::PeerCommunicationService &pcs) {
+  throw std::logic_error("Not implemented");


Here is also, possibly to provide the reason of an exception, isn't it?

Issues addressed

Signed-off-by: Andrei Lebedev <lebdron@gmail.com> Refactor ordering gate Signed-off-by: Andrei Lebedev <lebdron@gmail.com> Add a comment for reject case Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

sorabot · 2018-08-31T13:12:09Z

SonarQube analysis reported 2 issues

on_demand_ordering_service_impl.hpp#L57: Unused private function: 'OnDemandOrderingServiceImpl::packNextProposals'
on_demand_ordering_service_impl.hpp#L64: Unused private function: 'OnDemandOrderingServiceImpl::tryErase'

* BFT OS trunk * On demand OS transport (#1635) * On demand OS connection manager (#1645) * On-demand ordering gate and related fixes (#1675) Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

lebdron requested review from muratovv and kamilsa August 24, 2018 19:36

l4l added needs-review pr awaits review from maintainers ordering labels Aug 27, 2018

muratovv previously requested changes Aug 27, 2018

View reviewed changes

muratovv mentioned this pull request Aug 27, 2018

Feature/mst batches #1681

Merged

lebdron force-pushed the fix/bft-os-algo branch from 67ea0b5 to f8be7cc Compare August 27, 2018 15:36

lebdron force-pushed the feature/bft-os-ordering-gate branch from e36513c to fc30972 Compare August 28, 2018 08:56

lebdron requested a review from muratovv August 28, 2018 09:04

kamilsa previously requested changes Aug 29, 2018

View reviewed changes

lebdron changed the base branch from fix/bft-os-algo to trunk/bft-os August 31, 2018 06:06

lebdron force-pushed the feature/bft-os-ordering-gate branch from b176b5c to 6c9835e Compare August 31, 2018 06:06

muratovv approved these changes Aug 31, 2018

View reviewed changes

lebdron changed the base branch from trunk/bft-os to refactor/remove-proposal-gate August 31, 2018 10:45

lebdron force-pushed the feature/bft-os-ordering-gate branch from 6c9835e to c82d3ea Compare August 31, 2018 10:45

lebdron requested a review from kamilsa August 31, 2018 10:46

lebdron added 3 commits August 31, 2018 14:39

Ordering gate, tests, and related fixes

cc5052e

Signed-off-by: Andrei Lebedev <lebdron@gmail.com> Refactor ordering gate Signed-off-by: Andrei Lebedev <lebdron@gmail.com> Add a comment for reject case Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Review fixes

fa0c420

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

Fix wrong serialization in client

e672a04

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

lebdron force-pushed the feature/bft-os-ordering-gate branch from f693862 to e672a04 Compare August 31, 2018 11:40

lebdron changed the base branch from refactor/remove-proposal-gate to trunk/bft-os August 31, 2018 11:40

Remove unused method

919d72b

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>

kamilsa approved these changes Aug 31, 2018

View reviewed changes

lebdron merged commit ba05083 into trunk/bft-os Aug 31, 2018

lebdron deleted the feature/bft-os-ordering-gate branch August 31, 2018 15:43

kamilsa mentioned this pull request Nov 1, 2018

Hyperledger Iroha v1.0 beta-5 version #1825

Closed

neewy mentioned this pull request Nov 2, 2018

Hyperledger Iroha v1.0 beta-5 version #1832

Closed

kamilsa mentioned this pull request Dec 12, 2018

Hyperledger Iroha v1.0 Release Candidate 1 #1941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

On-demand ordering gate and related fixes #1675

On-demand ordering gate and related fixes #1675

lebdron commented Aug 24, 2018

muratovv Aug 27, 2018

muratovv Aug 27, 2018

lebdron Aug 28, 2018

muratovv Aug 27, 2018

muratovv Aug 27, 2018

muratovv Aug 27, 2018

muratovv Aug 27, 2018

muratovv Aug 27, 2018

muratovv Aug 27, 2018

kamilsa Aug 28, 2018

kamilsa Aug 29, 2018

kamilsa Aug 29, 2018

kamilsa Aug 29, 2018

kamilsa Aug 29, 2018

kamilsa Aug 29, 2018

muratovv Aug 31, 2018

muratovv Aug 31, 2018

muratovv Aug 31, 2018

sorabot commented Aug 31, 2018

On-demand ordering gate and related fixes #1675

On-demand ordering gate and related fixes #1675

Conversation

lebdron commented Aug 24, 2018

Description of the Change

Benefits

Possible Drawbacks

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

sorabot commented Aug 31, 2018