Feature/mst batches #1642

muratovv · 2018-08-13T08:34:42Z

Description of the Change

Rework MST with batches;
Update tests and related methods

Benefits

MST works with batches

Possible Drawbacks

Some tests are disabled.

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Tests logic needs to be verified. Need txBuilder that operates with signatures. Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

clean up a little bit Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

* fix ub with conditional variable * add test generator fo batches with keypair * fix operator == in transaction batch * extend delay on cv in test * mst state: fix operator ==, add sorting order to getBatches method Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

also fix comments a little bit Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Rework mst mocks with refactored interfaces; Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Rework txProcessor with batch semantic Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Rework mst mocks with refactored interfaces; Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

* remove redundant logs Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

igor-egorov

Update MstStorage too. It contains the method named getExpiredTransactions. Actualize its naming and contents if needed.

igor-egorov · 2018-08-14T12:08:18Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp

@@ -86,17 +87,17 @@ namespace iroha {

    // update state
    // todo wrap in method
-    auto new_transactions =
+    auto new_batches =


[required]
Please specify task id along with todo either remove the comment.

igor-egorov · 2018-08-14T12:10:15Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp

-    log_->info("New txes size: {}", new_transactions->getTransactions().size());
-    // completed transactions
-    shareState(storage_->apply(from, new_state), transactions_subject_);
+    log_->info("New batches size: {}", new_batches->getBatches().size());


[optional]
I suggest logging "new amount of batches" instead.

What do you mean?

rephrasing. amount of batches is not the same as their size.

igor-egorov · 2018-08-14T12:17:08Z

irohad/multi_sig_transactions/mst_processor.hpp


    /**
     * Prove updating of state for handling status of signing
     */
    rxcpp::observable<std::shared_ptr<MstState>> onStateUpdate() const;

    /**
-     * Observable emit transactions that prepared to processing in system
+     * Observable emit batches which are prepared to processing in system


[optional]
Observable that emits batches which are prepared for processing

or for further processing
or to be processed

igor-egorov · 2018-08-14T12:19:35Z

irohad/multi_sig_transactions/state/impl/mst_state.cpp

+                      rhs_batches.begin(),
+                      rhs_batches.end(),
+                      [](const auto &l, const auto &r) {
+                        return *l == *r;;


[required]
Extra ; at the end of the line.

igor-egorov · 2018-08-14T12:44:30Z

irohad/multi_sig_transactions/state/impl/mst_state.cpp

+    std::sort(
+        result.begin(), result.end(), [](const auto &left, const auto &right) {
+          return left->reducedHash().hex() < right->reducedHash().hex();
+        });


[required]
Please leave a comment in code why that sorting is needed.

igor-egorov · 2018-08-14T12:58:23Z

irohad/multi_sig_transactions/state/impl/mst_state.cpp

+   * Merge signatures in batches
+   * @param target - batch for inserting
+   * @param donor - batch with interested transactions
+   * @return false if hashes of transactions don't satisfy each other


[hashes satisfaction]
return false when sequences of transactions inside input batches are different

note that reduced hash of batch does not take into account batch type.

igor-egorov · 2018-08-14T13:10:10Z

irohad/multi_sig_transactions/state/impl/mst_state.cpp

+                                                  signature.publicKey());
+                        });
+        } else {
+          return false;


[required]
Depending on the result of DataType->reducedHash(), now it is possible to have several signatures being merged to the target batch and then false will be returned as the result of evaluation of if (target_tx->hash() == donor_tx->hash()).

A possible solution is to perform the check of transaction hashes equality before the act of signatures merging.

The point is avoiding an appearance of semi-merged batches.

Also, there is a bug. In a best case, signatures will be merged only for one pair of transactions.

igor-egorov · 2018-08-14T13:17:54Z

irohad/multi_sig_transactions/state/mst_state.hpp

    }
  };

  /**
   * Class provide default behaviour for transaction completer
   */
  class DefaultCompleter : public Completer {
-    bool operator()(const DataType transaction) const override {
-      return boost::size(transaction->signatures()) >= transaction->quorum();
+    bool operator()(const DataType &batch) const override {


[optional]
The docstring might need to be updated.

igor-egorov · 2018-08-14T13:44:52Z

irohad/multi_sig_transactions/state/mst_state.hpp

-    bool operator()(const DataType transaction) const override {
-      return boost::size(transaction->signatures()) >= transaction->quorum();
+    bool operator()(const DataType &batch) const override {
+      return std::accumulate(


igor-egorov · 2018-08-14T13:50:27Z

irohad/multi_sig_transactions/transport/impl/mst_transport_grpc.cpp

+
+  shared_model::interface::types::SharedTxsCollectionType collection;
+
+  for (const auto &proto_tx : request->transactions()) {


[required]
Please specify task id for todo below either remove the comment.

* cleanup documentation * fix bug in mst state on merge signatures in batches * add equalByValue method to signable Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

add missed comments for batch helper and state test Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

igor-egorov · 2018-08-20T13:37:26Z

Please update the branch and resolve conflicts

kamilsa · 2018-08-21T14:50:29Z

test/module/irohad/multi_sig_transactions/mst_test_helpers.hpp

+  return framework::batch::makeTestBatch(builders...);
+}
+
+template <typename Batch, typename... Signatures>


Rework variadic templates with variadic agruments with type Signature

Seems, that this is not possible, because we don't not the real type of signature here.

kamilsa · 2018-08-21T14:51:17Z

test/module/irohad/multi_sig_transactions/mst_test_helpers.hpp

+  return batch;
+}
+
+template <typename Batch, typename... KeyPairs>


Same, KeyPairs can be passed using variadic arguments, not templates

Seems, there is, also, not possible to fix your issue because we know a type, but don't know modifiers of the type.

kamilsa · 2018-08-21T14:54:01Z

test/module/irohad/multi_sig_transactions/mst_test_helpers.hpp

+                               KeyPairs... keypairs) {
+  auto create_signature = [&](auto &&key_pair) {
+    auto &payload = batch->transactions().at(tx_number)->payload();
+    auto signedBlob = shared_model::crypto::CryptoSigner<>::sign(


kamilsa · 2018-08-21T14:59:01Z

test/module/irohad/multi_sig_transactions/mst_test_helpers.hpp

+    batch->addSignature(tx_number, signedBlob, key_pair.publicKey());
+  };
+
+  int temp[] = {(create_signature(std::forward<KeyPairs>(keypairs)), 0)...};


Please leave a comment explaining that construction

lebdron · 2018-08-20T14:45:06Z

irohad/multi_sig_transactions/impl/mst_processor.cpp

@@ -19,22 +19,24 @@

 namespace iroha {

-  MstProcessor::MstProcessor() { log_ = logger::log("MstProcessor"); }
+  MstProcessor::MstProcessor() {
+    log_ = logger::log("MstProcessor");


https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c49-prefer-initialization-to-assignment-in-constructors

An initialization explicitly states that initialization, rather than assignment, is done and can be more elegant and efficient.

lebdron · 2018-08-20T14:46:53Z

irohad/multi_sig_transactions/impl/mst_processor_impl.cpp

+      auto completed_batches = state.getBatches();
+      std::for_each(completed_batches.begin(),
+                    completed_batches.end(),
+                    [&subject](const auto batch) {


Maybe use const auto & to avoid two copies - when lambda is called, and when on_next() is called in lambda.

lebdron · 2018-08-20T14:52:43Z

irohad/multi_sig_transactions/mst_processor.hpp

@@ -37,26 +37,27 @@ namespace iroha {
    // ---------------------------| user interface |----------------------------

    /**
-     * Propagate in network multi-signature transaction for signing by other
+     * Propagate in network batch for signing by other


Consider rephrasing, because it means 'network batch' now.
"Propagate batch in network ..."

lebdron · 2018-08-20T14:56:06Z

irohad/multi_sig_transactions/state/impl/mst_state.cpp

-          }
-          return false;
-        });
+    auto &&lhs_batches = getBatches();


getBatches returns a value, maybe use const auto & instead?

lebdron · 2018-08-20T14:57:58Z

irohad/multi_sig_transactions/state/impl/mst_state.cpp

+    std::for_each(internal_state_.begin(),
+                  internal_state_.end(),
+                  [&result](const auto &val) {
+                    val->transactions();


What is the purpose of this statement? Please remove or add a comment.

Also, why the previous version was changed? It can be something like:

std::vector<DataType> result(internal_state_.begin(), internal_state_.end()); std::sort(...); return result;

lebdron · 2018-08-21T15:08:30Z

test/module/irohad/multi_sig_transactions/state_test.cpp

  }
 };

+/**
+ * @given state with one transaction


-> given a timepoint AND a state with an expired transaction
when erase by time is called
then the resulting state contains the expired transaction

lebdron · 2018-08-21T15:14:48Z

test/module/irohad/multi_sig_transactions/state_test.cpp

 }

+/**
+ * @given init two states


-> given two states with expired transactions
when the states are merged
then the resulting state does not contain any transactions

given two states with expired transactions

Batches are not expired at this time.

lebdron · 2018-08-21T15:15:46Z

test/module/irohad/multi_sig_transactions/state_test.cpp


  auto completed_state = state1 += state2;
-  ASSERT_EQ(0, completed_state.getTransactions().size());
+  ASSERT_EQ(0, completed_state.getBatches().size());

  auto expired_state = state1.eraseByTime(time + 1);


Please check if this case is already covered in TimeIndexInsertionByTx

lebdron · 2018-08-21T15:19:20Z

test/module/irohad/multi_sig_transactions/state_test.cpp

 }

+/**
+ * @given first state with two batches, seconds is empty


-> given a state with two expired batches AND an empty state
when the second state is removed from the first state
AND erase by time is called
then the resulting state does not contain any transactions

lebdron · 2018-08-21T15:20:24Z

test/module/irohad/multi_sig_transactions/transport_test.cpp


  transport->sendState(*peer, state);
  std::unique_lock<std::mutex> lock(mtx);
-  cv.wait_for(lock, std::chrono::milliseconds(100));
+  cv.wait_for(lock, std::chrono::milliseconds(5000));


Maybe add a task to rewrite this with gRPC mocks?

Have we created it already?

# Conflicts: # irohad/multi_sig_transactions/transport/impl/mst_transport_grpc.cpp # shared_model/interfaces/iroha_internal/transaction_batch.cpp # test/framework/batch_helper.hpp

* rework many grammar issues * code clean ups * fix merge issues in mst grpc * rework state test with transaction content checks Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

# Conflicts: # irohad/torii/processor/impl/transaction_processor_impl.cpp

Add transaction batch ctor to explicit area; Clean up batch helper with batch ctor; Fix empty sequence test Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

pr is updated

lebdron · 2018-08-24T15:23:39Z

irohad/multi_sig_transactions/state/mst_state.hpp

@@ -113,7 +112,7 @@ namespace iroha {
    MstState operator-(const MstState &rhs) const;

    /**
-     * @return true, if there is no transactions inside
+     * @return true, if there is no bathes inside


lebdron · 2018-08-24T15:26:08Z

irohad/multi_sig_transactions/transport/impl/mst_transport_grpc.cpp

      // TODO (@l4l) 04/03/18 simplify with IR-1040
-      new (addtxs) protocol::Transaction(
+      *protoState.add_transactions() = protocol::Transaction(


No need for additional protocol::Transaction(), as it will make a copy anyway.

lebdron · 2018-08-24T15:35:26Z

test/module/irohad/multi_sig_transactions/mst_test_helpers.hpp

@@ -46,8 +46,12 @@ auto addSignatures(Batch &&batch, int tx_number, Signatures... signatures) {
    batch->addSignature(tx_number, sig_pair.first, sig_pair.second);
  };

+  // pack expansion trick:
+  // generate array with zeros. Operator , express left lambda and return right


-> an ellipsis applies insert_signatures to each signature, operator comma returns the rightmost argument, which is 0

lebdron · 2018-08-24T15:35:51Z

test/module/irohad/multi_sig_transactions/mst_test_helpers.hpp

  };

+  // pack expansion trick:
+  // generate array with zeros. Operator , express left lambda and return right


same as above

lebdron · 2018-08-24T15:37:31Z

test/module/irohad/multi_sig_transactions/state_test.cpp

- * @given state with one transaction
- * @when  call erase by time, where batch will be already expired
- * @then  checks that expired state contains batch and initial doesn't conitain
+ * @given given a timepoint


One "given" can be removed

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

neewy · 2018-08-24T16:21:44Z

👍

sorabot · 2018-08-24T18:16:29Z

SonarQube analysis reported 4 issues

mst_storage_impl.cpp#L12: Function parameter 'target_peer' should be passed by reference.
transport_builder_test.cpp#L64: The function 'createUnbuildTransaction' is never used.
transport_builder_test.cpp#L163: The function 'createInvalidProposal' is never used.
transport_builder_test.cpp#L462: Function parameter 'tx' should be passed by reference.

### Description of the Change Rework MST with batches; Update tests and related methods ### Benefits MST works with batches ### Possible Drawbacks Some tests are disabled.

muratovv and others added 23 commits August 3, 2018 13:13

build successful for mst_processor

b68adfa

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Add one reworked mst test

1d7e205

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Update MST tests (Needs additional fixes)

ca717fc

Tests logic needs to be verified. Need txBuilder that operates with signatures. Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

Compiled tests for mst_state

ce2be62

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Implement TransactionBatch::addSinature

891901f

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Remove redundant logging from hash

d18366b

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Clean up mst state

bc4aba9

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Fix initializer list bug in mst test helpers;

5e13baf

clean up a little bit Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Rework 2 mst state tests: compile and pass

3341cc2

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

add state tests are passed

8378abf

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Remove redundant this from lambda in mst state

e2b2d75

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Refactor names in mst_processor;

ac70ae6

also fix comments a little bit Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Add missed logger include for batch helper

65c650a

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Add required dependency for gossip propagation strategy

ecb6821

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Fix tests for mst processor with batch semantic

f9eb03b

Rework mst mocks with refactored interfaces; Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Merge branch 'develop' into feature/mst_batches

515d14b

Rework command service :: Torii with batch semantic

0faa112

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Remove transactionHandle method from TransactionProcessor;

1448adf

Rework txProcessor with batch semantic Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Fix tests for mst processor with batch semantic

dea7819

Rework mst mocks with refactored interfaces; Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Remove cli tests from build pipeline

5338f46

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Add explicit instantiation for transactionBatch::create factory

982e194

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

* fix number of task in todo

1645f54

* remove redundant logs Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

muratovv requested review from kamilsa and igor-egorov August 13, 2018 08:34

muratovv added 2 commits August 13, 2018 16:24

Disable tx processor tests

a4d3b51

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Remove redundant cout

01c65d7

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

muratovv changed the base branch from master to trunk/mst_batch August 14, 2018 07:18

igor-egorov reviewed Aug 14, 2018

View reviewed changes

Fix review issues:

9606637

* cleanup documentation * fix bug in mst state on merge signatures in batches * add equalByValue method to signable Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

muratovv requested a review from lebdron August 20, 2018 07:49

kamilsa approved these changes Aug 20, 2018

View reviewed changes

Fix review issues:

cc56c2e

add missed comments for batch helper and state test Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

kamilsa previously requested changes Aug 21, 2018

View reviewed changes

lebdron reviewed Aug 21, 2018

View reviewed changes

muratovv added 5 commits August 21, 2018 18:35

Merge branch 'develop' into feature/mst_batches

515ff36

# Conflicts: # irohad/multi_sig_transactions/transport/impl/mst_transport_grpc.cpp # shared_model/interfaces/iroha_internal/transaction_batch.cpp # test/framework/batch_helper.hpp

Fix review issues:

6af0b69

* rework many grammar issues * code clean ups * fix merge issues in mst grpc * rework state test with transaction content checks Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Merge branch 'develop' into feature/mst_batches

7cf5f3f

# Conflicts: # irohad/torii/processor/impl/transaction_processor_impl.cpp

Fix merge issues;

10d22da

Add transaction batch ctor to explicit area; Clean up batch helper with batch ctor; Fix empty sequence test Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

remove redundant universal references

cf87a14

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

muratovv changed the base branch from trunk/mst_batch to develop August 24, 2018 14:33

muratovv requested review from kamilsa and lebdron August 24, 2018 14:34

kamilsa approved these changes Aug 24, 2018

View reviewed changes

lebdron approved these changes Aug 24, 2018

View reviewed changes

muratovv added 2 commits August 24, 2018 18:50

Fix grammar and remove redundant copy

68beda7

Signed-off-by: Fedor Muratov <muratovfyodor@yandex.ru>

Merge branch 'develop' into feature/mst_batches

7d050ae

muratovv merged commit 8bb678d into develop Aug 24, 2018

muratovv mentioned this pull request Aug 27, 2018

Feature/mst batches #1681

Merged

neewy pushed a commit that referenced this pull request Aug 29, 2018

Feature/mst batches (#1642)

cc5ff52

### Description of the Change Rework MST with batches; Update tests and related methods ### Benefits MST works with batches ### Possible Drawbacks Some tests are disabled.

vdrobnyi pushed a commit that referenced this pull request Aug 31, 2018

Feature/mst batches (#1642)

c3fd7ab

### Description of the Change Rework MST with batches; Update tests and related methods ### Benefits MST works with batches ### Possible Drawbacks Some tests are disabled.

nickaleks pushed a commit that referenced this pull request Sep 10, 2018

Feature/mst batches (#1642)

5e23eab

### Description of the Change Rework MST with batches; Update tests and related methods ### Benefits MST works with batches ### Possible Drawbacks Some tests are disabled.

nickaleks pushed a commit that referenced this pull request Sep 10, 2018

Feature/mst batches (#1642)

1ae6ab6

### Description of the Change Rework MST with batches; Update tests and related methods ### Benefits MST works with batches ### Possible Drawbacks Some tests are disabled.

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


		shared_model::interface::types::SharedTxsCollectionType collection;

		for (const auto &proto_tx : request->transactions()) {

Feature/mst batches #1642

Feature/mst batches #1642

Conversation

muratovv commented Aug 13, 2018

Description of the Change

Benefits

Possible Drawbacks

igor-egorov 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

igor-egorov commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muratovv Aug 21, 2018 • edited Loading

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

muratovv Aug 21, 2018 • edited Loading

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

neewy commented Aug 24, 2018

sorabot commented Aug 24, 2018

muratovv Aug 21, 2018 •

edited

Loading

muratovv Aug 21, 2018 •

edited

Loading