BFT YacGate #1731

l4l · 2018-09-24T10:23:40Z

Description of the Change

YacGate that supports rounds and use more precise output on its outcome

Benefits

One more step toward bft consensus

Usage Examples or Tests

For now only yac target can be built

lebdron · 2018-09-24T13:07:46Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

+          // update BlockCreator iface according to YacGate
+          this->vote(
+              std::shared_ptr<shared_model::interface::Proposal>(
+                  (shared_model::proto::ProtoProposalFactory<


Proto should not be used even as a temporary solution. Consider making a dummy interface implementation in the same file.

Doesn't look like a good alternative here, since it's even more code which still remain boilerplate

lebdron · 2018-09-24T13:11:48Z

irohad/consensus/yac/yac_hash_provider.hpp

@@ -52,6 +42,9 @@ namespace iroha {
         */
        std::string block_hash;

+        /// Consensus round
+        ordering::transport::Round round;


No need to add this field in this pull request if storage and transport logic is unchanged.

lebdron · 2018-09-24T13:12:15Z

irohad/consensus/yac/yac_hash_provider.hpp

@@ -78,7 +71,8 @@ namespace iroha {
         * @return hashed value of block
         */
        virtual YacHash makeHash(
-            const shared_model::interface::Block &block) const = 0;
+            const shared_model::interface::Block &block,
+            const shared_model::interface::Proposal &proposal) const = 0;


Please add round parameter here.

lebdron · 2018-09-24T13:18:30Z

irohad/network/consensus_gate.hpp

+
+    /// Reject on block
+    struct BlockReject {
+      consensus::yac::YacHash hash;


YacHash is an implementation of consensus, it should not be present in this file

lebdron · 2018-09-24T13:19:03Z

irohad/network/consensus_gate.hpp

 */

 #ifndef IROHA_CONSENSUS_GATE_HPP
 #define IROHA_CONSENSUS_GATE_HPP

 #include <rxcpp/rx.hpp>

+#include "consensus/yac/yac_hash_provider.hpp"


Please remove this include from interface, because it is a part of implementation.

lebdron · 2018-09-24T13:20:46Z

irohad/consensus/yac/impl/yac_hash_provider_impl.cpp

-        result.proposal_hash = hex_hash;
-        result.block_hash = hex_hash;
+        result.proposal_hash = proposal.hash().hex();
+        result.block_hash = block.hash().hex();


No need to change this method in this pull request if other parts of Yac are not changed.

lebdron · 2018-09-24T13:32:28Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

+        return rxcpp::observable<>::create<GateObject>([&](auto subscriber) {
+          const auto hash = getHash(msg.votes);
+          if (not hash) {
+            log_->info("Invalid commit message, hashes are different");


Commit message cannot be invalid, since it is guaranteed by CommitMessage interface. Consider removing this conditional.

kamilsa · 2018-09-28T07:25:53Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

+
+          log_->info("Voted for another block, waiting for sync");
+          subscriber.on_next(network::VoteOther{current_block_});
+          subscriber.on_completed();


Here should be also processing of agreement on none

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron · 2018-10-04T15:30:51Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

-              });
+          return visit_in_place(
+              message,
+              [this](const CommitMessage &msg) { return handleCommit(msg); },


Please create a nested static visitor class instead.

What's the profit?

No redundant visit_in_place with two proxy methods.

...will be replaced with redundant visit class with two methods?
I don't notice any profit again, could you describe it?

lebdron · 2018-10-04T15:35:20Z

irohad/network/consensus_gate.hpp

 */

 #ifndef IROHA_CONSENSUS_GATE_HPP
 #define IROHA_CONSENSUS_GATE_HPP

 #include <rxcpp/rx.hpp>

+#include "ordering/on_demand_os_transport.hpp"


Use this instead
https://github.com/hyperledger/iroha/blob/114e25d9b642e0b3e2c797641e17d95f6ea637ae/irohad/consensus/round.hpp#L28

lebdron · 2018-10-04T15:37:44Z

irohad/network/consensus_gate.hpp

+          std::shared_ptr<shared_model::interface::Block> block,
+          Round round) = 0;
+
+      using GateObject = boost::variant<PairValid,


This will require creating a .cpp file for the interface, and another header.

Include variant_fwd.hpp in this header file

Create a separate header with variant.hpp include and `extern template for the variant

Create a .cpp file with variant instantiations

lebdron · 2018-10-04T15:38:12Z

irohad/network/consensus_gate.hpp

 }  // namespace shared_model

 namespace iroha {
  namespace network {
+    /// Current pair is valid
+    struct PairValid {
+      std::shared_ptr<shared_model::interface::Block> block_;


No need for trailing underscore, since it is a public member.

lebdron · 2018-10-04T15:38:21Z

irohad/network/consensus_gate.hpp

+
+    /// Network votes for another pair and round
+    struct VoteOther {
+      std::shared_ptr<shared_model::interface::Block> block_;


Is current block required for synchronizer to load the committed block? Maybe use something else instead?

I'm not really aware of it. I just guessed that Block will be enough.

lebdron · 2018-10-04T15:39:53Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

-                   block->hash().toString());
-        auto order = orderer_->getOrdering(hash);
+      void YacGateImpl::vote(
+          std::shared_ptr<shared_model::interface::Proposal> proposal,


Optionals should be used for both proposal and block, because there could be no proposal from the current ordering service, and therefore no block.

Optional sounds redundant, maybe just use (& notice in docs) nullptr as no proposa/block case?

No, since we guarantee that pointers are not nullable in server code.

lebdron · 2018-10-04T15:41:26Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

+
+      rxcpp::observable<YacGateImpl::GateObject> YacGateImpl::handleCommit(
+          const CommitMessage &msg) {
+        return rxcpp::observable<>::create<GateObject>([&](auto subscriber) {


It is better not to manually create an observable, but use existing method just to emit a single message, or empty if nothing is emitted.

To simplify the code and eliminate any possibilty to forget on_completed call.

lebdron · 2018-10-04T15:42:04Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

+          const CommitMessage &msg) {
+        return rxcpp::observable<>::create<GateObject>([&](auto subscriber) {
+          const auto hash = getHash(msg.votes).value();
+          if (hash.vote_hashes.proposal_hash.size() == 0) {


Maybe use .empty() instead?

lebdron · 2018-10-04T15:45:30Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

+
+      rxcpp::observable<YacGateImpl::GateObject> YacGateImpl::handleReject(
+          const RejectMessage &msg) {
+        return rxcpp::observable<>::create<GateObject>([&](auto subscriber) {


Same for using rxcpp::observable<>::just

lebdron · 2018-10-04T15:46:06Z

irohad/consensus/yac/yac_hash_provider.hpp

@@ -24,6 +12,7 @@
 #include "consensus/round.hpp"
 #include "consensus/yac/storage/yac_common.hpp"
 #include "interfaces/common_objects/types.hpp"
+#include "ordering/on_demand_os_transport.hpp"


This include should not be required in this file.

- Use just/empty instead of observable::create in YacGate - Replace outdated header with consensus/round.hpp - Remove underscores in GateObjects fields Signed-off-by: Kitsu <mail@kitsu.me>

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron · 2018-10-10T06:10:58Z

irohad/consensus/yac/impl/yac_gate_impl.hpp

@@ -78,8 +70,8 @@ namespace iroha {

        logger::Logger log_;

-        std::pair<YacHash, std::shared_ptr<shared_model::interface::Block>>
-            current_block_;
+        std::shared_ptr<shared_model::interface::Block> current_block_;


-> optional<shared_ptr>, same reason as vote arguments

lebdron · 2018-10-10T06:11:23Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

+          Round round) {
+        bool is_none = not proposal or not block;
+        if (is_none) {
+          current_block_ = nullptr;


-> nullopt

lebdron · 2018-10-10T06:13:03Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

+        bool is_none = not proposal or not block;
+        if (is_none) {
+          current_block_ = nullptr;
+          current_hash_ = {};


Round should be set here, please leave a comment.

lebdron · 2018-10-10T06:14:37Z

irohad/consensus/yac/impl/yac_gate_impl.cpp

+          return rxcpp::observable<>::just<GateObject>(
+              network::VoteOther{current_block_});
+        }
+        return rxcpp::observable<>::empty<GateObject>();


This line is unreachable since conditionals cover all the cases, please remove.

lebdron · 2018-10-10T06:18:59Z

irohad/consensus/yac/impl/yac_gate_impl.cpp


        // insert the block we voted for to the consensus cache
-        consensus_result_cache_->insert(block);
+        if (is_none) {


Code in this conditional might throw an exception in case there was no block:
https://github.com/hyperledger/iroha/blob/a52ca3e89997bcb45e04d3032dc8258da436673d/irohad/consensus/yac/impl/yac_gate_impl.cpp#L53
Check if this case is covered by the tests and change the conditional.

Signed-off-by: Kitsu <mail@kitsu.me>

l4l added the needs-review pr awaits review from maintainers label Sep 24, 2018

l4l requested review from kamilsa and lebdron September 24, 2018 10:23

l4l added the consensus label Sep 24, 2018

lebdron suggested changes Sep 24, 2018

View reviewed changes

lebdron mentioned this pull request Sep 27, 2018

Consensus Round as Key in YacStorages #1729

Merged

kamilsa reviewed Sep 28, 2018

View reviewed changes

l4l force-pushed the feature/yacgate_bft branch from c91f703 to 6eeaf7d Compare September 28, 2018 16:44

l4l changed the title ~~Draft on BFT YacGate~~ BFT YacGate Sep 28, 2018

kamilsa approved these changes Oct 1, 2018

View reviewed changes

l4l requested a review from lebdron October 2, 2018 22:26

Rework YAC toward BFT

07e1b02

Signed-off-by: Kitsu <mail@kitsu.me>

l4l force-pushed the feature/yacgate_bft branch from 217ca40 to 07e1b02 Compare October 3, 2018 14:28

l4l changed the base branch from trunk/bft_yac to dev October 4, 2018 08:11

l4l changed the base branch from dev to trunk/bft_yac October 4, 2018 08:12

lebdron suggested changes Oct 4, 2018

View reviewed changes

l4l added 2 commits October 4, 2018 22:13

Add review fixes

27f649c

- Use just/empty instead of observable::create in YacGate - Replace outdated header with consensus/round.hpp - Remove underscores in GateObjects fields Signed-off-by: Kitsu <mail@kitsu.me>

Rework YacGate::vote iface with optionals

cccd2a3

Signed-off-by: Kitsu <mail@kitsu.me>

l4l mentioned this pull request Oct 7, 2018

BFT-aware Synchronizer #1762

Merged

Add IR-1749 todo

a52ca3e

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron suggested changes Oct 10, 2018

View reviewed changes

l4l added 6 commits October 10, 2018 11:25

Make current_block optional and other minors

6ff62a3

Signed-off-by: Kitsu <mail@kitsu.me>

Update YacGate test

4b373c0

Signed-off-by: Kitsu <mail@kitsu.me>

Temporaralily disable tests

2582dc6

Signed-off-by: Kitsu <mail@kitsu.me>

Add YacGate testcase for AgreementOnNone

9c3ce04

Signed-off-by: Kitsu <mail@kitsu.me>

Revert temp disabled test

69b1810

Signed-off-by: Kitsu <mail@kitsu.me>

Fix dependencies build

a7c37f2

Signed-off-by: Kitsu <mail@kitsu.me>

l4l mentioned this pull request Oct 17, 2018

Prepare Consensus for Two-Phase Commit #1787

Merged

l4l added 2 commits October 18, 2018 00:34

Merge branch 'trunk/bft_yac' into feature/yacgate_bft

b0a0071

Signed-off-by: Kitsu <mail@kitsu.me>

Use explicit this

4ccb8e2

Signed-off-by: Kitsu <mail@kitsu.me>

l4l force-pushed the feature/yacgate_bft branch from f33078a to e7206a5 Compare October 18, 2018 11:57

Add missed proto link to postgres test

5766bdd

Signed-off-by: Kitsu <mail@kitsu.me>

l4l force-pushed the feature/yacgate_bft branch from e7206a5 to 5766bdd Compare October 18, 2018 12:09

l4l requested a review from lebdron October 19, 2018 07:41

lebdron approved these changes Oct 22, 2018

View reviewed changes

l4l removed the needs-review pr awaits review from maintainers label Oct 22, 2018

l4l merged commit f868861 into trunk/bft_yac Oct 22, 2018

l4l deleted the feature/yacgate_bft branch October 22, 2018 22:03

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

BFT YacGate #1731

BFT YacGate #1731

l4l commented Sep 24, 2018

lebdron Sep 24, 2018

l4l Sep 24, 2018

lebdron Sep 24, 2018

lebdron Sep 24, 2018

lebdron Sep 24, 2018

lebdron Sep 24, 2018

lebdron Sep 24, 2018

lebdron Sep 24, 2018

kamilsa Sep 28, 2018

lebdron Oct 4, 2018

l4l Oct 4, 2018

lebdron Oct 4, 2018

l4l Oct 4, 2018

lebdron Oct 4, 2018

lebdron Oct 4, 2018

l4l Oct 11, 2018

lebdron Oct 4, 2018

lebdron Oct 4, 2018

l4l Oct 4, 2018

lebdron Oct 4, 2018

l4l Oct 4, 2018

lebdron Oct 4, 2018

lebdron Oct 4, 2018

l4l Oct 4, 2018

lebdron Oct 4, 2018

lebdron Oct 4, 2018

lebdron Oct 4, 2018

lebdron Oct 4, 2018

lebdron Oct 10, 2018

lebdron Oct 10, 2018

lebdron Oct 10, 2018

lebdron Oct 10, 2018

lebdron Oct 10, 2018

BFT YacGate #1731

BFT YacGate #1731

Conversation

l4l commented Sep 24, 2018

Description of the Change

Benefits

Usage Examples or Tests

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

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