Block factory #1622

nickaleks · 2018-08-01T11:24:56Z

Description of the Change

This pull request adds factory for a block. It allows to polymorphically construct blocks in irohad

Benefits

Less dependency on protobuf

Possible Drawbacks

More code

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp # shared_model/interfaces/common_objects/common_objects_factory.hpp # test/module/shared_model/backend_proto/common_objects/proto_common_objects_factory_test.cpp

Previously crypto signer accepted block inside a simulator, now it is block variant Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp # shared_model/interfaces/common_objects/common_objects_factory.hpp # test/module/shared_model/backend_proto/common_objects/proto_common_objects_factory_test.cpp

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # shared_model/backend/protobuf/common_objects/proto_common_objects_factory.hpp # shared_model/interfaces/common_objects/common_objects_factory.hpp # test/module/shared_model/backend_proto/common_objects/proto_common_objects_factory_test.cpp

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # irohad/ordering/impl/ordering_gate_transport_grpc.hpp # irohad/ordering/impl/ordering_service_impl.cpp # test/module/irohad/validation/CMakeLists.txt Merge pull request #3 from laSinteZ/patch-1 fix: const "size" not matching size of message Signed-off-by: Nikita Alekseev <[email protected]>

vdrobnyi · 2018-08-08T12:07:15Z

shared_model/validators/signable_validator.hpp

            reason, model.signatures(), model.payload());
        if (not reason.second.empty()) {
          answer.addReason(std::move(reason));
        }
        return answer;
      }
+
+     private:
+      FieldValidator field_validator_;


What was the reason to introduce it?

Because if we pass any_block_validator as a ModelValidator, it does not have field_validator inside, so using ModelValidator::field_validator fails to compile.

Akvinikym · 2018-08-09T11:01:54Z

irohad/network/CMakeLists.txt

