BFT-aware Synchronizer #1762

l4l · 2018-10-07T16:09:38Z

Description of the Change

This pr fixes SynchronizerImpl to function with YacGateImpl, so it can handle ConsensusGate::GateObject and operate accordingly to the emitted variant.

Benefits

One more step toward bft consensus

Possible Drawbacks

~~Tests aren't fixed~~
~~AgreementOnNone isn't properly handled~~
~~No decision on behavior of reject cases~~

- Extended SyncEvent with height and kNothing outcome - Updated tests - Fixed accordingly GateObjects (Rejects & Agreement on None) Signed-off-by: Kitsu <mail@kitsu.me>

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

lebdron · 2018-10-22T06:56:44Z

irohad/synchronizer/synchronizer.hpp

-      /**
-       * Processing last committed block
-       */
-      virtual void process_commit(


Please add processing methods to the interface.

lebdron · 2018-10-23T08:36:42Z

irohad/synchronizer/impl/synchronizer_impl.cpp

+        std::shared_ptr<shared_model::interface::Block> commit_message) {
+      log_->info("at handleNext");
+      std::unique_ptr<ametsuchi::MutableStorage> storage = getStorage();
+      if (storage == nullptr) {


Please do not use nullptr as no-value.

It's an internal function, is it a big deal?

Yes, it is a convention used in the project

Ok, would be nice to write it down to CONTRIBUTING or similar

lebdron · 2018-10-23T08:42:38Z

irohad/synchronizer/synchronizer_common.hpp

@@ -36,6 +36,7 @@ namespace iroha {
    struct SynchronizationEvent {
      Chain synced_blocks;
      SynchronizationOutcomeType sync_outcome;
+      shared_model::interface::types::HeightType height;


Reject round number is also required in ordering gate, so HeightType is better replaced with Round.

test/module/irohad/synchronizer/synchronizer_test.cpp

lebdron · 2018-10-23T09:15:29Z

test/module/irohad/synchronizer/synchronizer_test.cpp

 */
-TEST_F(SynchronizerTest, RetrieveBlockTwoFailures) {


Why is this test case removed? It looks valid, however it seems better to check that it will be called n+1 times if required, where n is number of signatures in commit message.

lebdron · 2018-10-23T09:16:38Z

test/module/irohad/synchronizer/synchronizer_test.cpp

-  rxcpp::observable<std::shared_ptr<shared_model::interface::Block>>
-      commit_message_blocks = rxcpp::observable<>::just(commit_message);
+TEST_F(SynchronizerTest, RejectOutcome) {
+  constexpr int height = 1337;


Move to fixture? Height it used in all test cases.

lebdron · 2018-10-23T09:17:21Z

test/module/irohad/synchronizer/synchronizer_test.cpp

-      commit_message_blocks = rxcpp::observable<>::just(commit_message);
+TEST_F(SynchronizerTest, RejectOutcome) {
+  constexpr int height = 1337;
+  EXPECT_CALL(*consensus_gate, onOutcome())


This expectation seems to be set in all test cases, so it can be moved to setup.

lebdron · 2018-10-23T09:17:56Z

test/module/irohad/synchronizer/synchronizer_test.cpp

-  DefaultValue<expected::Result<std::unique_ptr<MutableStorage>, std::string>>::
-      SetFactory(&createMockMutableStorage);
-  EXPECT_CALL(*mutable_factory, createMutableStorage()).Times(1);
+  init();


This method and its calls can be removed if previous expectation is set in setup.

lebdron · 2018-10-23T09:19:56Z

test/module/irohad/synchronizer/synchronizer_test.cpp


-  EXPECT_CALL(*block_loader, retrieveBlocks(_))
-      .WillRepeatedly(Return(commit_message_blocks));
+  gate_outcome.get_subscriber().on_next(BlockReject{height});


Test case for proposal reject should be added.

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

kamilsa · 2018-10-24T07:09:25Z

test/module/irohad/synchronizer/synchronizer_test.cpp

-
-  init();
-}
+TEST_F(SynchronizerTest, ValidWhenInitialized) {}


Maybe remove that?

Please remove this test.

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

lebdron · 2018-10-26T10:33:43Z

irohad/synchronizer/impl/synchronizer_impl.cpp

-      log_->info("processing commit");
-
-      auto mutable_storage_var = mutable_factory_->createMutableStorage();
+    boost::optional<std::unique_ptr<ametsuchi::MutableStorage>> getStorage(


Maybe make it a class member to avoid passing private members as arguments?

lebdron · 2018-10-26T10:34:11Z

irohad/synchronizer/impl/synchronizer_impl.cpp

@@ -66,41 +89,56 @@ namespace iroha {
              and validator_->validateChain(chain, *storage)) {
            mutable_factory_->commit(std::move(storage));

-            return {chain, SynchronizationOutcomeType::kCommit};
+            return {chain, SynchronizationOutcomeType::kCommit, {}};


Looks like round from GateObject is lost, so it should be passed here as well.

lebdron · 2018-10-26T10:35:05Z

irohad/synchronizer/impl/synchronizer_impl.cpp

+      notifier_.get_subscriber().on_next(
+          SynchronizationEvent{rxcpp::observable<>::just(commit_message),
+                               SynchronizationOutcomeType::kCommit,
+                               {}});


Same for missing round from GateObject.

lebdron · 2018-10-26T10:36:58Z

test/module/irohad/ordering/ordering_gate_test.cpp

@@ -151,7 +151,8 @@ class QueueBehaviorTest : public ::testing::Test {
            std::static_pointer_cast<shared_model::interface::Block>(
                std::make_shared<shared_model::proto::Block>(
                    TestBlockBuilder().height(height).build()))),
-        SynchronizationOutcomeType::kCommit});
+        SynchronizationOutcomeType::kCommit,
+        {}});


Probably it should be {height, 1}.

lebdron · 2018-10-26T10:37:34Z

test/module/irohad/ordering/ordering_gate_test.cpp

+  commit_subject.get_subscriber().on_next(
+      SynchronizationEvent{rxcpp::observable<>::just(block),
+                           SynchronizationOutcomeType::kCommit,
+                           {}});


{block->height(), 1}.

lebdron · 2018-10-26T10:37:58Z

test/module/irohad/synchronizer/synchronizer_test.cpp

-
-  init();
-}
+TEST_F(SynchronizerTest, ValidWhenInitialized) {}


Please remove this test.

lebdron · 2018-10-26T10:42:06Z

test/module/irohad/synchronizer/synchronizer_test.cpp

+  });
+
+  gate_outcome.get_subscriber().on_next(
+      consensus::BlockReject{consensus::Round{}});


ProposalReject handler is still not covered by tests, please add one.

- Add Round to positive consensus outcomes - Make GetStorage class method - Fix related tests Signed-off-by: Kitsu <mail@kitsu.me>

lebdron · 2018-11-01T10:59:40Z

irohad/network/consensus_gate.hpp

 #include "consensus/round.hpp"
+#include "interfaces/common_objects/types.hpp"


Is types include required here?

lebdron · 2018-11-02T07:23:21Z

irohad/network/consensus_gate.hpp

@@ -7,9 +7,12 @@
 #define IROHA_CONSENSUS_GATE_HPP

 #include <boost/optional.hpp>
+#include <boost/variant.hpp>


Already included in gate_object.hpp, so it can be removed.

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

l4l added needs-review pr awaits review from maintainers synchronization labels Oct 7, 2018

l4l requested review from kamilsa and lebdron October 7, 2018 16:09

BFT Synchronizer

72a3ce7

- Extended SyncEvent with height and kNothing outcome - Updated tests - Fixed accordingly GateObjects (Rejects & Agreement on None) Signed-off-by: Kitsu <mail@kitsu.me>

l4l force-pushed the feature/synchronizer_bft branch from 4b11923 to 72a3ce7 Compare October 22, 2018 23:33

l4l changed the title ~~Draft on BFT-aware Synchronizer~~ BFT-aware Synchronizer Oct 22, 2018

Fix dependencies

d20cdd2

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

lebdron suggested changes Oct 23, 2018

View reviewed changes

l4l added 3 commits October 23, 2018 17:57

Make SynchronizerImpl::getStorage internal function

f4551b6

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

Propagate round on Rejects / AgreementOnNone cases

77229d7

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

Fix issues in sync_test

37d0693

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

kamilsa approved these changes Oct 24, 2018

View reviewed changes

l4l added 2 commits October 25, 2018 17:07

Undo removing process_commit

7e2c5f9

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

Move out GateObject to separate files

c90bad9

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

lebdron suggested changes Oct 26, 2018

View reviewed changes

Add review fixes

0b94b36

- Add Round to positive consensus outcomes - Make GetStorage class method - Fix related tests Signed-off-by: Kitsu <mail@kitsu.me>

lebdron approved these changes Nov 2, 2018

View reviewed changes

Fix consensus_gate includes

d3b8f5e

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

l4l merged commit cf87670 into trunk/bft_yac Nov 2, 2018

l4l deleted the feature/synchronizer_bft branch November 2, 2018 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BFT-aware Synchronizer #1762

BFT-aware Synchronizer #1762

l4l commented Oct 7, 2018 •

edited

Loading

lebdron Oct 22, 2018

lebdron Oct 23, 2018

l4l Oct 23, 2018

lebdron Oct 23, 2018

l4l Oct 23, 2018

lebdron Oct 23, 2018

lebdron Oct 23, 2018

lebdron Oct 23, 2018

lebdron Oct 23, 2018

lebdron Oct 23, 2018

lebdron Oct 23, 2018

kamilsa Oct 24, 2018

lebdron Oct 26, 2018

lebdron Oct 26, 2018

lebdron Oct 26, 2018

lebdron Oct 26, 2018

lebdron Oct 26, 2018

lebdron Oct 26, 2018

lebdron Oct 26, 2018

lebdron Oct 26, 2018

lebdron Nov 1, 2018

lebdron Nov 2, 2018

		#include "consensus/round.hpp"
		#include "interfaces/common_objects/types.hpp"

		*/
		TEST_F(SynchronizerTest, RetrieveBlockTwoFailures) {

BFT-aware Synchronizer #1762

BFT-aware Synchronizer #1762

Conversation

l4l commented Oct 7, 2018 • edited Loading

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

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

l4l commented Oct 7, 2018 •

edited

Loading