@@ -19,6 +19,7 @@ target_link_libraries(block_loader
    rxcpp
    shared_model_interfaces
    shared_model_proto_backend
+    shared_model_stateless_validation


Not sure, but is it really necessary?

Akvinikym · 2018-08-09T11:04:44Z

irohad/network/impl/block_loader_impl.cpp

+   * If @param block_variant contains block, return it.
+   * If empty block return error
+   */
+  iroha::expected::Result<std::shared_ptr<Block>, std::string> getNonEmptyBlock(


Is this function needed? As I see, later you unpack nonempty block with it and then immediately write a visitor again, but to unpack a result created by yourself. Maybe, it's better to match the block emptiness directly in the code, so you wouldn't have two visitors?

I am not sure what do you mean by this comment. Block Factory returns result, and I would need to add visit_in_place inside match in order to retrieve block. Since the empty block is an error case in block loader, it is logical to apply this sort of transformation

Akvinikym · 2018-08-09T11:08:01Z

irohad/simulator/impl/simulator.cpp

        : validator_(std::move(statefulValidator)),
          ametsuchi_factory_(std::move(factory)),
          block_queries_(std::move(blockQuery)),
-          crypto_signer_(std::move(crypto_signer)) {
+          crypto_signer_(std::move(crypto_signer)),
+          block_factory_(std::move(block_factory)) {
      log_ = logger::log("Simulator");


To ctor initialisation, please

Akvinikym · 2018-08-09T11:16:15Z

shared_model/backend/protobuf/impl/proto_block_factory.cpp

+
+  if (not txs.empty()) {
+    std::for_each(
+        std::begin(txs), std::end(txs), [&block_payload](const auto &tx) {


Capturing block_payload, which is a pointer, by reference? Is it good?

Akvinikym · 2018-08-09T11:19:52Z

shared_model/backend/protobuf/proto_block_factory.hpp

+#define IROHA_PROTO_BLOCK_FACTORY_HPP
+
+#include "backend/protobuf/block.hpp"
+#include "backend/protobuf/empty_block.hpp"


I think, this include can be moved to cpp file

Akvinikym · 2018-08-09T11:36:09Z

shared_model/validators/field_validator.hpp

@@ -183,7 +185,7 @@ namespace shared_model {
      // gap for future transactions
      time_t future_gap_;
      // time provider callback
-      TimeFunction time_provider_;
+      mutable TimeFunction time_provider_;


Why not removing mutable here and const from setTime(...)?

Because validaton methods are const, and they use setTime

Akvinikym · 2018-08-09T11:37:08Z

shared_model/validators/signable_validator.hpp

@@ -29,19 +29,22 @@ namespace shared_model {
     public:
      explicit SignableModelValidator(
          FieldValidator &&validator = FieldValidator())
-          : ModelValidator(std::move(validator)) {}
+          : ModelValidator(validator), field_validator_(validator) {}


Isn't there sense to move it, when you pass it second time?

Akvinikym · 2018-08-09T11:38:01Z

test/module/irohad/network/block_loader_test.cpp


 using testing::A;
 using testing::Return;

 using wPeer = std::shared_ptr<shared_model::interface::Peer>;
 using wBlock = std::shared_ptr<shared_model::interface::Block>;

+using namespace shared_model::validation;


Redundant, the same using in 40 line

Akvinikym · 2018-08-09T11:40:07Z

test/module/irohad/network/block_loader_test.cpp

@@ -76,14 +85,18 @@ class BlockLoaderTest : public testing::Test {
  }

  auto getBaseBlockBuilder() const {
+    iroha::protocol::Transaction proto_tx;
+    shared_model::proto::Transaction tx(proto_tx);


Maybe just shared_model::proto::Transaction tx{iroha::protocol::Transaction{}}?

Akvinikym · 2018-08-09T11:44:41Z

test/module/shared_model/cryptography/crypto_model_signer_mock.hpp

@@ -74,6 +75,13 @@ namespace shared_model {
      crypto_signer_expecter->sign(signable);
    }

+    template <>


Isn't second template a typo?

Nope, because it is a template function of a template class

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # irohad/ordering/impl/ordering_gate_transport_grpc.hpp # irohad/ordering/impl/ordering_service_impl.cpp # test/module/irohad/validation/CMakeLists.txt Merge pull request #3 from laSinteZ/patch-1 fix: const "size" not matching size of message Signed-off-by: Nikita Alekseev <[email protected]>

Akvinikym

Also please pay attention PR is not built

Akvinikym · 2018-08-13T11:43:06Z

irohad/main/application.cpp

@@ -7,6 +7,7 @@
 #include "ametsuchi/impl/postgres_ordering_service_persistent_state.hpp"
 #include "ametsuchi/impl/wsv_restorer_impl.hpp"
 #include "backend/protobuf/common_objects/proto_common_objects_factory.hpp"
+#include "backend/protobuf/proto_proposal_factory.hpp"


Is it used in this file?

Yes, check line 154

Akvinikym · 2018-08-13T11:49:32Z

irohad/network/impl/block_loader_impl.cpp

+                                shared_model::interface::Block>> &result) {
+                subscriber.on_next(std::move(result.value));
+              },
+              [this, &context](iroha::expected::Error<std::string> &error) {


const iroha::expected::Error<std::string> &error?

Akvinikym · 2018-08-13T11:52:51Z

irohad/network/impl/block_loader_impl.cpp

+  return result.match(
+      [](iroha::expected::Value<std::shared_ptr<shared_model::interface::Block>>
+             &v) { return boost::make_optional(std::move(v.value)); },
+      [this](iroha::expected::Error<std::string> &e)


Akvinikym · 2018-08-13T12:31:20Z

shared_model/backend/protobuf/proto_block_factory.hpp

+
+      /**
+       * Create block variant with nonempty block
+       * @return Error if block is empty, or if it is invalid


Clear, but as I understand, we document every parameter and return values

Akvinikym · 2018-08-13T12:33:14Z

shared_model/interfaces/iroha_internal/unsafe_block_factory.hpp

+       * Create block
+       * @param txs - if it is empty, create EmptyBlock,
+       *              else create regular block
+       */


Some parameters are not described

Akvinikym · 2018-08-13T12:37:13Z

test/module/irohad/network/block_loader_test.cpp

@@ -76,14 +83,18 @@ class BlockLoaderTest : public testing::Test {
  }

  auto getBaseBlockBuilder() const {
+//    iroha::protocol::Transaction proto_tx;


vdrobnyi · 2018-08-14T07:31:51Z

irohad/network/impl/block_loader_impl.cpp

@@ -23,39 +23,46 @@
 #include "builders/protobuf/transport_builder.hpp"
 #include "interfaces/common_objects/peer.hpp"
 #include "network/impl/grpc_channel_builder.hpp"
-#include "validators/default_validator.hpp"

 using namespace iroha::ametsuchi;
 using namespace iroha::network;
 using namespace shared_model::crypto;
 using namespace shared_model::interface;
 namespace val = shared_model::validation;


It is not used anymore

vdrobnyi · 2018-08-14T07:40:34Z

irohad/simulator/impl/simulator.cpp

-              .createdTime(proposal.createdTime())
-              .build());
+      auto height = block_queries_->getTopBlockHeight() + 1;
+      auto block = block_factory_->unsafeCreateBlock(height,


Would height be the same as previously in case of an empty block?

Yes, since it used proposal height, which is the same as the (last block height + 1)

vdrobnyi · 2018-08-14T07:48:27Z

irohad/simulator/impl/simulator.hpp

-              crypto_signer);
-
-      Simulator(const Simulator &) = delete;
-      Simulator &operator=(const Simulator &) = delete;


Why have you removed it? Does C++ delete these methods by default if there is a custom constructor?

They are automatically deleted now that one of the members of simulator is unique_ptr, since it is not copyable.

vdrobnyi · 2018-08-14T08:03:32Z

test/module/irohad/network/block_loader_test.cpp

@@ -231,6 +247,8 @@ TEST_F(BlockLoaderTest, ValidWhenBlockMissing) {
  auto present =
      getBaseBlockBuilder().build().signAndAddSignature(key).finish();

+  auto variant = shared_model::interface::BlockVariant(wBlock(clone(present)));


Seems that it is not used

Signed-off-by: Nikita Alekseev <[email protected]> # Conflicts: # irohad/ordering/impl/ordering_gate_transport_grpc.hpp # irohad/ordering/impl/ordering_service_impl.cpp # test/module/irohad/validation/CMakeLists.txt Merge pull request #3 from laSinteZ/patch-1 fix: const "size" not matching size of message Signed-off-by: Nikita Alekseev <[email protected]>

Add factory for blocks in shared model This allows for polymorphic construction of blocks. Signed-off-by: Nikita Alekseev <[email protected]>

nickaleks added 11 commits August 1, 2018 13:28

Merge remote-tracking branch 'origin/develop' into feature/block_factory

f1c0296

nickaleks force-pushed the feature/block_factory branch from 9015a4d to 52453f3 Compare August 7, 2018 20:23

nickaleks requested review from Akvinikym and vdrobnyi August 8, 2018 07:28

vdrobnyi suggested changes Aug 8, 2018

View reviewed changes

Akvinikym suggested changes Aug 9, 2018

View reviewed changes

Akvinikym suggested changes Aug 13, 2018

View reviewed changes

vdrobnyi suggested changes Aug 14, 2018

View reviewed changes

Akvinikym approved these changes Aug 14, 2018

View reviewed changes

vdrobnyi approved these changes Aug 15, 2018

View reviewed changes

nickaleks added 3 commits August 15, 2018 16:05

nickaleks merged commit 32a558f into develop Aug 17, 2018

nickaleks deleted the feature/block_factory branch August 17, 2018 07:36

kamilsa mentioned this pull request Nov 1, 2018

Hyperledger Iroha v1.0 beta-5 version #1825

Closed

bakhtin pushed a commit that referenced this pull request Nov 2, 2018

Block factory (#1622)

57a80f3

Add factory for blocks in shared model This allows for polymorphic construction of blocks. Signed-off-by: Nikita Alekseev <[email protected]>

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

Block factory #1622

Block factory #1622

nickaleks commented Aug 1, 2018

vdrobnyi Aug 8, 2018

nickaleks Aug 13, 2018

Akvinikym Aug 9, 2018

Akvinikym Aug 9, 2018

nickaleks Aug 10, 2018

Akvinikym Aug 9, 2018

Akvinikym Aug 9, 2018

Akvinikym Aug 9, 2018

Akvinikym Aug 9, 2018

nickaleks Aug 10, 2018

Akvinikym Aug 9, 2018

Akvinikym Aug 9, 2018

Akvinikym Aug 9, 2018

Akvinikym Aug 9, 2018

nickaleks Aug 10, 2018

Akvinikym left a comment

Akvinikym Aug 13, 2018

nickaleks Aug 14, 2018

Akvinikym Aug 13, 2018

Akvinikym Aug 13, 2018

Akvinikym Aug 13, 2018

Akvinikym Aug 13, 2018

Akvinikym Aug 13, 2018

vdrobnyi Aug 14, 2018

vdrobnyi Aug 14, 2018

nickaleks Aug 14, 2018

vdrobnyi Aug 14, 2018

nickaleks Aug 14, 2018

vdrobnyi Aug 14, 2018

Block factory #1622

Block factory #1622

Conversation

nickaleks commented Aug 1, 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

Akvinikym left a 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